linux-kernel - Re: [RFC PATCH v2 1/5] PM: domains: Allow power off queuing from providers

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:   Tue, 21 Mar 2023 14:01:49 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Abel Vesa <abel.vesa@...aro.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Kevin Hilman <khilman@...nel.org>, Pavel Machek <pavel@....cz>,
        Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Mike Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Saravana Kannan <saravanak@...gle.com>,
        linux-pm@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        Doug Anderson <dianders@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>
Subject: Re: [RFC PATCH v2 1/5] PM: domains: Allow power off queuing from providers

On Mon, 20 Mar 2023 at 14:42, Abel Vesa <abel.vesa@...aro.org> wrote:
>
> In some cases, the providers might choose to refuse powering off some
> domains until all of the consumer have had a chance to probe, that is,

/s/consumer/consumers

> until sync state callback has been called. Such providers might choose
> to disable such domains on their on, from the sync state callback. So,

/s/their on/their own

> in order to do that, they need a way to queue up a power off request.
> Since the generic genpd already has such API, make that available to
> those providers.
>
> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> ---
>  drivers/base/power/domain.c | 3 ++-
>  include/linux/pm_domain.h   | 6 ++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 32084e38b73d..97d4e2f2da91 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -649,10 +649,11 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed)
>   * Queue up the execution of genpd_power_off() unless it's already been done
>   * before.
>   */
> -static void genpd_queue_power_off_work(struct generic_pm_domain *genpd)

Please add a function description - and make sure to state that its
external use is explicitly intended for being called from genpd
providers ->sync_state callbacks.

> +void genpd_queue_power_off_work(struct generic_pm_domain *genpd)

As this becomes an exported function, we should also conform to
genpd's function naming rules, which is to use the "pm_genpd_*"
prefix.

While renaming it, perhaps it's sufficient with
"pm_genpd_queue_power_off" or maybe even better "pm_genpd_sync_state",
what do you think?

>  {
>         queue_work(pm_wq, &genpd->power_off_work);
>  }
> +EXPORT_SYMBOL_GPL(genpd_queue_power_off_work);
>
>  /**
>   * genpd_power_off - Remove power from a given PM domain.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index f776fb93eaa0..f9729640f87e 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -231,6 +231,7 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>  int pm_genpd_init(struct generic_pm_domain *genpd,
>                   struct dev_power_governor *gov, bool is_off);
>  int pm_genpd_remove(struct generic_pm_domain *genpd);
> +void genpd_queue_power_off_work(struct generic_pm_domain *genpd);
>  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
>  int dev_pm_genpd_add_notifier(struct device *dev, struct notifier_block *nb);
>  int dev_pm_genpd_remove_notifier(struct device *dev);
> @@ -278,6 +279,11 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
>         return -EOPNOTSUPP;
>  }
>
> +void genpd_queue_power_off_work(struct generic_pm_domain *genpd)
> +{
> +       return -EOPNOTSUPP;
> +}
> +
>  static inline int dev_pm_genpd_set_performance_state(struct device *dev,
>                                                      unsigned int state)
>  {
> --
> 2.34.1
>

Other than the minor things above, the approach looks reasonable to me!

Kind regards
Uffe