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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Returning -EAGAIN from `prepare` callback causes PM core busy-wait livelock
- [High] Race condition with non-cyclic channels during system suspend
- [High] Spurious timeouts on resume for short non-cyclic DMA transfers
- [High] Data loss for cyclic channels due to incomplete context restoration
- [Low] Cleanup framework violation by mixing goto-based error handling and scope-based guard()
- [Low] PM callback conditional compilation subsystem guide violation
--

commit 536ec6e9c07fa47423de7d20ebc829e45cbcedfb
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Subject: dmaengine: sh: rz-dmac: Add suspend to RAM support

This commit adds suspend to RAM support for the Renesas RZ/G3S SoC by
saving and restoring the DMA IP registers. It adds specific suspend and
resume handling for cyclic channels, and waits for non-cyclic channels
to finish before suspending.

> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index df91657fd5e3d..b0a8590f4a9da 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c
[ ... ]
> @@ -1002,20 +1008,57 @@ static int rz_dmac_device_pause(struct dma_chan *chan)
>  	return rz_dmac_device_pause_set(channel, BIT(RZ_DMAC_CHAN_STATUS_PAUSED));
>  }
>  
[ ... ]
>  static int rz_dmac_device_resume_set(struct rz_dmac_chan *channel,
>  				     unsigned long clear_bitmask)
>  {
> -	int ret = 0;
>  	u32 val;
> +	int ret;
>  
>  	lockdep_assert_held(&channel->vc.lock);
>  
[ ... ]
> +	rz_dmac_ch_writel(channel, CHCTRL_CLRSUS | CHCTRL_SETEN, CHCTRL, 1);
>  
> -	rz_dmac_ch_writel(channel, CHCTRL_CLRSUS, CHCTRL, 1);
>  	ret = read_poll_timeout_atomic(rz_dmac_ch_readl, val,
> -				       !(val & CHSTAT_SUS), 1, 1024, false,
> -				       channel, CHSTAT, 1);
> +				       ((val & (CHSTAT_SUS | CHSTAT_EN)) == CHSTAT_EN),
> +				       1, 1024, false, channel, CHSTAT, 1);

Can this condition time out spuriously for short non-cyclic DMA transfers? If
a transfer finishes successfully before the first poll read, the hardware
would clear CHSTAT_EN to 0. The polling condition would then evaluate to
false, causing the macro to incorrectly return -ETIMEDOUT.

>  
>  	channel->status &= ~clear_bitmask;
>  
[ ... ]
> @@ -1406,6 +1459,151 @@ 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);
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return 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);
> +
> +		/* 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;

Could returning -EAGAIN here cause a CPU livelock? In dpm_prepare(), an
-EAGAIN error is cleared and retried continuously in a while
(!list_empty(&dpm_list)) loop. Since the device is not advanced on error,
the PM core might loop infinitely without any backoff until the transfer
completes.

Also, is there a race condition with non-cyclic channels after this callback
returns 0? Since the DMA controller's prepare callback runs before consumer
peripherals, an active consumer could immediately submit new non-cyclic
transfers before the actual suspend phase begins.

> +	}
> +
> +	return 0;
> +}
> +
[ ... ]
> +static int rz_dmac_suspend(struct device *dev)
> +{
> +	struct rz_dmac *dmac = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	for (unsigned int i = 0; i < dmac->n_channels; i++) {
> +		struct rz_dmac_chan *channel = &dmac->channels[i];
> +
> +		guard(spinlock_irqsave)(&channel->vc.lock);

This isn't a bug, but does this function mix scope-based guard() cleanup
with goto-based error handling? The cleanup guidelines suggest that
functions should either convert all resources to scope-based cleanup or
convert none of them to avoid confusing ownership semantics.

> +
> +		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);

Does saving only the NXLA register result in data loss for cyclic channels?
When a cyclic channel is paused, it stops at its current descriptor. Without
saving and restoring the Current Link Address and Current Transfer Byte
count, resuming the channel might force it to skip the remaining bytes of the
interrupted descriptor and start fetching the next one.

> +	}
> +
> +	if (ret)
> +		goto suspend_recover;
> +
> +	ret = reset_control_assert(dmac->rstc);

Since non-cyclic channels are unconditionally skipped in the loop above and
the hardware reset is asserted here, could newly queued non-cyclic requests
from consumers be violently interrupted mid-transfer?

> +	if (ret)
> +		goto suspend_recover;
> +
> +	ret = pm_runtime_put_sync(dev);
> +	if (ret < 0)
> +		goto reset_deassert;
> +
> +	return 0;
> +
> +reset_deassert:
> +	reset_control_deassert(dmac->rstc);
> +suspend_recover:
> +	rz_dmac_suspend_recover(dmac);
> +	return ret;
> +}
> +
[ ... ]
> +static const struct dev_pm_ops rz_dmac_pm_ops = {
> +	.prepare = rz_dmac_suspend_prepare,

This isn't a bug, but should rz_dmac_suspend_prepare be wrapped in
pm_sleep_ptr() here? The power management subsystem guidelines recommend
using pm_sleep_ptr() for individual callbacks within the ops structure to
prevent dead code and unused variable warnings when CONFIG_PM_SLEEP is
disabled.

> +	SYSTEM_SLEEP_PM_OPS(rz_dmac_suspend, rz_dmac_resume)
> +};
> +
>  static const struct rz_dmac_info rz_dmac_v2h_info = {
>  	.icu_register_dma_req = rzv2h_icu_register_dma_req,
>  	.default_dma_req_no = RZV2H_ICU_DMAC_REQ_NO_DEFAULT,
> @@ -1432,6 +1630,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),

This isn't a bug, but should pm_ptr() be used for the dev_pm_ops structure
pointer instead of pm_sleep_ptr()? The power management guidelines recommend
using pm_ptr() for the structure pointer, while reserving pm_sleep_ptr() for
the individual sleep callbacks inside the structure.

>  	},
>  	.probe		= rz_dmac_probe,
>  	.remove		= rz_dmac_remove,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512121219.216159-1-claudiu.beznea.uj@bp.renesas.com?part=14