sashiko.dev review (Re: [PATCH v4 01/17] dmaengine: sh: rz-dmac: Move interrupt request after everything is set up)

[Claudiu Beznea]

> commit b3dfdd5fbd2463db407b1396ee1ae79801cb96bf
> Author: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> dmaengine: sh: rz-dmac: Move interrupt request after everything is set up
> This patch moves the channel interrupt request to the end of the channel
> probe function to prevent the IRQ handler from accessing uninitialized
> data. It also replaces dma_alloc_coherent() with dmam_alloc_coherent()
> to simplify the error path.
> > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> > index 625ff29024de0..9f206a33dcc6b 100644
> > --- a/drivers/dma/sh/rz-dmac.c
> > +++ b/drivers/dma/sh/rz-dmac.c
> [ ... ]
> > @@ -1210,16 +1207,6 @@ static int rz_dmac_probe(struct platform_device *pdev)
> >  dma_register_err:
> >     of_dma_controller_free(pdev->dev.of_node);
> >  err:
> > -   channel_num = i ? i - 1 : 0;
> > -   for (i = 0; i < channel_num; i++) {
> > -           struct rz_dmac_chan *channel = &dmac->channels[i];
> > -
> > -           dma_free_coherent(&pdev->dev,
> > -                             sizeof(struct rz_lmdesc) * DMAC_NR_LMDESC,
> > -                             channel->lmdesc.base,
> > -                             channel->lmdesc.base_dma);
> > -   }
> > -
> >     reset_control_assert(dmac->rstc);
> >  err_pm_runtime_put:
> >     pm_runtime_put(&pdev->dev);
> > @@ -1232,18 +1219,9 @@ static int rz_dmac_probe(struct platform_device *pdev)
> >  static void rz_dmac_remove(struct platform_device *pdev)
> >  {
> >     struct rz_dmac *dmac = platform_get_drvdata(pdev);
> > -   unsigned int i;
> >
> >     dma_async_device_unregister(&dmac->engine);
> >     of_dma_controller_free(pdev->dev.of_node);
> > -   for (i = 0; i < dmac->n_channels; i++) {
> > -           struct rz_dmac_chan *channel = &dmac->channels[i];
> > -
> > -           dma_free_coherent(&pdev->dev,
> > -                             sizeof(struct rz_lmdesc) * DMAC_NR_LMDESC,
> > -                             channel->lmdesc.base,
> > -                             channel->lmdesc.base_dma);
> > -   }
> >     reset_control_assert(dmac->rstc);
> >     pm_runtime_put(&pdev->dev);
> >     pm_runtime_disable(&pdev->dev);
> >  }
> While this patch fixes the initialization races, does it leave a similar
> vulnerability exposed during teardown?
> Since devm_request_threaded_irq() is used to allocate the interrupts, they
> will remain active until after rz_dmac_remove() and the rz_dmac_probe() error
> paths complete.
> If an interrupt fires during or just after rz_dmac_remove(), could the handler
> attempt to access hardware registers while the device is in reset or powered
> down by pm_runtime_put()?
> Would it be safer to explicitly free or disable the IRQs before asserting the
> hardware reset and disabling runtime PM, or perhaps manage the reset and PM
> states via devm actions to guarantee correct teardown ordering?

On either failure or remove path the device is with runtime PM put (clocks being 
disabled) and in reset state. The controller cannot generate interrupts from 
this state.

However, to keep this series simple, I'll add this in a cleanup patch after the 
current series will be merged.