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 <[email protected]>
---
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);
}
/**
--
2.41.0
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 <[email protected]>
> ---
> 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