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

[Vinod Koul <vkoul@kernel.org>]

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