Re: [PATCH v7 4/4] dmaengine: fsl-edma: add runtime suspend/resume support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] dmaengine: fsl-edma: unclocked register read in fsl_edma3_err_check()
- [Critical] dmaengine: fsl-edma: NULL dereference on masked channels in IRQ handlers
- [High] dmaengine: fsl-edma: missing CR restore in fsl_edma_resume_early()
- [High] dmaengine: fsl-edma: clocks disabled and IRQs broken when CONFIG_PM=n
- [High] dmaengine: fsl-edma: UAF on pd_dev_link in fsl_edma3_detach_pd()
- [High] dmaengine: fsl-edma: DMA registration before setup_irq() and channel PM
- [High] dmaengine: fsl-edma: broken channel allocation on mcf-edma with CONFIG_PM=y
- [High] dmaengine: fsl-edma: use-after-free on pd_dev in fsl_edma3_detach_pd()
- [Medium] dmaengine: fsl-edma: async runtime PM put in fsl_edma_probe() leaks clocks
- [Medium] dmaengine: fsl-edma: missing txirq_16_31 sync in channel runtime suspend
- [Medium] dmaengine: fsl-edma: active channels with srcid 0 skipped in suspend/resume
- [Medium] dmaengine: fsl-edma: storm in tx/err IRQ handlers when channel is suspending
--

commit 9e2928aa0a1257007f05ec7ae65bb12d57332723
Author: Joy Zou <joy.zou@oss.nxp.com>

dmaengine: fsl-edma: add runtime suspend/resume support

This commit adds runtime PM support for the FSL eDMA engine and its
per-channel devices, managing engine and channel clocks as well as
per-channel power domains through runtime suspend and resume callbacks.

> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
> index 4f7a942673ebc..4ab56294ac776 100644
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c

[ ... ]

> @@ -1087,7 +1084,12 @@ int fsl_edma_alloc_chan_resources(struct dma_chan *chan)
>  	struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
>  	int ret = 0;
>  
> -	clk_prepare_enable(fsl_chan->clk);
> +	ret = pm_runtime_resume_and_get(&fsl_chan->vchan.chan.dev->device);
> +	if (ret < 0) {
> +		dev_err(&fsl_chan->vchan.chan.dev->device, "Failed to resume device: %d\n", ret);
> +		return ret;
> +	}

[Severity: High]
Does this break channel allocation for the ColdFire mcf-edma driver when
CONFIG_PM is enabled?

drivers/dma/fsl-edma-common.c is shared with drivers/dma/mcf-edma-main.c,
where mcf_edma_probe() wires up fsl_edma_alloc_chan_resources() and
fsl_edma_free_chan_resources():

drivers/dma/mcf-edma-main.c:mcf_edma_probe() {
	...
	mcf_edma->dma_dev.device_alloc_chan_resources =
			fsl_edma_alloc_chan_resources;
	mcf_edma->dma_dev.device_free_chan_resources =
			fsl_edma_free_chan_resources;
	...
}

Unlike fsl_edma_probe(), mcf_edma_probe() never calls
devm_pm_runtime_enable() on the per-channel devices, so each channel device
remains with disable_depth == 1 and runtime_status == RPM_SUSPENDED. When a
client requests a channel on mcf-edma with CONFIG_PM=y,
pm_runtime_resume_and_get(&fsl_chan->vchan.chan.dev->device) in
fsl_edma_alloc_chan_resources() will fail with -EACCES.

> diff --git a/drivers/dma/fsl-edma-main.c b/drivers/dma/fsl-edma-main.c
> index be9bd9cc66306..c16280b836560 100644
> --- a/drivers/dma/fsl-edma-main.c
> +++ b/drivers/dma/fsl-edma-main.c
> @@ -37,15 +37,32 @@ static irqreturn_t fsl_edma_tx_handler(int irq, void *dev_id)
>  	unsigned int intr, ch;
>  	struct edma_regs *regs = &fsl_edma->regs;
>  
> +	/*
> +	 * A racy or spurious interrupt for an already-suspended
> +	 * controller or channel is an extremely rare corner case and
> +	 * highly unlikely in practice.
> +	 */
> +	if (pm_runtime_get_if_active(fsl_edma->dma_dev.dev) <= 0)
> +		return IRQ_NONE;

[Severity: High]
What happens here and in fsl_edma_probe() when the kernel is built with
CONFIG_PM=n?

