Message-ID: <ZhSxgAxCSeekTdNT@freebase>
Date: Tue, 9 Apr 2024 05:09:52 +0200
From: Olivier Dautricourt <olivierdautricourt@...il.com>
To: Eric Schwarz <eas@...optimization.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

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

Powered by blists - more mailing lists