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

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

 > commit e7af08b0f2dc569cc63b17e07dd4e506e50fee28
 > Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
 > 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.