linux-kernel - Re: [PATCH v4 1/5] PM: domains: Allow devices attached to genpd to be managed by HW

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]

Message-ID: <tax3c6o5qjegy6tv3zbgrd5rencfvypr3zg7twxfrmdngscp74@n44ei3q63g64>
Date: Tue, 30 Jan 2024 19:09:24 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Abel Vesa <abel.vesa@...aro.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, 
	Kevin Hilman <khilman@...nel.org>, Ulf Hansson <ulf.hansson@...aro.org>, 
	Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Andy Gross <agross@...nel.org>, 
	Konrad Dybcio <konrad.dybcio@...aro.org>, Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>, Stanimir Varbanov <stanimir.k.varbanov@...il.com>, 
	Vikash Garodia <quic_vgarodia@...cinc.com>, Bryan O'Donoghue <bryan.odonoghue@...aro.org>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Taniya Das <quic_tdas@...cinc.com>, 
	Jagadeesh Kona <quic_jkona@...cinc.com>, Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	linux-clk@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH v4 1/5] PM: domains: Allow devices attached to genpd to
 be managed by HW

On Mon, Jan 22, 2024 at 10:47:01AM +0200, Abel Vesa wrote:
> From: Ulf Hansson <ulf.hansson@...aro.org>
> 
> Some power-domains may be capable of relying on the HW to control the power
> for a device that's hooked up to it. Typically, for these kinds of
> configurations the consumer driver should be able to change the behavior of
> power domain at runtime, control the power domain in SW mode for certain
> configurations and handover the control to HW mode for other usecases.
> 
> To allow a consumer driver to change the behaviour of the PM domain for its
> device, let's provide a new function, dev_pm_genpd_set_hwmode(). Moreover,
> let's add a corresponding optional genpd callback, ->set_hwmode_dev(),
> which the genpd provider should implement if it can support switching
> between HW controlled mode and SW controlled mode. Similarly, add the
> dev_pm_genpd_get_hwmode() to allow consumers to read the current mode and
> its corresponding optional genpd callback, ->get_hwmode_dev(), which the
> genpd provider can also implement for reading back the mode from the
> hardware.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@...aro.org>
> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> ---
>  drivers/pmdomain/core.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_domain.h | 17 ++++++++++++
>  2 files changed, 86 insertions(+)
> 
> diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> index a1f6cba3ae6c..41b6411d0ef5 100644
> --- a/drivers/pmdomain/core.c
> +++ b/drivers/pmdomain/core.c
> @@ -548,6 +548,75 @@ void dev_pm_genpd_synced_poweroff(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_genpd_synced_poweroff);
>  
> +/**
> + * dev_pm_genpd_set_hwmode - Set the HW mode for the device and its PM domain.

This isn't proper kernel-doc

> + *
> + * @dev: Device for which the HW-mode should be changed.
> + * @enable: Value to set or unset the HW-mode.
> + *
> + * Some PM domains can rely on HW signals to control the power for a device. To
> + * allow a consumer driver to switch the behaviour for its device in runtime,
> + * which may be beneficial from a latency or energy point of view, this function
> + * may be called.
> + *
> + * It is assumed that the users guarantee that the genpd wouldn't be detached
> + * while this routine is getting called.
> + *
> + * Returns 0 on success and negative error values on failures.
> + */
> +int dev_pm_genpd_set_hwmode(struct device *dev, bool enable)
> +{
> +	struct generic_pm_domain *genpd;
> +	int ret = 0;
> +
> +	genpd = dev_to_genpd_safe(dev);
> +	if (!genpd)
> +		return -ENODEV;
> +
> +	if (!genpd->set_hwmode_dev)
> +		return -EOPNOTSUPP;
> +
> +	genpd_lock(genpd);
> +
> +	if (dev_gpd_data(dev)->hw_mode == enable)

Between this and the gdsc patch, the hw_mode state might not match the
hardware state at boot.

With hw_mode defaulting to false, your first dev_pm_genpd_set_hwmode(,
false) will not bring control to SW - which might be fatal.

Regards,
Bjorn