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

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

On 4/20/26 17:21, Biju Das wrote:
> Hi Claudiu,
> 
>> -----Original Message-----
>> From: Claudiu Beznea <claudiu.beznea@tuxon.dev>
>> 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.

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.

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.

Thank you,
Claudiu