When CONFIG_PM=n, the stub for pm_runtime_get_if_active() in
include/linux/pm_runtime.h unconditionally returns -EINVAL:

static inline int pm_runtime_get_if_active(struct device *dev)
{
	return -EINVAL;
}

That causes fsl_edma_tx_handler(), fsl_edma3_tx_handler(),
fsl_edma3_or_err_handler(), and fsl_edma_err_handler() to always return
IRQ_NONE.

In addition, fsl_edma_probe() replaces devm_clk_get_optional_enabled() and
devm_clk_bulk_get_optional_enable() with devm_clk_get_optional() and
devm_clk_bulk_get_optional(), and fsl_edma_alloc_chan_resources() replaces
clk_prepare_enable(fsl_chan->clk) with pm_runtime_resume_and_get(). Because
PM_RUNTIME_ACQUIRE_IF_ENABLED() and pm_runtime_resume_and_get() are no-ops
when CONFIG_PM=n, fsl_edma_runtime_resume() and
fsl_edma_chan_runtime_resume() never run, leaving dmaclk, muxclk, and
fsl_chan->clk disabled when accessing registers.

> +
>  	intr = edma_readl(fsl_edma, regs->intl);
> -	if (!intr)
> +	if (!intr) {
> +		pm_runtime_put(fsl_edma->dma_dev.dev);
>  		return IRQ_NONE;
> +	}
>  
> +	pm_runtime_put(fsl_edma->dma_dev.dev);
>  	for (ch = 0; ch < fsl_edma->n_chans; ch++) {
> +		struct fsl_edma_chan *fsl_chan = &fsl_edma->chans[ch];
> +
> +		if (pm_runtime_get_if_active(&fsl_chan->vchan.chan.dev->device) <= 0)
> +			continue;

[Severity: Critical]
Can this dereference a NULL pointer when dma-channel-mask is used?

In fsl_edma_probe(), channels with bits set in fsl_edma->chan_masked are
skipped before vchan_init(&fsl_chan->vchan, &fsl_edma->dma_dev):

drivers/dma/fsl-edma-main.c:fsl_edma_probe() {
	...
	for (i = 0; i < fsl_edma->n_chans; i++) {
		struct fsl_edma_chan *fsl_chan = &fsl_edma->chans[i];
		int len;

		if (fsl_edma->chan_masked & BIT(i))
			continue;
		...
		vchan_init(&fsl_chan->vchan, &fsl_edma->dma_dev);
	}
}

Because masked channels are not added to fsl_edma->dma_dev.channels,
dmaenginem_async_device_register() never allocates fsl_chan->vchan.chan.dev,
leaving it NULL.

Here in fsl_edma_tx_handler(), as well as in fsl_edma3_or_err_handler(),
fsl_edma_err_handler(), and fsl_edma3_or_tx_handler() ->
fsl_edma3_tx_handler(), the loop does not check fsl_edma->chan_masked &
BIT(ch) (or check intr & (0x1 << ch) / err & (0x1 << ch) first) before
passing &fsl_chan->vchan.chan.dev->device to pm_runtime_get_if_active().

> +
>  		if (intr & (0x1 << ch)) {
>  			edma_writeb(fsl_edma, EDMA_CINT_CINT(ch), regs->cint);
>  			fsl_edma_tx_chan_handler(&fsl_edma->chans[ch]);
>  		}
> +		pm_runtime_put(&fsl_chan->vchan.chan.dev->device);
>  	}
>  	return IRQ_HANDLED;
>  }

[Severity: Medium]
What happens in fsl_edma_tx_handler() and fsl_edma_err_handler() if an
interrupt is pending when a channel is being runtime-suspended?

When fsl_edma_free_chan_resources() calls fsl_edma_disable_request() and
then pm_runtime_put_sync_suspend(&fsl_chan->vchan.chan.dev->device),
fsl_edma_disable_request() disables future hardware requests in regs->cerq
and regs->ceei, but does not clear any pending interrupt bit in regs->intl
or regs->errl.

Inside rpm_suspend(), the runtime PM core sets
chan_dev->power.runtime_status = RPM_SUSPENDING before invoking
fsl_edma_chan_runtime_suspend(), which calls synchronize_irq(fsl_edma->txirq)
and synchronize_irq(fsl_edma->errirq). When fsl_edma_tx_handler() or
fsl_edma_err_handler() runs for that pending interrupt,
pm_runtime_get_if_active(&fsl_chan->vchan.chan.dev->device) returns 0
because the status is already RPM_SUSPENDING. The loop then skips writing
EDMA_CINT_CINT(ch) to regs->cint (or EDMA_CERR_CERR(ch) to regs->cerr) while
the handler still returns IRQ_HANDLED, leaving the level-triggered interrupt
asserted in an infinite storm.

