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

[Claudiu Beznea]

> commit f7b3ac8bceb85f09d10e2c6b3d49ae768415c77c
> Author: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> 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.