[PATCH v2 1/3] dmaengine: altera-msgdma: use irq variant of spin_lock/unlock while invoking callbacks

[PATCH v2 1/3] dmaengine: altera-msgdma: use irq variant of spin_lock/unlock while invoking callbacks

[Olivier Dautricourt]

As we first take the lock with spin_lock_irqsave in msgdma_tasklet, Lockdep
might complain about this. Inspired by commit 9558cf4ad07e
("dmaengine: zynqmp_dma: fix lockdep warning in tasklet")

Signed-off-by: Olivier Dautricourt <olivierdautricourt@gmail.com>
Tested-by: Olivier Dautricourt <olivierdautricourt@gmail.com>
Suggested-by: Eric Schwarz <eas@sw-optimization.com>
---
 drivers/dma/altera-msgdma.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
index a8e3615235b8..160a465b06dd 100644
--- a/drivers/dma/altera-msgdma.c
+++ b/drivers/dma/altera-msgdma.c
@@ -583,6 +583,7 @@ static void msgdma_issue_pending(struct dma_chan *chan)
 static void msgdma_chan_desc_cleanup(struct msgdma_device *mdev)
 {
 	struct msgdma_sw_desc *desc, *next;
+	unsigned long irqflags;
 
 	list_for_each_entry_safe(desc, next, &mdev->done_list, node) {
 		struct dmaengine_desc_callback cb;
@@ -591,9 +592,9 @@ static void msgdma_chan_desc_cleanup(struct msgdma_device *mdev)
 
 		dmaengine_desc_get_callback(&desc->async_tx, &cb);
 		if (dmaengine_desc_callback_valid(&cb)) {
-			spin_unlock(&mdev->lock);
+			spin_unlock_irqrestore(&mdev->lock, irqflags);
 			dmaengine_desc_callback_invoke(&cb, NULL);
-			spin_lock(&mdev->lock);
+			spin_lock_irqsave(&mdev->lock, irqflags);
 		}
 
 		/* Run any dependencies, then free the descriptor */
-- 
2.45.0

[PATCH v2 2/3] dmaengine: altera-msgdma: cleanup after completing all descriptors

[Olivier Dautricourt]

msgdma_chan_desc_cleanup iterates the done list for each completed
descriptor while we need to do it once after all descriptors are
completed.

This fixes a Sparse warning because we first take the lock in
msgdma_tasklet.
- Move locking to msgdma_chan_desc_cleanup.
- Move call to msgdma_chan_desc_cleanup outside of the critical section of
msgdma_tasklet.

Inspired by: commit 16ed0ef3e931 ("dmaengine: zynqmp_dma: cleanup after
                                   completing all descriptors")

Signed-off-by: Olivier Dautricourt <olivierdautricourt@gmail.com>
Tested-by: Olivier Dautricourt <olivierdautricourt@gmail.com>
Suggested-by: Eric Schwarz <eas@sw-optimization.com>
---
 drivers/dma/altera-msgdma.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
index 160a465b06dd..f32453c97dac 100644
--- a/drivers/dma/altera-msgdma.c
+++ b/drivers/dma/altera-msgdma.c
@@ -585,6 +585,8 @@ static void msgdma_chan_desc_cleanup(struct msgdma_device *mdev)
 	struct msgdma_sw_desc *desc, *next;
 	unsigned long irqflags;
 
+	spin_lock_irqsave(&mdev->lock, irqflags);
+
 	list_for_each_entry_safe(desc, next, &mdev->done_list, node) {
 		struct dmaengine_desc_callback cb;
 
@@ -600,6 +602,8 @@ static void msgdma_chan_desc_cleanup(struct msgdma_device *mdev)
 		/* Run any dependencies, then free the descriptor */
 		msgdma_free_descriptor(mdev, desc);
 	}
+
+	spin_unlock_irqrestore(&mdev->lock, irqflags);
 }
 
 /**
@@ -714,10 +718,11 @@ static void msgdma_tasklet(struct tasklet_struct *t)
 		}
 
 		msgdma_complete_descriptor(mdev);
-		msgdma_chan_desc_cleanup(mdev);
 	}
 
 	spin_unlock_irqrestore(&mdev->lock, flags);
+
+	msgdma_chan_desc_cleanup(mdev);
 }
 
 /**
-- 
2.45.0

Re: [PATCH v2 2/3] dmaengine: altera-msgdma: cleanup after completing all descriptors

[Markus Elfring]

…
> This fixes a Sparse warning because we first take the lock in
> msgdma_tasklet.
…

Can the tag “Fixes” become relevant for the proposed change?


…
> +++ b/drivers/dma/altera-msgdma.c
> @@ -585,6 +585,8 @@ static void msgdma_chan_desc_cleanup(struct msgdma_device *mdev)
>  	struct msgdma_sw_desc *desc, *next;
>  	unsigned long irqflags;
>
> +	spin_lock_irqsave(&mdev->lock, irqflags);
> +
>  	list_for_each_entry_safe(desc, next, &mdev->done_list, node) {
>  		struct dmaengine_desc_callback cb;
>
> @@ -600,6 +602,8 @@ static void msgdma_chan_desc_cleanup(struct msgdma_device *mdev)
>  		/* Run any dependencies, then free the descriptor */
>  		msgdma_free_descriptor(mdev, desc);
>  	}
> +
> +	spin_unlock_irqrestore(&mdev->lock, irqflags);
>  }
…