Also, both handlers call pm_runtime_put(fsl_edma->dma_dev.dev) before the
loop even though regs->cint, regs->cerr, regs->cerq, and regs->ceei accessed
inside the loop are global controller registers clocked by dmaclk.

> @@ -132,13 +149,23 @@ static irqreturn_t fsl_edma3_tx_handler(int irq, void *dev_id)
>  	struct fsl_edma_chan *fsl_chan = dev_id;
>  	unsigned int intr;
>  
> -	intr = edma_readl_chreg(fsl_chan, ch_int);
> -	if (!intr)
> +	/*
> +	 * A racy or spurious interrupt for an already-suspended
> +	 * channel is an extremely rare corner case and highly unlikely in
> +	 * practice.
> +	 */
> +	if (pm_runtime_get_if_active(&fsl_chan->vchan.chan.dev->device) <= 0)
>  		return IRQ_NONE;

[Severity: Critical]
Were fsl_edma3_err_check(), fsl_edma3_err_handler_per_chan(), and
fsl_edma3_err_handler_shared() missed when adding runtime PM checks to the
interrupt handlers?

Just above fsl_edma3_tx_handler(), fsl_edma3_err_handler_shared() loops over
all unmasked channels and calls fsl_edma3_err_check(), and
fsl_edma3_err_handler_per_chan() (registered with IRQF_SHARED) also calls
fsl_edma3_err_check():

drivers/dma/fsl-edma-main.c:fsl_edma3_err_check() {
	...
	scoped_guard(spinlock, &fsl_chan->vchan.lock) {
		ch_err = edma_readl_chreg(fsl_chan, ch_es);
		if (!(ch_err & EDMA_V3_CH_ERR))
			return;
	...
}

drivers/dma/fsl-edma-main.c:fsl_edma3_err_handler_shared() {
	...
	for (ch = 0; ch < fsl_edma->n_chans; ch++) {
		if (fsl_edma->chan_masked & BIT(ch))
			continue;

		fsl_edma3_err_check(&fsl_edma->chans[ch]);
	}
	...
}

When idle channels are runtime-suspended with their channel clocks disabled
(or their per-channel power domains powered off on platforms with
FSL_EDMA_DRV_HAS_PD), an error interrupt on any active channel causes
fsl_edma3_err_handler_shared() -> fsl_edma3_err_check() to read ch_es on the
suspended channels without calling pm_runtime_get_if_active(), triggering a
bus hang or synchronous external abort.

[ ... ]

> @@ -630,11 +691,12 @@ static void fsl_edma3_detach_pd(struct fsl_edma_engine *fsl_edma)
>  		if (fsl_edma->chan_masked & BIT(i))
>  			continue;
>  		fsl_chan = &fsl_edma->chans[i];
> -		if (fsl_chan->pd_dev_link)
> +		if (fsl_chan->pd_dev_link) {
>  			device_link_del(fsl_chan->pd_dev_link);
> +			fsl_chan->pd_dev_link = NULL;
> +		}

[Severity: High]
Can device_link_del(fsl_chan->pd_dev_link) cause a use-after-free during
devres teardown?

In fsl_edma_probe(), devm_add_action_or_reset(&pdev->dev,
devm_fsl_edma3_detach_pd, fsl_edma) is registered before
dmaenginem_async_device_register(&fsl_edma->dma_dev), and
fsl_chan->pd_dev_link is now created with chan_dev
(&fsl_chan->vchan.chan.dev->device) as the consumer.

When probe fails after creating pd_dev_link or when the driver is unbound,
devres unwinds in reverse order:
1. dmaenginem_async_device_unregister() runs first and calls
   device_unregister(chan_dev) -> device_del(chan_dev) ->
   device_links_purge(chan_dev), which deletes and frees
   fsl_chan->pd_dev_link.
2. devm_fsl_edma3_detach_pd() -> fsl_edma3_detach_pd() runs next and calls
   device_link_del(fsl_chan->pd_dev_link) on the already-freed device_link.

>  		if (fsl_chan->pd_dev) {
>  			dev_pm_domain_detach(fsl_chan->pd_dev, false);
> -			pm_runtime_dont_use_autosuspend(fsl_chan->pd_dev);
>  			pm_runtime_set_suspended(fsl_chan->pd_dev);
>  		}
>  	}

