Message-ID: <20250923105646.abrbuf4xblctjl4c@lcpd911>
Date: Tue, 23 Sep 2025 16:26:46 +0530
From: Dhruva Gole <d-gole@...com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
CC: Linux PM <linux-pm@...r.kernel.org>, Takashi Iwai <tiwai@...e.de>,
        LKML
	<linux-kernel@...r.kernel.org>,
        Linux PCI <linux-pci@...r.kernel.org>,
        "Alex
 Williamson" <alex.williamson@...hat.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Zhang Qilong <zhangqilong3@...wei.com>,
        Ulf Hansson <ulf.hansson@...aro.org>
Subject: Re: [PATCH v3 1/3] PM: runtime: Add auto-cleanup macros for "resume
 and get" operations

On Sep 23, 2025 at 12:43:21 +0200, Rafael J. Wysocki wrote:
> On Tue, Sep 23, 2025 at 10:53 AM Dhruva Gole <d-gole@...com> wrote:
> >
> > On Sep 22, 2025 at 17:30:43 +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > >
> > > It is generally useful to be able to automatically drop a device's
> > > runtime PM usage counter incremented by runtime PM operations that
> > > resume a device and bump up its usage counter [1].
> > >
> > > To that end, add DEFINE_CLASS() macros allowing pm_runtime_put()
> > > and pm_runtime_put_autosuspend() to be used for the auto-cleanup in
> > > those cases.
> > >
> > > Simply put, a piece of code like below:
> > >
> > >       pm_runtime_get_sync(dev);
> > >       .....
> > >       pm_runtime_put(dev);
> > >       return 0;
> > >
> > > can be transformed with CLASS(pm_runtime_get_sync) like:
> > >
> > >       guard(pm_runtime_get_sync)(dev);
> > >       .....
> > >       return 0;
> > >
> > > (see pm_runtime_put() call is gone).
> > >
> > > However, it is better to do proper error handling in the majority of
> > > cases, so doing something like this instead of the above is recommended:
> > >
> > >       CLASS(pm_runtime_get_active, pm)(dev);
> > >       if (IS_ERR(pm))
> > >               return PTR_ERR(pm);
> > >       .....
> > >       return 0;
> > >
> > > In all of the cases in which runtime PM is known to be enabled for the
> > > given device or the device can be regarded as operational (and so it can
> > > be accessed) with runtime PM disabled, a piece of code like:
> > >
> > >       ret = pm_runtime_resume_and_get(dev);
> > >       if (ret < 0)
> > >               return ret;
> > >       .....
> > >       pm_runtime_put(dev);
> > >       return 0;
> > >
> > > can be simplified with CLASS() like:
> > >
> > >       CLASS(pm_runtime_get_active, pm)(dev);
> > >       if (IS_ERR(pm))
> > >               return PTR_ERR(pm);
> > >       .....
> > >       return 0;
> > >
> > > (again, see pm_runtime_put() call is gone).
> > >
> > > Still, if the device cannot be accessed unless runtime PM has been
> > > enabled for it, the CLASS(pm_runtime_get_active_enabled) variant
> > > needs to be used, that is (in the context of the example above):
> > >
> > >       CLASS(pm_runtime_get_active_enabled, pm)(dev);
> > >       if (IS_ERR(pm))
> > >               return PTR_ERR(pm);
> > >       .....
> > >       return 0;
> > >
> > > When the original code calls pm_runtime_put_autosuspend(), use one
> > > of the "auto" class variants, CLASS(pm_runtime_get_active_auto) or
> > > CLASS(pm_runtime_get_active_enabled_auto), so for example, a piece
> > > of code like:
> > >
> > >       ret = pm_runtime_resume_and_get(dev);
> > >       if (ret < 0)
> > >               return ret;
> > >       .....
> > >       pm_runtime_put_autosuspend(dev);
> > >       return 0;
> > >
> > > will become:
> > >
> > >       CLASS(pm_runtime_get_active_enabled_auto, pm)(dev);
> > >       if (IS_ERR(pm))
> > >               return PTR_ERR(pm);
> > >       .....
> > >       return 0;
> > >
> > > Note that the cases in which the return value of pm_runtime_get_sync()
> > > is checked can also be handled with the help of the new class macros.
> > > For example, a piece of code like:
> > >
> > >       ret = pm_runtime_get_sync(dev);
> > >       if (ret < 0) {
> > >               pm_runtime_put(dev);
> > >               return ret;
> > >       }
> > >       .....
> > >       pm_runtime_put(dev);
> > >       return 0;
> > >
> > > can be rewritten as:
> > >
> > >       CLASS(pm_runtime_get_active_enabled, pm)(dev);
> > >       if (IS_ERR(pm))
> > >               return PTR_ERR(pm);
> > >       .....
> > >       return 0;
> > >
> > > or CLASS(pm_runtime_get_active) can be used if transparent handling of
> > > disabled runtime PM is desirable.
> > >
> >
> > Firstly, please can we add all this documentation in runtime_pm [1]
> > Otherwise there's just far less developers aware of the new APIs getting
> > introduced other than people directly involved. Not everyone is going to
> > come down here to look at git log for API docs (even though we proud
> > ourselves in having git log as our main source of Documentation in
> > kernel ;) )
> >
> > [1] https://docs.kernel.org/power/runtime_pm.html
> 
> That will happen when the early adopters tell me that it works for them.

