sashiko.dev review (Re: [PATCH v4 14/17] dmaengine: sh: rz-dmac: Add suspend to RAM support)

[Claudiu Beznea]

> commit e7af08b0f2dc569cc63b17e07dd4e506e50fee28
> Author: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> dmaengine: sh: rz-dmac: Add suspend to RAM support
> The Renesas RZ/G3S SoC supports a power saving mode in which power to most
> of the SoC components is turned off, including the DMA IP. Add suspend to
> RAM support to save and restore the DMA IP registers.
> > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> > --- a/drivers/dma/sh/rz-dmac.c
> > +++ b/drivers/dma/sh/rz-dmac.c
> [ ... ]
> > @@ -1354,6 +1407,130 @@ static void rz_dmac_remove(struct platform_device 
*pdev)
> >     pm_runtime_disable(&pdev->dev);
> >  }
> >
> > +static int rz_dmac_suspend_prepare(struct device *dev)
> > +{
> > +   struct rz_dmac *dmac = dev_get_drvdata(dev);
> > +
> > +   for (unsigned int i = 0; i < dmac->n_channels; i++) {
> > +           struct rz_dmac_chan *channel = &dmac->channels[i];
> > +
> > +           guard(spinlock_irqsave)(&channel->vc.lock);
> > +
> > +           /* Wait for transfer completion, except in cyclic case. */
> > +           if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC))
> > +                   continue;
> > +
> > +           if (rz_dmac_chan_is_enabled(channel))
> > +                   return -EAGAIN;
> Is it intended that this aborts the system suspend instead of waiting?

Returning -EAGAIN here makes the core suspend code to keep trying calling the 
prepare API of this driver. That's why it has mentioned waiting.

> The commit message states the .prepare callback "waits for all the ongoing
> transfers to complete before allowing suspend-to-RAM to proceed."
> However, returning -EAGAIN from a system sleep .prepare callback immediately
> aborts the system suspend process instead of waiting.
> Additionally, since the PM core executes the .prepare phase for all devices
> before executing any .suspend callbacks, consumer devices haven't yet run
> their own .suspend callbacks to stop their DMA transfers.
> If any normal background DMA activity is ongoing, won't this immediately
> abort the entire system suspend transition?

If DMA activity is ongoing the expectation is that it will finish and the 
suspend_prepare() will go forward.

> > +   }
> > +
> > +   return 0;
> > +}
> [ ... ]
> > +static int rz_dmac_suspend(struct device *dev)
> > +{
> > +   struct rz_dmac *dmac = dev_get_drvdata(dev);
> > +   int ret;
> Can ret be used uninitialized here?

Yes, this is a typo. I'll properly initialize this here.

> If the DMA controller has no cyclic channels configured, the loop below will
> skip every iteration due to the continue statement, leaving ret
> uninitialized.
> > +
> > +   for (unsigned int i = 0; i < dmac->n_channels; i++) {
> > +           struct rz_dmac_chan *channel = &dmac->channels[i];
> > +
> > +           guard(spinlock_irqsave)(&channel->vc.lock);
> > +
> > +           if (!(channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)))
> > +                   continue;
> > +
> > +           ret = rz_dmac_device_pause_internal(channel);
> > +           if (ret) {
> > +                   dev_err(dev, "Failed to suspend channel %s\n",
> > +                           dma_chan_name(&channel->vc.chan));
> > +                   break;
> > +           }
> > +
> > +           channel->pm_state.nxla = rz_dmac_ch_readl(channel, NXLA, 1);
> > +   }
> > +
> > +   if (ret) {
> If ret contains uninitialized stack memory that evaluates to true, could this
> trigger the error recovery path and silently abort the system suspend
> transition?
> > +           rz_dmac_suspend_recover(dmac);
> > +           return ret;
> > +   }
> > +
> > +   pm_runtime_put_sync(dmac->dev);
> > +
> > +   ret = reset_control_assert(dmac->rstc);
> Is the ordering of runtime PM and reset control inverted here compared to
> rz_dmac_remove()?

Yes, I'll adjust it.

> In rz_dmac_remove(), reset_control_assert() is called before pm_runtime_put().
> Operating on the reset control after the device's power domain is suspended
> might cause the operations to be silently ignored by the hardware or lead to
> system bus lockups.
> > +   if (ret) {
> > +           pm_runtime_resume_and_get(dmac->dev);
> > +           rz_dmac_suspend_recover(dmac);
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +static int rz_dmac_resume(struct device *dev)
> > +{
> > +   struct rz_dmac *dmac = dev_get_drvdata(dev);
> > +   int errors = 0, ret;
> > +
> > +   ret = reset_control_deassert(dmac->rstc);
> Similarly, is the ordering of reset control and runtime PM inverted here
> compared to rz_dmac_probe()?

Yes, I'll adjust it.

> In rz_dmac_probe(), pm_runtime_resume_and_get() is called before
> reset_control_deassert(). Doing this in the reverse order here means the
> reset is deasserted before the hardware power domain is enabled.
> > +   if (ret)
> > +           return ret;
> > +
> > +   ret = pm_runtime_resume_and_get(dmac->dev);
> > +   if (ret) {
> > +           reset_control_assert(dmac->rstc);
> > +           return ret;
> > +   }
> [ ... ]
> > @@ -1380,6 +1557,7 @@ static struct platform_driver rz_dmac_driver = {
> >     .driver         = {
> >             .name   = "rz-dmac",
> >             .of_match_table = of_rz_dmac_match,
> > +           .pm     = pm_sleep_ptr(&rz_dmac_pm_ops),
> Should this use pm_ptr() instead of pm_sleep_ptr()?
> If CONFIG_PM_SLEEP is disabled, pm_sleep_ptr() evaluates to NULL, but the
> struct rz_dmac_pm_ops itself and its unconditional callbacks like
> rz_dmac_suspend_prepare will remain fully compiled into the binary as unused
> dead code, potentially leading to compiler warnings.
> Using pm_ptr() for the dev_pm_ops structure pointer and pm_sleep_ptr()
> around the sleep callbacks inside the structure might resolve this.
> >     },
> >     .probe          = rz_dmac_probe,
> >     .remove         = rz_dmac_remove,

I think pm_sleep_ptr() fits better here as CONFIG_PM_SLEEP depends on 
CONFIG_SUSPEND which is the flag under which the system suspend/resume code 
resides. Also, CONFIG_PM_SLEEP selects CONFIG_PM, so CONFIG_PM_SLEEP cannot be 
enabled w/o CONFIG_PM.