linux-kernel - Re: [PATCH v2 3/4] PM: domains: Drop/restore performance state votes for devices at 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPDyKFq=mGOqYrX5322JOmJXm6Yg-zNh+g66BdaVVfE5JgciAQ@mail.gmail.com>
Date:   Fri, 4 Jun 2021 09:45:35 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Linux PM <linux-pm@...r.kernel.org>,
        Dmitry Osipenko <digetx@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Stephan Gerhold <stephan@...hold.net>,
        Roja Rani Yarubandi <rojay@...eaurora.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] PM: domains: Drop/restore performance state votes
 for devices at runtime PM

On Fri, 4 Jun 2021 at 05:53, Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> On 03-06-21, 13:17, Ulf Hansson wrote:
> > On Thu, 3 Jun 2021 at 12:31, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> > > > > +static int genpd_drop_performance_state(struct device *dev)
> > > > > +{
> > > > > +     unsigned int prev_state = dev_gpd_data(dev)->performance_state;
> > > > > +
> > > > > +     if (!genpd_set_performance_state(dev, 0))
> > > > > +             return prev_state;
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static void genpd_restore_performance_state(struct device *dev,
> > > > > +                                         unsigned int state)
> > > > > +{
> > > > > +     if (state)
> > > >
> > > > I will skip this check, as we are checking it in
> > > > genpd_set_performance_state() anyway ?
> > >
> > > I don't want us to override OPP votes made by the subsystem/driver
> > > level runtime PM callbacks. For example, if the drivers manage this
> > > thing themselves, that should be preserved.
> > >
> > > That said, by the check above I want to avoid setting the state to
> > > zero internally by genpd, if the driver level ->runtime_resume()
> > > callback has already restored the state.
> >
> > Ehh, forget about what I said about the ->runtime_resume() callback.
> >
> > I am mostly trying to avoid restoring a state that is zero, just to be
> > sure nobody else on some different level outside gendp, have decided
> > to set a new OPP in-between our calls to
> > genpd_drop|restore_performance state.
>
> What stops the core to call genpd_drop_performance_state() in the
> first place here, if the driver was doing its own thing ? If that gets
> called, then restore should be without any checks IMO. The state
> should already be 0 at this point of time, I don't know why this will
> get called again with state 0, but it will have no effect.
>
> Can you give some sort of flow sequence where I can see the problem a
> bit more clearly ?

Starting calls from the subsystem/driver:

------
dev_pm_genpd_set_performance_state(dev, 100);
"run a use case with device runtime resumed"
...
"use case ends"
dev_pm_genpd_set_performance_state(dev, 0);
pm_runtime_put()
    ->genpd_runtime_suspend()
    gpd_data->performance_state == 0, -> gpd_data->rpm_pstate = 0;
...
"new use case start"
dev_pm_genpd_set_performance_state(dev, 100);
pm_runtime_get_sync()
    ->genpd_runtime_resume()
    gpd_data->performance_state == 100, -> gpd_data->rpm_pstate = 0;
(This is where we need to check for "zero" to not override the value)
.....
------

I wouldn't say that the above is the way how I see the calls to
dev_pm_genpd_set_performance_state (or actually
dev_pm_opp_set_rate|opp()) being deployed. The calls should rather be
done from the subsystem/driver's ->runtime_suspend|resume() callback,
then the path above would work in the way you suggest.

Although, as we currently treat performance states and power states in
genpd orthogonally, I wanted to make sure we could cope with both
situations.

Did this help? :-)

Kind regards
Uffe