linux-kernel - Re: [PATCH] dmaengine: pl330: Fix unbalanced runtime PM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Yl/dTqT9MLhws6g6@matsya>
Date:   Wed, 20 Apr 2022 15:45:42 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     sugar zhang <sugar.zhang@...k-chips.com>
Cc:     Huibin Hong <huibin.hong@...k-chips.com>,
        dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dmaengine: pl330: Fix unbalanced runtime PM

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