| Message ID | 20230920200636.32870-3-olivierdautricourt@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dmaengine: altera-msgdma: fix descriptors freeing logic | expand |
Hello Olivier, thanks for following up on my comment first. I really appreciate. - I don't have access to the hardware anymore, so I cannot test changes myself. This patch addresses IMHO three fixes. - Shouldn't it be split up into three small junks so one could also later work w/ git bisect / separate ack's? - That way it is an all or nothing thing. Please regard this remark as cosmetics. Am 20.09.2023 um 21:58 schrieb Olivier Dautricourt: > Sparse complains because we first take the lock in msgdma_tasklet -> move > locking to msgdma_chan_desc_cleanup. > In consequence, move calling of msgdma_chan_desc_cleanup outside of the > critical section of function msgdma_tasklet. > > Use spin_unlock_irqsave/restore instead of just spinlock/unlock to keep > state of irqs while executing the callbacks. What about the locking in the IRQ handler msgdma_irq_handler() itself? - Shouldn't spin_unlock_irqsave/restore() be used there as well instead of just spinlock/unlock()? > 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__ seems to free correctly the descriptors as firsts nodes > where not removed from the specified list. > s/__not__/_not_/ s/seems/seem/ s/firsts/first/ => Actually I would omit it. s/where/were/ "Fixes: <12 digits git hash> ("commit-message")" is missing [1] isn't it? [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > Signed-off-by: Olivier Dautricourt <olivierdautricourt@gmail.com> > --- > Following Eric Schwarz comments on altera-msgdma driver not having some > of the fixes made to zynqmp-dma driver (which msgdma driver is based on): > This patch should address at least the spinlock part, it __has not__ been > tested yet so please don't accept it right away. I'm in the process of > getting a new hardware to test with. Meanwhile it is open to reviews > and even better if someone is able to test it. > > drivers/dma/altera-msgdma.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c > index 4153c2edb049..c39937bfcdf1 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); > @@ -583,22 +583,25 @@ 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; > + > + spin_lock_irqsave(&mdev->lock, irqflags); > > 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(&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 */ > msgdma_free_descriptor(mdev, desc); > } > + > + spin_unlock_irqrestore(&mdev->lock, irqflags); > } > > /** > @@ -713,10 +716,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); > } > > /**
On Thu, Sep 21, 2023 at 03:07:15PM +0200, Eric Schwarz wrote: > Hello Olivier, > > thanks for following up on my comment first. I really appreciate. - I don't > have access to the hardware anymore, so I cannot test changes myself. > > This patch addresses IMHO three fixes. - Shouldn't it be split up into three > small junks so one could also later work w/ git bisect / separate ack's? - > That way it is an all or nothing thing. Please regard this remark as > cosmetics. > > Am 20.09.2023 um 21:58 schrieb Olivier Dautricourt: > > Sparse complains because we first take the lock in msgdma_tasklet -> move > > locking to msgdma_chan_desc_cleanup. > > In consequence, move calling of msgdma_chan_desc_cleanup outside of the > > critical section of function msgdma_tasklet. > > > > Use spin_unlock_irqsave/restore instead of just spinlock/unlock to keep > > state of irqs while executing the callbacks. > > What about the locking in the IRQ handler msgdma_irq_handler() itself? - > Shouldn't spin_unlock_irqsave/restore() be used there as well instead of > just spinlock/unlock()? IMO no: It is covered by [1]("Locking Between Hard IRQ and Softirqs/Tasklets") The irq handler cannot be preempted by the tasklet, so the spin_lock/unlock version is ok. However the tasklet could be interrupted by the Hard IRQ hence the disabling of irqs with save/restore when entering critical section. It should not be needed to keep interrupts locally disabled while invoking callbacks, will add this to the commit description. [1] https://www.kernel.org/doc/Documentation/kernel-hacking/locking.rst > > > 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__ seems to free correctly the descriptors as firsts nodes > > where not removed from the specified list. > > > s/__not__/_not_/ > s/seems/seem/ > s/firsts/first/ => Actually I would omit it. > s/where/were/ > > "Fixes: <12 digits git hash> ("commit-message")" is missing [1] isn't it? > > [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes Thank you for your remarks/corrections, i will take them into account in next version of the patch. Kr, Olivier Dautricourt
Hello Olivier, >> Am 20.09.2023 um 21:58 schrieb Olivier Dautricourt: >>> Sparse complains because we first take the lock in msgdma_tasklet -> move >>> locking to msgdma_chan_desc_cleanup. >>> In consequence, move calling of msgdma_chan_desc_cleanup outside of the >>> critical section of function msgdma_tasklet. >>> >>> Use spin_unlock_irqsave/restore instead of just spinlock/unlock to keep >>> state of irqs while executing the callbacks. >> >> What about the locking in the IRQ handler msgdma_irq_handler() itself? - >> Shouldn't spin_unlock_irqsave/restore() be used there as well instead of >> just spinlock/unlock()? > > IMO no: > It is covered by [1]("Locking Between Hard IRQ and Softirqs/Tasklets") > The irq handler cannot be preempted by the tasklet, so the > spin_lock/unlock version is ok. However the tasklet could be interrupted > by the Hard IRQ hence the disabling of irqs with save/restore when > entering critical section. > > It should not be needed to keep interrupts locally disabled while invoking > callbacks, will add this to the commit description. > > [1] https://www.kernel.org/doc/Documentation/kernel-hacking/locking.rst Thanks for the link. I have read differently here [2] w/ special emphasis on "Lesson 3: spinlocks revisited.". [2] https://www.kernel.org/doc/Documentation/locking/spinlocks.txt Cheers Eric
Hi Eric, On Fri, Sep 22, 2023 at 09:49:59AM +0200, Eric Schwarz wrote: > Hello Olivier, > > > > Am 20.09.2023 um 21:58 schrieb Olivier Dautricourt: > > > > Sparse complains because we first take the lock in msgdma_tasklet -> move > > > > locking to msgdma_chan_desc_cleanup. > > > > In consequence, move calling of msgdma_chan_desc_cleanup outside of the > > > > critical section of function msgdma_tasklet. > > > > > > > > Use spin_unlock_irqsave/restore instead of just spinlock/unlock to keep > > > > state of irqs while executing the callbacks. > > > > > > What about the locking in the IRQ handler msgdma_irq_handler() itself? - > > > Shouldn't spin_unlock_irqsave/restore() be used there as well instead of > > > just spinlock/unlock()? > > > > IMO no: > > It is covered by [1]("Locking Between Hard IRQ and Softirqs/Tasklets") > > The irq handler cannot be preempted by the tasklet, so the > > spin_lock/unlock version is ok. However the tasklet could be interrupted > > by the Hard IRQ hence the disabling of irqs with save/restore when > > entering critical section. > > > > It should not be needed to keep interrupts locally disabled while invoking > > callbacks, will add this to the commit description. > > > > [1] https://www.kernel.org/doc/Documentation/kernel-hacking/locking.rst > > Thanks for the link. I have read differently here [2] w/ special emphasis on > "Lesson 3: spinlocks revisited.". > > [2] https://www.kernel.org/doc/Documentation/locking/spinlocks.txt > This chapter [2] says that our code must use irq versions of spin_lock because our handler does indeed play with the lock. However this requirement does not apply to the irq handler itself, as we know that the interrupt line is disabled during the execution of the handler (and our handler is not shared with another irq). Kr, Olivier > Cheers > Eric
Hello Olivier, Am 22.09.2023 um 18:33 schrieb Olivier Dautricourt: > Hi Eric, > > On Fri, Sep 22, 2023 at 09:49:59AM +0200, Eric Schwarz wrote: >> Hello Olivier, >> >>>> Am 20.09.2023 um 21:58 schrieb Olivier Dautricourt: >>>>> Sparse complains because we first take the lock in msgdma_tasklet -> move >>>>> locking to msgdma_chan_desc_cleanup. >>>>> In consequence, move calling of msgdma_chan_desc_cleanup outside of the >>>>> critical section of function msgdma_tasklet. >>>>> >>>>> Use spin_unlock_irqsave/restore instead of just spinlock/unlock to keep >>>>> state of irqs while executing the callbacks. >>>> >>>> What about the locking in the IRQ handler msgdma_irq_handler() itself? - >>>> Shouldn't spin_unlock_irqsave/restore() be used there as well instead of >>>> just spinlock/unlock()? >>> >>> IMO no: >>> It is covered by [1]("Locking Between Hard IRQ and Softirqs/Tasklets") >>> The irq handler cannot be preempted by the tasklet, so the >>> spin_lock/unlock version is ok. However the tasklet could be interrupted >>> by the Hard IRQ hence the disabling of irqs with save/restore when >>> entering critical section. >>> >>> It should not be needed to keep interrupts locally disabled while invoking >>> callbacks, will add this to the commit description. >>> >>> [1] https://www.kernel.org/doc/Documentation/kernel-hacking/locking.rst >> >> Thanks for the link. I have read differently here [2] w/ special emphasis on >> "Lesson 3: spinlocks revisited.". >> >> [2] https://www.kernel.org/doc/Documentation/locking/spinlocks.txt >> > > This chapter [2] says that our code must use irq versions of spin_lock > because our handler does indeed play with the lock. However this > requirement does not apply to the irq handler itself, as we know that the > interrupt line is disabled during the execution of the handler (and our > handler is not shared with another irq). "... as we know that the interrupt line is disabled during the execution of the handler (and our handler is not shared with another irq)." That was the point I wanted to be sure about. So if the IRQ handler cannot be called twice ensured by architecture neither on single or multi CPU systems (SMP or others) I am fine. Thanks for your response on that. Appreciated. Because you take the effort to set up hardware and environment again you may also test following fixes/improvements from zynqmp driver which could then be merged into altera-msgdma driver. Please check yourself: f2b816a1dfb8 ("dmaengine: zynqmp_dma: Add device_synchronize support") # Caught by your patchset #9558cf4ad07e ("dmaengine: zynqmp_dma: fix lockdep warning in tasklet") # Caught by your patchset #16ed0ef3e931 ("dmaengine: zynqmp_dma: cleanup after completing all descriptors") # Caught by your patchset - For the altera-msgdma driver it is a real fix not an optimization. #48594dbf793a ("dmaengine: zynqmp_dma: Use list_move_tail instead of list_del/list_add_tail") 5ba080aada5e ("dmaengine: zynqmp_dma: Fix race condition in the probe") Note: If the sequence is applied in reverse order the log would be comparable to zynqmp driver's log. IMHO your patchset could/should be extended by two more patches and split into small junks as mentioned. Then history would stay intact to be compared to zynqmp driver. Note: Take care about "Developer’s Certificate of Origin 1.1". IMHO "Signed-off-by" tags from the other patches might/must be copied at least for most of the patches then, which would make it easier to get it into mainline. Btw, some cosmetic changes could be made in the mainlined driver: 30s/implements/Implements/ 31s/data/Data/ 32s/data/Data/ 33s/the/The/ 39s/data/Data/ 40s/data/Data/ 41s/characteristics/Characteristics/ 109s/response/Response/ 154s/implements/Implements/ 154s/sw\ /SW\ / 155s/support/Support/ 155s/api/API/ 156s/assosiated/Associated/ 157s/node\ /Node\ / 158s/transmit/Transmit/ 259s/Hw/HW/ 291s/Hw/HW/ 322s/prepare/Prepare/ 327s/transfer/Transfer/ 378s/prepare/Prepare/ 384s/transfer/Transfer/ 385s/transfer/Transfer/ 502s/its/it\'s/ 514s/oder/order/ 530s/copy\ /Copy\ / 680s/sSGDMA/mSGDMA/ 723s/Interrupt/interrupt/ 752s/\(\)// 921s/\(\)// ... and another patch, if that is taken into account. Cheers Eric
Hello Olivier, just a ping on getting the patches / fixes below mainline. - Were you able to get hardware for testing? Many thanks Eric Am 28.09.2023 um 09:57 schrieb Eric Schwarz: > Hello Olivier, > > Am 22.09.2023 um 18:33 schrieb Olivier Dautricourt: >> Hi Eric, >> >> On Fri, Sep 22, 2023 at 09:49:59AM +0200, Eric Schwarz wrote: >>> Hello Olivier, >>> >>>>> Am 20.09.2023 um 21:58 schrieb Olivier Dautricourt: >>>>>> Sparse complains because we first take the lock in msgdma_tasklet >>>>>> -> move >>>>>> locking to msgdma_chan_desc_cleanup. >>>>>> In consequence, move calling of msgdma_chan_desc_cleanup outside >>>>>> of the >>>>>> critical section of function msgdma_tasklet. >>>>>> >>>>>> Use spin_unlock_irqsave/restore instead of just spinlock/unlock to >>>>>> keep >>>>>> state of irqs while executing the callbacks. >>>>> >>>>> What about the locking in the IRQ handler msgdma_irq_handler() >>>>> itself? - >>>>> Shouldn't spin_unlock_irqsave/restore() be used there as well >>>>> instead of >>>>> just spinlock/unlock()? >>>> >>>> IMO no: >>>> It is covered by [1]("Locking Between Hard IRQ and Softirqs/Tasklets") >>>> The irq handler cannot be preempted by the tasklet, so the >>>> spin_lock/unlock version is ok. However the tasklet could be >>>> interrupted >>>> by the Hard IRQ hence the disabling of irqs with save/restore when >>>> entering critical section. >>>> >>>> It should not be needed to keep interrupts locally disabled while >>>> invoking >>>> callbacks, will add this to the commit description. >>>> >>>> [1] https://www.kernel.org/doc/Documentation/kernel-hacking/locking.rst >>> >>> Thanks for the link. I have read differently here [2] w/ special >>> emphasis on >>> "Lesson 3: spinlocks revisited.". >>> >>> [2] https://www.kernel.org/doc/Documentation/locking/spinlocks.txt >>> >> >> This chapter [2] says that our code must use irq versions of spin_lock >> because our handler does indeed play with the lock. However this >> requirement does not apply to the irq handler itself, as we know that the >> interrupt line is disabled during the execution of the handler (and our >> handler is not shared with another irq). > > "... as we know that the interrupt line is disabled during the execution > of the handler (and our handler is not shared with another irq)." > > That was the point I wanted to be sure about. So if the IRQ handler > cannot be called twice ensured by architecture neither on single or > multi CPU systems (SMP or others) I am fine. > Thanks for your response on that. Appreciated. > > Because you take the effort to set up hardware and environment again you > may also test following fixes/improvements from zynqmp driver which > could then be merged into altera-msgdma driver. Please check yourself: > > f2b816a1dfb8 ("dmaengine: zynqmp_dma: Add device_synchronize support") > # Caught by your patchset > #9558cf4ad07e ("dmaengine: zynqmp_dma: fix lockdep warning in tasklet") > # Caught by your patchset > #16ed0ef3e931 ("dmaengine: zynqmp_dma: cleanup after completing all > descriptors") > # Caught by your patchset - For the altera-msgdma driver it is a real > fix not an optimization. > #48594dbf793a ("dmaengine: zynqmp_dma: Use list_move_tail instead of > list_del/list_add_tail") > 5ba080aada5e ("dmaengine: zynqmp_dma: Fix race condition in the probe") > > Note: If the sequence is applied in reverse order the log would be > comparable to zynqmp driver's log. > > IMHO your patchset could/should be extended by two more patches and > split into small junks as mentioned. Then history would stay intact to > be compared to zynqmp driver. > > Note: Take care about "Developer’s Certificate of Origin 1.1". IMHO > "Signed-off-by" tags from the other patches might/must be copied at > least for most of the patches then, which would make it easier to get it > into mainline. > > Btw, some cosmetic changes could be made in the mainlined driver: > > 30s/implements/Implements/ > 31s/data/Data/ > 32s/data/Data/ > 33s/the/The/ > 39s/data/Data/ > 40s/data/Data/ > 41s/characteristics/Characteristics/ > 109s/response/Response/ > 154s/implements/Implements/ > 154s/sw\ /SW\ / > 155s/support/Support/ > 155s/api/API/ > 156s/assosiated/Associated/ > 157s/node\ /Node\ / > 158s/transmit/Transmit/ > 259s/Hw/HW/ > 291s/Hw/HW/ > 322s/prepare/Prepare/ > 327s/transfer/Transfer/ > 378s/prepare/Prepare/ > 384s/transfer/Transfer/ > 385s/transfer/Transfer/ > 502s/its/it\'s/ > 514s/oder/order/ > 530s/copy\ /Copy\ / > 680s/sSGDMA/mSGDMA/ > 723s/Interrupt/interrupt/ > 752s/\(\)// > 921s/\(\)// > > ... and another patch, if that is taken into account. > > Cheers > Eric
Hi Eric, Changes were tested successfully, i will resend v2 soon. Olivier On Sun, Feb 25, 2024 at 09:05:37PM +0100, Eric Schwarz wrote: > Hello Olivier, > > just a ping on getting the patches / fixes below mainline. - Were you able > to get hardware for testing? > > Many thanks > Eric > > > Am 28.09.2023 um 09:57 schrieb Eric Schwarz: > > Hello Olivier, > > > > Am 22.09.2023 um 18:33 schrieb Olivier Dautricourt: > > > Hi Eric, > > > > > > On Fri, Sep 22, 2023 at 09:49:59AM +0200, Eric Schwarz wrote: > > > > Hello Olivier, > > > > > > > > > > Am 20.09.2023 um 21:58 schrieb Olivier Dautricourt: > > > > > > > Sparse complains because we first take the lock in > > > > > > > msgdma_tasklet -> move > > > > > > > locking to msgdma_chan_desc_cleanup. > > > > > > > In consequence, move calling of > > > > > > > msgdma_chan_desc_cleanup outside of the > > > > > > > critical section of function msgdma_tasklet. > > > > > > > > > > > > > > Use spin_unlock_irqsave/restore instead of just > > > > > > > spinlock/unlock to keep > > > > > > > state of irqs while executing the callbacks. > > > > > > > > > > > > What about the locking in the IRQ handler > > > > > > msgdma_irq_handler() itself? - > > > > > > Shouldn't spin_unlock_irqsave/restore() be used there as > > > > > > well instead of > > > > > > just spinlock/unlock()? > > > > > > > > > > IMO no: > > > > > It is covered by [1]("Locking Between Hard IRQ and Softirqs/Tasklets") > > > > > The irq handler cannot be preempted by the tasklet, so the > > > > > spin_lock/unlock version is ok. However the tasklet could be > > > > > interrupted > > > > > by the Hard IRQ hence the disabling of irqs with save/restore when > > > > > entering critical section. > > > > > > > > > > It should not be needed to keep interrupts locally disabled > > > > > while invoking > > > > > callbacks, will add this to the commit description. > > > > > > > > > > [1] https://www.kernel.org/doc/Documentation/kernel-hacking/locking.rst > > > > > > > > Thanks for the link. I have read differently here [2] w/ special > > > > emphasis on > > > > "Lesson 3: spinlocks revisited.". > > > > > > > > [2] https://www.kernel.org/doc/Documentation/locking/spinlocks.txt > > > > > > > > > > This chapter [2] says that our code must use irq versions of spin_lock > > > because our handler does indeed play with the lock. However this > > > requirement does not apply to the irq handler itself, as we know that the > > > interrupt line is disabled during the execution of the handler (and our > > > handler is not shared with another irq). > > > > "... as we know that the interrupt line is disabled during the execution > > of the handler (and our handler is not shared with another irq)." > > > > That was the point I wanted to be sure about. So if the IRQ handler > > cannot be called twice ensured by architecture neither on single or > > multi CPU systems (SMP or others) I am fine. > > Thanks for your response on that. Appreciated. > > > > Because you take the effort to set up hardware and environment again you > > may also test following fixes/improvements from zynqmp driver which > > could then be merged into altera-msgdma driver. Please check yourself: > > > > f2b816a1dfb8 ("dmaengine: zynqmp_dma: Add device_synchronize support") > > # Caught by your patchset > > #9558cf4ad07e ("dmaengine: zynqmp_dma: fix lockdep warning in tasklet") > > # Caught by your patchset > > #16ed0ef3e931 ("dmaengine: zynqmp_dma: cleanup after completing all > > descriptors") > > # Caught by your patchset - For the altera-msgdma driver it is a real > > fix not an optimization. > > #48594dbf793a ("dmaengine: zynqmp_dma: Use list_move_tail instead of > > list_del/list_add_tail") > > 5ba080aada5e ("dmaengine: zynqmp_dma: Fix race condition in the probe") > > > > Note: If the sequence is applied in reverse order the log would be > > comparable to zynqmp driver's log. > > > > IMHO your patchset could/should be extended by two more patches and > > split into small junks as mentioned. Then history would stay intact to > > be compared to zynqmp driver. > > > > Note: Take care about "Developer’s Certificate of Origin 1.1". IMHO > > "Signed-off-by" tags from the other patches might/must be copied at > > least for most of the patches then, which would make it easier to get it > > into mainline. > > > > Btw, some cosmetic changes could be made in the mainlined driver: > > > > 30s/implements/Implements/ > > 31s/data/Data/ > > 32s/data/Data/ > > 33s/the/The/ > > 39s/data/Data/ > > 40s/data/Data/ > > 41s/characteristics/Characteristics/ > > 109s/response/Response/ > > 154s/implements/Implements/ > > 154s/sw\ /SW\ / > > 155s/support/Support/ > > 155s/api/API/ > > 156s/assosiated/Associated/ > > 157s/node\ /Node\ / > > 158s/transmit/Transmit/ > > 259s/Hw/HW/ > > 291s/Hw/HW/ > > 322s/prepare/Prepare/ > > 327s/transfer/Transfer/ > > 378s/prepare/Prepare/ > > 384s/transfer/Transfer/ > > 385s/transfer/Transfer/ > > 502s/its/it\'s/ > > 514s/oder/order/ > > 530s/copy\ /Copy\ / > > 680s/sSGDMA/mSGDMA/ > > 723s/Interrupt/interrupt/ > > 752s/\(\)// > > 921s/\(\)// > > > > ... and another patch, if that is taken into account. > > > > Cheers > > Eric
diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c index 4153c2edb049..c39937bfcdf1 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); @@ -583,22 +583,25 @@ 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; + + spin_lock_irqsave(&mdev->lock, irqflags); 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(&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 */ msgdma_free_descriptor(mdev, desc); } + + spin_unlock_irqrestore(&mdev->lock, irqflags); } /** @@ -713,10 +716,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); } /**
Sparse complains because we first take the lock in msgdma_tasklet -> move locking to msgdma_chan_desc_cleanup. In consequence, move calling of msgdma_chan_desc_cleanup outside of the critical section of function msgdma_tasklet. Use spin_unlock_irqsave/restore instead of just spinlock/unlock to keep state of irqs while executing the callbacks. 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__ seems to free correctly the descriptors as firsts nodes where not removed from the specified list. Signed-off-by: Olivier Dautricourt <olivierdautricourt@gmail.com> --- Following Eric Schwarz comments on altera-msgdma driver not having some of the fixes made to zynqmp-dma driver (which msgdma driver is based on): This patch should address at least the spinlock part, it __has not__ been tested yet so please don't accept it right away. I'm in the process of getting a new hardware to test with. Meanwhile it is open to reviews and even better if someone is able to test it. drivers/dma/altera-msgdma.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)