All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.