Would you become interested to apply the guard “spinlock_irqsave”?
https://elixir.bootlin.com/linux/v6.10-rc2/source/include/linux/spinlock.h#L574

Regards,
Markus

Re: [PATCH v2 2/3] dmaengine: altera-msgdma: cleanup after completing all descriptors

[Olivier Dautricourt]

On Sun, Jun 09, 2024 at 07:20:49PM +0200, Markus Elfring wrote:
> …
> > This fixes a Sparse warning because we first take the lock in
> > msgdma_tasklet.
> …
> 
> Can the tag “Fixes” become relevant for the proposed change?

I can add a Fixes tag, it will target only the first commit introducing
this driver.

> 
> 
> …
> > +++ b/drivers/dma/altera-msgdma.c
> > @@ -585,6 +585,8 @@ static void msgdma_chan_desc_cleanup(struct msgdma_device *mdev)
> >  	struct msgdma_sw_desc *desc, *next;
> >  	unsigned long irqflags;
> >
> > +	spin_lock_irqsave(&mdev->lock, irqflags);
> > +
> >  	list_for_each_entry_safe(desc, next, &mdev->done_list, node) {
> >  		struct dmaengine_desc_callback cb;
> >
> > @@ -600,6 +602,8 @@ static void msgdma_chan_desc_cleanup(struct msgdma_device *mdev)
> >  		/* Run any dependencies, then free the descriptor */
> >  		msgdma_free_descriptor(mdev, desc);
> >  	}
> > +
> > +	spin_unlock_irqrestore(&mdev->lock, irqflags);
> >  }
> …
> 
> Would you become interested to apply the guard “spinlock_irqsave”?
> https://elixir.bootlin.com/linux/v6.10-rc2/source/include/linux/spinlock.h#L574

I could but arent these type of things to be integrated in a more global patch serie
targeting (let say) all drivers in one subsystem ?
Currently it seems only one driver uses a guard in the dmaengine subsystem.


Kr,

Olivier

[PATCH v2 3/3] dmaengine: altera-msgdma: properly free descriptor in msgdma_free_descriptor

[Olivier Dautricourt]

Remove list_del call in msgdma_chan_desc_cleanup, this should be the role
of msgdma_free_descriptor. In consequence replace list_add_tail with
list_move_tail in msgdma_free_descriptor.

This fixes the path:
   msgdma_free_chan_resources -> msgdma_free_descriptors ->
   msgdma_free_desc_list -> msgdma_free_descriptor

which does not correctly free the descriptors as first nodes were not
removed from the list.

