Re: [PATCH v6 14/18] dmaengine: sh: rz-dmac: Add runtime PM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Early return on PM resume failure in rz_dmac_issue_pending() leaves descriptors in the submitted list, causing incorrect residue reporting.
- [Low] Misplaced exclamation mark in error message formatting (\n!).

Pre-existing issues:
- [High] Asynchronous pm_runtime_put() immediately followed by pm_runtime_disable() leaves hardware in an incorrect state.
- [High] IRQ handlers access hardware registers without verifying that the device is runtime active.
- [High] Race condition between devm-managed IRQs and device removal leads to hardware register access while the device is in reset or suspended.
--

commit 1bdb517f551fc047d57449a7201275740a491091
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Subject: dmaengine: sh: rz-dmac: Add runtime PM support

This patch protects exposed APIs with runtime PM calls before accessing
hardware registers. It also marks the DMA device as pm_runtime_irq_safe()
to support APIs called from atomic context. Error handling paths in
termination and cleanup functions are updated to skip hardware access if
RPM resume fails.

> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> index 93394b9934c89..bd4ca8e939f17 100644
> --- a/drivers/dma/sh/rz-dmac.c
> +++ b/drivers/dma/sh/rz-dmac.c
> @@ -549,12 +549,22 @@ static void rz_dmac_free_chan_resources(struct dma_chan *chan)
>  	struct rz_dmac *dmac = to_rz_dmac(chan->device);
>  	struct rz_dmac_desc *desc, *_desc;
>  	unsigned long flags;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret) {
> +		dev_err(dmac->dev, "RPM resume failed for channel %s, ret=%d\n!",
> +			dma_chan_name(chan), ret);
> +	}

[Severity: Low]
Does this error message format string place the exclamation mark correctly?

The "\n!" sequence places the exclamation mark at the beginning of the next
line instead of the end of the sentence, breaking the intended log output.

[ ... ]

> @@ -716,13 +737,20 @@ static int rz_dmac_terminate_all(struct dma_chan *chan)
>  	spin_unlock_irqrestore(&channel->vc.lock, flags);
>  	vchan_dma_desc_free_list(&channel->vc, &head);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void rz_dmac_issue_pending(struct dma_chan *chan)
>  {
>  	struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> +	struct rz_dmac *dmac = to_rz_dmac(chan->device);
>  	unsigned long flags;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dmac->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return;

[Severity: High]
If this returns early on failure, does it leave newly submitted descriptors
stuck in the vc.desc_submitted list?

If vchan_issue_pending() is skipped, descriptors are never moved to the
vc.desc_issued list. Later, if a client calls dmaengine_tx_status(),
rz_dmac_chan_get_residue() will use vchan_find_desc() which only checks
the desc_issued list:

drivers/dma/virt-dma.c:vchan_find_desc() {
	list_for_each_entry(vd, &vc->desc_issued, node)
		if (vd->tx.cookie == cookie)
			return vd;
}

This will fail to find the pending descriptor, causing
rz_dmac_chan_get_residue() to incorrectly return 0 for the residue.

This could mislead the client into thinking the transfer has fully completed
while the status remains DMA_IN_PROGRESS. Should vchan_issue_pending()
still be called to update the software state correctly?

[ ... ]

> @@ -1274,6 +1327,7 @@ static int rz_dmac_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(dmac->rstc),
>  				     "failed to get resets\n");
>  
> +	pm_runtime_irq_safe(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but does adding pm_runtime_irq_safe() expose
the interrupt handlers to accessing powered-down hardware?

The IRQ handler rz_dmac_irq_handle_channel() directly reads hardware
registers like rz_dmac_ch_readl().

If a spurious or delayed interrupt occurs while the device is runtime
suspended (e.g., after a failed system suspend or during driver teardown),
it will access hardware without verifying it is powered, resulting in a
kernel crash.

Should IRQ handlers use pm_runtime_get_if_active() to verify the device is
powered before touching registers?

[Severity: High]
This is also a pre-existing issue, but is there a race condition between
the devm-managed IRQs and device removal?

The driver requests channel IRQs using devm_request_threaded_irq(). During
module removal in rz_dmac_remove(), it asserts the hardware reset and
suspends the device:

drivers/dma/sh/rz-dmac.c:rz_dmac_remove() {
	...
	reset_control_assert(dmac->rstc);
	pm_runtime_put(&pdev->dev);
	pm_runtime_disable(&pdev->dev);
}

Because devm-managed resources are freed only after the remove() function
returns, an in-flight interrupt can execute concurrently with or immediately
after the reset assertion and suspension.

The interrupt handler will attempt to read hardware registers while the
controller is held in reset or suspended, leading to a system hang or
synchronous abort.

Should synchronize_irq() be called, or the IRQs manually freed, before
asserting the hardware reset and disabling runtime PM?

[Severity: High]
This isn't a bug introduced by this patch, but does the error handling in
rz_dmac_probe() and rz_dmac_remove() leave the hardware in an incorrect state?

In rz_dmac_probe() error paths and rz_dmac_remove(), the driver calls
pm_runtime_put() immediately followed by pm_runtime_disable():

drivers/dma/sh/rz-dmac.c:rz_dmac_probe() {
	...
err_pm_runtime_put:
	pm_runtime_put(&pdev->dev);
err_pm_disable:
	pm_runtime_disable(&pdev->dev);
}

Calling pm_runtime_put() queues an async idle notification, but
pm_runtime_disable() invokes __pm_runtime_barrier() which cancels any
pending asynchronous requests. This causes the device to never enter the
runtime suspended state, leaving hardware resources powered on.

Should pm_runtime_put_sync() be used instead?

>  	pm_runtime_enable(&pdev->dev);
>  	ret = pm_runtime_resume_and_get(&pdev->dev);
>  	if (ret < 0) {

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