Re: [PATCH v6 15/18] dmaengine: sh: rz-dmac: Add suspend to RAM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The error recovery path in `rz_dmac_suspend()` fails to restore essential hardware registers before re-enabling cyclic channels.
- [High] Error paths in system suspend and resume callbacks fail to properly manage the runtime PM usage counter, causing an imbalance that leads to underflows.
- [High] The IRQ handler accesses hardware registers without verifying the device power state, risking a crash if an interrupt fires during or after system suspend.
- [Medium] The `rz_dmac_suspend()` function incorrectly mixes scope-based `guard()` locking with `goto`-based error handling.
--

commit 2944b998cf54bad86273dc3ee963623d49e8fffa
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

dmaengine: sh: rz-dmac: Add suspend to RAM support

This patch adds suspend to RAM support for the Renesas RZ/G3S SoC DMA IP to
save and restore DMA IP registers. It adds specific handling for cyclic
channels by pausing and resuming them during system suspend/resume.

> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index bd4ca8e939f17..2a7124e4aea3a 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c
[ ... ]
> @@ -1421,6 +1474,122 @@ static void rz_dmac_remove(struct platform_device *pdev)
[ ... ]
> +static void rz_dmac_suspend_recover(struct rz_dmac *dmac)
> +{
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return;
> +
> +	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;
> +
> +		rz_dmac_device_resume_internal(channel);

[Severity: Critical]
Does this error recovery path fail to restore essential hardware registers
before re-enabling cyclic channels?

If pm_runtime_put_sync() fails in rz_dmac_suspend(), the driver jumps to
reset_deassert, calls reset_control_deassert(), and then executes this
function. However, the prior reset_control_assert() wiped the hardware state.

This function simply calls rz_dmac_device_resume_internal(), which writes
CHCTRL_SETEN to start the channel. Since essential configuration registers
like NXLA, CHCFG, and DCTRL are never restored, the hardware could attempt to
execute DMA transfers with zeroed configurations, potentially causing wild
memory accesses or silent data corruption.

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

[Severity: Medium]
Does this function mix guard() based locking with goto based error handling?

According to the subsystem cleanup guidelines, a single function must never
mix goto statements with __free() or guard() declarations. This function uses
guard(spinlock_irqsave)(&channel->vc.lock) inside the loop, while
goto suspend_recover and goto reset_deassert are used for error handling
outside the loop.

> +
> +		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)
> +		goto suspend_recover;
> +
> +	ret = reset_control_assert(dmac->rstc);

[Severity: High]
Does this logic risk a regression if an interrupt fires during or after system
suspend?

This driver implements system suspend using SIMPLE_DEV_PM_OPS, so this
function runs during the normal suspend phase where device interrupts are
still enabled. This function asserts the hardware reset and powers down the
DMA controller. 

If a delayed interrupt fires concurrently on another CPU,
rz_dmac_irq_handle_channel() will execute and directly read the CHSTAT
register. Since the driver neither disables the IRQ, calls synchronize_irq(),
nor verifies the power state via pm_runtime_get_if_active(), the handler will
read from powered-off or reset hardware.

> +	if (ret)
> +		goto suspend_recover;
> +
> +	ret = pm_runtime_put_sync(dev);
> +	if (ret < 0)
> +		goto reset_deassert;

[Severity: High]
Does this error path fail to properly manage the runtime PM usage counter?

pm_runtime_put_sync() unconditionally decrements the usage counter. If it
fails, the function jumps to reset_deassert and aborts system suspend without
restoring the usage counter via pm_runtime_get_noresume(). 

Because the system suspend is aborted, the resume callback will be skipped,
leaving the usage counter permanently decremented. Subsequent suspends could
underflow the counter and break runtime PM functionality.

> +
> +	return 0;
> +
> +reset_deassert:
> +	reset_control_deassert(dmac->rstc);
> +suspend_recover:
> +	rz_dmac_suspend_recover(dmac);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526084710.3491480-1-claudiu.beznea@kernel.org?part=15