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
