RE: [PATCH v4 14/17] dmaengine: sh: rz-dmac: Add suspend to RAM support

[Biju Das]

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@xxxxxxxxx>
> Sent: 20 April 2026 15:37
> Subject: Re: [PATCH v4 14/17] dmaengine: sh: rz-dmac: Add suspend to RAM support
> 
> 
> 
> On 4/20/26 17:21, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu Beznea <claudiu.beznea@xxxxxxxxx>
> >> Sent: 20 April 2026 15:15
> >> Subject: Re: [PATCH v4 14/17] dmaengine: sh: rz-dmac: Add suspend to
> >> RAM support
> >>
> >>
> >>
> >> On 4/20/26 10:42, Biju Das wrote:
> >>>> +static int rz_dmac_suspend(struct device *dev) {
> >>>> +	struct rz_dmac *dmac = dev_get_drvdata(dev);
> >>>> +	int ret;
> >>>> +
> >>>> +	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) {
> >>>> +		rz_dmac_suspend_recover(dmac);
> >>>> +		return ret;
> >>>> +	}
> >>>> +
> >>>> +	pm_runtime_put_sync(dmac->dev);
> >>>> +
> >>>> +	ret = reset_control_assert(dmac->rstc);
> >>>> +	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);
> >>>> +	if (ret)
> >>>> +		return ret;
> >>>> +
> >>>> +	ret = pm_runtime_resume_and_get(dmac->dev);
> >>>
> >>> If this fails for any reason, the next suspend still be called and
> >>> it will decrement the counter,
> >> potentially undeflowing it.
> >>> Consider switching to pm_runtime_get_sync(), which suits better here
> >>
> >>
> >> I think runtime PM usage counter underflow will be the less
> >> significant problem in case runtime PM fails.
> >>
> >> Anyhow, could you please provide the code pattern you consider would
> >> be better for both suspend and resume?
> >
> >
> > system_resume()
> > {
> >            pm_runtime_resume_and_get() --> PM counter is not
> > incremented in case of error }
> >
> > system_suspend()
> > {
> >         pm_runtime_put() --> counter is decremented and prints a noisy
> > WARN message }
> >
> > Just replace pm_runtime_resume_and_get()->pm_runtime_get_sync()
> > this will return the error to caller like previously and also
> > increment the counter which avoids warning on the subsequent suspend()
> 
> This wouldn't solve anything.

It basically avoids printing the messasge from [1]
dev_warn(dev, "Runtime PM usage count underflow!\n");

[1] https://elixir.bootlin.com/linux/v7.0-rc7/source/drivers/base/power/runtime.c#L1094

> 
> If the newly added pm_runtime_get_sync() fails the next dev_pm_ops::prepare() call, accesses DMA IP
> registers. That will sync abort (due to MSTOP) even before any warning (I guess underflow runtime PM
> usage counter) will be printed.

This can happen with current patch as well as both the api's reports same error to caller
and the clock is not turned on by the clock framework and accessing clk register will lead
to undesired effects.

> 
> If we add runtime PM resumes in the dev_pm_ops::prepare() to overcome part of the sync abort in the
> next dev_pm_ops::prepare() call and keep
> pm_runtime_get_sync() blindly, w/o dropping the usage counter on failure, that will still lead to sync
> aborts, because the runtime PM resumes in
> dev_pm_ops::prepare() should only increase the runtime PM ref counter and return success.

How is this behaviour different from current patch see [2]? 

[2] https://elixir.bootlin.com/linux/v7.0-rc7/source/drivers/base/power/runtime.c#L1093

Cheers,
Biju