* [PATCH] dmaengine: pl330: Fix unbalanced runtime PM
@ 2022-03-26 12:16 Sugar Zhang
2022-04-11 13:48 ` Vinod Koul
0 siblings, 1 reply; 4+ messages in thread
From: Sugar Zhang @ 2022-03-26 12:16 UTC (permalink / raw)
To: vkoul; +Cc: Sugar Zhang, Huibin Hong, dmaengine, linux-kernel
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(force)
| |
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:
commit 5c9e6c2b2ba3 ("dmaengine: pl330: Fix runtime PM support for terminated transfers")
commit 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>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] dmaengine: pl330: Fix unbalanced runtime PM
2022-03-26 12:16 [PATCH] dmaengine: pl330: Fix unbalanced runtime PM Sugar Zhang
@ 2022-04-11 13:48 ` Vinod Koul
2022-04-17 13:53 ` sugar zhang
0 siblings, 1 reply; 4+ messages in thread
From: Vinod Koul @ 2022-04-11 13:48 UTC (permalink / raw)
To: Sugar Zhang; +Cc: Huibin Hong, dmaengine, linux-kernel
On 26-03-22, 20:16, Sugar Zhang wrote:
> 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(force)
maybe unconditional is a better word than force here?
> | |
>
> 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:
> commit 5c9e6c2b2ba3 ("dmaengine: pl330: Fix runtime PM support for terminated transfers")
> commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
That is not the right way for Fixes tag
>
> Signed-off-by: Huibin Hong <huibin.hong@rock-chips.com>
> Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> ---
>
> 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
--
~Vinod
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] dmaengine: pl330: Fix unbalanced runtime PM
2022-04-11 13:48 ` Vinod Koul
@ 2022-04-17 13:53 ` sugar zhang
2022-04-20 10:15 ` Vinod Koul
0 siblings, 1 reply; 4+ messages in thread
From: sugar zhang @ 2022-04-17 13:53 UTC (permalink / raw)
To: Vinod Koul; +Cc: Huibin Hong, dmaengine, linux-kernel
Hi Vinod,
在 2022/4/11 21:48, Vinod Koul 写道:
> On 26-03-22, 20:16, Sugar Zhang wrote:
>> 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(force)
> maybe unconditional is a better word than force here?
okay, will do in v2.
>
>> | |
>>
>> 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:
>> commit 5c9e6c2b2ba3 ("dmaengine: pl330: Fix runtime PM support for terminated transfers")
>> commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
> That is not the right way for Fixes tag
like this?
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>
>> ---
>>
>> 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
--
Best Regards!
张学广/Sugar
瑞芯微电子股份有限公司
Rockchip Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] dmaengine: pl330: Fix unbalanced runtime PM
2022-04-17 13:53 ` sugar zhang
@ 2022-04-20 10:15 ` Vinod Koul
0 siblings, 0 replies; 4+ messages in thread
From: Vinod Koul @ 2022-04-20 10:15 UTC (permalink / raw)
To: sugar zhang; +Cc: Huibin Hong, dmaengine, linux-kernel
On 17-04-22, 21:53, sugar zhang wrote:
> Hi Vinod,
>
> 在 2022/4/11 21:48, Vinod Koul 写道:
> > On 26-03-22, 20:16, Sugar Zhang wrote:
> > > 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(force)
> > maybe unconditional is a better word than force here?
> okay, will do in v2.
> >
> > > | |
> > >
> > > 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:
> > > commit 5c9e6c2b2ba3 ("dmaengine: pl330: Fix runtime PM support for terminated transfers")
> > > commit ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
> > That is not the right way for Fixes tag
>
> like this?
>
> Fixes: 5c9e6c2b2ba3 ("dmaengine: pl330: Fix runtime PM support for terminated transfers")
> Fixes: ae43b3289186 ("ARM: 8202/1: dmaengine: pl330: Add runtime Power Management support v12")
yes, this is the correct way
--
~Vinod
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-20 10:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-26 12:16 [PATCH] dmaengine: pl330: Fix unbalanced runtime PM Sugar Zhang
2022-04-11 13:48 ` Vinod Koul
2022-04-17 13:53 ` sugar zhang
2022-04-20 10:15 ` Vinod Koul
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.