linux-kernel - Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by the OPP core

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20200826083148.GA869@gerhold.net>
Date:   Wed, 26 Aug 2020 10:31:48 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     Viresh Kumar <viresh.kumar@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Kevin Hilman <khilman@...nel.org>, Nishanth Menon <nm@...com>,
        Stephen Boyd <sboyd@...nel.org>,
        Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Niklas Cassel <nks@...wful.org>
Subject: Re: [RFC PATCH 3/3] opp: Power on (virtual) power domains managed by
 the OPP core

On Tue, Aug 25, 2020 at 02:42:54PM +0200, Ulf Hansson wrote:
> On Tue, 25 Aug 2020 at 09:34, Stephan Gerhold <stephan@...hold.net> wrote:
> >
> > On Tue, Aug 25, 2020 at 08:43:42AM +0200, Ulf Hansson wrote:
> > > On Tue, 25 Aug 2020 at 06:43, Viresh Kumar <viresh.kumar@...aro.org> wrote:
> > > >
> > > > On 24-08-20, 17:08, Stephan Gerhold wrote:
> > > > > On Mon, Aug 24, 2020 at 04:36:57PM +0200, Ulf Hansson wrote:
> > > > > > That said, perhaps should rely on the consumer to deploy runtime PM
> > > > > > support, but let the OPP core to set up the device links for the genpd
> > > > > > virtual devices!?
> > > > > >
> > > > >
> > > > > Yes, that would be the alternative option.
> > > >
> > > > That is the right option IMO.
> > > >
> > > > > I would be fine with it as long as it also works for the CPUfreq case.
> > > > >
> > > > > I don't think anything manages runtime PM for the CPU device, just
> > > > > like no-one calls dev_pm_opp_set_rate(cpu_dev, 0). So with my patch the
> > > > > power domain is essentially kept always-on (except for system suspend).
> > > > > At least in my case this is intended.
> > > > >
> > > > > If device links also keep the power domains on if the consumer device
> > > > > does not make use of runtime PM it should work fine for my case.
> > > >
> > > > With device link, you only need to do rpm enable/disable on the consumer device
> > > > and it will get propagated by itself.
> > >
> > > Note that the default state for the genpd virtual device(s) is that
> > > runtime PM has been enabled for them. This means it's left in runtime
> > > suspended state, which allows its PM domain to be powered off (if all
> > > other devices and child domains for it allow that too, of course).
> > >
> > > >
> > > > > Personally, I think my original patch (without device links) fits better
> > > > > into the OPP API, for the following two reasons.
> > > > >
> > > > > With device links:
> > > > >
> > > > >   1. Unlike regulators/interconnects, attached power domains would be
> > > > >      controlled by runtime PM instead of dev_pm_opp_set_rate(opp_dev, 0).
> > > > >
> > > > >   2. ... some driver using OPP tables might not make use of runtime PM.
> > > > >      In that case, the power domains would stay on the whole time,
> > > > >      even if dev_pm_opp_set_rate(opp_dev, 0) was called.
> > > > >
> > > > > With my patch, the power domain state is directly related to the
> > > > > dev_pm_opp_set_rate(opp_dev, 0) call, which is more intuitive than
> > > > > relying on the runtime PM state in my opinion.
> > > >
> > > > So opp-set-rate isn't in the best of shape TBH, some things are left for the
> > > > drivers while other are done by it. Regulator-enable/disable was moved to it
> > > > some time back as people needed something like that. While on the other hand,
> > > > clk_enable/disable doesn't happen there, nor does rpm enable/disable.
> > > >
> > > > Maybe one day we may want to do that, but lets make sure someone wants to do
> > > > that first.
> > > >
> > > > Anyway, even in that case both of the changes would be required. We must make
> > > > device links nevertheless first. And later on if required, we may want to do rpm
> > > > enable/disable on the consumer device itself.
> > >
> > > This sounds like a reasonable step-by-step approach.
> > >
> > > Then, to create the device links, we should use DL_FLAG_PM_RUNTIME,
> > > DL_FLAG_STATELESS.
> > >
> >
> > OK, I will give this a try later this week.
> >
> > > But whether we should use DL_FLAG_RPM_ACTIVE as well, to initially
> > > runtime resume the supplier (the genpd virtual device), is harder to
> > > know - as that kind of depends on expectations by the consumer device
> > > driver.
> > >
> >
> > I'm not sure I understand the purpose of that flag. I thought we want to
> > link the PM state of the virtual genpd device (supplier) to the PM state
> > of the device of the OPP table (consumer).
> 
> Correct, but this is about synchronizing the initial runtime PM state
> of the consumer and supplier.
> 
> >
> > Shouldn't it just determine the initial state based on the state of the
> > consumer device?
> 
> We could do that. Then something along the lines of the below, should work:
> 
> pm_runtime_get_noresume(consumer) - to prevent runtime suspend, temporarily.
> 
> if(pm_runtime_active(consumer))
>     create links with DL_FLAG_RPM_ACTIVE
> else
>     create links without DL_FLAG_RPM_ACTIVE
> 
> pm_runtime_put(consumer)
> 

This seems to work as expected, thanks for the suggestion!
I will submit a v2 with that shortly.

Thanks!
Stephan