[Severity: High]
Does pm_runtime_set_suspended(fsl_chan->pd_dev) dereference fsl_chan->pd_dev
after it has already been freed by dev_pm_domain_detach()?

Because device_link_add() was moved out of fsl_edma3_attach_pd() into a
later loop in fsl_edma_probe(), if fsl_edma3_attach_pd() fails midway or if
fsl_edma_probe() fails before device_link_add() runs for a channel,
fsl_chan->pd_dev holds only the single initial reference from
dev_pm_domain_attach_by_id(). Calling dev_pm_domain_detach(fsl_chan->pd_dev,
false) invokes genpd_dev_pm_detach() -> device_unregister(fsl_chan->pd_dev),
which drops the last reference and frees fsl_chan->pd_dev before
pm_runtime_set_suspended(fsl_chan->pd_dev) accesses it.

[ ... ]

> @@ -689,6 +736,42 @@ static int fsl_edma3_attach_pd(struct platform_device *pdev, struct fsl_edma_eng
>  	return -EINVAL;
>  }
>  
> +/* Per channel dma power domain */
> +static int fsl_edma_chan_runtime_suspend(struct device *dev)
> +{
> +	struct fsl_edma_chan *fsl_chan = dev_get_drvdata(dev);
> +	struct fsl_edma_engine *fsl_edma = fsl_chan->edma;
> +
> +	if (fsl_edma->txirq > 0)
> +		synchronize_irq(fsl_edma->txirq);
> +
> +	if (fsl_edma->errirq > 0)
> +		synchronize_irq(fsl_edma->errirq);
> +
> +	if (fsl_chan->txirq > 0)
> +		synchronize_irq(fsl_chan->txirq);
> +
> +	if (fsl_chan->errirq > 0)
> +		synchronize_irq(fsl_chan->errirq);
> +
> +	clk_disable_unprepare(fsl_chan->clk);
> +
> +	return 0;
> +}

[Severity: Medium]
Should fsl_edma_chan_runtime_suspend() also synchronize
fsl_edma->txirq_16_31 (and should fsl_edma_probe() initialize
fsl_edma->txirq_16_31 = -EINVAL)?

On platforms using fsl_edma3_or_irq_init() (such as nxp,s32g2-edma),
channels 16..31 are serviced by fsl_edma->txirq_16_31 via
fsl_edma3_tx_16_31_handler(), which is not synchronized here before
disabling fsl_chan->clk.

[ ... ]

> @@ -870,11 +945,69 @@ static int fsl_edma_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, fsl_edma);
>  
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Can't enable eDMA engine PM runtime!\n");
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(&pdev->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to resume eDMA engine!\n");
> +
>  	ret = dmaenginem_async_device_register(&fsl_edma->dma_dev);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret,
>  				     "Can't register Freescale eDMA engine.\n");
>  
> +	for (i = 0; i < fsl_edma->n_chans; i++) {
> +		struct fsl_edma_chan *fsl_chan = &fsl_edma->chans[i];
> +		struct device *chan_dev;
> +
> +		if (fsl_edma->chan_masked & BIT(i))
> +			continue;
> +
> +		chan_dev = &fsl_chan->vchan.chan.dev->device;
> +		dev_set_drvdata(chan_dev, fsl_chan);
> +		dev_pm_domain_set(chan_dev, &fsl_edma_chan_pm_domain);
> +
> +		if (fsl_chan->pd_dev) {
> +			fsl_chan->pd_dev_link = device_link_add(chan_dev, fsl_chan->pd_dev,
> +								DL_FLAG_STATELESS |
> +								DL_FLAG_PM_RUNTIME);
> +			if (!fsl_chan->pd_dev_link) {
> +				return dev_err_probe(&pdev->dev, -EINVAL,
> +						     "Failed to add device_link to %d!\n", i);
> +			}
> +		}
> +		ret = devm_pm_runtime_enable(chan_dev);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "Can't enable eDMA channel PM runtime!\n");
> +
> +		if (fsl_chan->pd_dev) {
> +			ret = pm_runtime_resume_and_get(fsl_chan->pd_dev);
> +			if (ret)
> +				return dev_err_probe(&pdev->dev, ret,
> +						     "Failed to power on eDMA channel %d!\n",
> +						     fsl_chan->vchan.chan.chan_id);
> +		}
> +
> +		PM_RUNTIME_ACQUIRE_IF_ENABLED(chan_dev, pm);
> +		ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +		if (ret) {
> +			if (fsl_chan->pd_dev)
> +				pm_runtime_put_sync_suspend(fsl_chan->pd_dev);
> +
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "Failed to resume eDMA channel %d!\n",
> +					     fsl_chan->vchan.chan.chan_id);
> +		}
> +		edma_write_tcdreg(fsl_chan, cpu_to_le32(0), csr);
> +		fsl_edma_chan_mux(fsl_chan, 0, false);
> +		if (fsl_chan->pd_dev)
> +			pm_runtime_put_sync_suspend(fsl_chan->pd_dev);
> +	}

