linux-kernel - Re: [PATCH v2 4/4] PM: domains: Drop/restore performance state votes for devices at system 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]

Date:   Thu, 3 Jun 2021 12:20:57 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Mark Brown <broonie@...nel.org>,
        Linux PM <linux-pm@...r.kernel.org>
Cc:     Dmitry Osipenko <digetx@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Roja Rani Yarubandi <rojay@...eaurora.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Stephan Gerhold <stephan@...hold.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rajendra Nayak <rnayak@...eaurora.org>
Subject: Re: [PATCH v2 4/4] PM: domains: Drop/restore performance state votes
 for devices at system PM

+ Mark Brown, Dmitry Baryshkov

On Thu, 3 Jun 2021 at 11:34, Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
> Recent changes in genpd drops and restore performance state votes for
> devices during runtime PM.
>
> For the similar reasons, but to avoid the same kind of boilerplate code in
> device PM callbacks for system sleep in subsystems/drivers, let's drop and
> restore performance states votes in genpd for the attached devices during
> system sleep.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>

After a second thought, it looks like we maybe should defer to apply
this final patch of the series. At least until we figured out how to
address the below issue:

So, I noticed that we have things like "regulator-fixed-domain", that
uses "required-opps" to enable/disable a regulator through the
dev_pm_set_performance_state() interface. We likely don't want to drop
the performance state internally in genpd when genpd_suspend_noirq()
gets called, for the corresponding struct device for the regulator.

I guess if genpd should drop performance states like $subject patch
suggest, we need some kind of additional coordination, that allows a
subsystem/driver to inform genpd when it should avoid it. Or something
along those lines.

Kind regards
Uffe

> ---
>
> Changes in v2:
>         - Rebased.
>         - A few cosmetic changes.
>
> ---
>  drivers/base/power/domain.c | 9 +++++++++
>  include/linux/pm_domain.h   | 1 +
>  2 files changed, 10 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index e5d97174c254..a33e5b341f3f 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1171,6 +1171,7 @@ static int genpd_prepare(struct device *dev)
>   */
>  static int genpd_finish_suspend(struct device *dev, bool poweroff)
>  {
> +       struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
>         struct generic_pm_domain *genpd;
>         int ret = 0;
>
> @@ -1201,6 +1202,7 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff)
>         }
>
>         genpd_lock(genpd);
> +       gpd_data->pm_pstate = genpd_drop_performance_state(dev);
>         genpd->suspended_count++;
>         genpd_sync_power_off(genpd, true, 0);
>         genpd_unlock(genpd);
> @@ -1245,6 +1247,7 @@ static int genpd_resume_noirq(struct device *dev)
>         genpd_lock(genpd);
>         genpd_sync_power_on(genpd, true, 0);
>         genpd->suspended_count--;
> +       genpd_restore_performance_state(dev, dev_gpd_data(dev)->pm_pstate);
>         genpd_unlock(genpd);
>
>         if (genpd->dev_ops.stop && genpd->dev_ops.start &&
> @@ -1364,6 +1367,7 @@ static int genpd_restore_noirq(struct device *dev)
>         }
>
>         genpd_sync_power_on(genpd, true, 0);
> +       genpd_restore_performance_state(dev, dev_gpd_data(dev)->pm_pstate);
>         genpd_unlock(genpd);
>
>         if (genpd->dev_ops.stop && genpd->dev_ops.start &&
> @@ -1409,23 +1413,28 @@ static void genpd_complete(struct device *dev)
>  static void genpd_switch_state(struct device *dev, bool suspend)
>  {
>         struct generic_pm_domain *genpd;
> +       struct generic_pm_domain_data *gpd_data;
>         bool use_lock;
>
>         genpd = dev_to_genpd_safe(dev);
>         if (!genpd)
>                 return;
>
> +       gpd_data = dev_gpd_data(dev);
> +
>         use_lock = genpd_is_irq_safe(genpd);
>
>         if (use_lock)
>                 genpd_lock(genpd);
>
>         if (suspend) {
> +               gpd_data->pm_pstate = genpd_drop_performance_state(dev);
>                 genpd->suspended_count++;
>                 genpd_sync_power_off(genpd, use_lock, 0);
>         } else {
>                 genpd_sync_power_on(genpd, use_lock, 0);
>                 genpd->suspended_count--;
> +               genpd_restore_performance_state(dev, gpd_data->pm_pstate);
>         }
>
>         if (use_lock)
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 21a0577305ef..f6e9dc28621c 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -199,6 +199,7 @@ struct generic_pm_domain_data {
>         int cpu;
>         unsigned int performance_state;
>         unsigned int rpm_pstate;
> +       unsigned int pm_pstate;
>         ktime_t next_wakeup;
>         void *data;
>  };
> --
> 2.25.1
>