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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAPDyKFr-gfpVypFs_13hb9Pi5FfQoB32fg=C_gtdSKVDN1U=gQ@mail.gmail.com>
Date:   Tue, 25 Aug 2020 14:42:54 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Stephan Gerhold <stephan@...hold.net>
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, 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)

Kind regards
Uffe