Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 20 Apr 2022 15:45:42 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     sugar zhang <sugar.zhang@rock-chips.com>
Cc:     Huibin Hong <huibin.hong@rock-chips.com>,
        dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] dmaengine: pl330: Fix unbalanced runtime PM
Message-ID: <Yl/dTqT9MLhws6g6@matsya>
References: <1648296988-45745-1-git-send-email-sugar.zhang@rock-chips.com>
 <YlQxy0e/39M4xTdL@matsya>
 <5f13aa47-92cd-6c71-66f5-c5513a36b277@rock-chips.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <5f13aa47-92cd-6c71-66f5-c5513a36b277@rock-chips.com>
Precedence: bulk

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