[PATCH v2] dmaengine: pl330: Fix unbalanced runtime PM

[PATCH v2] dmaengine: pl330: Fix unbalanced runtime PM

[Sugar Zhang]

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
  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")

Signed-off-by: Huibin Hong <huibin.hong@rock-chips.com>
Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>

---

Changes in v2:
- fixup commit message per Vinod suggestion.

 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 858400e..ccd430e 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2084,7 +2084,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 */
-- 
2.7.4

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

[Mizuno Miotsuki via B4 Relay]

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
  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>

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

[Frank Li]

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>
>

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

[Mizuno Miotsuki]

Hi Frank,

Thanks for the review.

> 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.

Both pl330_issue_pending() and pl330_terminate_all() already use
pch->lock to protect the channel state. However, issue_pending()
also invokes pl330_tasklet() after releasing the lock, so this
does not establish complete synchronization.

I ran a source-based interleaving test. The original
code reproduces the extra runtime PM put after termination, while
the proposed one-line fix balances the references under the same
interleaving.

> This function can be called in an interrupt context
> 
> call pm_runtime_get() is risk, which may sleep.

pl330_probe() calls pm_runtime_irq_safe(&adev->dev), and the AMBA
runtime PM callbacks use clk_enable()/clk_disable() for IRQ-safe
devices. This allows pm_runtime_get_sync() to be called from
atomic context, provided the relevant PM callbacks and dependencies
satisfy the IRQ-safe requirements.

I agree that the remaining synchronization and error-handling
paths deserve further investigation.

Would it be reasonable to keep this patch focused on the
reproduced reference-count imbalance and address any confirmed
synchronization issues separately?

Thanks,
Mizuno

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

[Frank Li]

On Thu, Oct 15, 2026 at 07:58:08AM +0800, Mizuno Miotsuki via B4 Relay wrote:
> [You don't often get email from devnull+founder.crepuse.com@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> 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
>   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)
>             |                            |

But if without lock, sequency May

             |                     pch->active = true
             |                       pm_runtime_get
             |                            |
                                          |
             |                      work_list empty
             |                            |
             |                   pm_runtime_put(if active)
             |                            |
   pm_runtime_put(if active)
        pch->active = false

so put will call twice.

Without resolve thread DMA TERMINATE and TASKLET ROUTINE's sync problem,
it is hard to fix it.

Frank

>
> 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>
>
>

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

[Mizuno Miotsuki]

Hi Frank,

Thanks for taking another look.

I think the interleaving you described is already prevented by
pch->lock.

Both pl330_tasklet() and pl330_terminate_all() check pch->active
and clear it while holding the same lock. Whichever gets the lock
first can claim the PM put, and the other should then see
pch->active == false.

The actual pm_runtime_put_autosuspend() calls happen after
releasing the lock, but the decision to make each call is still
protected by it.

The issue this patch addresses is that the tasklet currently sets
power_down unconditionally when work_list is empty. If
terminate_all() has already cleared pch->active and released the
PM reference, the tasklet can still issue another put.

I agree there may be other synchronization issues around
issue_pending() and tasklet callbacks, and I don't mean to claim
this patch fixes all of them.

But I don't see how both paths in your example could observe
pch->active == true for the same activation. Am I missing another
interleaving?

Thanks,
Mizuno