linux-kernel - Re: PM runtime auto-cleanup macros

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: <CAJZ5v0hUTByfxkE=-SGSHDDd=mPw694yD7PuuJ1LLRjp-H4=uA@mail.gmail.com>
Date: Thu, 18 Sep 2025 13:28:39 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Takashi Iwai <tiwai@...e.de>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: PM runtime auto-cleanup macros

On Thu, Sep 18, 2025 at 9:10 AM Takashi Iwai <tiwai@...e.de> wrote:
>
> On Wed, 17 Sep 2025 20:58:36 +0200,
> Rafael J. Wysocki wrote:
> >
> > Hi,
> >
> > Sorry for the delay.
> >
> > On Thu, Sep 11, 2025 at 9:31 AM Takashi Iwai <tiwai@...e.de> wrote:
> > >
> > > On Wed, 10 Sep 2025 16:00:17 +0200,
> > > Takashi Iwai wrote:
> > > >
> > > > Hi,
> > > >
> > > > while I worked on the code cleanups in the drivers with the recent
> > > > auto-cleanup macros, I noticed that pm_runtime_get*() and _put*() can
> > > > be also managed with the auto-cleanup gracefully, too.  Actually we
> > > > already defined the __free(pm_runtime_put) in commit bfa4477751e9, and
> > > > there is a (single) user of it in pci-sysfs.c.
> > > >
> > > > Now I wanted to extend it to pm_runtime_put_autosuspend() as:
> > > >
> > > > DEFINE_FREE(pm_runtime_put_autosuspend, struct device *,
> > > >            if (_T) pm_runtime_put_autosuspend(_T))
> > > >
> > > > Then one can use it like
> > > >
> > > >       ret = pm_runtime_resume_and_get(dev);
> > > >       if (ret < 0)
> > > >               return ret;
> > > >       struct device *pmdev __free(pm_runtime_put_autosuspend) = dev;
> > > >
> > > > that is similar as done in pci-sysfs.c.  So far, so good.
> > > >
> > > > But, I find putting the line like above at each place a bit ugly.
> > > > So I'm wondering whether it'd be better to introduce some helper
> > > > macros, e.g.
> > > >
> > > > #define pm_runtime_auto_clean(dev, var) \
> > > >       struct device *var __free(pm_runtime_put) = (dev)
> > >
> > > It can be even simpler by assigning a temporary variable such as:
> > >
> > > #define pm_runtime_auto_clean(dev) \
> > >         struct device *__pm_runtime_var ## __LINE__ __free(pm_runtime_put) = (dev)
> >
> > Well, if there's something like
> >
> > struct device *pm_runtime_resume_and_get_dev(struct device *dev)
> > {
> >         int ret = pm_runtime_resume_and_get(dev);
> >         if (ret < 0)
> >                 return ERR_PTR(ret);
> >
> >         return dev;
> > }
> >
> > It would be a matter of redefining the FREE to also take error
> > pointers into account and you could do
> >
> > struct device *__dev __free(pm_runtim_put) = pm_runtime_resume_and_get_dev(dev);
> > if (IS_ERR(__dev))
> >         return PTR_ERR(__dev);
>
> That'll work, too.  Though, I find the notion of __free() and a
> temporary variable __dev a bit too cumbersome; it's used only for
> auto-clean stuff, so it could be somewhat anonymous.

No, it is not used only for auto-clean, it is also used for return
value checking and it represents a reference on the original dev.  It
cannot be entirely anonymous because of the error checking part.

The point is that this is one statement instead of two and so it is
arguably harder to mess up with.

> But it's all about a matter of taste, and I'd follow what you and
> other guys suggest.
>
> FWIW, there are lots of code doing like
>
>         pm_runtime_get_sync(dev);
>         mutex_lock(&foo);
>         ....
>         mutex_unlock(&foo);
>         pm_runtime_put(dev);
>         return;
>
> or
>
>         ret = pm_runtime_resume_and_get(dev);
>         if (ret)
>                 return ret;
>         mutex_lock(&foo);
>         ....
>         mutex_unlock(&foo);
>         pm_runtime_put_autosuspend(dev);
>         return 0;
>
> and they can be converted nicely with guard() once when PM runtime can
> be automatically unreferenced.  With my proposed change, it would
> become like:
>
>         pm_runtime_get_sync(dev);
>         pm_runtime_auto_clean(dev);

For the case in which the pm_runtime_get_sync() return value is
discarded, you could define a guard and do

guard(pm_runtime_get_sync)(dev);

here.

The case checking the return value is less straightforward.

>         guard(mutex)(&foo);
>         ....
>         return;
>
> or
>
>         ret = pm_runtime_resume_and_get(dev);
>         if (ret)
>                 return ret;
>         pm_runtime_auto_clean_autosuspend(dev);
>         guard(mutex)(&foo);
>         ....
>         return 0;
>
>