PM runtime auto-cleanup macros

PM runtime auto-cleanup macros

[Takashi Iwai]

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)

#define pm_runtime_auto_clean_autosuspend(dev, var) \
	struct device *var __free(pm_runtime_put_autosuspend) = (dev)

and the code will be like:

	pm_runtime_get_sync(dev);
	pm_runtime_auto_clean(dev, pmdev);

or
	ret = pm_runtime_resume_and_get(dev);
	if (ret < 0)
		return ret;
	pm_runtime_auto_clean_autosuspend(dev, pmdev);

Alternatively, we may define a class, e.g.

	CLASS(pm_runtime_resume_and_get, pmdev);
	if (pmdev.ret < 0)
		return pmdev.ret;

but it'll be a bit more code to define the full class, and the get*()
and put*() combination would be fixed with this approach -- which is a
downside.

All above are an idea for now.  Let me know if I should go further
along with this, or there is already a better another approach.

(And the macros can be better named, sure :)


thanks,

Takashi

Re: PM runtime auto-cleanup macros

[Takashi Iwai]

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)


Takashi

> 
> #define pm_runtime_auto_clean_autosuspend(dev, var) \
> 	struct device *var __free(pm_runtime_put_autosuspend) = (dev)
> 
> and the code will be like:
> 
> 	pm_runtime_get_sync(dev);
> 	pm_runtime_auto_clean(dev, pmdev);
> 
> or
> 	ret = pm_runtime_resume_and_get(dev);
> 	if (ret < 0)
> 		return ret;
> 	pm_runtime_auto_clean_autosuspend(dev, pmdev);
> 
> Alternatively, we may define a class, e.g.
> 
> 	CLASS(pm_runtime_resume_and_get, pmdev);
> 	if (pmdev.ret < 0)
> 		return pmdev.ret;
> 
> but it'll be a bit more code to define the full class, and the get*()
> and put*() combination would be fixed with this approach -- which is a
> downside.
> 
> All above are an idea for now.  Let me know if I should go further
> along with this, or there is already a better another approach.
> 
> (And the macros can be better named, sure :)
> 
> 
> thanks,
> 
> Takashi

Re: PM runtime auto-cleanup macros

[Rafael J. Wysocki]

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

Re: PM runtime auto-cleanup macros

[Takashi Iwai]

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

Re: PM runtime auto-cleanup macros

[Rafael J. Wysocki]

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

Re: PM runtime auto-cleanup macros

[Rafael J. Wysocki]

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().

Re: PM runtime auto-cleanup macros

[Rafael J. Wysocki]

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.

Re: PM runtime auto-cleanup macros

[Takashi Iwai]

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.


Thanks!

Takashi

Re: PM runtime auto-cleanup macros

[Rafael J. Wysocki]

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?

---
 include/linux/pm_runtime.h |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -533,6 +533,30 @@ static inline int pm_runtime_resume_and_
 }
 
 /**
+ * pm_runtime_resume_and_get_dev - Resume device and bump up its usage counter.
+ * @dev: Target device.
+ *
+ * Resume @dev synchronously and if that is successful, increment its runtime
+ * PM usage counter.
+ *
+ * Return:
+ * * 0 if the runtime PM usage counter of @dev has been incremented.
+ * * Negative error code otherwise.
+ */
+static inline struct device *pm_runtime_resume_and_get_dev(struct device *dev)
+{
+	int ret;
+
+	ret = __pm_runtime_resume(dev, RPM_GET_PUT);
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
+		return ERR_PTR(ret);
+	}
+
+	return dev;
+}
+
+/**
  * pm_runtime_put - Drop device usage counter and queue up "idle check" if 0.
  * @dev: Target device.
  *
@@ -606,6 +630,25 @@ static inline int pm_runtime_put_autosus
 	return __pm_runtime_put_autosuspend(dev);
 }
 
+/*
+ * The way to use the classes defined below is to define a class variable and
+ * use it going forward for representing the target device until it goes out of
+ * the scope.  For example:
+ *
+ * CLASS(pm_runtime_resume_and_get, active_dev)(dev);
+ * if (IS_ERR(active_dev))
+ *         return PTR_ERR(active_dev);
+ *
+ * ... do something with active_dev (which is guaranteed to never suspend) ...
+ */
+DEFINE_CLASS(pm_runtime_resume_and_get, struct device *,
+	     if (!IS_ERR_OR_NULL(_T)) pm_runtime_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_runtime_put_autosuspend(_T),
+	     pm_runtime_resume_and_get_dev(dev), struct device *dev)
+
 /**
  * pm_runtime_put_sync - Drop device usage counter and run "idle check" if 0.
  * @dev: Target device.

Re: PM runtime auto-cleanup macros

[Takashi Iwai]

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.
(It's the only user of __free(pm_runtime_*) in linux-next as of now.)


thanks,

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..1fa329f95844 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_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

Re: PM runtime auto-cleanup macros

[Takashi Iwai]

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

Re: PM runtime auto-cleanup macros

[Rafael J. Wysocki]

On Fri, Sep 19, 2025 at 3:43 PM Takashi Iwai <tiwai@suse.de> wrote:
>
> 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);

This error will propagate to user space AFAICS and some of the values
may be confusing in that respect, so it may be better to return -ENXIO
here.

>
>         if (sysfs_streq(buf, "default")) {
>                 pci_init_reset_methods(pdev);
> --

Re: PM runtime auto-cleanup macros

[Rafael J. Wysocki]

On Fri, Sep 19, 2025 at 3:41 PM Takashi Iwai <tiwai@suse.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@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!

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?

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.

Re: PM runtime auto-cleanup macros

[Takashi Iwai]

On Fri, 19 Sep 2025 17:49:16 +0200,
Rafael J. Wysocki wrote:
> 
> On Fri, Sep 19, 2025 at 3:43 PM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > 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);
> 
> This error will propagate to user space AFAICS and some of the values
> may be confusing in that respect, so it may be better to return -ENXIO
> here.

Fair enough.


thanks,

Takashi

Re: PM runtime auto-cleanup macros

[Takashi Iwai]

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@suse.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@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!
> 
> 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