| Message ID | 20240608213216.25087-2-olivierdautricourt@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d3ddfab0969b19a7dee3753010bb3ea94a0cccd1 |
| Headers | show |
| Series | [v2,1/3] dmaengine: altera-msgdma: use irq variant of spin_lock/unlock while invoking callbacks | expand |
… > 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
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
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); } /**
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(-)