| 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: <CAJZ5v0gF93Qm0_0jGQjAAo6qBJHmEEzByE243nZAyEccsvj=Bw@mail.gmail.com>
Date: Tue, 23 Sep 2025 12:43:21 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Dhruva Gole <d-gole@...com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Linux PM <linux-pm@...r.kernel.org>,
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>
Subject: Re: [PATCH v3 1/3] PM: runtime: Add auto-cleanup macros for "resume
and get" operations
On Tue, Sep 23, 2025 at 10:53 AM Dhruva Gole <d-gole@...com> wrote:
>
> On Sep 22, 2025 at 17:30:43 +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 DEFINE_CLASS() 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 CLASS(pm_runtime_get_sync) like:
> >
> > guard(pm_runtime_get_sync)(dev);
> > .....
> > return 0;
> >
> > (see 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:
> >
> > CLASS(pm_runtime_get_active, pm)(dev);
> > if (IS_ERR(pm))
> > return PTR_ERR(pm);
> > .....
> > 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 simplified with CLASS() like:
> >
> > CLASS(pm_runtime_get_active, pm)(dev);
> > if (IS_ERR(pm))
> > return PTR_ERR(pm);
> > .....
> > return 0;
> >
> > (again, see 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):
> >
> > CLASS(pm_runtime_get_active_enabled, pm)(dev);
> > if (IS_ERR(pm))
> > return PTR_ERR(pm);
> > .....
> > return 0;
> >
> > When the original code calls pm_runtime_put_autosuspend(), use one
> > of the "auto" class variants, CLASS(pm_runtime_get_active_auto) or
> > CLASS(pm_runtime_get_active_enabled_auto), 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:
> >
> > CLASS(pm_runtime_get_active_enabled_auto, pm)(dev);
> > if (IS_ERR(pm))
> > return PTR_ERR(pm);
> > .....
> > 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:
> >
> > CLASS(pm_runtime_get_active_enabled, pm)(dev);
> > if (IS_ERR(pm))
> > return PTR_ERR(pm);
> > .....
> > return 0;
> >
> > or CLASS(pm_runtime_get_active) can be used if transparent handling of
> > disabled runtime PM is desirable.
> >
>
> Firstly, please can we add all this documentation in runtime_pm [1]
> Otherwise there's just far less developers aware of the new APIs getting
> introduced other than people directly involved. Not everyone is going to
> come down here to look at git log for API docs (even though we proud
> ourselves in having git log as our main source of Documentation in
> kernel ;) )
>
> [1] https://docs.kernel.org/power/runtime_pm.html
That will happen when the early adopters tell me that it works for them.
> > Link: https://lore.kernel.org/linux-pm/878qimv24u.wl-tiwai@suse.de/ [1]
> > Co-developed-by: Takashi Iwai <tiwai@...e.de>
> > Signed-off-by: Takashi Iwai <tiwai@...e.de>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > ---
> >
> > 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 | 82 +++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 84 insertions(+)
>
> >
> > --- 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;
>
> "TRANSPARENT" doesn't tell you exactly what happens. It should be something like
> RPM_IGNORE_DISABLED or RPM_ALLOW_DISABLED IMO.
There is a description in the header file and TRANSPARENT is shorter
than the alternatives. Besides, this is mostly for internal use.
> > 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
> > @@ -533,6 +534,32 @@ static inline int pm_runtime_resume_and_
> > }
> >
> > /**
> > + * pm_runtime_get_active_dev - Resume a device and bump up its usage counter.
>
> I am getting no clue as to why this is different than regular
> pm_runtime_get_sync then? Can we describe this API better?
Again, this is for internal use.
I may as well drop the kerneldoc for this function altogether, but I'm
not going to send a v4 just for this purpose.
Powered by blists - more mailing lists