Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Fri, 22 Sep 2023 18:33:56 +0200
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
Message-ID: <ZQ3B9NWVmLvaVhJX@freebase>
References: <20230920200636.32870-3-olivierdautricourt@gmail.com>
 <22402987-305b-024b-044e-53db17037d90@sw-optimization.com>
 <ZQyWsvcQCJgmG5aO@freebase>
 <8d18106d-444e-9346-26cc-3767540df5d8@sw-optimization.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8d18106d-444e-9346-26cc-3767540df5d8@sw-optimization.com>
Precedence: bulk

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).

Kr,
Olivier

> Cheers
> Eric