Re: [PATCH v3] dmaengine: pl330: Fix unbalanced runtime PM

[Frank Li <Frank.li@oss.nxp.com>]

On Thu, Oct 15, 2026 at 07:58:08AM +0800, Mizuno Miotsuki wrote:
> From: Sugar Zhang <sugar.zhang@rock-chips.com>
>
> This driver use runtime PM autosuspend mechanism to manager clk.
>
>   pm_runtime_use_autosuspend(&adev->dev);
>   pm_runtime_set_autosuspend_delay(&adev->dev, PL330_AUTOSUSPEND_DELAY);
>
> So, after ref count reached to zero, it will enter suspend
> after the delay time elapsed.
>
> The unbalanced PM:
>
> * May cause dmac the next start failed.
> * May cause dmac read unexpected state.
> * May cause dmac stall if power down happen at the middle of the transfer.
>   e.g. may lose ack from AXI bus and stall.
>
> Considering the following situation:
>
>       DMA TERMINATE               TASKLET ROUTINE
>             |                            |
>             |                       issue_pending
>             |                            |
>             |                     pch->active = true
>             |                       pm_runtime_get

Look like need lock between terminate and issue_pendings. There are should
other problems if interfere each others.

terminate should wait issue_pending finish and block queue new request.

And

https://docs.kernel.org/driver-api/dmaengine/provider.html

device_issue_pending

Takes the first transaction descriptor in the pending queue, and starts the
transfer. Whenever that transfer is done, it should move to the next
transaction in the list.

This function can be called in an interrupt context

call pm_runtime_get() is risk, which may sleep.

Frank


>   pm_runtime_put(if active)              |
>     pch->active = false                  |
>             |                      work_list empty
>             |                            |
>             |                     pm_runtime_put(unconditional)
>             |                            |
>
> At this point, it's unbalanced(1 get / 2 put).
>
> After this patch:
>
>       DMA TERMINATE               TASKLET ROUTINE
>             |                            |
>             |                       issue_pending
>             |                            |
>             |                     pch->active = true
>             |                       pm_runtime_get
>   pm_runtime_put(if active)              |
>     pch->active = false                  |
>             |                      work_list empty
>             |                            |
>             |                   pm_runtime_put(if active)
>             |                            |
>
> Now, it's balanced(1 get / 1 put).
>
> Fixes: 5c9e6c2b2ba3 ("dmaengine: pl330: Fix runtime PM support for terminated transfers")
> Fixes: ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
>
> Link: https://lore.kernel.org/r/1650450456-34351-1-git-send-email-sugar.zhang@rock-chips.com
> Signed-off-by: Huibin Hong <huibin.hong@rock-chips.com>
> Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> Signed-off-by: Mizuno Miotsuki <founder@crepuse.com>
> ---
> Hi Vinod, Sugar and Huibin,
>
> I am forwarding Sugar's April 2022 v2 as v3, with the same one-line
> change, original authorship and original Signed-off-by chain. The
> original submission is linked in the commit message. I found no mainline
> merge or revert of this change; current dmaengine fixes, next and master
> still use the unconditional assignment.
>
> Our SaneBSP investigation on an RK3308B UNIHIKER M10 independently
> rediscovered the same accounting issue. The experimental G-only kernel
> logged:
>
>   dma-pl330 ff2c0000.dma-controller: Runtime PM usage count underflow!
>
> The subsequent H kernel, which included this gate, did not report the
> warning during a four-rate 62.625 MiB qualification and a bounded serdev
> regression. H also contains other changes, and G/H were experimental
> kernels. This is additional runtime evidence, not an isolated hardware
> proof of this one-line fix on upstream. None of the experimental cyclic
> DMA modifications is included here.
>
> A source-anchored reference-count model reproduces the extra put after
> termination in the old path and balances the active-gated path, including
> cyclic requeue. That is a model, not a compiled-kernel race reproducer.
>
> Validation of this rebased patch:
> - ARM64 W=1 drivers/dma/pl330.o build on dmaengine/fixes.
> - Strict checkpatch, including the two historical Fixes IDs.
> - Clean application on current dmaengine fixes/next/master and mainline.
>
> Would you reconsider the outstanding patch? Please let us know if there
> was an objection or an alternative fix outside the archived discussion.
>
> Changes in v3:
> - Rebase the unchanged code change onto dmaengine/fixes, Linux 7.3-rc4.
> - Add an archival Link to the original v2.
> - Preserve the original author, commit explanation, Fixes and DCO trailers.
> - Add the bounded new evidence and validation here, outside the commit.
>
> Changes in v2 (Sugar, 2022):
> - Update the commit message following Vinod's suggestions.
>
> Regards,
> Mizuno Miotsuki
> ---
>  drivers/dma/pl330.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 6214d9000..17264a05d 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2090,7 +2090,7 @@ static void pl330_tasklet(struct tasklet_struct *t)
>  		spin_lock(&pch->thread->dmac->lock);
>  		_stop(pch->thread);
>  		spin_unlock(&pch->thread->dmac->lock);
> -		power_down = true;
> +		power_down = pch->active;
>  		pch->active = false;
>  	} else {
>  		/* Make sure the PL330 Channel thread is active */
>
> ---
> base-commit: 93f51579e7df248780214094418f205253383cc5
> change-id: 20261015-review-pl330-pm-original-v3-be881d29c3bf
>
> Best regards,
> --
> Mizuno Miotsuki <founder@crepuse.com>
>