sashiko.dev review (Re: [PATCH v4 04/17] dmaengine: sh: rz-dmac: Use rz_dmac_disable_hw())

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

 > commit f7b3ac8bceb85f09d10e2c6b3d49ae768415c77c
 > Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
 > dmaengine: sh: rz-dmac: Use rz_dmac_disable_hw()
 > This patch unifies the hardware disable code by replacing an open-coded
 > register write with rz_dmac_disable_hw(), moving it after vchan_init()
 > so the channel device is properly initialized.
 > > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
 > > index 1717b407ab9e9..40ddf534c0948 100644
 > > --- a/drivers/dma/sh/rz-dmac.c
 > > +++ b/drivers/dma/sh/rz-dmac.c
 > [ ... ]
 > > @@ -1000,15 +1000,15 @@ static int rz_dmac_chan_probe(struct rz_dmac *dmac,
 > >     }
 > >     rz_lmdesc_setup(channel, lmdesc);
 > >
 > > -   /* Initialize register for each channel */
 > > -   rz_dmac_ch_writel(channel, CHCTRL_DEFAULT, CHCTRL, 1);
 > > -
 > >     channel->vc.desc_free = rz_dmac_virt_desc_free;
 > >     vchan_init(&channel->vc, &dmac->engine);
 > This is not a bug introduced by this patch, but seeing the vchan
 > initialization here made me wonder about the driver teardown path.
 > When vchan_init() is called, it sets up a background tasklet for the
 > channel. During driver removal in rz_dmac_remove(), the DMA device is
 > unregistered, but it does not appear that tasklet_kill() is ever called
 > for this tasklet.
 > Because the channel structures are allocated with devm_kzalloc, their
 > memory is automatically freed immediately after rz_dmac_remove() returns.
 > If a DMA transfer is active when the driver is forcefully unbound, could
 > the IRQ thread schedule the tasklet via vchan_cookie_complete() right
 > before the IRQ is freed by devres?
 > Since free_irq() waits for the IRQ thread to finish but does not flush
 > tasklets, could this scheduled tasklet execute after the device memory
 > has been freed, resulting in a use-after-free?
 > Should the driver explicitly kill the tasklet before returning from
 > rz_dmac_remove()?
 > >     INIT_LIST_HEAD(&channel->ld_queue);
 > >     INIT_LIST_HEAD(&channel->ld_free);
 > >     INIT_LIST_HEAD(&channel->ld_active);

tasklet_setup() only sets the members of a struct tasklet object. The tasklet 
itself is scheduled at runtime when the DMA clients requests transfers. On 
remove the DMA clients would have already been called the 
rz_dmac_terminate_all() and/or rz_dmac_device_synchronize() which should kill 
the previously scheduled tasklet.