linux-kernel - Re: [PATCH v4 1/3] PM: runtime: Add auto-cleanup macros for "resume and get" operations

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: <CAJZ5v0iaEyW743NePaVvZnshpRFtxq3QnCrRb7nJ9oNAJnvVdA@mail.gmail.com>
Date: Fri, 26 Sep 2025 21:56:45 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: dan.j.williams@...el.com
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, 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>, Dhruva Gole <d-gole@...com>
Subject: Re: [PATCH v4 1/3] PM: runtime: Add auto-cleanup macros for "resume
 and get" operations

On Fri, Sep 26, 2025 at 9:48 PM <dan.j.williams@...el.com> wrote:
>
> 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
>
> Leftover from CLASS() approach?

Yup.

> s/CLASS(pm_runtime_get_active_enabled)/ACQUIRE(pm_runtime_active_try_enabled)/

I'll fix this when applying.

> > 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.
>
> s/class/guard/

Right, thanks!

> > 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;
>
> I like that this appears to unify the pm_runtime_resume_and_get() and
> pm_runtime_get_sync() usages into common pattern.

That's intentional.

> > or pm_runtime_get_active_try can be used if transparent handling of
> > disabled runtime PM is desirable.
>
> Do you think the above should go in Documentation too?

It will, when early adopters tell me that they are happy with it.

> Either way, for the usage of ACQUIRE(), looks good to me.
>
> Acked-by: Dan Williams <dan.j.williams@...el.com>

Thank you!