Re: [PATCH v2 3/8] PM: runtime: Misc improvements to runtime_pm.rst

[Brian Norris <briannorris@chromium.org>]

On Mon, Sep 28, 2026 at 03:15:43PM +0200, Ulf Hansson wrote:
> On Thu, Sep 24, 2026 at 6:56 PM Brian Norris <briannorris@chromium.org> wrote:
> >
> > On Thu, Sep 24, 2026 at 04:01:00PM +0200, Ulf Hansson wrote:
> > > On Wed, Sep 23, 2026 at 7:47 PM Brian Norris <briannorris@chromium.org> wrote:
> > > > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> > > > index 39fdeeda7a1e..352cdaf0650d 100644
> > > > --- a/Documentation/power/runtime_pm.rst
> > > > +++ b/Documentation/power/runtime_pm.rst
> >
> > > > @@ -315,7 +319,10 @@ removal of their drivers.
> > > >
> > > >  Drivers in ->remove() callback should undo the runtime PM changes done
> > > >  in ->probe(). Usually this means calling pm_runtime_disable(),
> > > > -pm_runtime_dont_use_autosuspend() etc.
> > > > +pm_runtime_dont_use_autosuspend() etc. Alternatively, drivers can use
> > > > +devm_pm_runtime_enable() during probe, which automatically takes care of
> > > > +calling pm_runtime_disable() and pm_runtime_dont_use_autosuspend() upon driver
> > > > +detachment.
> > >
> > > As I have stated in earlier discussions at LKML, the
> > > devm_pm_runtime_enable() API is not entirely easy to use correctly by
> > > drivers. It means that pm_runtime_disable() gets called at some point
> > > *after* the ->remove() callback has been invoked, which can cause
> > > problems, unless the driver's ->remove() callback has managed things
> > > correctly.
> >
> > Yeah. And I think it's rare for drivers to have done a thorough job. A
> > rare exception: I found commit 2d90ecdfa326 ("ASoC: rockchip: i2s: Use
> > managed hclk and runtime PM cleanup") an interesting outlier -- it adds
> > an additional devres teardown to power things off afterward.
> >
> > OTOH, between v1 and v2, I chose to tweak one of the Examples to avoid
> > devm, precisely because it was committing (or hinting at) these kinds of
> > mistakes.
> >
> > > My point is, the above makes it sounds like it's easy to switch to the
> > > devm managed version, while it certainly isn't that straight forward.
> >
> > Right, I said as much in the cover letter too:
> >
> > (possible future work)
> > <quote>
> >  * Adjust the way devm_pm_runtime_enable() works, specifically for
> >    remove()/teardown. Currently, this is very hard to use correctly --
> >    some common driver patterns may assume that a device will tear down
> >    while RPM_SUSPENDED; but that's not actually guaranteed. Notably,
> >    this makes some of the "Examples" section fairly tricky/subtle.
> > </quote>
> 
> I think having some examples that *don't* use devm_pm_runtime_enable()
> would make better sense, as it would show what is needed to take care
> of things correctly.
> 
> Stating that there is devm_pm_runtime_enable() available would of
> course be fine too, but in that context, I think we should point out
> that the user really needs to address the ordering problems that get
> introduced when using it.

Yep, that's exactly I took out of the patch 8 discussion. I have such a
revision ready to send out in v3.

(Some part of this is a general problem with devm_*; for one, if you
only use it partially, and still have some manual teardown in remove(),
there's a high probability you'll get the ordering wrong. But that's
general advice, and not really specific to RPM documentation, IMO.)

> > Would this be a good moment to pass this possibility by you? What if we
> > taught the teardown to force a device back to RPM_SUSPENDED? Something
> > like:
> >
> >     static void pm_runtime_disable_action(void *data)
> >     {
> >             pm_runtime_dont_use_autosuspend(data);
> >             pm_runtime_disable(data);
> >
> >             // New code:
> >             if (pm_runtime_status_suspended(data)) {
> >                     int (*callback)(struct device *);
> >                     int ret;
> >
> >                     callback = GET_CALLBACK(data, runtime_suspend);
> >                     ret = callback ? callback(data) : 0;
> >                     if (ret)
> >                             return;
> >
> >                     pm_runtime_set_suspended(data);
> >             }
> >     }
> 
> pm_runtime_reinit() is already taking care of some of the above.

It gets the set_suspended() part, but not the real key point -- running
the suspend callback.

I believe this is one of the bigger RPM-specific misconceptions and
pitfalls people make, and is made worse with devm_pm_runtime_enable():
people assume that as long as their driver isn't trying to use the
device (e.g., they've closed any open handles, etc.), then the device
will leave in the same state as it came in -- suspended. But that's
absolutely not the case, due to:

1) race conditions -- async put()/suspend is not guaranteed to complete
   before pm_runtime_disable(), and therefore the post-disable status
   is not guaranteed.

2) forbid() -- if user space forbade runtime PM (on >
   .../power/control), the device will not suspend.

I think it's a clear win to ensure balance -- that
devm_pm_runtime_enable() can ensure the device leaves the same way it
came in -- suspended. (Or, if that was somehow not true: I could add a
check into devm_pm_runtime_enable() to make this conditional on initial
runtime_status.)

Note that many (most?) drivers *do* care about balance -- e.g., they
expect a balanced regulator_enable()/disable(), because resource
teardown (regulator_put()) will fire a WARN_ON() otherwise.

Anyway, unless I hear major objection, I'll try to put my proposal into
a proper patch + description, so it can be reviewed on its own.

> Moreover, we have pm_runtime_force_suspend(), which may fit well for
> some cases, but not for all.

Yeah, I was imitating that, more or less. I suppose there's no harm in
using it here though. I'll think about it.

Brian