Re: [PATCH v5.10-rt] rt: dma: fix build issue in at_hdmac

On Fri, Aug 16, 2024 at 08:46:25AM +0200, Sebastian Andrzej Siewior wrote:
> On 2024-08-15 21:41:03 [-0300], Luis Claudio R. Goncalves wrote:
> > The functions atc_advance_work() and atc_issue_pending() both have a
> > similar statement
> > 
> >     return spin_unlock_irqrestore(&atchan->lock, flags);
> > 
> > That results in the following errors during the build:
> > 
> >     drivers/dma/at_hdmac.c: In function ‘atc_advance_work’:
> >     ./include/linux/spinlock_rt.h:115:9: error: expected expression before ‘do’
> >       115 |         do {                                            \
> >           |         ^~
> >     drivers/dma/at_hdmac.c:487:24: note: in expansion of macro ‘spin_unlock_irqrestore’
> >       487 |                 return spin_unlock_irqrestore(&atchan->lock, flags);
> >           |                        ^~~~~~~~~~~~~~~~~~~~~~
> >     ./include/linux/spinlock_rt.h:115:9: error: ‘return’ with a value, in function returning void [-Werror=return-type]
> >       115 |         do {                                            \
> >           |         ^~
> >     drivers/dma/at_hdmac.c:487:24: note: in expansion of macro ‘spin_unlock_irqrestore’
> >       487 |                 return spin_unlock_irqrestore(&atchan->lock, flags);
> >           |                        ^~~~~~~~~~~~~~~~~~~~~~
> > 
> > Fix this by splitting the spin_unlock_irqrestore() call and the return
> > statement in both functions.
> 
> If I see this right, then this code has been replaced by commit
>   ac803b56860f6 ("dmaengine: at_hdmac: Convert driver to use virt-dma")
> 
> which has been merged in v6.2-rc1. This has been introduced in commits
>   fcd37565efdaf ("dmaengine: at_hdmac: Fix premature completion of desc in issue_pending")
>      v6.1-rc5
>   c6babed879fbe ("dmaengine: at_hdmac: Fix concurrency problems by removing atc_complete_all()")
>      v6.1-rc5
> 
> This means v6.1 is affected and the earlier version got it via the
> stable queue.
> This compiles here with and without RT on v6.1 with gcc version 14.2.0.
> The question would, while it is bad in your case and I don't see. Also
> if this is visible in your RT queue, it should be visible in the stable
> queue without -RT, too. And then once all details known I would like a
> patch that goes upstream and fixes the breakage at its root.

Sebastian, I do believe this problem is unique to v5.10-rt. My bad for not
stating that clearly on the description. Also, specific to aarch64 AT91-based
boards as AT_HDMAC requires ARCH_AT91 to build.

The definition of spin_unlock_irqrestore() at include/linux/spinlock_rt.h
is slightly different between v5.10-rt and v5.15-rt:

$ git diff origin/v5.10-rt origin/v5.15-rt include/linux/spinlock_rt.h
...
 
-#define spin_unlock_irqrestore(lock, flags)		\
-	do {						\
-		typecheck(unsigned long, flags);	\
-		(void) flags;				\
-		spin_unlock(lock);			\
-	} while (0)
+static __always_inline void spin_unlock_irqrestore(spinlock_t *lock,
+						   unsigned long flags)
+{
+	rt_spin_unlock(lock);
+}
 
...

And that seems to be why the problem pops up v5.10 and not on newer RT
versions.

I see that there was a revamp of the rtmutex code between v5.10-rt and
v5.15-rt, but given the advanced stage of v5.10-rt in its lifetime it
sounded more reasonable to simply fix the symptom.

Do you have any suggestion on how to proceed?

Thanks in advance,
Luis

> I don't see anything in v6.1.104. Unlike the recent RiscV fallout, this
> is not RT specific.
> 
> Sebastian
> 
---end quoted text---