Re: PM runtime auto-cleanup macros

[Takashi Iwai <tiwai@suse.de>]

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.

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);
	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;


thanks,

Takashi