| 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: <878qia8m0z.wl-tiwai@suse.de>
Date: Fri, 19 Sep 2025 18:06:20 +0200
From: Takashi Iwai <tiwai@...e.de>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Takashi Iwai <tiwai@...e.de>,
linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: PM runtime auto-cleanup macros
On Fri, 19 Sep 2025 17:52:32 +0200,
Rafael J. Wysocki wrote:
>
> On Fri, Sep 19, 2025 at 3:41 PM Takashi Iwai <tiwai@...e.de> wrote:
> >
> > On Fri, 19 Sep 2025 15:05:04 +0200,
> > Rafael J. Wysocki wrote:
> > >
> > > On Friday, September 19, 2025 9:37:06 AM CEST Takashi Iwai wrote:
> > > > On Thu, 18 Sep 2025 22:41:32 +0200,
> > > > Rafael J. Wysocki wrote:
> > > > >
> > > > > On Thu, Sep 18, 2025 at 10:19 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > > > >
> > > > > > On Thu, Sep 18, 2025 at 1:28 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > > > > >
> > > > > > > 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;
> > > > > > > >
> > > > > >
> > > > > > I guess what I'm saying means basically something like this:
> > > > > >
> > > > > > DEFINE_CLASS(pm_runtime_resume_and_get, struct device *,
> > > > > > if (!IS_ERR_OR_NULL(_T)) pm_tuntime_put(_T),
> > > > > > pm_runtime_resume_and_get_dev(dev), struct device *dev)
> > > > > >
> > > > > > DEFINE_CLASS(pm_runtime_resume_and_get_auto, struct device *,
> > > > > > if (!IS_ERR_OR_NULL(_T)) pm_tuntime_put_autosuspend(_T),
> > > > > > pm_runtime_resume_and_get_dev(dev), struct device *dev)
> > > > > >
> > > > > > and analogously for pm_runtime_get_sync().
> > > > >
> > > > > And it kind of makes sense either. Do
> > > > >
> > > > > CLASS(pm_runtime_resume_and_get, active_dev)(dev);
> > > > > if (IS_ERR(active_dev))
> > > > > return PTR_ERR(active_dev);
> > > > >
> > > > > and now use active_dev for representing the device until it gets out
> > > > > of the scope.
> > > >
> > > > Yes, that's what I thought of as an alternative, too, but I didn't
> > > > consider using only pm_runtime_resume_and_get(). Actually by this
> > > > action, we can also "clean up" the API usage at the same time to use a
> > > > single recommended API function, which is a good thing.
> > > >
> > > > That said, I like this way :)
> > > >
> > > > It'd be nice if this change can go into 6.18, then I can put the
> > > > driver cleanup works for 6.19. It's a bit late stage for 6.18, but
> > > > this change is definitely safe and can't break, per se.
> > >
> > > OK, do you mean something like the patch below?
> >
> > Yes!
>
> OK
>
> > An easy follower is the patch like below.
> > (It's the only user of __free(pm_runtime_*) in linux-next as of now.)
>
> So the __free(pm_runtime_*) could be dropped after this patch I suppose?
Yes, for now it seems so. It was the only user as far as I can see in
linux-next.
> In that case, let me send a series of 3 patches which will add the new
> class definitions, switch over PCI to using them (your patch), and
> drop the existing pm_runtime_put FREE.
OK, will do that.
Takashi
Powered by blists - more mailing lists