| 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: <hojdkntm3q5a5ywya7n5i4zy24auko4u6zdqm25infhd44nyfx@x2evb6sc2d45>
Date: Tue, 27 May 2025 14:27:56 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, dakr@...nel.org,
len.brown@...el.com, pavel@...nel.org, ulf.hansson@...aro.org, jic23@...nel.org,
daniel.lezcano@...aro.org, linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
linux-iio@...r.kernel.org, linux-renesas-soc@...r.kernel.org, bhelgaas@...gle.com,
geert@...ux-m68k.org, Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v2 1/2] PM: domains: Add devres variant for
dev_pm_domain_attach()
Hi Claudiu,
On Mon, May 26, 2025 at 03:20:53PM +0300, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> The dev_pm_domain_attach() function is typically used in bus code alongside
> dev_pm_domain_detach(), often following patterns like:
>
> static int bus_probe(struct device *_dev)
> {
> struct bus_driver *drv = to_bus_driver(dev->driver);
> struct bus_device *dev = to_bus_device(_dev);
> int ret;
>
> // ...
>
> ret = dev_pm_domain_attach(_dev, true);
> if (ret)
> return ret;
>
> if (drv->probe)
> ret = drv->probe(dev);
>
> // ...
> }
>
> static void bus_remove(struct device *_dev)
> {
> struct bus_driver *drv = to_bus_driver(dev->driver);
> struct bus_device *dev = to_bus_device(_dev);
>
> if (drv->remove)
> drv->remove(dev);
> dev_pm_domain_detach(_dev);
> }
>
> When the driver's probe function uses devres-managed resources that depend
> on the power domain state, those resources are released later during
> device_unbind_cleanup().
>
> Releasing devres-managed resources that depend on the power domain state
> after detaching the device from its PM domain can cause failures.
>
> For example, if the driver uses devm_pm_runtime_enable() in its probe
> function, and the device's clocks are managed by the PM domain, then
> during removal the runtime PM is disabled in device_unbind_cleanup() after
> the clocks have been removed from the PM domain. It may happen that the
> devm_pm_runtime_enable() action causes the device to be runtime-resumed.
> If the driver specific runtime PM APIs access registers directly, this
> will lead to accessing device registers without clocks being enabled.
> Similar issues may occur with other devres actions that access device
> registers.
I think you are concentrating too much on runtime PM aspect of this. As
you mentioned in the last sentence the same issue may happen in the
absence of runtime PM if the power domain code will shut down the device
while it is not fully cleaned up.
>
> Add devm_pm_domain_attach(). When replacing the dev_pm_domain_attach() and
> dev_pm_domain_detach() in bus probe and bus remove, it ensures that the
> device is detached from its PM domain in device_unbind_cleanup(), only
> after all driver's devres-managed resources have been release.
>
> For flexibility, the implemented devm_pm_domain_attach() has 2 state
> arguments, one for the domain state on attach, one for the domain state on
> detach.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> ---
>
> Changes in v2:
> - none; this patch is new
>
> drivers/base/power/common.c | 59 +++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 8 +++++
> 2 files changed, 67 insertions(+)
>
> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index 781968a128ff..6ef0924efe2e 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -115,6 +115,65 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
> }
> EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>
> +/**
> + * devm_pm_domain_detach_off - devres action for devm_pm_domain_attach() to
> + * detach a device and power it off.
> + * @dev: device to detach.
> + *
> + * This function reverse the actions from devm_pm_domain_attach().
> + * It will be invoked during the remove phase from drivers implicitly.
> + */
> +static void devm_pm_domain_detach_off(void *dev)
> +{
> + dev_pm_domain_detach(dev, true);
> +}
> +
> +/**
> + * devm_pm_domain_detach_on - devres action for devm_pm_domain_attach() to
> + * detach a device and power it on.
> + * @dev: device to detach.
> + *
> + * This function reverse the actions from devm_pm_domain_attach().
> + * It will be invoked during the remove phase from drivers implicitly.
> + */
> +static void devm_pm_domain_detach_on(void *dev)
> +{
> + dev_pm_domain_detach(dev, false);
> +}
> +
> +/**
> + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
> + * @dev: Device to attach.
> + * @attach_power_on: Use to indicate whether we should power on the device
> + * when attaching (true indicates the device is powered on
> + * when attaching).
> + * @detach_power_off: Used to indicate whether we should power off the device
> + * when detaching (true indicates the device is powered off
> + * when detaching).
> + *
> + * NOTE: this will also handle calling dev_pm_domain_detach() for
> + * you during remove phase.
> + *
> + * Returns 0 on successfully attached PM domain, or a negative error code in
> + * case of a failure.
> + */
> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> + bool detach_power_off)
Do we have examples where we power on a device and leave it powered on
(or do not power on device on attach but power off it on detach)? I
believe devm release should strictly mirror the acquisition, so separate
flag is not needed.
> +{
> + int ret;
> +
> + ret = dev_pm_domain_attach(dev, attach_power_on);
> + if (ret)
> + return ret;
> +
> + if (detach_power_off)
> + return devm_add_action_or_reset(dev, devm_pm_domain_detach_off,
> + dev);
> +
> + return devm_add_action_or_reset(dev, devm_pm_domain_detach_on, dev);
Instead of 2 separate cleanup methods maybe define dedicated devres:
struct dev_pm_domain_devres {
struct device *dev;
bool power_off;
}
?
> +}
> +EXPORT_SYMBOL_GPL(devm_pm_domain_attach);
> +
> /**
> * dev_pm_domain_attach_by_id - Associate a device with one of its PM domains.
> * @dev: The device used to lookup the PM domain.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 0b18160901a2..ee798b090d17 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -509,6 +509,8 @@ struct device *dev_pm_domain_attach_by_name(struct device *dev,
> int dev_pm_domain_attach_list(struct device *dev,
> const struct dev_pm_domain_attach_data *data,
> struct dev_pm_domain_list **list);
> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> + bool detach_power_off);
> int devm_pm_domain_attach_list(struct device *dev,
> const struct dev_pm_domain_attach_data *data,
> struct dev_pm_domain_list **list);
> @@ -539,6 +541,12 @@ static inline int dev_pm_domain_attach_list(struct device *dev,
> return 0;
> }
>
> +static int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
Needs to be marked "inline".
> + bool detach_power_off)
> +{
> + return 0;
> +}
> +
> static inline int devm_pm_domain_attach_list(struct device *dev,
> const struct dev_pm_domain_attach_data *data,
> struct dev_pm_domain_list **list)
Thanks.
--
Dmitry
Powered by blists - more mailing lists