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