[Severity: Medium]
Does using PM_RUNTIME_ACQUIRE_IF_ENABLED() here leak prepared/enabled clocks
when probe fails, and prevent fsl_chan->pd_dev from suspending?

PM_RUNTIME_ACQUIRE_IF_ENABLED() releases its reference via asynchronous
pm_runtime_put() when exiting the scope. If fsl_edma_probe() returns an
error after resuming &pdev->dev or chan_dev, the guard queues an async idle
request on pm_wq, and devres teardown immediately runs pm_runtime_disable()
(from devm_pm_runtime_enable()), which calls __pm_runtime_barrier() and
cancels the pending work before fsl_edma_chan_runtime_suspend() or
fsl_edma_runtime_suspend() can disable fsl_chan->clk, fsl_edma->dmaclk, and
fsl_edma->muxclk.

In addition, inside the channel loop, PM_RUNTIME_ACQUIRE_IF_ENABLED(chan_dev,
pm) (which also shadows the outer pm variable) is still in scope and holds a
runtime PM reference on the supplier fsl_chan->pd_dev via
fsl_chan->pd_dev_link when pm_runtime_put_sync_suspend(fsl_chan->pd_dev) is
called at the end of the loop body, so pm_runtime_put_sync_suspend() on
pd_dev will fail with -EAGAIN.

> +
>  	ret = devm_of_dma_controller_register(&pdev->dev, np,
>  			drvdata->dmamuxs ? fsl_edma_xlate : fsl_edma3_xlate,
>  			fsl_edma);
> @@ -886,6 +1019,10 @@ static int fsl_edma_probe(struct platform_device *pdev)
>  	if (!(drvdata->flags & FSL_EDMA_DRV_SPLIT_REG))
>  		edma_writel(fsl_edma, EDMA_CR_ERGA | EDMA_CR_ERCA, regs->cr);
>  
> +	ret = fsl_edma->drvdata->setup_irq(pdev, fsl_edma);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to setup IRQ!\n");
> +
>  	return 0;
>  }

[Severity: High]
Is it safe to call dmaenginem_async_device_register() before enabling
per-channel runtime PM, and to call setup_irq() after both
dmaenginem_async_device_register() and devm_of_dma_controller_register()?

Once dmaenginem_async_device_register() and
devm_of_dma_controller_register() return, the DMA channels are published and
can be requested by consumers on another CPU before the channel loop has
configured chan_dev runtime PM (causing pm_runtime_resume_and_get() in
fsl_edma_alloc_chan_resources() to fail with -EACCES) or before setup_irq()
has populated fsl_chan->txirq, fsl_chan->errirq, fsl_chan->irq_handler, and
fsl_chan->errirq_handler (causing fsl_edma_alloc_chan_resources() to skip
request_irq() while fsl_edma_free_chan_resources() later calls free_irq()).

> @@ -908,6 +1045,19 @@ static int fsl_edma_suspend_late(struct device *dev)
>  		fsl_chan = &fsl_edma->chans[i];
>  		if (fsl_edma->chan_masked & BIT(i))
>  			continue;
> +
> +		/*
> +		 * Skip channel if:
> +		 * 1. Runtime PM already suspended.
> +		 * 2. Channel without power domain, and the channel source ID is zero,
> +		 * so the channel isn't assigned.
> +		 */
> +		if (pm_runtime_status_suspended(&fsl_chan->vchan.chan.dev->device) ||
> +		    (!(fsl_edma->drvdata->flags & FSL_EDMA_DRV_HAS_PD) &&
> +		     (fsl_edma->drvdata->flags & FSL_EDMA_DRV_SPLIT_REG) &&
> +		     !fsl_chan->srcid))
> +			continue;

