Re: [PATCH] dmaengine: altera-msgdma: fix descriptors freeing logic

[Eric Schwarz <eas@sw-optimization.com>]

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