Signed-off-by: Olivier Dautricourt <olivierdautricourt@gmail.com>
Tested-by: Olivier Dautricourt <olivierdautricourt@gmail.com>
---
 drivers/dma/altera-msgdma.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
index f32453c97dac..0968176f323d 100644
--- a/drivers/dma/altera-msgdma.c
+++ b/drivers/dma/altera-msgdma.c
@@ -233,7 +233,7 @@ static void msgdma_free_descriptor(struct msgdma_device *mdev,
 	struct msgdma_sw_desc *child, *next;
 
 	mdev->desc_free_cnt++;
-	list_add_tail(&desc->node, &mdev->free_list);
+	list_move_tail(&desc->node, &mdev->free_list);
 	list_for_each_entry_safe(child, next, &desc->tx_list, node) {
 		mdev->desc_free_cnt++;
 		list_move_tail(&child->node, &mdev->free_list);
@@ -590,8 +590,6 @@ static void msgdma_chan_desc_cleanup(struct msgdma_device *mdev)
 	list_for_each_entry_safe(desc, next, &mdev->done_list, node) {
 		struct dmaengine_desc_callback cb;
 
-		list_del(&desc->node);
-
 		dmaengine_desc_get_callback(&desc->async_tx, &cb);
 		if (dmaengine_desc_callback_valid(&cb)) {
 			spin_unlock_irqrestore(&mdev->lock, irqflags);
-- 
2.45.0

Re: [PATCH v2 3/3] dmaengine: altera-msgdma: properly free descriptor in msgdma_free_descriptor

[Markus Elfring]

> Remove list_del call in msgdma_chan_desc_cleanup, this should be the role
> of msgdma_free_descriptor. In consequence replace list_add_tail with
> list_move_tail in msgdma_free_descriptor.
…

Would you like to add the tag “Fixes”?

Regards,
Markus

Re: [PATCH v2 3/3] dmaengine: altera-msgdma: properly free descriptor in msgdma_free_descriptor

[Olivier Dautricourt]

On Sun, Jun 09, 2024 at 07:30:04PM +0200, Markus Elfring wrote:
> > Remove list_del call in msgdma_chan_desc_cleanup, this should be the role
> > of msgdma_free_descriptor. In consequence replace list_add_tail with
> > list_move_tail in msgdma_free_descriptor.
> …
> 
> Would you like to add the tag “Fixes”?

Right, the Fixes tag will target the first commit
introducing this driver (a85c6f1b2921)

Thanks,
Olivier

Re: [PATCH v2 1/3] dmaengine: altera-msgdma: use irq variant of spin_lock/unlock while invoking callbacks

[Markus Elfring]

I find that a cover letter can be helpful for the presented patch series.


> As we first take the lock with spin_lock_irqsave in msgdma_tasklet, Lockdep
…

I suggest to move the last word into the subsequent text line.

Regards,
Markus

Re: [PATCH v2 1/3] dmaengine: altera-msgdma: use irq variant of spin_lock/unlock while invoking callbacks

[Vinod Koul]

On Sat, 08 Jun 2024 23:31:46 +0200, Olivier Dautricourt wrote:
> As we first take the lock with spin_lock_irqsave in msgdma_tasklet, Lockdep
> might complain about this. Inspired by commit 9558cf4ad07e
> ("dmaengine: zynqmp_dma: fix lockdep warning in tasklet")
> 
> 

Applied, thanks!

[1/3] dmaengine: altera-msgdma: use irq variant of spin_lock/unlock while invoking callbacks
      commit: 261d3a85d959841821ca0d69f9d7b0d4087661c4
[2/3] dmaengine: altera-msgdma: cleanup after completing all descriptors
      commit: d3ddfab0969b19a7dee3753010bb3ea94a0cccd1
[3/3] dmaengine: altera-msgdma: properly free descriptor in msgdma_free_descriptor
      commit: 54e4ada1a4206f878e345ae01cf37347d803d1b1

Best regards,
-- 
Vinod Koul <vkoul@kernel.org>