[Severity: Medium]
Can checking !fsl_chan->srcid here and in fsl_edma_resume_early() skip
active, allocated channels?

There are two cases where an allocated, runtime-active channel on a
FSL_EDMA_DRV_SPLIT_REG controller without FSL_EDMA_DRV_HAS_PD has
fsl_chan->srcid == 0:
1. A channel used for DMA_MEMCPY via fsl_edma_prep_memcpy(), which sets
   fsl_chan->is_sw = true and leaves fsl_chan->srcid as 0.
2. Channel 0 on a controller without FSL_EDMA_DRV_HAS_CHMUX (such as
   imx93_data3), where fsl_edma3_xlate() sets
   fsl_chan->srcid = dma_spec->args[0] (the channel index 0).

Since unassigned channels are already runtime-suspended and filtered out by
pm_runtime_status_suspended(&fsl_chan->vchan.chan.dev->device), is the
extra !fsl_chan->srcid check needed?

> @@ -927,19 +1077,56 @@ static int fsl_edma_resume_early(struct device *dev)
>  {
>  	struct fsl_edma_engine *fsl_edma = dev_get_drvdata(dev);
>  	struct fsl_edma_chan *fsl_chan;
> -	struct edma_regs *regs = &fsl_edma->regs;
>  	int i;
>  
>  	for (i = 0; i < fsl_edma->n_chans; i++) {
>  		fsl_chan = &fsl_edma->chans[i];
>  		if (fsl_edma->chan_masked & BIT(i))
>  			continue;
> +
> +		if (pm_runtime_status_suspended(&fsl_chan->vchan.chan.dev->device) ||
> +		    (!(fsl_edma->drvdata->flags & FSL_EDMA_DRV_HAS_PD) &&
> +		     (fsl_edma->drvdata->flags & FSL_EDMA_DRV_SPLIT_REG) &&
> +		     !fsl_chan->srcid))
> +			continue;
> +
>  		fsl_chan->pm_state = RUNNING;
>  		edma_write_tcdreg(fsl_chan, 0, csr);
>  		if (fsl_chan->srcid != 0)
>  			fsl_edma_chan_mux(fsl_chan, fsl_chan->srcid, true);
>  	}
>  
> +	return 0;
> +}
> +
> +/* edma engine runtime system/resume */
> +static int fsl_edma_runtime_suspend(struct device *dev)
> +{
> +	struct fsl_edma_engine *fsl_edma = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable_unprepare(fsl_edma->drvdata->dmamuxs, fsl_edma->muxclk);
> +
> +	clk_disable_unprepare(fsl_edma->dmaclk);
> +
> +	return 0;
> +}
> +
> +static int fsl_edma_runtime_resume(struct device *dev)
> +{
> +	struct fsl_edma_engine *fsl_edma = dev_get_drvdata(dev);
> +	struct edma_regs *regs = &fsl_edma->regs;
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(fsl_edma->drvdata->dmamuxs, fsl_edma->muxclk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(fsl_edma->dmaclk);
> +	if (ret) {
> +		clk_bulk_disable_unprepare(fsl_edma->drvdata->dmamuxs, fsl_edma->muxclk);
> +		return ret;
> +	}
> +
>  	if (!(fsl_edma->drvdata->flags & FSL_EDMA_DRV_SPLIT_REG))
>  		edma_writel(fsl_edma, EDMA_CR_ERGA | EDMA_CR_ERCA, regs->cr);
>  

[Severity: High]
If a DMA channel is allocated across system suspend and resume on a
non-FSL_EDMA_DRV_SPLIT_REG platform (such as vf610, ls1028a, or imx7ulp),
will regs->cr still be restored on resume?

Moving the EDMA_CR_ERGA | EDMA_CR_ERCA write out of fsl_edma_resume_early()
into fsl_edma_runtime_resume() means it only runs on a runtime PM resume
transition. When any channel is allocated during system suspend, the
channel holds a runtime PM reference that keeps the parent controller
RPM_ACTIVE across suspend/resume, so fsl_edma_resume_early() runs on system
resume while fsl_edma_runtime_resume() does not.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-b4-edma-runtime-opt-mo-v7-0-418e53c5f9b7@oss.nxp.com?part=4