linux-kernel - Re: [PATCH v14 01/39] soc/tegra: Enable runtime PM during OPP state-syncing

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:   Wed, 27 Oct 2021 17:06:19 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Viresh Kumar <vireshk@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Peter De Schrijver <pdeschrijver@...dia.com>,
        Mikko Perttunen <mperttunen@...dia.com>,
        Lee Jones <lee.jones@...aro.org>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, Nishanth Menon <nm@...com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Michael Turquette <mturquette@...libre.com>,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-pm@...r.kernel.org, linux-pwm@...r.kernel.org,
        linux-mmc@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-clk@...r.kernel.org, David Heidelberg <david@...t.cz>
Subject: Re: [PATCH v14 01/39] soc/tegra: Enable runtime PM during OPP state-syncing

On Tue, 26 Oct 2021 at 00:45, Dmitry Osipenko <digetx@...il.com> wrote:
>
> GENPD core now can set up domain's performance state properly while device
> is RPM-suspended. Runtime PM of a device must be enabled during setup
> because GENPD checks whether device is suspended and check doesn't work
> while RPM is disabled. Instead of replicating the boilerplate RPM-enable
> code around OPP helper for each driver, let's make OPP helper to take care
> of enabling it.
>
> Signed-off-by: Dmitry Osipenko <digetx@...il.com>

Just a minor nitpick, see below. Nevertheless feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>

Kind regards
Uffe

> ---
>  drivers/soc/tegra/common.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/soc/tegra/common.c b/drivers/soc/tegra/common.c
> index cd33e99249c3..d930a2b4facc 100644
> --- a/drivers/soc/tegra/common.c
> +++ b/drivers/soc/tegra/common.c
> @@ -10,6 +10,7 @@
>  #include <linux/export.h>
>  #include <linux/of.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pm_runtime.h>
>
>  #include <soc/tegra/common.h>
>  #include <soc/tegra/fuse.h>
> @@ -43,6 +44,7 @@ static int tegra_core_dev_init_opp_state(struct device *dev)
>  {
>         unsigned long rate;
>         struct clk *clk;
> +       bool rpm_enabled;
>         int err;
>
>         clk = devm_clk_get(dev, NULL);
> @@ -57,8 +59,22 @@ static int tegra_core_dev_init_opp_state(struct device *dev)
>                 return -EINVAL;
>         }
>
> +       /*
> +        * Runtime PM of the device must be enabled in order to set up
> +        * GENPD's performance properly because GENPD core checks whether
> +        * device is suspended and this check doesn't work while RPM is
> +        * disabled.
> +        */
> +       rpm_enabled = pm_runtime_enabled(dev);
> +       if (!rpm_enabled)
> +               pm_runtime_enable(dev);

This makes sure the OPP vote below gets cached in genpd for the
device. Instead, the vote is done the next time the device gets
runtime resumed.

I don't have an issue doing it like this, but at the same time it does
remove some flexibility for the drivers/subsystem that calls
tegra_core_dev_init_opp_state().

Isn't it better to leave this to be flexible - or you prefer to have
it done like this for everybody?

> +
>         /* first dummy rate-setting initializes voltage vote */
>         err = dev_pm_opp_set_rate(dev, rate);
> +
> +       if (!rpm_enabled)
> +               pm_runtime_disable(dev);
> +
>         if (err) {
>                 dev_err(dev, "failed to initialize OPP clock: %d\n", err);
>                 return err;

Kind regards
Uffe