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

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

[Luis Claudio R. Goncalves]

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.

Signed-off-by: Luis Claudio R. Goncalves <lgoncalv@redhat.com>
---
 drivers/dma/at_hdmac.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Many thanks to Daniel Wagner who spotted and reported this build issue!

diff --git a/drivers/dma/at_hdmac.c b/drivers/dma/at_hdmac.c
index 6a4f9697b574..bdbd85adeea9 100644
--- a/drivers/dma/at_hdmac.c
+++ b/drivers/dma/at_hdmac.c
@@ -483,8 +483,10 @@ static void atc_advance_work(struct at_dma_chan *atchan)
 	dev_vdbg(chan2dev(&atchan->chan_common), "advance_work\n");
 
 	spin_lock_irqsave(&atchan->lock, flags);
-	if (atc_chan_is_enabled(atchan) || list_empty(&atchan->active_list))
-		return spin_unlock_irqrestore(&atchan->lock, flags);
+	if (atc_chan_is_enabled(atchan) || list_empty(&atchan->active_list)) {
+		spin_unlock_irqrestore(&atchan->lock, flags);
+		return;
+	}
 
 	desc = atc_first_active(atchan);
 	/* Remove the transfer node from the active list. */
@@ -1477,8 +1479,10 @@ static void atc_issue_pending(struct dma_chan *chan)
 	dev_vdbg(chan2dev(chan), "issue_pending\n");
 
 	spin_lock_irqsave(&atchan->lock, flags);
-	if (atc_chan_is_enabled(atchan) || list_empty(&atchan->queue))
-		return spin_unlock_irqrestore(&atchan->lock, flags);
+	if (atc_chan_is_enabled(atchan) || list_empty(&atchan->queue)) {
+		spin_unlock_irqrestore(&atchan->lock, flags);
+		return;
+	}
 
 	desc = atc_first_queued(atchan);
 	list_move_tail(&desc->desc_node, &atchan->active_list);
-- 
2.46.0

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

[Sebastian Andrzej Siewior]

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.

I don't see anything in v6.1.104. Unlike the recent RiscV fallout, this
is not RT specific.

Sebastian

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

[Luis Claudio R. Goncalves]

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---

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

[Sebastian Andrzej Siewior]

On 2024-08-16 06:44:21 [-0300], Luis Claudio R. Goncalves wrote:
> > 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.

It is also part of one of ARM's defconfig which I tested with. So this
is not so unique.

> 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?

I would start by explaining that this only affects <= v5.10 because of
the different spin_unlock_irqrestore() implementation (macro vs
function) and the compiler does not complain about the latter.
This would also make clear why it affects certain RT versions and why
mainline is not affected in general. Also a reference to why this is an
issue now and not earlier.
This information looks more important to me than one line of compiler's
complain/ warning.
That said, with this additional information I don't mind getting this
into the affected kernels.

> Thanks in advance,
> Luis

Sebastian