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]
Date:   Fri, 25 Nov 2022 19:33:20 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Ulf Hansson <ulf.hansson@...aro.org>,
        Abel Vesa <abel.vesa@...aro.org>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Kevin Hilman <khilman@...nel.org>,
        Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Bjorn Andersson <andersson@...nel.org>,
        linux-pm@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2] PM: domains: Reverse the order of performance and
 enabling ops

On Wed, Nov 16, 2022 at 1:48 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
> On Tue, 15 Nov 2022 at 22:25, Abel Vesa <abel.vesa@...aro.org> wrote:
> >
> > The ->set_performance_state() needs to be called before ->power_on()
> > when a genpd is powered on, and after ->power_off() when a genpd is
> > powered off. Do this in order to let the provider know to which
> > performance state to power on the genpd, on the power on sequence, and
> > also to maintain the performance for that genpd until after powering off,
> > on power off sequence.
> >
> > There is no scenario where a consumer would need its genpd enabled and
> > then its performance state increased. Instead, in every scenario, the
> > consumer needs the genpd to be enabled from the start at a specific
> > performance state.
> >
> > And same logic applies to the powering down. No consumer would need its
> > genpd performance state dropped right before powering down.
> >
> > Now, there are currently two vendors which use ->set_performance_state()
> > in their genpd providers. One of them is Tegra, but the only genpd provider
> > (PMC) that makes use of ->set_performance_state() doesn't implement the
> > ->power_on() or ->power_off(), and so it will not be affected by the ops
> > reversal.
> >
> > The other vendor that uses it is Qualcomm, in multiple genpd providers
> > actually (RPM, RPMh and CPR). But all Qualcomm genpd providers that make
> > use of ->set_performance_state() need the order between enabling ops and
> > the performance setting op to be reversed. And the reason for that is that
> > it currently translates into two different voltages in order to power on
> > a genpd to a specific performance state. Basically, ->power_on() switches
> > to the minimum (enabling) voltage for that genpd, and then
> > ->set_performance_state() sets it to the voltage level required by the
> > consumer.
> >
> > By reversing the call order, we rely on the provider to know what to do
> > on each call, but most popular usecase is to cache the performance state
> > and postpone the voltage setting until the ->power_on() gets called.
> >
> > As for the reason of still needing the ->power_on() and ->power_off() for a
> > provider which could get away with just having ->set_performance_state()
> > implemented, there are consumers that do not (nor should) provide an
> > opp-table. For those consumers, ->set_performance_state() will not be
> > called, and so they will enable the genpd to its minimum performance state
> > by a ->power_on() call. Same logic goes for the disabling.
> >
> > Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
>
> Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>

Applied as 6.2 material, thanks!

> > ---
> >
> > Changes since v1:
> >  - Added performance state drop on power on failure, like Ulf suggested
> >
> >  drivers/base/power/domain.c | 36 +++++++++++++++++++++---------------
> >  1 file changed, 21 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index e5f4e5a2eb9e..967bcf9d415e 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -964,8 +964,8 @@ static int genpd_runtime_suspend(struct device *dev)
> >                 return 0;
> >
> >         genpd_lock(genpd);
> > -       gpd_data->rpm_pstate = genpd_drop_performance_state(dev);
> >         genpd_power_off(genpd, true, 0);
> > +       gpd_data->rpm_pstate = genpd_drop_performance_state(dev);
> >         genpd_unlock(genpd);
> >
> >         return 0;
> > @@ -1003,9 +1003,8 @@ static int genpd_runtime_resume(struct device *dev)
> >                 goto out;
> >
> >         genpd_lock(genpd);
> > +       genpd_restore_performance_state(dev, gpd_data->rpm_pstate);
> >         ret = genpd_power_on(genpd, 0);
> > -       if (!ret)
> > -               genpd_restore_performance_state(dev, gpd_data->rpm_pstate);
> >         genpd_unlock(genpd);
> >
> >         if (ret)
> > @@ -1043,8 +1042,8 @@ static int genpd_runtime_resume(struct device *dev)
> >  err_poweroff:
> >         if (!pm_runtime_is_irq_safe(dev) || genpd_is_irq_safe(genpd)) {
> >                 genpd_lock(genpd);
> > -               gpd_data->rpm_pstate = genpd_drop_performance_state(dev);
> >                 genpd_power_off(genpd, true, 0);
> > +               gpd_data->rpm_pstate = genpd_drop_performance_state(dev);
> >                 genpd_unlock(genpd);
> >         }
> >
> > @@ -2733,17 +2732,6 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
> >         dev->pm_domain->detach = genpd_dev_pm_detach;
> >         dev->pm_domain->sync = genpd_dev_pm_sync;
> >
> > -       if (power_on) {
> > -               genpd_lock(pd);
> > -               ret = genpd_power_on(pd, 0);
> > -               genpd_unlock(pd);
> > -       }
> > -
> > -       if (ret) {
> > -               genpd_remove_device(pd, dev);
> > -               return -EPROBE_DEFER;
> > -       }
> > -
> >         /* Set the default performance state */
> >         pstate = of_get_required_opp_performance_state(dev->of_node, index);
> >         if (pstate < 0 && pstate != -ENODEV && pstate != -EOPNOTSUPP) {
> > @@ -2755,6 +2743,24 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
> >                         goto err;
> >                 dev_gpd_data(dev)->default_pstate = pstate;
> >         }
> > +
> > +       if (power_on) {
> > +               genpd_lock(pd);
> > +               ret = genpd_power_on(pd, 0);
> > +               genpd_unlock(pd);
> > +       }
> > +
> > +       if (ret) {
> > +               /* Drop the default performance state */
> > +               if (dev_gpd_data(dev)->default_pstate) {
> > +                       dev_pm_genpd_set_performance_state(dev, 0);
> > +                       dev_gpd_data(dev)->default_pstate = 0;
> > +               }
> > +
> > +               genpd_remove_device(pd, dev);
> > +               return -EPROBE_DEFER;
> > +       }
> > +
> >         return 1;
> >
> >  err:
> > --
> > 2.34.1
> >

Powered by blists - more mailing lists