Message-ID: <5e2404d4-f36c-7718-c0fc-d226aefdf2f6@sw-optimization.com>
Date:   Thu, 28 Sep 2023 09:57:23 +0200
From:   Eric Schwarz <eas@...optimization.com>
To:     Olivier Dautricourt <olivierdautricourt@...il.com>
Cc:     dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
        Vinod Koul <vkoul@...nel.org>, Stefan Roese <sr@...x.de>
Subject: Re: [PATCH] dmaengine: altera-msgdma: fix descriptors freeing logic

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

Powered by blists - more mailing lists