Re: [PATCH v4 2/2] PM: runtime: Add Example Driver Patterns section

[Brian Norris <briannorris@chromium.org>]

Hi Rafael,

On Wed, Oct 14, 2026 at 09:07:54PM +0200, Rafael J. Wysocki (Intel) wrote:
> On Thu, Oct 1, 2026 at 5:51 PM Brian Norris <briannorris@chromium.org> wrote:
> >
> > The runtime PM API surface is pretty large, but there are a few common
> > patterns that many drivers should follow. Add some illustrative
> > examples, to help guide the most common audience for runtime PM docs --
> > driver writers.
> >
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> >
> > Changes in v4:
> >  * "state" to "status", to be consistent with Introduction terminology
> >
> > Changes in v3:
> >  * Rewrite Examples to avoid devm_* at first, to highlight correct
> >    approaches
> >  * Include error-handling and remove() in any probe() Examples
> >  * Advise more caution when using devm_*, as ordering issues are subtle
> >
> > Changes in v2:
> >  * Add appropriate teardown to "Probe with Hardware Powered Off"
> >    Example, as the remove() + power-off behavior is subtle here, and
> >    easy to get wrong
> >
> >  Documentation/power/runtime_pm.rst | 475 ++++++++++++++++++++++++++++-
> >  1 file changed, 473 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> > index e4407b687f8b..cb25e0bb4ca7 100644
> > --- a/Documentation/power/runtime_pm.rst
> > +++ b/Documentation/power/runtime_pm.rst
> > @@ -429,7 +429,8 @@ in ->probe(). Usually this means calling pm_runtime_disable(),
> >  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.
> > +detachment. See also the Examples in `Section 10`_ for some effective
> > +strategies for driver teardown.
> >
> >  The user space can effectively disallow the driver of the device to power manage
> >  it at run time by changing the value of its /sys/devices/.../power/control
> > @@ -654,7 +655,9 @@ The implementation is well suited for asynchronous use in interrupt contexts.
> >  However such use inevitably involves races, because the PM core can't
> >  synchronize ->runtime_suspend() callbacks with the arrival of I/O requests.
> >  This synchronization must be handled by the driver, using its private lock.
> > -Here is a schematic pseudo-code example::
> > +Here is a schematic pseudo-code example:
> > +
> > +.. code-block:: c
> >
> >         foo_read_or_write(struct foo_priv *foo, void *data)
> >         {
> > @@ -720,3 +723,471 @@ pm_runtime_autosuspend_expiration() from within the ->runtime_suspend()
> >  callback while holding its private lock.  If the function returns a nonzero
> >  value then the delay has not yet expired and the callback should return
> >  -EAGAIN.
> > +
> > +.. _Section 10:
> > +
> > +10. Example Driver Patterns
> > +===========================
> > +
> > +The runtime PM API is large and complex, but most device drivers follow a small
> > +set of canonical patterns when interacting with runtime PM. This section
> > +illustrates standard patterns for device probing, performing I/O, and handling
> > +interrupts.
> > +
> > +Probe and Initialization
> > +------------------------
> > +
> > +Basic Probe
> > +~~~~~~~~~~~
> > +
> > +A driver that powers on its hardware during probe and does not use autosuspend
> > +can initialize runtime PM using standard helpers:
> > +
> > +.. code-block:: c
> > +
> > +       static int foo_probe(struct platform_device *pdev)
> > +       {
> > +               struct device *dev = &pdev->dev;
> > +               struct foo_priv *priv;
> > +               int ret;
> > +
> > +               priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +               if (!priv)
> > +                       return -ENOMEM;
> > +
> > +               platform_set_drvdata(pdev, priv);
> > +
> > +               /* Power on and initialize hardware registers... */
> > +
> > +               /*
> > +                * The code above left hardware powered on and operational, so
> > +                * tell the PM core that the device is active before enabling
> > +                * runtime PM.
> > +                */
> > +               pm_runtime_set_active(dev);
> > +               pm_runtime_enable(dev);
> > +
> > +               /*
> > +                * Perform remaining driver setup (e.g. register interrupts, device nodes).
> > +                * If any step fails after enabling runtime PM:
> > +                */
> > +               ret = foo_register_device(priv);
> > +               if (ret)
> > +                       goto err_pm_disable;
> > +
> > +               /*
> > +                * Upon successful return from ->probe(), the driver core
> > +                * automatically executes pm_request_idle(dev), allowing the
> > +                * device to suspend asynchronously if its usage counter is zero.
> > +                */
> > +               return 0;
> > +
> > +       err_pm_disable:
> > +               pm_runtime_disable(dev);
> > +               if (!pm_runtime_status_suspended(dev)) {
> > +                       /*
> > +                        * Power back off, in case device did not suspend yet.
> > +                        * For example, leverage your .runtime_suspend()
> > +                        * implementation:
> > +                        */
> > +                       foo_runtime_suspend(dev);
> 
> There are two reasons why this may not work, here and below.
> 
> First off, if a non-trivial PM domain or bus type etc is involved, its
> .runtime_suspend() callback may need to be invoked in addition to
> foo_runtime_suspend() to actually power off the device.

Good point. While it's a platform_device (not a complex bus), I suppose
the PM domain may be relevant, at least for remove(). (probe() is
already handled automatically by the device core.)

> That's why
> pm_runtime_force_suspend() looks for the callback to invoke.

Would you be OK with pm_runtime_force_suspend() as an alternative here?
It seems like some drivers have started using that in remove(), which
doesn't quite match its docs, but otherwise seems OK to me. I was
considering that already here, and I even think it might be a good idea
to do automatically in devm_*, as I noted here:

  https://lore.kernel.org/all/arVWVA3lks7SopQH@google.com/

  (note: there's definitely at least one typo in there; should be
  !pm_runtime_status_suspended(); and it needs some other adjustments to
  actually verify the device was suspended at time of enable(); ... so
  consider it a concept, not a patch.)

But I kinda wanted a maintainer opinion before I went too far on that.

> Second, if the driver expects the device to be powered up and
> operational at the beginning of probe, which this one does AFAICS,

No, that's not the intention. See:

  "A driver that powers on its hardware during probe"

And the pseudocode/comment:

               /* Power on and initialize hardware registers... */

i.e., probe() is doing the power-on work, but it's elided.

Perhaps that wasn't obvious enough, so I can replace that with
more-convincing code-like structures
(regulator_get()/regulator_enable(), etc., for example) if you think
that would help.

Brian

> it
> better should leave the device in that state on failing probe and
> remove or its own expectations regarding the initial state will not be
> met when it is re-probed.
> 
> > +                       pm_runtime_set_suspended(dev);
> > +               }
> > +               return ret;
> > +       }
> > +
> > +       static void foo_remove(struct platform_device *pdev)
> > +       {
> > +               struct device *dev = &pdev->dev;
> > +               struct foo_priv *priv = platform_get_drvdata(pdev);
> > +
> > +               foo_unregister_device(priv);
> > +
> > +               pm_runtime_disable(dev);
> > +               /*
> > +                * The runtime PM status is not guaranteed at this point. We
> > +                * could be either suspended or active, so we need to handle
> > +                * both.
> > +                */
> > +               if (!pm_runtime_status_suspended(dev)) {
> > +                       /* Power back off, in case device did not suspend yet. */
> > +                       foo_runtime_suspend(dev);
> > +                       pm_runtime_set_suspended(dev);
> > +               }
> > +       }
> > +