Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Thu, 21 Sep 2023 21:17:06 +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: <ZQyWsvcQCJgmG5aO@freebase>
References: <20230920200636.32870-3-olivierdautricourt@gmail.com>
 <22402987-305b-024b-044e-53db17037d90@sw-optimization.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <22402987-305b-024b-044e-53db17037d90@sw-optimization.com>
Precedence: bulk

On Thu, Sep 21, 2023 at 03:07:15PM +0200, Eric Schwarz wrote:
> Hello Olivier,
> 
> thanks for following up on my comment first. I really appreciate. - I don't
> have access to the hardware anymore, so I cannot test changes myself.
> 
> This patch addresses IMHO three fixes. - Shouldn't it be split up into three
> small junks so one could also later work w/ git bisect / separate ack's? -
> That way it is an all or nothing thing. Please regard this remark as
> cosmetics.
> 
> 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

> 
> > Remove list_del call in msgdma_chan_desc_cleanup, this should be the role
> > of msgdma_free_descriptor. In consequence replace list_add_tail with
> > list_move_tail in msgdma_free_descriptor. This fixes the path:
> > msgdma_free_chan_resources -> msgdma_free_descriptors ->
> > msgdma_free_desc_list -> msgdma_free_descriptor
> > which does __not__ seems to free correctly the descriptors as firsts nodes
> > where not removed from the specified list.
> > 
> s/__not__/_not_/
> s/seems/seem/
> s/firsts/first/ => Actually I would omit it.
> s/where/were/
> 
> "Fixes: <12 digits git hash> ("commit-message")" is missing [1] isn't it?
> 
> [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

Thank you for your remarks/corrections, i will take them into account
in next version of the patch.

Kr,

Olivier Dautricourt