Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency

[Rafael J. Wysocki (Intel)]

I should have responded to the v1 of this, but didn't manage to get to
it.  Sorry about that.

On Tue, Nov 3, 2026 at 11:23 PM Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
>
> When a child device suspends, it does not update the last_busy timestamp
> for its parent. If that parent configured autosuspend and didn't
> otherwise maintain its last_busy timestamp, it may now be immediately
> eligible to suspend. This is probably not expected -- the parent should
> wait for its autosuspend delay before suspending.

Let's first talk about what autosuspend is for: My understanding is
that it is there to avoid incurring the runtime PM suspend and resume
latency (and cost in terms of energy) in the cases when the device is
known or at least is expected to be needed in the near future.  IOW,
we want to defer the runtime suspend of the device because if it
suspended, it would be resumed shortly thereafter.

I'm not sure if that is always the case when a dependent device
suspends and its reference to the parent/supplier is dropped.

The core calls pm_runtime_mark_last_busy() for resuming devices under
the assumption that they are resumed for a reason and so it is likely
that they will be needed in near future, but I'm not sure if that
assumption can be extended to a child suspend case.  One might argue
though that extending it to the child/consumer resume case would make
sense.

> The effect of this behavior is that a parent device may suspend sooner
> than its autosuspend delay, simply because its usage was accounted by
> its children, and not by direct references to the parent device.

The distinction between "direct references to the parent device" and
"usage accounted for by its children" is not clear to me TBH.

There are references to the parent device in both cases and it is not
like drivers can always expect all runtime PM references to come from
specific code paths updating last_busy.  There is at least one
reference controlled by user space (the "control" attribute in sysfs)
and dropping it may also cause the device to suspend immediately if
last_busy has not been updated recently.  How is this situation
different from the one related to child/consumer suspend?

> This was noticed in several cases, and some have implemented
> workarounds, such as in commit c537d3457542 ("iio: adc: stm32-adc: fix
> runtime autosuspend delay when slow polling"). At the same time, Ulf
> suggested these problems "should be solved in the runtime PM core" [1].

So I need to be explained how commit 9bdbb1139ca1 ("iio: adc:
stm32-adc: add power management support") works because ATM I don't
seem to understand it.  It looks to me like the .runtime_idle()
callback updates last_busy for the device for which it is called, not
for its parent.

> Instead of working around the problem in drivers, we propagate last_busy
> timestamps from a dependent device to its dependencies any time it may
> allow a dependency to suspend -- i.e., when releasing a refcount for its
> parent or suppliers. We take care to only propagate the timestamp if it
> is larger than the existing busy timestamp.

I'm not sure why propagating last_busy timestamps when dropping the
reference to the parent/supplier is a good idea.

Why is the child's last_busy timestamp suitable for the parent in the
first place?

Why isn't done when dropping the reference?

> Note that this works best if the dependent device is using autosuspend
> (and therefore updates its last_busy timestamps appropriately), but even
> for a non-autosuspend child, this is still somewhat useful --
> non-autosuspend devices still automatically update their last_busy every
> time they resume.
>
> Link: https://lore.kernel.org/all/CAPDyKFp=KTf8=zGBSzPYqhjnZpY8xwvjCeM1e-WTKT1QLSxaDA@xxxxxxxxxxxxxx/ [1]
> Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> ---
> Cc: Ulf Hansson <ulfh@xxxxxxxxxx>
>
> Changes in v2:
>  * Update comments to explain when/why rpm_propagate_last_busy()
>    performs the update
>
>  drivers/base/power/runtime.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 2a117d6a776f..dd306cbabcd3 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -318,6 +318,22 @@ static int rpm_get_suppliers(struct device *dev)
>         return 0;
>  }
>
> +/*
> + * Propagate last_busy timestamp from one device to another, typically when a
> + * device (@dev) releases a reference count on a dependency (@target) and might
> + * allow it to (auto)suspend. This can, for example, prevent overactive suspend
> + * when a dependency's usage is primarily driven by one of its dependents.

AFAIAC, you need to say much more than this to explain all of it.

> + */
> +static void rpm_propagate_last_busy(struct device *dev, struct device *target)
> +{
> +       s64 busy = atomic64_read(&dev->power.last_busy);
> +       s64 target_busy = atomic64_read(&target->power.last_busy);
> +
> +       while (target_busy < busy)
> +               if (atomic64_try_cmpxchg(&target->power.last_busy, &target_busy, busy))
> +                       return;
> +}

And I'd rather not do anything like the above unless absolutely necessary.

> +
>  /**
>   * pm_runtime_release_supplier - Drop references to device link's supplier.
>   * @link: Target device link.
> @@ -348,6 +364,7 @@ static void __rpm_put_suppliers(struct device *dev, bool try_to_suspend)
>                 if (!device_link_test(link, DL_FLAG_PM_RUNTIME))
>                         continue;
>
> +               rpm_propagate_last_busy(dev, link->supplier);
>                 pm_runtime_release_supplier(link);
>                 if (try_to_suspend)
>                         pm_request_idle(link->supplier);
> @@ -710,6 +727,8 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>
>         if (dev->parent) {
>                 parent = dev->parent;
> +               if (!parent->power.ignore_children)
> +                       rpm_propagate_last_busy(dev, parent);
>                 atomic_add_unless(&parent->power.child_count, -1, 0);
>         }
>         wake_up_all(&dev->power.wait_queue);
> @@ -1368,8 +1387,10 @@ int __pm_runtime_set_status(struct device *dev, unsigned int status)
>                 goto out_set;
>
>         if (status == RPM_SUSPENDED) {
> -               atomic_add_unless(&parent->power.child_count, -1, 0);
>                 notify_parent = !parent->power.ignore_children;
> +               if (notify_parent)
> +                       rpm_propagate_last_busy(dev, parent);
> +               atomic_add_unless(&parent->power.child_count, -1, 0);
>         } else {
>                 spin_lock_nested(&parent->power.lock, SINGLE_DEPTH_NESTING);
>
> --