From: Olivier Dautricourt <olivierdautricourt@gmail.com>
To: Eric Schwarz <eas@sw-optimization.com>
Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
Vinod Koul <vkoul@kernel.org>, Stefan Roese <sr@denx.de>
Subject: Re: [PATCH] dmaengine: altera-msgdma: fix descriptors freeing logic
Date: Tue, 9 Apr 2024 05:09:52 +0200 [thread overview]
Message-ID: <ZhSxgAxCSeekTdNT@freebase> (raw)
In-Reply-To: <245a848c-5bbc-463d-b7e1-b82cea2c4dba@sw-optimization.com>
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
prev parent reply other threads:[~2024-04-09 3:09 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-20 19:58 [PATCH] dmaengine: altera-msgdma: fix descriptors freeing logic Olivier Dautricourt
2023-09-21 13:07 ` Eric Schwarz
2023-09-21 19:17 ` Olivier Dautricourt
2023-09-22 7:49 ` Eric Schwarz
2023-09-22 16:33 ` Olivier Dautricourt
2023-09-28 7:57 ` Eric Schwarz
2024-02-25 20:05 ` Eric Schwarz
2024-04-09 3:09 ` Olivier Dautricourt [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZhSxgAxCSeekTdNT@freebase \
--to=olivierdautricourt@gmail.com \
--cc=dmaengine@vger.kernel.org \
--cc=eas@sw-optimization.com \
--cc=linux-kernel@vger.kernel.org \
--cc=sr@denx.de \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).