Okay..

> 
> > > Link: https://lore.kernel.org/linux-pm/878qimv24u.wl-tiwai@suse.de/ [1]
> > > Co-developed-by: Takashi Iwai <tiwai@...e.de>
> > > Signed-off-by: Takashi Iwai <tiwai@...e.de>
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> > > ---
> > >
> > > v2 -> v3:
> > >    * Two more class definitions for the case in which resume errors can be
> > >      neglected.
> > >    * Update of new code comments (for more clarity).
> > >    * Changelog update.
> > >
> > > v1 -> v2:
> > >    * Rename the new classes and the new static inline helper.
> > >    * Add two classes for handling disabled runtime PM.
> > >    * Expand the changelog.
> > >    * Adjust the subject.
> > >
> > > ---
> > >  drivers/base/power/runtime.c |    2 +
> > >  include/linux/pm_runtime.h   |   82 +++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 84 insertions(+)
> >
> > >
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -796,6 +796,8 @@ static int rpm_resume(struct device *dev
> > >               if (dev->power.runtime_status == RPM_ACTIVE &&
> > >                   dev->power.last_status == RPM_ACTIVE)
> > >                       retval = 1;
> > > +             else if (rpmflags & RPM_TRANSPARENT)
> > > +                     goto out;
> >
> > "TRANSPARENT" doesn't tell you exactly what happens. It should be something like
> > RPM_IGNORE_DISABLED or RPM_ALLOW_DISABLED IMO.
> 
> There is a description in the header file and TRANSPARENT is shorter
> than the alternatives.  Besides, this is mostly for internal use.

OK.

> 
> > >               else
> > >                       retval = -EACCES;
> > >       }
> > > --- a/include/linux/pm_runtime.h
> > > +++ b/include/linux/pm_runtime.h
> > > @@ -21,6 +21,7 @@
> > >  #define RPM_GET_PUT          0x04    /* Increment/decrement the
> > >                                           usage_count */
> > >  #define RPM_AUTO             0x08    /* Use autosuspend_delay */
> > > +#define RPM_TRANSPARENT      0x10    /* Succeed if runtime PM is disabled */
> > >
> > >  /*
> > >   * Use this for defining a set of PM operations to be used in all situations
> > > @@ -533,6 +534,32 @@ static inline int pm_runtime_resume_and_
> > >  }
> > >
> > >  /**
> > > + * pm_runtime_get_active_dev - Resume a device and bump up its usage counter.
> >
> > I am getting no clue as to why this is different than regular
> > pm_runtime_get_sync then? Can we describe this API better?
> 
> Again, this is for internal use.
> 
> I may as well drop the kerneldoc for this function altogether, but I'm
> not going to send a v4 just for this purpose.

Alright then, fine by me since most comments were nits, and I am ok with
the general idea here...

Reviewed-by: Dhruva Gole <d-gole@...com>

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated

Powered by blists - more mailing lists