Re: PM runtime auto-cleanup macros

[Takashi Iwai <tiwai@suse.de>]

On Fri, 19 Sep 2025 15:41:42 +0200,
Takashi Iwai 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@kernel.org> wrote:
> > > > >
> > > > > On Thu, Sep 18, 2025 at 1:28 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > >
> > > > > > On Thu, Sep 18, 2025 at 9:10 AM Takashi Iwai <tiwai@suse.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@suse.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!
> 
> An easy follower is the patch like below.

Err, I forgot to refresh before generating a patch.
The proper one is below.


Takashi

-- 8< --
Subject: [PATCH] PCI: Use PM runtime class macro for the auto cleanup

The newly introduced class macro can simplify the code.
Also, add the proper error handling for the PM runtime get, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/pci/pci-sysfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 5eea14c1f7f5..87c2311494bf 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1475,8 +1475,9 @@ static ssize_t reset_method_store(struct device *dev,
 		return count;
 	}
 
-	pm_runtime_get_sync(dev);
-	struct device *pmdev __free(pm_runtime_put) = dev;
+	CLASS(pm_runtime_resume_and_get, pmdev)(dev);
+	if (IS_ERR(pmdev))
+		return PTR_ERR(pmdev);
 
 	if (sysfs_streq(buf, "default")) {
 		pci_init_reset_methods(pdev);
-- 
2.50.1