| 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
| ||
|
Message-ID: <CAPDyKFq1fvKBtPhmd3YbYcg-hWPRkqGoB=-f8SwTG2OSiKjzdw@mail.gmail.com>
Date: Tue, 1 Nov 2022 11:00:23 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: 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>
Subject: Re: [PATCH] PM: domains: Reverse the order of performance and
enabling ops
On Mon, 24 Oct 2022 at 16:44, 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>
> ---
> drivers/base/power/domain.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index ead135c7044c..e66a711fec88 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -939,8 +939,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;
> @@ -978,9 +978,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)
> @@ -1018,8 +1017,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);
> }
>
> @@ -2749,17 +2748,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) {
> @@ -2771,6 +2759,18 @@ 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) {
> + genpd_remove_device(pd, dev);
> + return -EPROBE_DEFER;
If we manage to request a default performance state
(dev_gpd_data(dev)->default_pstate != 0) a few lines above, we should
probably drop that request before returning here.
Similar to what we do in genpd_dev_pm_detach().
> + }
> +
> return 1;
>
> err:
Other than the rather small thing above, this looks good to me!
Kind regards
Uffe
Powered by blists - more mailing lists