| 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: <20250929132443.6uueg5uazguvdklc@lcpd911>
Date: Mon, 29 Sep 2025 18:54:43 +0530
From: Dhruva Gole <d-gole@...com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
CC: Linux PM <linux-pm@...r.kernel.org>,
Jonathan Cameron
<jonathan.cameron@...wei.com>,
Takashi Iwai <tiwai@...e.de>, LKML
<linux-kernel@...r.kernel.org>,
Linux PCI <linux-pci@...r.kernel.org>,
Alex
Williamson <alex.williamson@...hat.com>,
Bjorn Helgaas <helgaas@...nel.org>,
Zhang Qilong <zhangqilong3@...wei.com>,
Ulf Hansson <ulf.hansson@...aro.org>, Frank Li <Frank.Li@....com>
Subject: Re: [PATCH v4 1/3] PM: runtime: Add auto-cleanup macros for "resume
and get" operations
On Sep 26, 2025 at 17:47:14 +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> It is generally useful to be able to automatically drop a device's
> runtime PM usage counter incremented by runtime PM operations that
> resume a device and bump up its usage counter [1].
>
> To that end, add guard definition macros allowing pm_runtime_put()
> and pm_runtime_put_autosuspend() to be used for the auto-cleanup in
> those cases.
>
> Simply put, a piece of code like below:
>
> pm_runtime_get_sync(dev);
> .....
> pm_runtime_put(dev);
> return 0;
>
> can be transformed with guard() like:
>
> guard(pm_runtime_active)(dev);
> .....
> return 0;
>
> (see the pm_runtime_put() call is gone).
>
> However, it is better to do proper error handling in the majority of
> cases, so doing something like this instead of the above is recommended:
>
> ACQUIRE(pm_runtime_active_try, pm)(dev);
> if (ACQUIRE_ERR(pm_runtime_active_try, &pm))
> return -ENXIO;
> .....
> return 0;
>
> In all of the cases in which runtime PM is known to be enabled for the
> given device or the device can be regarded as operational (and so it can
> be accessed) with runtime PM disabled, a piece of code like:
>
> ret = pm_runtime_resume_and_get(dev);
> if (ret < 0)
> return ret;
> .....
> pm_runtime_put(dev);
> return 0;
>
> can be changed as follows:
>
> ACQUIRE(pm_runtime_active_try, pm)(dev);
> ret = ACQUIRE_ERR(pm_runtime_active_try, &pm);
> if (ret < 0)
> return ret;
> .....
> return 0;
>
> (again, see the pm_runtime_put() call is gone).
>
> Still, if the device cannot be accessed unless runtime PM has been
> enabled for it, the CLASS(pm_runtime_get_active_enabled) variant
> needs to be used, that is (in the context of the example above):
>
> ACQUIRE(pm_runtime_active_try_enabled, pm)(dev);
> ret = ACQUIRE_ERR(pm_runtime_active_try_enabled, &pm);
> if (ret < 0)
> return ret;
> .....
> return 0;
>
> When the original code calls pm_runtime_put_autosuspend(), use one
> of the "auto" guard variants, pm_runtime_active_auto/_try/_enabled,
> so for example, a piece of code like:
>
> ret = pm_runtime_resume_and_get(dev);
> if (ret < 0)
> return ret;
> .....
> pm_runtime_put_autosuspend(dev);
> return 0;
>
> will become:
>
> ACQUIRE(pm_runtime_active_auto_try_enabled, pm)(dev);
> ret = ACQUIRE_ERR(pm_runtime_active_auto_try_enabled, &pm);
> if (ret < 0)
> return ret;
> .....
> return 0;
>
> Note that the cases in which the return value of pm_runtime_get_sync()
> is checked can also be handled with the help of the new class macros.
> For example, a piece of code like:
>
> ret = pm_runtime_get_sync(dev);
> if (ret < 0) {
> pm_runtime_put(dev);
> return ret;
> }
> .....
> pm_runtime_put(dev);
> return 0;
>
> can be rewritten as:
>
> ACQUIRE(pm_runtime_active_auto_try_enabled, pm)(dev);
> ret = ACQUIRE_ERR(pm_runtime_active_auto_try_enabled, &pm);
> if (ret < 0)
> return ret;
> .....
> return 0;
>
> or pm_runtime_get_active_try can be used if transparent handling of
> disabled runtime PM is desirable.
>
> Link: https://lore.kernel.org/linux-pm/878qimv24u.wl-tiwai@suse.de/ [1]
> Link: https://lore.kernel.org/linux-pm/20250926150613.000073a4@huawei.com/
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
>
> v3 -> v4:
> * Use guard definition macros instead of raw DEFINE_CLASS() (Jonathan)
> * Change pm_runtime_get_active() helper definition to return an int instead
> of a pointer
> * Update changelog to match the new code
It does look like a lot has changed since I last gave my R-by so thanks
for not including it.
>
> v2 -> v3:
> * Two more class definitions for the case in which resume errors can be
> neglected.
> * Update of new code comments (for more clarity).
> * Changelog update.
>
> v1 -> v2:
> * Rename the new classes and the new static inline helper.
> * Add two classes for handling disabled runtime PM.
> * Expand the changelog.
> * Adjust the subject.
>
> ---
> drivers/base/power/runtime.c | 2 +
> include/linux/pm_runtime.h | 44 ++++++++++++++++++++++++++++++++++---------
> 2 files changed, 37 insertions(+), 9 deletions(-)
>
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -796,6 +796,8 @@ static int rpm_resume(struct device *dev
> if (dev->power.runtime_status == RPM_ACTIVE &&
> dev->power.last_status == RPM_ACTIVE)
> retval = 1;
> + else if (rpmflags & RPM_TRANSPARENT)
> + goto out;
> else
> retval = -EACCES;
> }
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -21,6 +21,7 @@
> #define RPM_GET_PUT 0x04 /* Increment/decrement the
> usage_count */
> #define RPM_AUTO 0x08 /* Use autosuspend_delay */
> +#define RPM_TRANSPARENT 0x10 /* Succeed if runtime PM is disabled */
>
> /*
> * Use this for defining a set of PM operations to be used in all situations
> @@ -511,6 +512,19 @@ static inline int pm_runtime_get_sync(st
> return __pm_runtime_resume(dev, RPM_GET_PUT);
> }
>
> +static inline int pm_runtime_get_active(struct device *dev, int rpmflags)
> +{
> + int ret;
> +
> + ret = __pm_runtime_resume(dev, RPM_GET_PUT | rpmflags);
> + if (ret < 0) {
> + pm_runtime_put_noidle(dev);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> /**
> * pm_runtime_resume_and_get - Bump up usage counter of a device and resume it.
> * @dev: Target device.
> @@ -521,15 +535,7 @@ static inline int pm_runtime_get_sync(st
> */
> static inline int pm_runtime_resume_and_get(struct device *dev)
> {
> - int ret;
> -
> - ret = __pm_runtime_resume(dev, RPM_GET_PUT);
> - if (ret < 0) {
> - pm_runtime_put_noidle(dev);
> - return ret;
> - }
> -
> - return 0;
> + return pm_runtime_get_active(dev, 0);
> }
I do like this reuse indeed.
>
> /**
> @@ -606,6 +612,26 @@ static inline int pm_runtime_put_autosus
> return __pm_runtime_put_autosuspend(dev);
> }
>
> +DEFINE_GUARD(pm_runtime_active, struct device *,
> + pm_runtime_get_sync(_T), pm_runtime_put(_T));
> +DEFINE_GUARD(pm_runtime_active_auto, struct device *,
> + pm_runtime_get_sync(_T), pm_runtime_put_autosuspend(_T));
> +/*
> + * Use the following guards with ACQUIRE()/ACQUIRE_ERR().
> + *
> + * The difference between the "_try" and "_try_enabled" variants is that the
> + * former do not produce an error when runtime PM is disabled for the given
> + * device.
> + */
> +DEFINE_GUARD_COND(pm_runtime_active, _try,
> + pm_runtime_get_active(_T, RPM_TRANSPARENT))
> +DEFINE_GUARD_COND(pm_runtime_active, _try_enabled,
> + pm_runtime_resume_and_get(_T))
> +DEFINE_GUARD_COND(pm_runtime_active_auto, _try,
> + pm_runtime_get_active(_T, RPM_TRANSPARENT))
> +DEFINE_GUARD_COND(pm_runtime_active_auto, _try_enabled,
> + pm_runtime_resume_and_get(_T))
Overall looks better to me than the earlier revisions, thanks.
Reviewed-by: Dhruva Gole <d-gole@...com>
--
Best regards,
Dhruva Gole
Texas Instruments Incorporated
Powered by blists - more mailing lists