* [PATCH v2 0/5] PM: runtime: Improve autosuspend child to parent propagation
@ 2026-11-03 22:14 Brian Norris
2026-11-03 22:14 ` [PATCH v2 1/5] PM: runtime: Only queue an idle check for RPM-linked suppliers (part 2) Brian Norris
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: Brian Norris @ 2026-11-03 22:14 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson,
linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron,
David Lechner, Alexandre Torgue, Nuno Sá,
Maxime Coquelin, Brian Norris
v1:
https://lore.kernel.org/all/20261002230714.507921-1-briannorris@chromium.org/
[PATCH 0/5] PM: runtime: Improve autosuspend child to parent propagation
See patch 3 for the primary change here.
In short: today, children can resume/suspend without extending the
autosuspend expiration of their parent. For parents where their activity
is largely driven by their children, this makes it easy to experience
"premature" suspend.
Many .runtime_idle() implementations seem to try to work around this
problem, and one in particular (stm32-adc) documented their problems
heavily in c537d3457542 ("iio: adc: stm32-adc: fix runtime autosuspend
delay when slow polling").
This is an attempt to improve the PM core, as suggested in review for that
change:
https://lore.kernel.org/all/CAPDyKFp=KTf8=zGBSzPYqhjnZpY8xwvjCeM1e-WTKT1QLSxaDA@mail.gmail.com/
This will make it easier to simplify many other drivers too, as a
significant use of .runtime_idle() is to simply extend a parent's
last_busy timestamp, as a proxy for "last activity performed by a
child".
The changes:
1) Preparatory fix, noticed while making the following changes
2) Because the last_busy mathematics get a little more complex, I have
to change last_busy to use an atomic64_t type
3) Propagate last_busy from dependent devices (children, consumers) to
their dependencies (parent, suppliers)
4) Add KUnit tests
5) Remove the pm_runtime_mark_last_busy() workaround from the stm32-adc
driver
To follow on with #5: I believe almost every other .runtime_idle()
implementation in the tree can be removed after patch 3. (There are a
handful of drivers that should just use autosuspend. And perhaps 2 or 3
other drivers that do other non-trivial work with .runtime_idle(). But
most are redundant.) I may work on that next, if this works out.
- Brian
Changes in v2:
* Update comments to explain when/why rpm_propagate_last_busy()
performs the update
* Minor kunit test improvements
Brian Norris (5):
PM: runtime: Only queue an idle check for RPM-linked suppliers (part
2)
PM: runtime: Convert last_busy to atomic64_t
PM: runtime: Propagate last_busy from dependent to dependency
PM: runtime: Add tests for last_busy propagation
iio: adc: stm32-adc: Drop runtime_idle()
drivers/base/power/runtime-test.c | 99 +++++++++++++++++++++++++++++++
drivers/base/power/runtime.c | 30 +++++++++-
drivers/iio/adc/stm32-adc-core.c | 9 +--
include/linux/pm.h | 3 +-
include/linux/pm_runtime.h | 3 +-
5 files changed, 132 insertions(+), 12 deletions(-)
--
2.56.0.398.g882cbad275-goog
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 1/5] PM: runtime: Only queue an idle check for RPM-linked suppliers (part 2) 2026-11-03 22:14 [PATCH v2 0/5] PM: runtime: Improve autosuspend child to parent propagation Brian Norris @ 2026-11-03 22:14 ` Brian Norris 2026-11-04 14:44 ` Rafael J. Wysocki (Intel) 2026-11-03 22:14 ` [PATCH v2 2/5] PM: runtime: Convert last_busy to atomic64_t Brian Norris ` (3 subsequent siblings) 4 siblings, 1 reply; 22+ messages in thread From: Brian Norris @ 2026-11-03 22:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin, Brian Norris Commit be5755f023e7 ("PM: runtime: Only queue an idle check for RPM-linked suppliers") was incomplete. There are two other cases where we perform idle checks on non-DL_FLAG_PM_RUNTIME suppliers -- in pm_runtime_set_suspended(), and in cases where we failed to resume suppliers. Fixes: 5244f5e2d801 ("PM: runtime: Defer suspending suppliers") Signed-off-by: Brian Norris <briannorris@chromium.org> --- Noticed while touching this function in subsequent work. (no changes since v1) drivers/base/power/runtime.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 304a2b123420..9d3508d9b4a2 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -343,6 +343,9 @@ static void __rpm_put_suppliers(struct device *dev, bool try_to_suspend) list_for_each_entry_rcu(link, &dev->links.suppliers, c_node, device_links_read_lock_held()) { + if (!device_link_test(link, DL_FLAG_PM_RUNTIME)) + continue; + pm_runtime_release_supplier(link); if (try_to_suspend) pm_request_idle(link->supplier); -- 2.56.0.398.g882cbad275-goog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 1/5] PM: runtime: Only queue an idle check for RPM-linked suppliers (part 2) 2026-11-03 22:14 ` [PATCH v2 1/5] PM: runtime: Only queue an idle check for RPM-linked suppliers (part 2) Brian Norris @ 2026-11-04 14:44 ` Rafael J. Wysocki (Intel) 0 siblings, 0 replies; 22+ messages in thread From: Rafael J. Wysocki (Intel) @ 2026-11-04 14:44 UTC (permalink / raw) To: Brian Norris Cc: Rafael J. Wysocki, Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin On Tue, Nov 3, 2026 at 11:23 PM Brian Norris <briannorris@chromium.org> wrote: > > Commit be5755f023e7 ("PM: runtime: Only queue an idle check for > RPM-linked suppliers") was incomplete. There are two other cases where > we perform idle checks on non-DL_FLAG_PM_RUNTIME suppliers -- in > pm_runtime_set_suspended(), and in cases where we failed to resume > suppliers. > > Fixes: 5244f5e2d801 ("PM: runtime: Defer suspending suppliers") > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > Noticed while touching this function in subsequent work. > > (no changes since v1) > > drivers/base/power/runtime.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 304a2b123420..9d3508d9b4a2 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -343,6 +343,9 @@ static void __rpm_put_suppliers(struct device *dev, bool try_to_suspend) > > list_for_each_entry_rcu(link, &dev->links.suppliers, c_node, > device_links_read_lock_held()) { > + if (!device_link_test(link, DL_FLAG_PM_RUNTIME)) > + continue; > + > pm_runtime_release_supplier(link); > if (try_to_suspend) > pm_request_idle(link->supplier); > -- Applied as 7.4-rc material, thanks! ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 2/5] PM: runtime: Convert last_busy to atomic64_t 2026-11-03 22:14 [PATCH v2 0/5] PM: runtime: Improve autosuspend child to parent propagation Brian Norris 2026-11-03 22:14 ` [PATCH v2 1/5] PM: runtime: Only queue an idle check for RPM-linked suppliers (part 2) Brian Norris @ 2026-11-03 22:14 ` Brian Norris 2026-11-04 8:21 ` Joshua Crofts 2026-11-03 22:14 ` [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency Brian Norris ` (2 subsequent siblings) 4 siblings, 1 reply; 22+ messages in thread From: Brian Norris @ 2026-11-03 22:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin, Brian Norris I want to use the atomic64 compare/exchange helpers. There are no similar 32-bit-portable helpers for non-atomic64_t. As a side effect, this also should avoid some minor load/store tearing corner cases on 32-bit architectures. Signed-off-by: Brian Norris <briannorris@chromium.org> --- Changes in v2: * Add <linux/atomic.h> to pm_runtime.h too drivers/base/power/runtime.c | 4 +++- include/linux/pm.h | 3 ++- include/linux/pm_runtime.h | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 9d3508d9b4a2..2a117d6a776f 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -5,6 +5,8 @@ * Copyright (c) 2009 Rafael J. Wysocki <rjw@sisk.pl>, Novell Inc. * Copyright (C) 2010 Alan Stern <stern@rowland.harvard.edu> */ + +#include <linux/atomic.h> #include <linux/sched/mm.h> #include <linux/ktime.h> #include <linux/hrtimer.h> @@ -186,7 +188,7 @@ u64 pm_runtime_autosuspend_expiration(struct device *dev) if (autosuspend_delay < 0) return 0; - expires = READ_ONCE(dev->power.last_busy); + expires = atomic64_read(&dev->power.last_busy); expires += (u64)autosuspend_delay * NSEC_PER_MSEC; if (expires > ktime_get_mono_fast_ns()) return expires; /* Expires in the future */ diff --git a/include/linux/pm.h b/include/linux/pm.h index ef3f1310e749..531e46ff878e 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -8,6 +8,7 @@ #ifndef _LINUX_PM_H #define _LINUX_PM_H +#include <linux/atomic.h> #include <linux/completion.h> #include <linux/export.h> #include <linux/hrtimer_types.h> @@ -812,7 +813,7 @@ struct dev_pm_info { enum rpm_status last_status; int runtime_error; int autosuspend_delay; - u64 last_busy; + atomic64_t last_busy; u64 active_time; u64 suspended_time; u64 accounting_timestamp; diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index ae68a9dc7fc5..da537be94732 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -8,6 +8,7 @@ #ifndef _LINUX_PM_RUNTIME_H #define _LINUX_PM_RUNTIME_H +#include <linux/atomic.h> #include <linux/device.h> #include <linux/notifier.h> #include <linux/pm.h> @@ -242,7 +243,7 @@ static inline bool pm_runtime_has_no_callbacks(struct device *dev) */ static inline void pm_runtime_mark_last_busy(struct device *dev) { - WRITE_ONCE(dev->power.last_busy, ktime_get_mono_fast_ns()); + atomic64_set(&dev->power.last_busy, ktime_get_mono_fast_ns()); } /** -- 2.56.0.398.g882cbad275-goog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 2/5] PM: runtime: Convert last_busy to atomic64_t 2026-11-03 22:14 ` [PATCH v2 2/5] PM: runtime: Convert last_busy to atomic64_t Brian Norris @ 2026-11-04 8:21 ` Joshua Crofts 0 siblings, 0 replies; 22+ messages in thread From: Joshua Crofts @ 2026-11-04 8:21 UTC (permalink / raw) To: Brian Norris Cc: Rafael J. Wysocki, Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin On Tue, 3 Nov 2026 14:14:40 -0800 Brian Norris <briannorris@chromium.org> wrote: > I want to use the atomic64 compare/exchange helpers. There are no > similar 32-bit-portable helpers for non-atomic64_t. IMO starting a commit message with "I want to use" isn't exactly great... -- Kind regards, Joshua Crofts ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-03 22:14 [PATCH v2 0/5] PM: runtime: Improve autosuspend child to parent propagation Brian Norris 2026-11-03 22:14 ` [PATCH v2 1/5] PM: runtime: Only queue an idle check for RPM-linked suppliers (part 2) Brian Norris 2026-11-03 22:14 ` [PATCH v2 2/5] PM: runtime: Convert last_busy to atomic64_t Brian Norris @ 2026-11-03 22:14 ` Brian Norris 2026-11-03 23:17 ` Jonathan Cameron 2026-11-04 13:34 ` Rafael J. Wysocki (Intel) 2026-11-03 22:14 ` [PATCH v2 4/5] PM: runtime: Add tests for last_busy propagation Brian Norris 2026-11-03 22:14 ` [PATCH v2 5/5] iio: adc: stm32-adc: Drop runtime_idle() Brian Norris 4 siblings, 2 replies; 22+ messages in thread From: Brian Norris @ 2026-11-03 22:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin, Brian Norris 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. 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. 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]. 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. 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@mail.gmail.com/ [1] Signed-off-by: Brian Norris <briannorris@chromium.org> --- Cc: Ulf Hansson <ulfh@kernel.org> 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. + */ +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; +} + /** * 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); -- 2.56.0.398.g882cbad275-goog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-03 22:14 ` [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency Brian Norris @ 2026-11-03 23:17 ` Jonathan Cameron 2026-11-04 0:32 ` Brian Norris 2026-11-04 13:34 ` Rafael J. Wysocki (Intel) 1 sibling, 1 reply; 22+ messages in thread From: Jonathan Cameron @ 2026-11-03 23:17 UTC (permalink / raw) To: Brian Norris Cc: Rafael J. Wysocki, Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin On Tue, 3 Nov 2026 14:14:41 -0800 Brian Norris <briannorris@chromium.org> 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. So reading this again - I wonder if this text is describing the expected behaviour. If the child is also using autosupend then, in extreme case, that time might be longer than the autosuspend time of the parent. If so it seems odd the parent still waits. So most expected to me would be if the parent were to suspend it's autosuspend time after the parent or child was last used for something that indicate a desire not to suspend (that shouldn't include actions the child did whilst suspending). Maybe there isn't a sensible way to deal with this and what you have here is the best we can do. Note that this is just commenting on this statement in the commit description. From what follows I think this is just a commit description thing rather than functional. > > 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. Sooner is indeed bad. > > 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]. > > 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. So this sounds fine but I don't think aligns with that first paragraph. > > 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. Can we set the last busy on the suspend of child devices that aren't suspending as a result of autosuspend timeout but for other reasons? (may be some fiddly corners!) > > Link: https://lore.kernel.org/all/CAPDyKFp=KTf8=zGBSzPYqhjnZpY8xwvjCeM1e-WTKT1QLSxaDA@mail.gmail.com/ [1] > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > Cc: Ulf Hansson <ulfh@kernel.org> > > 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. > + */ > +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; > +} > + > /** > * 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); > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-03 23:17 ` Jonathan Cameron @ 2026-11-04 0:32 ` Brian Norris 2026-11-04 1:40 ` Jonathan Cameron 0 siblings, 1 reply; 22+ messages in thread From: Brian Norris @ 2026-11-04 0:32 UTC (permalink / raw) To: Jonathan Cameron Cc: Rafael J. Wysocki, Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin Hi Jonathan, On Tue, Nov 03, 2026 at 11:17:19PM +0000, Jonathan Cameron wrote: > On Tue, 3 Nov 2026 14:14:41 -0800 > Brian Norris <briannorris@chromium.org> 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. > > So reading this again - I wonder if this text is describing the > expected behaviour. Agreed, this might give the wrong impression, depending on the assumptions the reader makes about autosuspend delay. (I think I had a particular problem scenario in mind, but didn't translate that properly here.) > If the child is also using autosupend then, in extreme case, that time > might be longer than the autosuspend time of the parent. > If so it seems odd the parent still waits. Right, that seems wrong, and indeed that's not what this patch does. > So most expected to me > would be if the parent were to suspend it's autosuspend time after > the parent or child was last used for something that indicate a > desire not to suspend (that shouldn't include actions the child > did whilst suspending). (Not sure I'm parsing this sentence quite right, but...) The point is that last_busy should propagate (rather than, say, the expiration time). So each device still gets its own-delay-based autosuspend timer expiration. That may mean the parent waits longer than the child, but it may not, if the child's delay was longer than the parent's. > Maybe there isn't a sensible way to deal with this and what you have here > is the best we can do. Note that this is just commenting on this > statement in the commit description. From what follows I think > this is just a commit description thing rather than functional. If I'm understanding what you're saying correctly: maybe it would be an improvement here to say: If that parent configured an autosuspend delay longer than its child's and didn't otherwise maintain its last_busy timestamp, it may now be immediately eligible to suspend. I think that would more accurately describe the target problem, even if it doesn't cover all cases (such as when the parent delay is shorter). > > 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. > > Sooner is indeed bad. > > > > > 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]. > > > > 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. > > So this sounds fine but I don't think aligns with that first paragraph. > > > > > 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. > > Can we set the last busy on the suspend of child devices that aren't > suspending as a result of autosuspend timeout but for other reasons? > (may be some fiddly corners!) Do you have concrete use cases in mind, or do you bring this up simply because of my note? For one, I feel like for devices with power.use_autosuspend==true, it's reasonable to expect drivers to use the appropriate helpers to update last_busy at the appropriate times, and so any "corner cases" are from misuse. But if you mean non-use_autosuspend devices: I suppose it'd be possible but I haven't reasoned through the implementation very far yet. It almost seems like we'd need to plumb pm_runtime_mark_last_busy() into *all* the put() helpers, including the non-autosuspend ones (pm_runtime_put(), pm_runtime_put_sync()). I'm not quite sure if that's desirable. But I could try to look a bit more, if you think there are important use cases that would need this. Brian > > Link: https://lore.kernel.org/all/CAPDyKFp=KTf8=zGBSzPYqhjnZpY8xwvjCeM1e-WTKT1QLSxaDA@mail.gmail.com/ [1] > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > --- > > Cc: Ulf Hansson <ulfh@kernel.org> > > > > 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. > > + */ > > +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; > > +} > > + > > /** > > * 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); > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-04 0:32 ` Brian Norris @ 2026-11-04 1:40 ` Jonathan Cameron 0 siblings, 0 replies; 22+ messages in thread From: Jonathan Cameron @ 2026-11-04 1:40 UTC (permalink / raw) To: Brian Norris Cc: Rafael J. Wysocki, Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin On Tue, 3 Nov 2026 16:32:44 -0800 Brian Norris <briannorris@chromium.org> wrote: > Hi Jonathan, > > On Tue, Nov 03, 2026 at 11:17:19PM +0000, Jonathan Cameron wrote: > > On Tue, 3 Nov 2026 14:14:41 -0800 > > Brian Norris <briannorris@chromium.org> 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. > > > > So reading this again - I wonder if this text is describing the > > expected behaviour. > > Agreed, this might give the wrong impression, depending on the > assumptions the reader makes about autosuspend delay. (I think I had a > particular problem scenario in mind, but didn't translate that properly > here.) > > > If the child is also using autosupend then, in extreme case, that time > > might be longer than the autosuspend time of the parent. > > If so it seems odd the parent still waits. > > Right, that seems wrong, and indeed that's not what this patch does. > > > So most expected to me > > would be if the parent were to suspend it's autosuspend time after > > the parent or child was last used for something that indicate a > > desire not to suspend (that shouldn't include actions the child > > did whilst suspending). > > (Not sure I'm parsing this sentence quite right, but...) > > The point is that last_busy should propagate (rather than, say, the > expiration time). So each device still gets its own-delay-based > autosuspend timer expiration. That may mean the parent waits longer than > the child, but it may not, if the child's delay was longer than the > parent's. > > > Maybe there isn't a sensible way to deal with this and what you have here > > is the best we can do. Note that this is just commenting on this > > statement in the commit description. From what follows I think > > this is just a commit description thing rather than functional. > > If I'm understanding what you're saying correctly: maybe it would be an > improvement here to say: > > If that parent configured an autosuspend delay longer than its child's > and didn't otherwise maintain its last_busy timestamp, it may now be > immediately eligible to suspend. > > I think that would more accurately describe the target problem, even if > it doesn't cover all cases (such as when the parent delay is shorter). Agreed. That covers the main case nicely. > > > > 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. > > > > Sooner is indeed bad. > > > > > > > > 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]. > > > > > > 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. > > > > So this sounds fine but I don't think aligns with that first paragraph. > > > > > > > > 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. > > > > Can we set the last busy on the suspend of child devices that aren't > > suspending as a result of autosuspend timeout but for other reasons? > > (may be some fiddly corners!) > > Do you have concrete use cases in mind, or do you bring this up simply > because of my note? Just your note ;) > > For one, I feel like for devices with power.use_autosuspend==true, it's > reasonable to expect drivers to use the appropriate helpers to update > last_busy at the appropriate times, and so any "corner cases" are from > misuse. > > But if you mean non-use_autosuspend devices: I suppose it'd be possible > but I haven't reasoned through the implementation very far yet. It > almost seems like we'd need to plumb pm_runtime_mark_last_busy() into > *all* the put() helpers, including the non-autosuspend ones > (pm_runtime_put(), pm_runtime_put_sync()). I'm not quite sure if that's > desirable. But I could try to look a bit more, if you think there are > important use cases that would need this. If it is hard not a problem to leave it as is and call out the limitation as you have done. Someone who needs it can figure it out! Jonathan > > Brian > > > > Link: https://lore.kernel.org/all/CAPDyKFp=KTf8=zGBSzPYqhjnZpY8xwvjCeM1e-WTKT1QLSxaDA@mail.gmail.com/ [1] > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > --- > > > Cc: Ulf Hansson <ulfh@kernel.org> > > > > > > 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. > > > + */ > > > +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; > > > +} > > > + > > > /** > > > * 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); > > > > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-03 22:14 ` [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency Brian Norris 2026-11-03 23:17 ` Jonathan Cameron @ 2026-11-04 13:34 ` Rafael J. Wysocki (Intel) 2026-11-04 19:31 ` Brian Norris 1 sibling, 1 reply; 22+ messages in thread From: Rafael J. Wysocki (Intel) @ 2026-11-04 13:34 UTC (permalink / raw) To: Brian Norris Cc: Rafael J. Wysocki, Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin 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@chromium.org> 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@mail.gmail.com/ [1] > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > Cc: Ulf Hansson <ulfh@kernel.org> > > 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); > > -- ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-04 13:34 ` Rafael J. Wysocki (Intel) @ 2026-11-04 19:31 ` Brian Norris 2026-11-05 11:59 ` Rafael J. Wysocki (Intel) 0 siblings, 1 reply; 22+ messages in thread From: Brian Norris @ 2026-11-04 19:31 UTC (permalink / raw) To: Rafael J. Wysocki (Intel) Cc: Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin On Wed, Nov 04, 2026 at 02:34:22PM +0100, Rafael J. Wysocki (Intel) wrote: > I should have responded to the v1 of this, but didn't manage to get to > it. Sorry about that. Hehe, well that's what v2 is for :) Thanks for looking. > On Tue, Nov 3, 2026 at 11:23 PM Brian Norris <briannorris@chromium.org> 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. To be clear: dropping a reference is not the point at which we extend the timer -- it's simply the point at which we propagate the last_busy timestamp. The naive algorithm is: Every time a device updates its last_busy, sync that timestamp to all its dependencies. My proposal modifies that naive algorithm so that it avoids additional graph walks -- it defers that "sync" step until later. (Now that I'm spelling this out, I feel like I should have included this in the commit message...) With that said: > 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. But that's not what I'm doing: I'm not calling mark_last_busy() -- I'm just making mark_last_busy() transitive to all dependencies. So any time we *already* say the child is busy, I also extend that to the parent -- I'm not generating any new timestamps; I only reuse the timestamps we already selected. My view on the logic (using your terms above): when a child calls mark_last_busy(), it suggests that child might be needed in the near future. Whenever a child device is needed, so is its parent (barring ignore_children). Thus, mark_last_busy() should also refresh the parent's busy timestamp. Importantly (in the same topic Jonathan was already asking about): this does *not* mean the parent gets a whole new autosuspend delay period -- but it *does* mean that if the parent has a longer delay period (presumably because it has a more expensive/slow suspend/resume latency) it waits a bit longer (in case the child wakes back up, for one) to autosuspend. In concrete terms of commit c537d3457542 ("iio: adc: stm32-adc: fix runtime autosuspend delay when slow polling"): * child device has 100ms delay * parent devices has 2s delay SEQUENCE: 1) child is active for longer than 2 seconds 2) [timestamp N] child finishes I/O, drops reference with put_autosuspend() 3) [N + 100ms] child suspends CURRENT BEHVIOR: 4) [N + 100ms] parent suspends immediately after #3 PROPOSAL: 4) [N + 2s] parent suspends (if there was no other last_busy update) The only last_busy timestamp is N; we just use it in more places, because the parent was, in fact, busy at timestamp N. > 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. "Direct reference" is to mean "pm_runtime_put_autosuspend()" or anything similar that updates last_busy. We expect that when the device in question is the target object for I/O or whatever is keeping the device busy. But if the I/O is performed by a child, then it's no longer "direct reference" -- it's "usage accounted by its children". > 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? I think I've improperly directed you to focus on the "reference" part, which IMO is a bit of a distraction. The point is: when was the device "busy" (or "last used")? The 'control' attribute does not represent "last used"; it just represents some unrelated policy to prevent suspension. > > 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. Note: stm32-adc-core (drivers/iio/adc/stm32-adc-core.c) is the parent. stm32-adc (drivers/iio/adc/stm32-adc.c) is the child. When the child suspends, the parent checks .runtime_idle(). This updates last_busy but still falls through to autosuspend -- that means we get a slightly suboptimal sequence like the following: 1) child is active for longer than 2 seconds 2) [timestamp N] child finishes I/O, drops reference with put_autosuspend() 3) [N + 100ms] child suspends 4) parent runtime_idle() updates last_busy to [P = approx N + 100ms] 5) [P + 2s] parent suspends Step 5 happens approximately 100ms later than it really should, but it's good enough. NB: I think this is also why greybus (gb_bundle_idle(), gb_interface_runtime_idle(), etc.) and USB (usb_runtime_idle(), dwc3_runtime_idle()) look the way they do, with a built-in mark_last_busy() in runtime_idle(). I also think that some other areas could use some logic like this. For example, drivers/pci/pcie/portdrv.c has a 100ms autosuspend delay so that lspci doesn't cause repeated suspend/resume. But suppose config access for a child of a port are slow ... then the port still might suspend immediately, between config accesses. (Never mind that pci_config_pm_runtime_put() doesn't update last_busy...) > > 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? I think that's an area I could improve in the commit message. But see my comments above ("My view on the logic") which I hope answers your question. > 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@mail.gmail.com/ [1] > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > --- > > Cc: Ulf Hansson <ulfh@kernel.org> > > > > 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. OK. If we agree on the what and why, I'll take another pass at this. For one, I imagine describing the "naive algorithm" I mention above would be a good start. And maybe the principles below. > > + */ > > +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. Which part do you not like? The loop? The use of compare/exchange? The principles involved are: 1) copy the timestamp 2) avoid going backward in time (e.g., if target already updated to a newer stamp) 3) avoid TOCTOU races between reading the target stamp and replacing it (e.g., if multiple children are doing this at the same time) I suppose I could compromise on #3 by only doing a single compare/exchange. This would remove the looping (which is admittedly a little hard to reason about), at the expense of getting a suboptimal update in the (likely rare) case of concurrent updates. Brian > > + > > /** > > * 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); > > > > -- ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-04 19:31 ` Brian Norris @ 2026-11-05 11:59 ` Rafael J. Wysocki (Intel) 2026-11-05 20:21 ` Brian Norris 0 siblings, 1 reply; 22+ messages in thread From: Rafael J. Wysocki (Intel) @ 2026-11-05 11:59 UTC (permalink / raw) To: Brian Norris Cc: Rafael J. Wysocki (Intel), Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin [-- Attachment #1: Type: text/plain, Size: 10593 bytes --] On Wed, Nov 4, 2026 at 8:31 PM Brian Norris <briannorris@chromium.org> wrote: > > On Wed, Nov 04, 2026 at 02:34:22PM +0100, Rafael J. Wysocki (Intel) wrote: > > I should have responded to the v1 of this, but didn't manage to get to > > it. Sorry about that. > > Hehe, well that's what v2 is for :) > > Thanks for looking. > > > On Tue, Nov 3, 2026 at 11:23 PM Brian Norris <briannorris@chromium.org> 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. > > To be clear: dropping a reference is not the point at which we extend > the timer -- it's simply the point at which we propagate the last_busy > timestamp. > > The naive algorithm is: > Every time a device updates its last_busy, sync that timestamp to all > its dependencies. > > My proposal modifies that naive algorithm so that it avoids additional > graph walks -- it defers that "sync" step until later. (Now that I'm > spelling this out, I feel like I should have included this in the commit > message...) OK, thanks for explaining! You seem to be arguing that the current behavior is counter-intuitive because people would generally expect the parent's (or supplier's) last_busy to be automatically advanced to the last "busy" time of its last active child (or consumer). That is a bit far-fetched IMV because the child's last "busy" time need not be the same as the parent's last "busy" time, at least in principle (of course, that is debatable, but that debate can be had later). Still, the underlying observation seems to be that if the child suspends and then resumes relatively shortly, it may not make sense to suspend and resume the parent in the meantime, which is fair enough. Of course, advancing the parent's last_busy will only help here if its autosuspend delay is long enough though. So you can argue that if the parent's autosuspend delay is long enough, then it should prevent the parent from suspending across relatively short periods of child suspension. Again, this is fair enough, so why does it not happen at times? It certainly does happen when the parent actually has to resume because rpm_resume() calls pm_runtime_mark_last_busy() for it then and (given a sufficiently long autosuspend delay) it will stay active if the child resumes a short enough time after suspending. However, when the child resumes, the parent is still active, so rpm_resume() triggered by the child does not advance its last_busy timestamp, so at one point in the future it will suspend when the child suspends. This arguably is inconsistent, so my approach to addressing the problem at hand would be the attached patch (sorry for the attachment, but GMail is generally not good at handling appended patches). If I'm not mistaken, it should be sufficient to address the particular issue that triggered commit c537d3457542. Note that I'm not worried about the potential raciness of this new last_busy update with other possible concurrent updates of last_busy because they all use approximations of the current time and if they are close enough to each other, it should not matter who wins the race. > With that said: > > > 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. > > But that's not what I'm doing: I'm not calling mark_last_busy() -- I'm > just making mark_last_busy() transitive to all dependencies. So any time > we *already* say the child is busy, I also extend that to the parent -- > I'm not generating any new timestamps; I only reuse the timestamps we > already selected. > > My view on the logic (using your terms above): when a child calls > mark_last_busy(), it suggests that child might be needed in the near > future. Whenever a child device is needed, so is its parent (barring > ignore_children). Thus, mark_last_busy() should also refresh the > parent's busy timestamp. Of course, it can be argued this way though there are counter-arguments (probably not worth discussing in detail at this point) and there is the question how much of a difference this would make after addressing the rpm_resume() inconsistency mentioned above (which IMV needs to be address regardless because it is not limited to the parent-child or supplier-consumer situations). > Importantly (in the same topic Jonathan was already asking about): this > does *not* mean the parent gets a whole new autosuspend delay period -- > but it *does* mean that if the parent has a longer delay period > (presumably because it has a more expensive/slow suspend/resume latency) > it waits a bit longer (in case the child wakes back up, for one) to > autosuspend. > > In concrete terms of commit c537d3457542 ("iio: adc: stm32-adc: fix > runtime autosuspend delay when slow polling"): > > * child device has 100ms delay > * parent devices has 2s delay > > SEQUENCE: > 1) child is active for longer than 2 seconds > 2) [timestamp N] child finishes I/O, drops reference with put_autosuspend() > 3) [N + 100ms] child suspends > > CURRENT BEHVIOR: > 4) [N + 100ms] parent suspends immediately after #3 > > PROPOSAL: > 4) [N + 2s] parent suspends (if there was no other last_busy update) > > The only last_busy timestamp is N; we just use it in more places, > because the parent was, in fact, busy at timestamp N. > > > 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. > > "Direct reference" is to mean "pm_runtime_put_autosuspend()" or anything > similar that updates last_busy. We expect that when the device in > question is the target object for I/O or whatever is keeping the device > busy. > > But if the I/O is performed by a child, then it's no longer "direct > reference" -- it's "usage accounted by its children". > > > 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? > > I think I've improperly directed you to focus on the "reference" part, > which IMO is a bit of a distraction. The point is: when was the device > "busy" (or "last used")? The 'control' attribute does not represent > "last used"; it just represents some unrelated policy to prevent > suspension. Or not, depending on why user space flips the "control" attribute which is not known to the kernel. > > > 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. > > Note: stm32-adc-core (drivers/iio/adc/stm32-adc-core.c) is the parent. > stm32-adc (drivers/iio/adc/stm32-adc.c) is the child. It would really have helped if that had been described properly in the changelog of commit c537d3457542. Oh well. > When the child suspends, the parent checks .runtime_idle(). This updates > last_busy but still falls through to autosuspend -- that means we get a > slightly suboptimal sequence like the following: > > 1) child is active for longer than 2 seconds > 2) [timestamp N] child finishes I/O, drops reference with put_autosuspend() > 3) [N + 100ms] child suspends > 4) parent runtime_idle() updates last_busy to [P = approx N + 100ms] > 5) [P + 2s] parent suspends > > Step 5 happens approximately 100ms later than it really should, but it's > good enough. > > NB: I think this is also why greybus (gb_bundle_idle(), > gb_interface_runtime_idle(), etc.) and USB (usb_runtime_idle(), > dwc3_runtime_idle()) look the way they do, with a built-in > mark_last_busy() in runtime_idle(). So I'm wondering how much of this will still be needed with the attached patch applied. > I also think that some other areas could use some logic like this. For > example, drivers/pci/pcie/portdrv.c has a 100ms autosuspend delay so > that lspci doesn't cause repeated suspend/resume. But suppose config > access for a child of a port are slow ... then the port still might > suspend immediately, between config accesses. (Never mind that > pci_config_pm_runtime_put() doesn't update last_busy...) But at the point when the first config space access takes place, it is unclear that the next one will happen, which is part of the problem here: The child resume needs to be expected to happen shortly for all of this to make sense. [-- Attachment #2: pm-runtime-last_busy.patch --] [-- Type: text/x-patch, Size: 566 bytes --] --- drivers/base/power/runtime.c | 6 ++++++ 1 file changed, 6 insertions(+) --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -819,6 +819,12 @@ static int rpm_resume(struct device *dev pm_runtime_deactivate_timer(dev); if (dev->power.runtime_status == RPM_ACTIVE) { + /* + * An attempt to resume the device means that it is needed for + * something, so update its last_busy timestamp in analogy with + * the case in which it actually resumes below. + */ + pm_runtime_mark_last_busy(dev); retval = 1; goto out; } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-05 11:59 ` Rafael J. Wysocki (Intel) @ 2026-11-05 20:21 ` Brian Norris 2026-11-05 21:18 ` Rafael J. Wysocki (Intel) 0 siblings, 1 reply; 22+ messages in thread From: Brian Norris @ 2026-11-05 20:21 UTC (permalink / raw) To: Rafael J. Wysocki (Intel) Cc: Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin Hi Rafael, On Thu, Nov 05, 2026 at 12:59:24PM +0100, Rafael J. Wysocki (Intel) wrote: > On Wed, Nov 4, 2026 at 8:31 PM Brian Norris <briannorris@chromium.org> wrote: > > > > On Wed, Nov 04, 2026 at 02:34:22PM +0100, Rafael J. Wysocki (Intel) wrote: > > > I should have responded to the v1 of this, but didn't manage to get to > > > it. Sorry about that. > > > > Hehe, well that's what v2 is for :) > > > > Thanks for looking. > > > > > On Tue, Nov 3, 2026 at 11:23 PM Brian Norris <briannorris@chromium.org> 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. > > > > To be clear: dropping a reference is not the point at which we extend > > the timer -- it's simply the point at which we propagate the last_busy > > timestamp. > > > > The naive algorithm is: > > Every time a device updates its last_busy, sync that timestamp to all > > its dependencies. > > > > My proposal modifies that naive algorithm so that it avoids additional > > graph walks -- it defers that "sync" step until later. (Now that I'm > > spelling this out, I feel like I should have included this in the commit > > message...) > > OK, thanks for explaining! > > You seem to be arguing that the current behavior is counter-intuitive > because people would generally expect the parent's (or supplier's) > last_busy to be automatically advanced to the last "busy" time of its > last active child (or consumer). More or less. Or, I don't see why "child was recently busy and may be busy again soon" should not translate to "everything the child uses might be needed again soon". > That is a bit far-fetched IMV because the child's last "busy" time > need not be the same as the parent's last "busy" time, at least in > principle (of course, that is debatable, but that debate can be had > later). I don't get this -- I'm very much asking for that debate right now, because so far, I disagree :) > Still, the underlying observation seems to be that if the child > suspends and then resumes relatively shortly, it may not make sense to > suspend and resume the parent in the meantime, which is fair enough. > Of course, advancing the parent's last_busy will only help here if its > autosuspend delay is long enough though. Right. But that's the whole point of this argument -- if all autosuspend delays were the same, this wouldn't really matter. But the presumption is that the parent wants delay because it's more costly -- it will be imposing extra wakeup cost for its children if it's not accounted properly. > So you can argue that if the parent's autosuspend delay is long > enough, then it should prevent the parent from suspending across > relatively short periods of child suspension. Again, this is fair > enough, so why does it not happen at times? > > It certainly does happen when the parent actually has to resume > because rpm_resume() calls pm_runtime_mark_last_busy() for it then and > (given a sufficiently long autosuspend delay) it will stay active if > the child resumes a short enough time after suspending. However, when > the child resumes, the parent is still active, so rpm_resume() > triggered by the child does not advance its last_busy timestamp, so at > one point in the future it will suspend when the child suspends. > > This arguably is inconsistent, so my approach to addressing the > problem at hand would be the attached patch (sorry for the attachment, > but GMail is generally not good at handling appended patches). If I'm > not mistaken, it should be sufficient to address the particular issue > that triggered commit c537d3457542. It's only sufficient if the child is not active for "too long". Suppose the child stays active for 2s and then suspends briefly; your solution does not work. In commit c537d3457542, I imagine the 0.2s polling example is just an example; what if that period was 0.1s, with some natural variance up or down? Then I could easily see how the child remains active for longer than 2s, and then occasionally is inactive for longer than 0.1s, allowing both the child and parent to autosuspend. Or in concrete terms: see the appended test diff, which attempts to emulate: - 200 cycles of child get/put - delay in between each put -> get cycle - delay of approximately 100ms, with a configurable +/- variation - apply the shorter variation (e.g., 90ms) most of the time, with a longer variation approximately every 2s So for example, that looks something like: cycle 0 to 22: delay 90ms cycle 23: delay 110ms cycle 24 to 46: delay 90ms ... With your patch: 0ms variation == PASS (child suspends 200 times, parent suspends 0 times) 10ms variation == FAIL (child suspends 21 times, parent suspends 4 times) 20ms variation == FAIL (child suspends 7 times, parent suspends 7 times) 30ms variation == FAIL (child suspends 6 times, parent suspends 6 times) 40ms variation == FAIL (child suspends 5 times, parent suspends 5 times) 50ms variation == FAIL (child suspends 4 times, parent suspends 4 times) 60ms variation == FAIL (child suspends 3 times, parent suspends 3 times) 70ms variation == FAIL (child suspends 2 times, parent suspends 2 times) 80ms variation == FAIL (child suspends 1 time, parent suspends 1 time) 90ms variation == PASS (test didn't run long enough to fail) With my patch, all cases pass. (The child suspends in the same pattern, but the parent does not suspend in between.) For comparison, with neither patch: 0ms variation == FAIL (child suspends 200 times, parent suspends 10 times) 10ms variation == FAIL (child suspends 14 times, parent suspends 8 times) 20ms variation == FAIL (child suspends 7 times, parent suspends 7 times) 30ms variation == FAIL (child suspends 6 times, parent suspends 6 times) 40ms variation == FAIL (child suspends 5 times, parent suspends 5 times) 50ms variation == FAIL (child suspends 4 times, parent suspends 4 times) 60ms variation == FAIL (child suspends 3 times, parent suspends 3 times) 70ms variation == FAIL (child suspends 2 times, parent suspends 2 times) 80ms variation == FAIL (child suspends 1 time, parent suspends 1 time) 90ms variation == PASS (test didn't run long enough to fail) So, your patch makes an improvement, but it doesn't handle this kind of variation very well. > Note that I'm not worried about the potential raciness of this new > last_busy update with other possible concurrent updates of last_busy > because they all use approximations of the current time and if they > are close enough to each other, it should not matter who wins the > race. Ack, that's one advantage in your style of solution, because you're only handling timestamps "in the moment." > > With that said: > > > > > 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. > > > > But that's not what I'm doing: I'm not calling mark_last_busy() -- I'm > > just making mark_last_busy() transitive to all dependencies. So any time > > we *already* say the child is busy, I also extend that to the parent -- > > I'm not generating any new timestamps; I only reuse the timestamps we > > already selected. > > > > My view on the logic (using your terms above): when a child calls > > mark_last_busy(), it suggests that child might be needed in the near > > future. Whenever a child device is needed, so is its parent (barring > > ignore_children). Thus, mark_last_busy() should also refresh the > > parent's busy timestamp. > > Of course, it can be argued this way though there are > counter-arguments (probably not worth discussing in detail at this > point) and there is the question how much of a difference this would > make after addressing the rpm_resume() inconsistency mentioned above > (which IMV needs to be address regardless because it is not limited to > the parent-child or supplier-consumer situations). I would love to hear your counterarguments. I don't believe your solution solves the problem, and my solution does while also handling the inconsistency you mention. > > Importantly (in the same topic Jonathan was already asking about): this > > does *not* mean the parent gets a whole new autosuspend delay period -- > > but it *does* mean that if the parent has a longer delay period > > (presumably because it has a more expensive/slow suspend/resume latency) > > it waits a bit longer (in case the child wakes back up, for one) to > > autosuspend. > > > > In concrete terms of commit c537d3457542 ("iio: adc: stm32-adc: fix > > runtime autosuspend delay when slow polling"): > > > > * child device has 100ms delay > > * parent devices has 2s delay > > > > SEQUENCE: > > 1) child is active for longer than 2 seconds > > 2) [timestamp N] child finishes I/O, drops reference with put_autosuspend() > > 3) [N + 100ms] child suspends > > > > CURRENT BEHVIOR: > > 4) [N + 100ms] parent suspends immediately after #3 > > > > PROPOSAL: > > 4) [N + 2s] parent suspends (if there was no other last_busy update) > > > > The only last_busy timestamp is N; we just use it in more places, > > because the parent was, in fact, busy at timestamp N. > > > > > 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. > > > > "Direct reference" is to mean "pm_runtime_put_autosuspend()" or anything > > similar that updates last_busy. We expect that when the device in > > question is the target object for I/O or whatever is keeping the device > > busy. > > > > But if the I/O is performed by a child, then it's no longer "direct > > reference" -- it's "usage accounted by its children". > > > > > 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? > > > > I think I've improperly directed you to focus on the "reference" part, > > which IMO is a bit of a distraction. The point is: when was the device > > "busy" (or "last used")? The 'control' attribute does not represent > > "last used"; it just represents some unrelated policy to prevent > > suspension. > > Or not, depending on why user space flips the "control" attribute > which is not known to the kernel. > > > > > 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. > > > > Note: stm32-adc-core (drivers/iio/adc/stm32-adc-core.c) is the parent. > > stm32-adc (drivers/iio/adc/stm32-adc.c) is the child. > > It would really have helped if that had been described properly in the > changelog of commit c537d3457542. Oh well. > > > When the child suspends, the parent checks .runtime_idle(). This updates > > last_busy but still falls through to autosuspend -- that means we get a > > slightly suboptimal sequence like the following: > > > > 1) child is active for longer than 2 seconds > > 2) [timestamp N] child finishes I/O, drops reference with put_autosuspend() > > 3) [N + 100ms] child suspends > > 4) parent runtime_idle() updates last_busy to [P = approx N + 100ms] > > 5) [P + 2s] parent suspends > > > > Step 5 happens approximately 100ms later than it really should, but it's > > good enough. > > > > NB: I think this is also why greybus (gb_bundle_idle(), > > gb_interface_runtime_idle(), etc.) and USB (usb_runtime_idle(), > > dwc3_runtime_idle()) look the way they do, with a built-in > > mark_last_busy() in runtime_idle(). > > So I'm wondering how much of this will still be needed with the > attached patch applied. > > > I also think that some other areas could use some logic like this. For > > example, drivers/pci/pcie/portdrv.c has a 100ms autosuspend delay so > > that lspci doesn't cause repeated suspend/resume. But suppose config > > access for a child of a port are slow ... then the port still might > > suspend immediately, between config accesses. (Never mind that > > pci_config_pm_runtime_put() doesn't update last_busy...) > > But at the point when the first config space access takes place, it is > unclear that the next one will happen, which is part of the problem > here: The child resume needs to be expected to happen shortly for all > of this to make sense. I don't understand this argument. Sure, "it is unclear" ... but that's still the whole point of mark_last_busy() -- the autosuspend algorithm suggests, "if we recently used the device, we might use it again soon." Yes, that's not guaranteed ("might"), but it's still the whole design, because predicting the future is hard, and getting the answer wrong occasionally (small additional suspend delay) is not too expensive. If we didn't want to make predictions based on last-use (but instead only "last resume"), pm_runtime_mark_last_busy() would largely be an internal detail within the runtime PM core; and it should most certainly not be baked into pm_runtime_put_autosuspend() and similar. Or, why did you apply commit b3db492e8335 ("PM: runtime: Mark last busy stamp in pm_runtime_put_autosuspend()")? Or why (prior to that commit) did the documentation say "Drivers should call pm_runtime_mark_last_busy() to update this field after carrying out I/O"? And why, with all that, does PCI config access need to differ from that advice? To me, it seems like an accident, and not intention. > --- > drivers/base/power/runtime.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -819,6 +819,12 @@ static int rpm_resume(struct device *dev > pm_runtime_deactivate_timer(dev); > > if (dev->power.runtime_status == RPM_ACTIVE) { > + /* > + * An attempt to resume the device means that it is needed for > + * something, so update its last_busy timestamp in analogy with > + * the case in which it actually resumes below. > + */ > + pm_runtime_mark_last_busy(dev); > retval = 1; > goto out; > } Brian Test patch follows. Example use: ## get()/put the child 200 times, with varied delay in between: ## 100ms +/- 0ms ## 100ms +/- 10m ## ... ## We purposely use the shorter delay most of the time, with the ## longer variation occurring approximately every 2000ms ## ## This test usually produces excess parent-suspend with Rafael's ## patch. $ for i in $(seq 0 10 90); do tools/testing/kunit/kunit.py run \ --kconfig_add CONFIG_PM=y \ pm_runtime_test_cases.pm_runtime_parent_periodic_use_test \ --kernel_args runtime-test.period_variation=$i; \ done diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c index 685a3504cd7b..12b8eb1d10c9 100644 --- a/drivers/base/power/runtime-test.c +++ b/drivers/base/power/runtime-test.c @@ -5,10 +5,12 @@ #include <linux/cleanup.h> #include <linux/delay.h> +#include <linux/moduleparam.h> #include <linux/pm_runtime.h> #include <linux/workqueue.h> #include <kunit/device.h> #include <kunit/test.h> +#include <kunit/test-bug.h> #define DEVICE_NAME "pm_runtime_test_device" @@ -386,6 +388,127 @@ static void pm_runtime_supplier_last_used_test(struct kunit *test) KUNIT_EXPECT_GT(test, expiry2, expiry1); } +struct pm_runtime_test_data { + struct device_driver *driver; +}; + +struct pm_runtime_device_data { + int suspend_count; +}; + +static int test_runtime_suspend(struct device *dev) +{ + struct pm_runtime_device_data *data = dev_get_drvdata(dev); + + data->suspend_count++; + + return 0; +} + +static DEFINE_RUNTIME_DEV_PM_OPS(test_ops, test_runtime_suspend, NULL, NULL); + +static int test_runtime_probe(struct device *dev) +{ + struct kunit *test = kunit_get_current_test(); + struct pm_runtime_device_data *data; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, data); + + dev_set_drvdata(dev, data); + + return 0; +} + +static int pm_runtime_test_init(struct kunit *test) +{ + struct pm_runtime_test_data *priv; + + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); + + priv->driver = kunit_driver_create(test, "pm_runtime_test_driver"); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->driver); + + priv->driver->pm = &test_ops; + priv->driver->probe = &test_runtime_probe; + test->priv = priv; + + return 0; +} + +static void pm_runtime_test_exit(struct kunit *test) +{ + struct pm_runtime_test_data *priv = test->priv; + + priv->driver->pm = NULL; +} + +/* Default: delay 100ms, variation +-50ms */ +static const unsigned int child_delay = 100; +static unsigned int period_variation = 50; +module_param(period_variation, uint, 0444); + +static void pm_runtime_parent_periodic_use_test(struct kunit *test) +{ + struct device *parent, *dev; + struct pm_runtime_test_data *priv = test->priv; + struct pm_runtime_device_data *parent_data, *child_data; + + parent = kunit_device_register_with_driver(test, DEVICE_NAME "_parent", priv->driver); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent); + + parent_data = dev_get_drvdata(parent); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent_data); + + dev = kunit_device_register(test, DEVICE_NAME); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); + + child_data = dev_get_drvdata(dev); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, child_data); + + KUNIT_ASSERT_EQ(test, 0, device_move(dev, parent, DPM_ORDER_NONE)); + + pm_runtime_set_active(parent); + pm_runtime_set_active(dev); + pm_runtime_use_autosuspend(parent); + pm_runtime_use_autosuspend(dev); + + pm_runtime_set_autosuspend_delay(dev, child_delay); /* default 100 */ + pm_runtime_set_autosuspend_delay(parent, 2000); + + KUNIT_ASSERT_GT(test, child_delay, period_variation); + + pm_runtime_enable(parent); + pm_runtime_enable(dev); + + /* Aim for 1 variation every 2000ms */ + const unsigned int variation_interval = 2000 / (child_delay - period_variation) + 1; + + for (int i = 0; i < 200; i++) { + KUNIT_EXPECT_EQ(test, 0, pm_runtime_resume_and_get(dev)); + + /* do some I/O */ + + pm_runtime_put_autosuspend(dev); + + if ((i + 1) % variation_interval == 0) + msleep(child_delay + period_variation); + else + msleep(child_delay - period_variation); + + flush_workqueue(pm_wq); + } + + pm_runtime_disable(dev); + pm_runtime_disable(parent); + + KUNIT_EXPECT_EQ(test, 0, parent_data->suspend_count); + kunit_info(test, "Test params: delay %u +/- %u; variation interval %u\n", child_delay, period_variation, variation_interval); + kunit_info(test, "Parent suspend executions: %d\n", parent_data->suspend_count); + kunit_info(test, "Child suspend executions: %d\n", child_data->suspend_count); +} + static struct kunit_case pm_runtime_test_cases[] = { KUNIT_CASE(pm_runtime_depth_test), KUNIT_CASE(pm_runtime_already_suspended_test), @@ -396,12 +519,15 @@ static struct kunit_case pm_runtime_test_cases[] = { KUNIT_CASE(pm_runtime_supplier_suspend_test), KUNIT_CASE(pm_runtime_parent_last_used_test), KUNIT_CASE(pm_runtime_supplier_last_used_test), + KUNIT_CASE_SLOW(pm_runtime_parent_periodic_use_test), {} }; static struct kunit_suite pm_runtime_test_suite = { .name = "pm_runtime_test_cases", .test_cases = pm_runtime_test_cases, + .init = pm_runtime_test_init, + .exit = pm_runtime_test_exit, }; kunit_test_suite(pm_runtime_test_suite); ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-05 20:21 ` Brian Norris @ 2026-11-05 21:18 ` Rafael J. Wysocki (Intel) 2026-11-06 1:06 ` Brian Norris 0 siblings, 1 reply; 22+ messages in thread From: Rafael J. Wysocki (Intel) @ 2026-11-05 21:18 UTC (permalink / raw) To: Brian Norris Cc: Rafael J. Wysocki (Intel), Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin On Thu, Nov 5, 2026 at 9:21 PM Brian Norris <briannorris@chromium.org> wrote: > > Hi Rafael, > > On Thu, Nov 05, 2026 at 12:59:24PM +0100, Rafael J. Wysocki (Intel) wrote: > > On Wed, Nov 4, 2026 at 8:31 PM Brian Norris <briannorris@chromium.org> wrote: > > > > > > On Wed, Nov 04, 2026 at 02:34:22PM +0100, Rafael J. Wysocki (Intel) wrote: > > > > I should have responded to the v1 of this, but didn't manage to get to > > > > it. Sorry about that. > > > > > > Hehe, well that's what v2 is for :) > > > > > > Thanks for looking. > > > > > > > On Tue, Nov 3, 2026 at 11:23 PM Brian Norris <briannorris@chromium.org> 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. > > > > > > To be clear: dropping a reference is not the point at which we extend > > > the timer -- it's simply the point at which we propagate the last_busy > > > timestamp. > > > > > > The naive algorithm is: > > > Every time a device updates its last_busy, sync that timestamp to all > > > its dependencies. > > > > > > My proposal modifies that naive algorithm so that it avoids additional > > > graph walks -- it defers that "sync" step until later. (Now that I'm > > > spelling this out, I feel like I should have included this in the commit > > > message...) > > > > OK, thanks for explaining! > > > > You seem to be arguing that the current behavior is counter-intuitive > > because people would generally expect the parent's (or supplier's) > > last_busy to be automatically advanced to the last "busy" time of its > > last active child (or consumer). > > More or less. Or, I don't see why "child was recently busy and may be > busy again soon" should not translate to "everything the child uses > might be needed again soon". > > > That is a bit far-fetched IMV because the child's last "busy" time > > need not be the same as the parent's last "busy" time, at least in > > principle (of course, that is debatable, but that debate can be had > > later). > > I don't get this -- I'm very much asking for that debate right now, > because so far, I disagree :) > > > Still, the underlying observation seems to be that if the child > > suspends and then resumes relatively shortly, it may not make sense to > > suspend and resume the parent in the meantime, which is fair enough. > > Of course, advancing the parent's last_busy will only help here if its > > autosuspend delay is long enough though. > > Right. But that's the whole point of this argument -- if all autosuspend > delays were the same, this wouldn't really matter. But the presumption > is that the parent wants delay because it's more costly -- it will be > imposing extra wakeup cost for its children if it's not accounted > properly. > > > So you can argue that if the parent's autosuspend delay is long > > enough, then it should prevent the parent from suspending across > > relatively short periods of child suspension. Again, this is fair > > enough, so why does it not happen at times? > > > > It certainly does happen when the parent actually has to resume > > because rpm_resume() calls pm_runtime_mark_last_busy() for it then and > > (given a sufficiently long autosuspend delay) it will stay active if > > the child resumes a short enough time after suspending. However, when > > the child resumes, the parent is still active, so rpm_resume() > > triggered by the child does not advance its last_busy timestamp, so at > > one point in the future it will suspend when the child suspends. > > > > This arguably is inconsistent, so my approach to addressing the > > problem at hand would be the attached patch (sorry for the attachment, > > but GMail is generally not good at handling appended patches). If I'm > > not mistaken, it should be sufficient to address the particular issue > > that triggered commit c537d3457542. > > It's only sufficient if the child is not active for "too long". Suppose > the child stays active for 2s and then suspends briefly; your solution > does not work. It doesn't work in the sense that it doesn't do what you want. But fair enough. > In commit c537d3457542, I imagine the 0.2s polling example is just an > example; I'm not sure about that, but OK, say it's just an example. > what if that period was 0.1s, with some natural variance up or > down? Then I could easily see how the child remains active for longer > than 2s, and then occasionally is inactive for longer than 0.1s, > allowing both the child and parent to autosuspend. > > Or in concrete terms: see the appended test diff, which attempts to > emulate: > > - 200 cycles of child get/put > - delay in between each put -> get cycle > - delay of approximately 100ms, with a configurable +/- variation > - apply the shorter variation (e.g., 90ms) most of the time, with a > longer variation approximately every 2s > > So for example, that looks something like: > > cycle 0 to 22: delay 90ms > cycle 23: delay 110ms > cycle 24 to 46: delay 90ms > ... > > With your patch: > > 0ms variation == PASS (child suspends 200 times, parent suspends 0 times) > 10ms variation == FAIL (child suspends 21 times, parent suspends 4 times) > 20ms variation == FAIL (child suspends 7 times, parent suspends 7 times) > 30ms variation == FAIL (child suspends 6 times, parent suspends 6 times) > 40ms variation == FAIL (child suspends 5 times, parent suspends 5 times) > 50ms variation == FAIL (child suspends 4 times, parent suspends 4 times) > 60ms variation == FAIL (child suspends 3 times, parent suspends 3 times) > 70ms variation == FAIL (child suspends 2 times, parent suspends 2 times) > 80ms variation == FAIL (child suspends 1 time, parent suspends 1 time) > 90ms variation == PASS (test didn't run long enough to fail) > > With my patch, all cases pass. (The child suspends in the same pattern, > but the parent does not suspend in between.) > > For comparison, with neither patch: > > 0ms variation == FAIL (child suspends 200 times, parent suspends 10 times) > 10ms variation == FAIL (child suspends 14 times, parent suspends 8 times) > 20ms variation == FAIL (child suspends 7 times, parent suspends 7 times) > 30ms variation == FAIL (child suspends 6 times, parent suspends 6 times) > 40ms variation == FAIL (child suspends 5 times, parent suspends 5 times) > 50ms variation == FAIL (child suspends 4 times, parent suspends 4 times) > 60ms variation == FAIL (child suspends 3 times, parent suspends 3 times) > 70ms variation == FAIL (child suspends 2 times, parent suspends 2 times) > 80ms variation == FAIL (child suspends 1 time, parent suspends 1 time) > 90ms variation == PASS (test didn't run long enough to fail) > > So, your patch makes an improvement, but it doesn't handle this kind of > variation very well. And it is not designed to do so. Of course, it will not prevent the parent from suspending if the child does not attempt to resume it for a time longer than its autosuspend delay. That's by design and you didn't have to take the time to demonstrate it. The question is how often those kinds of patterns occur in practice. Or IOW, whether or not they occur often enough to justify adding complexity related specifically to addressing them. > > Note that I'm not worried about the potential raciness of this new > > last_busy update with other possible concurrent updates of last_busy > > because they all use approximations of the current time and if they > > are close enough to each other, it should not matter who wins the > > race. > > Ack, that's one advantage in your style of solution, because you're only > handling timestamps "in the moment." > > > > With that said: > > > > > > > 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. > > > > > > But that's not what I'm doing: I'm not calling mark_last_busy() -- I'm > > > just making mark_last_busy() transitive to all dependencies. So any time > > > we *already* say the child is busy, I also extend that to the parent -- > > > I'm not generating any new timestamps; I only reuse the timestamps we > > > already selected. > > > > > > My view on the logic (using your terms above): when a child calls > > > mark_last_busy(), it suggests that child might be needed in the near > > > future. Whenever a child device is needed, so is its parent (barring > > > ignore_children). Thus, mark_last_busy() should also refresh the > > > parent's busy timestamp. > > > > Of course, it can be argued this way though there are > > counter-arguments (probably not worth discussing in detail at this > > point) and there is the question how much of a difference this would > > make after addressing the rpm_resume() inconsistency mentioned above > > (which IMV needs to be address regardless because it is not limited to > > the parent-child or supplier-consumer situations). > > I would love to hear your counterarguments. OK, I'll say more about that tomorrow (it's past 10 PM here, time to go away from the keyboard). > I don't believe your solution solves the problem, That depends on what problem in particular you mean. It addresses an inconsistency in rpm_resume() and as I said above, this needs to be done anyway because it is not only related to the cases when children or consumers are involved. There are other cases in which rpm_resume() gets called for an active device and the lack of last_busy update causes the device to suspend earlier than expected. > and my solution does while also handling > the inconsistency you mention. I'm not sure about that. For example, say a device has an autosuspend delay of 2 s. Say it is initially suspended and user space writes "on" to its "power/control", so the device resumes. Now, 1 s later, user space writes "auto" to the device's "power/control" and the autosuspend delay is respected, so the device doesn't suspend. Next, 1.5 s after the first resume, user space writes "on" to the device's "power/control" and the device does not resume because it is active (its last_busy is not updated). Finally, user space writes "auto" to the device's "power/control", say 1 s later, and the device suspends even though it can be expected to stay active for at least 1 s more. How is that going to be addressed by propagating last_busy from children/consumers to parents/suppliers? Note that I'm not dismissing your proposal, I'm just having doubts about the need to do this kind of thing in the core code. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-05 21:18 ` Rafael J. Wysocki (Intel) @ 2026-11-06 1:06 ` Brian Norris 2026-11-09 15:24 ` Rafael J. Wysocki (Intel) 0 siblings, 1 reply; 22+ messages in thread From: Brian Norris @ 2026-11-06 1:06 UTC (permalink / raw) To: Rafael J. Wysocki (Intel) Cc: Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin On Thu, Nov 05, 2026 at 10:18:52PM +0100, Rafael J. Wysocki (Intel) wrote: > Of course, it will not prevent the parent from suspending if the child > does not attempt to resume it for a time longer than its autosuspend > delay. That's by design and you didn't have to take the time to > demonstrate it. To be clear, I wasn't just trying to demonstrate for fun or because it wasn't clear. I think there's value in putting concrete reproducible test cases out there. (KUnit is an easier common ground than some particular piece of obscure hardware.) > The question is how often those kinds of patterns occur in practice. > Or IOW, whether or not they occur often enough to justify adding > complexity related specifically to addressing them. Well, I tried to keep very close to the spirit of the actual real reports in question. So when commit c537d3457542 says, "As an example [...] cat in_voltageY_raw at a 0.2s rate", I think it's not a far stretch to test 0.1s too. And it's actually quite odd (IMO) if 0.2s behaves *better* than 0.1s or 0.09s, just because it hits the parent's rpm_resume() more often. > > On Thu, Nov 05, 2026 at 12:59:24PM +0100, Rafael J. Wysocki (Intel) wrote: > > I would love to hear your counterarguments. > > OK, I'll say more about that tomorrow (it's past 10 PM here, time to > go away from the keyboard). Great! And don't rush on my account. I'm interested in eventual progress, even if not rapid. I'm sure you're busy. Or, maybe I should have gone to LPC... > > I don't believe your solution solves the problem, > > That depends on what problem in particular you mean. > > It addresses an inconsistency in rpm_resume() and as I said above, > this needs to be done anyway because it is not only related to the > cases when children or consumers are involved. There are other cases > in which rpm_resume() gets called for an active device and the lack of > last_busy update causes the device to suspend earlier than expected. Yes, on second thought, I agree that your patch addresses a real problem, even if it's not (IMO) quite a full solution for commit c537d3457542's. > > and my solution does while also handling > > the inconsistency you mention. > > I'm not sure about that. Sorry, I see that part was inaccurate. > For example, say a device has an autosuspend delay of 2 s. Say it is > initially suspended and user space writes "on" to its "power/control", > so the device resumes. Now, 1 s later, user space writes "auto" to > the device's "power/control" and the autosuspend delay is respected, > so the device doesn't suspend. Next, 1.5 s after the first resume, > user space writes "on" to the device's "power/control" and the device > does not resume because it is active (its last_busy is not updated). > Finally, user space writes "auto" to the device's "power/control", say > 1 s later, and the device suspends even though it can be expected to > stay active for at least 1 s more. > > How is that going to be addressed by propagating last_busy from > children/consumers to parents/suppliers? Agreed, your patch addresses some scenarios mine doesn't. I also agree the power/control behavior is a bit awkward, although it's not very convincing to me as a use case. (Arbitrary/periodic fiddling with power/control seems more like a nuisance API than a real expected use case.) Or if it were, I would suggest solving that by adding pm_runtime_mark_last_busy() to pm_runtime_allow() :) To me, a more convincing one is: a non-autosuspending child with an autosuspending parent, in a situation otherwise similar to commit c537d3457542. The child is not likely to intentionally update last_busy, but only implicitly via rpm_resume(). When you solve the inconsistency with your patch, then it enables my patch to be more effective w.r.t. the parent. > Note that I'm not dismissing your proposal, I'm just having doubts > about the need to do this kind of thing in the core code. OK. I agree it adds a bit of complexity, but at the moment, it seems to me like the most thorough way to improve the ergonomics for clients (drivers) here. And that's pretty much my entire motivation here: to improve the ergonomics of runtime PM -- even if in this case, it requires a little (core) complexity. So, I'd love to hear what can make the ergonomics better here, with less core complexity. Thanks, Brian ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-06 1:06 ` Brian Norris @ 2026-11-09 15:24 ` Rafael J. Wysocki (Intel) 2026-11-11 0:04 ` Brian Norris 0 siblings, 1 reply; 22+ messages in thread From: Rafael J. Wysocki (Intel) @ 2026-11-09 15:24 UTC (permalink / raw) To: Brian Norris Cc: Rafael J. Wysocki (Intel), Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin On Fri, Nov 6, 2026 at 2:06 AM Brian Norris <briannorris@chromium.org> wrote: > > On Thu, Nov 05, 2026 at 10:18:52PM +0100, Rafael J. Wysocki (Intel) wrote: > > Of course, it will not prevent the parent from suspending if the child > > does not attempt to resume it for a time longer than its autosuspend > > delay. That's by design and you didn't have to take the time to > > demonstrate it. > > To be clear, I wasn't just trying to demonstrate for fun or because it > wasn't clear. I think there's value in putting concrete reproducible > test cases out there. (KUnit is an easier common ground than some > particular piece of obscure hardware.) > > > The question is how often those kinds of patterns occur in practice. > > Or IOW, whether or not they occur often enough to justify adding > > complexity related specifically to addressing them. > > Well, I tried to keep very close to the spirit of the actual real > reports in question. So when commit c537d3457542 says, "As an example > [...] cat in_voltageY_raw at a 0.2s rate", I think it's not a far > stretch to test 0.1s too. And it's actually quite odd (IMO) if 0.2s > behaves *better* than 0.1s or 0.09s, just because it hits the parent's > rpm_resume() more often. > > > > On Thu, Nov 05, 2026 at 12:59:24PM +0100, Rafael J. Wysocki (Intel) wrote: > > > I would love to hear your counterarguments. > > > > OK, I'll say more about that tomorrow (it's past 10 PM here, time to > > go away from the keyboard). > > Great! And don't rush on my account. I'm interested in eventual > progress, even if not rapid. I'm sure you're busy. Having to context-switch back to this later would probably be harder than continuing it now. > Or, maybe I should have gone to LPC... I was busy all the time there though, so I guess no regrets at least from that standpoint. > > > I don't believe your solution solves the problem, > > > > That depends on what problem in particular you mean. > > > > It addresses an inconsistency in rpm_resume() and as I said above, > > this needs to be done anyway because it is not only related to the > > cases when children or consumers are involved. There are other cases > > in which rpm_resume() gets called for an active device and the lack of > > last_busy update causes the device to suspend earlier than expected. > > Yes, on second thought, I agree that your patch addresses a real > problem, even if it's not (IMO) quite a full solution for commit > c537d3457542's. No, it is not a full solution for this kind of a problem. > > > and my solution does while also handling > > > the inconsistency you mention. > > > > I'm not sure about that. > > Sorry, I see that part was inaccurate. No worries. > > For example, say a device has an autosuspend delay of 2 s. Say it is > > initially suspended and user space writes "on" to its "power/control", > > so the device resumes. Now, 1 s later, user space writes "auto" to > > the device's "power/control" and the autosuspend delay is respected, > > so the device doesn't suspend. Next, 1.5 s after the first resume, > > user space writes "on" to the device's "power/control" and the device > > does not resume because it is active (its last_busy is not updated). > > Finally, user space writes "auto" to the device's "power/control", say > > 1 s later, and the device suspends even though it can be expected to > > stay active for at least 1 s more. > > > > How is that going to be addressed by propagating last_busy from > > children/consumers to parents/suppliers? > > Agreed, your patch addresses some scenarios mine doesn't. > > I also agree the power/control behavior is a bit awkward, although it's > not very convincing to me as a use case. Well, that was just the first example I could think about while writing the above. > (Arbitrary/periodic fiddling > with power/control seems more like a nuisance API than a real expected > use case.) Or if it were, I would suggest solving that by adding > pm_runtime_mark_last_busy() to pm_runtime_allow() :) Interesting thought, and hold on to it. > To me, a more convincing one is: a non-autosuspending child with an > autosuspending parent, in a situation otherwise similar to commit > c537d3457542. The child is not likely to intentionally update last_busy, > but only implicitly via rpm_resume(). When you solve the inconsistency > with your patch, then it enables my patch to be more effective w.r.t. > the parent. > > > Note that I'm not dismissing your proposal, I'm just having doubts > > about the need to do this kind of thing in the core code. > > OK. I agree it adds a bit of complexity, but at the moment, it seems to > me like the most thorough way to improve the ergonomics for clients > (drivers) here. And that's pretty much my entire motivation here: to > improve the ergonomics of runtime PM -- even if in this case, it > requires a little (core) complexity. > > So, I'd love to hear what can make the ergonomics better here, with less > core complexity. Let me start with a clear problem statement to kind of refresh the context. In some cases, there are unwanted parent suspends occurring between child suspend and resume that delay the latter unnecessarily and generally waste energy. They mostly occur when the child suspends and resumes relatively often and the parent's autosuspend delay is too short to cover the entire period between the child suspend and resume along with the preceding period of the child activity following the most recent update of the parent's last_busy timestamp, at least sometimes. Of course, this also equally applies to suppliers and consumers, but let's focus the attention on parents and children. The problem addressed by https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=55b2b8af704f845e01fa2f31ee259b07641ce3a9 makes the unwanted parent suppends to occur more often because it causes the parent to entirely miss its last_busy updates in the cases when pm_runtime_resume() (or equivalent) is called for it when it is active. When this goes away, it should be possible to address those "unwanted parent suspends" at least in some cases by increasing the parent's autosuspend delay. However, using larger autosuspend delay values generally causes system power to grow (on average) because it effectively causes devices to spend less time in suspend, so increasing autosuspend delay values is not a particularly attractive option. Notice though that advancing the last_busy timestamp more often without reducing the autosuspend delay values of the affected devices is also likely to lead to the overall increase of system power because it would effectively cause the devices to spend less time in suspend (on average) likely increasing their power (unless the majority of their suspends are unwanted). Now, let me acknowledge that some drivers evidently want rpm_idle() to always set the autosuspend timer instead of suspending the device right away. That may be because of the "unwanted parent suspends" problem at hand or for other reasons. Regardless, it is effectively equivalent to setting last_busy in rpm_idle(), so they use their .runtime_idle() callbacks for calling pm_runtime_mark_last_busy(). It arguably would be nice to allow them to just declare that, for example by introducing something like pm_runtime_use_autosuspend_always() to be called instead of pm_runtime_use_autosuspend() if the driver wants rpm_idle() to always set the autosuspend timer instead of attempting to suspend the device directly. I think that using pm_runtime_use_autosuspend_always(), if it existed, might be attractive for a couple of reasons. First off, which is rather obvious, a driver using pm_runtime_use_autosuspend_always() wouldn't need to worry about last_busy at all, so it would never need to call pm_runtime_mark_last_busy() or pm_runtime_put_autosuspend(). It would just need to call pm_runtime_use_autosuspend_always() once and set the autosuspend delay (which may even be combined with the former). Second, the autosuspend delay of a driver using pm_runtime_use_autosuspend_always() would only need to cover the period after the last runtime PM reference to the given device had been dropped while currently an autosuspend delay needs to cover that period along with some (usually unknown) period of device activity between the most recent last_busy update and the time when rpm_idle() gets called. Thus, with pm_runtime_use_autosuspend_always(), the autosuspend delay usage would be arguably more straightforward and easier to reason about, and also the autosuspend delay might be reduced in the majority of cases. Of course, drivers that have the "unwanted parent suspends" problem might switch over to using pm_runtime_use_autosuspend_always() and adjust their autosuspend delay values to cover full periods of their children's suspension. In theory, it should be possible to reduce their autosuspend delay values while at it, for reasons explained above, so the total average system power might even remain unchanged at least roughly. That overall might be a zero-overhead change. Using pm_runtime_use_autosuspend_always() would also make autosuspend work consistently in all situations regardless of the reason why the last runtime PM reference to the given device had been dropped, including the "power/control" manipulations from user space, in which case it would be equivalent to calling pm_runtime_mark_last_busy() in the sysfs code path mentioned by you above. Moreover, suppose that pm_runtime_use_autosuspend_always() turns out to work for everybody, regardless of whether or not they have the "unwanted parent suspends" problem. In that case, it will be possible to simplify the whole autosuspend delay handling quite a bit. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-09 15:24 ` Rafael J. Wysocki (Intel) @ 2026-11-11 0:04 ` Brian Norris 2026-11-11 3:47 ` Alan Stern 0 siblings, 1 reply; 22+ messages in thread From: Brian Norris @ 2026-11-11 0:04 UTC (permalink / raw) To: Rafael J. Wysocki (Intel) Cc: Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin, Alan Stern (+CC Alan, since he wrote much of the autosuspend stuff, but also because he might(?) have better context for some of the .runtime_idle() + pm_runtime_mark_last_busy() questions) Hi Rafael, I was a bit slow to respond here, because I wanted to consider this carefully, and not simply give a knee-jerk reaction. Overall, I have some positive and negative thoughts. I'm still leaning toward preferring a proposal more like my own, but I think with a few things cleared up, I may be convinced of yours instead. On Mon, Nov 09, 2026 at 04:24:33PM +0100, Rafael J. Wysocki (Intel) wrote: > On Fri, Nov 6, 2026 at 2:06 AM Brian Norris <briannorris@chromium.org> wrote: > > So, I'd love to hear what can make the ergonomics better here, with less > > core complexity. > > Let me start with a clear problem statement to kind of refresh the context. > > In some cases, there are unwanted parent suspends occurring between > child suspend and resume that delay the latter unnecessarily and > generally waste energy. They mostly occur when the child suspends and > resumes relatively often and the parent's autosuspend delay is too > short to cover the entire period between the child suspend and resume > along with the preceding period of the child activity following the > most recent update of the parent's last_busy timestamp, at least > sometimes. Sure, that's one way to frame it. > Of course, this also equally applies to suppliers and consumers, but > let's focus the attention on parents and children. > > The problem addressed by > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=linux-next&id=55b2b8af704f845e01fa2f31ee259b07641ce3a9 > > makes the unwanted parent suppends to occur more often because it > causes the parent to entirely miss its last_busy updates in the cases > when pm_runtime_resume() (or equivalent) is called for it when it is > active. When this goes away, it should be possible to address those > "unwanted parent suspends" at least in some cases by increasing the > parent's autosuspend delay. I think "in some cases" is holding a lot of weight. For every increased parent autosuspend delay, there is always some usage pattern that will subvert it (even post-commit-55b2b8af704f) -- the child simply needs to stay "in use" for a bit longer. We shouldn't have to think about how long a device is in use when determining the autosuspend delay. So I think this is a non-starter line of thought, and not just because of your system power arguments. To me, autosuspend delay is simply broken today when looking at parent devices. Maybe we can glean some intention from the comments and context around any given delay used by drivers today, but they aren't reliably meaningful. > However, using larger autosuspend delay > values generally causes system power to grow (on average) because it > effectively causes devices to spend less time in suspend, so > increasing autosuspend delay values is not a particularly attractive > option. > > Notice though that advancing the last_busy timestamp more often > without reducing the autosuspend delay values of the affected devices > is also likely to lead to the overall increase of system power because > it would effectively cause the devices to spend less time in suspend > (on average) likely increasing their power (unless the majority of > their suspends are unwanted). I don't think we should be treating current autosuspend delays for parent devices as meaningful at all, per the above. So I think this is a chance to try to improve the architecture so it makes sense, and then figure out if any existing delays should be tuned/reduced. Ideally, yes, we can avoid making significant system power changes to the negative, but I don't think that we should prioritize that above all else, as long as it's *possible* to tune the autosuspend delays in a way that makes sense. > Now, let me acknowledge that some drivers evidently want rpm_idle() to > always set the autosuspend timer instead of suspending the device > right away. That may be because of the "unwanted parent suspends" > problem at hand or for other reasons. Do we have any idea what the "other reasons" could be? For example, because driver writers found it too difficult to find all the appropriate places to call pm_runtime_mark_last_busy()? (Does commit b3db492e8335 ("PM: runtime: Mark last busy stamp in pm_runtime_put_autosuspend()") help? Or should pm_runtime_put() behave similarly?) I'm genuinely interested. Because I think that you *might* be reasoning us into a weird corner where we "hope" things can be improved architecturally in the long run, but we don't really know, and aren't going to actually make it happen without knowing why things are the way that they are. > Regardless, it is effectively > equivalent to setting last_busy in rpm_idle(), so they use their > .runtime_idle() callbacks for calling pm_runtime_mark_last_busy(). Side note: I think .runtime_idle() should be removed. As far as I can tell, it's only used for working around architectural problems in runtime PM (like $subject), or for implementing autosuspend without actually using autosuspend (usually because they predated autosuspend). At the same time, it makes it harder to reason about the runtime PM API surface -- because currently, .runtime_idle() is a primary reason that pm_runtime_put() and pm_runtime_put_autosuspend() are different. (A secondary reason is the last_busy update -- which is exactly the problem we're talking about here.) I have some experimental patches to remove it entirely, but I wanted to clear this obvious problem up first. > It > arguably would be nice to allow them to just declare that, for example > by introducing something like pm_runtime_use_autosuspend_always() to > be called instead of pm_runtime_use_autosuspend() if the driver wants > rpm_idle() to always set the autosuspend timer instead of attempting > to suspend the device directly. Maybe. I don't really like forcing driver writers to think about and opt into yet another quirk of RPM, especially if we can reason out whether the existing behavior is bad/quirky/broken, and that the new behavior is in fact better. (Frankly, I don't like making driver writers think about rpm_idle() at all. IMO, it makes more sense as an internal implementation detail than as a part of the client API.) If we go this route, would you be open to making it opt *out*, so drivers can opt back out (hopefully with clearly-documented reasons we can treat as problem reports) if necessary? Otherwise, I think we'll have a hard time bringing drivers along, and instead we'll simply have a more fragmented and confusing set of runtime PM features. > I think that using pm_runtime_use_autosuspend_always(), if it existed, > might be attractive for a couple of reasons. First off, which is > rather obvious, a driver using pm_runtime_use_autosuspend_always() > wouldn't need to worry about last_busy at all, so it would never need > to call pm_runtime_mark_last_busy() or pm_runtime_put_autosuspend(). Yes, that would be very nice. I also don't think drivers should ever have to think about the difference between pm_runtime_put_autosuspend() and pm_runtime_put(), although I think we can get there in other ways (removing .runtime_idle(); rolling pm_runtime_mark_last_busy() into all the put() helpers, or possibly to rpm_drop_usage_count()). > It would just need to call pm_runtime_use_autosuspend_always() once > and set the autosuspend delay (which may even be combined with the > former). > Second, the autosuspend delay of a driver using > pm_runtime_use_autosuspend_always() would only need to cover the > period after the last runtime PM reference to the given device had > been dropped I'd agree, this sounds very nice, except for one concern: "Last PM reference" can be a bit subtle; particularly, the child only drops a reference on its parent once it *completes* autosuspend (even if it was last "busy" much earlier). That means autosuspend delays become additive along the hierarchy. Is that desirable? Personally, I don't think so, and that's why I chose propagation (my proposal), rather than extension (what I'd call the behavior you're describing). I also _think_ that's what Jonathan was saying here: https://lore.kernel.org/all/20261103231719.486a18de@jic23-hlaptop/ But I'm not quite sure, so I'll leave room for correction here. And really, I think this is the main distinction between our proposals. (The other is that you're proposing making this behavior conditional / opt-in.) To me, I think it's important to distinguish when the last real *use* was, versus when the runtime PM framework (timers, etc.) hadn't yet decided to suspend. Autosuspend is the trailing indicator, while mark_last_busy() is the leading indicator. Maybe I'm missing something though. > while currently an autosuspend delay needs to cover that > period along with some (usually unknown) period of device activity > between the most recent last_busy update and the time when rpm_idle() > gets called. > > Thus, with pm_runtime_use_autosuspend_always(), the > autosuspend delay usage would be arguably more straightforward and > easier to reason about, and also the autosuspend delay might be > reduced in the majority of cases. > > Of course, drivers that have the "unwanted parent suspends" problem > might switch over to using pm_runtime_use_autosuspend_always() and > adjust their autosuspend delay values to cover full periods of their > children's suspension. In theory, it should be possible to reduce > their autosuspend delay values while at it, for reasons explained > above, so the total average system power might even remain unchanged > at least roughly. That overall might be a zero-overhead change. > > Using pm_runtime_use_autosuspend_always() would also make autosuspend > work consistently in all situations regardless of the reason why the > last runtime PM reference to the given device had been dropped, > including the "power/control" manipulations from user space, in which > case it would be equivalent to calling pm_runtime_mark_last_busy() in > the sysfs code path mentioned by you above. > > Moreover, suppose that pm_runtime_use_autosuspend_always() turns out > to work for everybody, regardless of whether or not they have the > "unwanted parent suspends" problem. In that case, it will be possible > to simplify the whole autosuspend delay handling quite a bit. I think many of these simplifications/improvements you talk about are good. I also don't really think they're unique to your described solution; I think we can get many or most of them in other ways too. For example, I think that with my propagation solution, we also can likely reduce autosuspend delays for many of the same cases that could be improved with yours, if that's deemed valuable. Or if we really want to go all-out on reducing inconsistency in mark_last_busy() placement: we could add pm_runtime_mark_last_busy() into rpm_drop_usage_count(). I also would like to add / repeat a few questions: What, specifically, do you think my proposal misses? Too complex? Doesn't go far enough on updating last_busy (e.g., with the pm_runtime_allow() use cases)? Wastes power? (In what ways relative to your proposal? Simply because yours is opt-in?) Something else? Or because its concept is not good? For example, I don't think you really addressed a question from a few replies up: https://lore.kernel.org/all/auzlLwlleGLPsLcw@google.com/ Rephrased: why shouldn't the parent's last busy time be the same (or later) than the child's? Your proposal seems to achieve almost the same (in fact, it's more aggressive), with slightly different means, and with more complexity for users -- that everyone has to opt in, and to think about what rpm_idle is. And a similar theme for other unanswered questions: https://lore.kernel.org/all/auzlLwlleGLPsLcw@google.com/ Why shouldn't pci_config_pm_runtime_put() (and similar) update last_busy? To me, if we can't properly address the question of "when should we update last_busy?" (and instead defer to yet another opt-in feature within runtime PM, without making sure everyone can use it), then I think we'll never *really* move things forward. (And to be clear: I'm not trying to ask these questions to drive "my solution is better; why don't you agree?!" I just want to understand the principles in the design, and in the decisions you'd like to make here, so I can help make sure they do indeed make the driver experience better, and that there is a good path to convergence.) Brian ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-11 0:04 ` Brian Norris @ 2026-11-11 3:47 ` Alan Stern 2026-11-11 16:42 ` Brian Norris 0 siblings, 1 reply; 22+ messages in thread From: Alan Stern @ 2026-11-11 3:47 UTC (permalink / raw) To: Brian Norris Cc: Rafael J. Wysocki (Intel), Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin On Tue, Nov 10, 2026 at 04:04:15PM -0800, Brian Norris wrote: > (+CC Alan, since he wrote much of the autosuspend stuff, but also > because he might(?) have better context for some of the .runtime_idle() > + pm_runtime_mark_last_busy() questions) I'm coming into this discussion in the middle, so I probably don't have a good picture of all the issues you've been talking about. From what I gather, it sounds like this is mostly a question of how long a parent device should delay autosuspending after all its children go into runtime suspend. Is that right? The idea behind autosuspend was simple enough: Try to optimize energy usage by putting a device into runtime suspend only when it seems likely that the device will remain suspended for more than a brief period of time -- long enough to make the extra work of suspending and resuming worthwhile. In principle the driver core could collect statistics for each device about how long it remains resumed and suspended, but the benefits of such a detailed analysis didn't seem worth the effort. And given that drivers know more about how their devices will tend to behave than the core does, it made sense to let drivers determine the best autosuspend delay time for each device individually. In theory, a driver or bus type might want to have differing delay values for the time after all children are suspended vs. the time after the device had some other work to do (not related to its children). How much of an advantage would that yield? I don't know, but I felt that it wasn't large enough to put support for it into the first implementation of autosuspend. So yes, in the current scheme a parent's inactivity period doesn't start until all its children are suspended (unless the ignore_children flag is set, but never mind that now). The parent's autosuspend delay therefore should take into account the expected time before one of the children may have to be resumed, as well as the expected time before the parent itself may be asked to do something that doesn't involve any children. (Note that if a device has more than one child, the expected time before any of its children will be resumed can be different from the expected time before any single particular child will be resumed.) The simplest way to carry this out is to do pm_runtime_mark_last_busy() whenever a child suspends. But other approaches to the same goal are conceivable. Of course it's not possible to determine an "ideal" autosuspend delay time. In fact, I suspect that in most cases we aren't even close. But I think doing it this way is still better than anything else that isn't a lot more complex. > I don't think we should be treating current autosuspend delays for > parent devices as meaningful at all, per the above. So I think this is a > chance to try to improve the architecture so it makes sense, and then > figure out if any existing delays should be tuned/reduced. > > Ideally, yes, we can avoid making significant system power changes to > the negative, but I don't think that we should prioritize that above all > else, as long as it's *possible* to tune the autosuspend delays in a way > that makes sense. Of course it's possible to tune the autosuspend delays. That's what /sys/.../power/autosuspend_delay_ms is for. Whether it's possible to do so in a way that makes sense will depend very heavily on the type of device in question and the way the system is used. One possible approach might be to have a userspace daemon that tracks all runtime PM transitions and tries to do some sort of dynamic tuning. How well would that work? I don't know, but it would be a good research project for somebody. > Side note: I think .runtime_idle() should be removed. As far as I can > tell, it's only used for working around architectural problems in > runtime PM (like $subject), or for implementing autosuspend without > actually using autosuspend (usually because they predated autosuspend). I have never given much thought to runtime_idle(). When we were doing the initial implementation of runtime PM I didn't see any use for it, but Rafael was quite certain that it had its place and the issue wasn't important enough to argue about. I wouldn't mind a bit if it were eliminated, but I also don't really mind the extra overhead of the idle callback pointers and support routines. > At the same time, it makes it harder to reason about the runtime PM API > surface -- because currently, .runtime_idle() is a primary reason that > pm_runtime_put() and pm_runtime_put_autosuspend() are different. I wouldn't put it that way. My feeling is that pm_runtime_put() could be replaced by pm_runtime_put_autosuspend(), since they should behave identically for devices that don't use autosuspend. (If there are any devices that _do_ use autosuspend and also use pm_runtime_put() -- well, I'd say that sounds like either a design error or a simple oversight.) The reason for having two different routines is mainly historical: pm_runtime_put_autosuspend() was added some time after pm_runtime_put() and we didn't want to mess up the existing callers. Maybe this judgement was a mistake. > "Last PM reference" can be a bit subtle; particularly, the child only > drops a reference on its parent once it *completes* autosuspend (even if > it was last "busy" much earlier). That means autosuspend delays become > additive along the hierarchy. Is that desirable? That's up to the maintainers of the device and its bus subsystem. They can arrange the default autosuspend delays any way they like, taking the children's autosuspend delays into account if they want -- and the user can always override the defaults. But one thing that does _not_ make sense would be to have non-additive delays where a parent's delay is shorter than its child's. Your email was fairly long, and I have touched on only a few of the issues it brought up. Is there anything significant you still want to discuss? Alan Stern ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency 2026-11-11 3:47 ` Alan Stern @ 2026-11-11 16:42 ` Brian Norris 0 siblings, 0 replies; 22+ messages in thread From: Brian Norris @ 2026-11-11 16:42 UTC (permalink / raw) To: Alan Stern Cc: Rafael J. Wysocki (Intel), Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin Hi Alan, On Tue, Nov 10, 2026 at 10:47:59PM -0500, Alan Stern wrote: > On Tue, Nov 10, 2026 at 04:04:15PM -0800, Brian Norris wrote: > > (+CC Alan, since he wrote much of the autosuspend stuff, but also > > because he might(?) have better context for some of the .runtime_idle() > > + pm_runtime_mark_last_busy() questions) > > I'm coming into this discussion in the middle, so I probably don't have > a good picture of all the issues you've been talking about. From what I > gather, it sounds like this is mostly a question of how long a parent > device should delay autosuspending after all its children go into > runtime suspend. Is that right? How long, and when should the inactivity timer start. (The latter more than the former, really.) > The idea behind autosuspend was simple enough: Try to optimize energy > usage by putting a device into runtime suspend only when it seems likely > that the device will remain suspended for more than a brief period of > time -- long enough to make the extra work of suspending and resuming > worthwhile. In principle the driver core could collect statistics for > each device about how long it remains resumed and suspended, but the > benefits of such a detailed analysis didn't seem worth the effort. And > given that drivers know more about how their devices will tend to behave > than the core does, it made sense to let drivers determine the best > autosuspend delay time for each device individually. > > In theory, a driver or bus type might want to have differing delay > values for the time after all children are suspended vs. the time after > the device had some other work to do (not related to its children). How > much of an advantage would that yield? I don't know, but I felt that it > wasn't large enough to put support for it into the first implementation > of autosuspend. Right, I don't think anybody is asking for this right now. We're just trying to clean up the corners on how the existing timer works. > So yes, in the current scheme a parent's inactivity period doesn't start > until all its children are suspended (unless the ignore_children flag is > set, but never mind that now). But to be clear, that's not actually how runtime PM works today. It only works this way for the very small handful of drivers where the parent calls pm_runtime_mark_last_busy() in .runtime_idle(). I think several people have acknolwedged that this is a bit of a hack, or at least a missing feature in runtime PM, and probably should be extended more widely. > The parent's autosuspend delay therefore > should take into account the expected time before one of the children > may have to be resumed, as well as the expected time before the parent > itself may be asked to do something that doesn't involve any children. > (Note that if a device has more than one child, the expected time before > any of its children will be resumed can be different from the expected > time before any single particular child will be resumed.) "Expected time before a child may have to resume": isn't that child last_busy + child delay? So that expected time is already in the past by the time the child autosuspends. But: > The simplest way to carry this out is to do pm_runtime_mark_last_busy() > whenever a child suspends. But other approaches to the same goal are > conceivable. Yes, I agree it's simple. Maybe the distinction I'm making doesn't matter that much. > Of course it's not possible to determine an "ideal" autosuspend delay > time. In fact, I suspect that in most cases we aren't even close. But > I think doing it this way is still better than anything else that isn't > a lot more complex. Ack. > > I don't think we should be treating current autosuspend delays for > > parent devices as meaningful at all, per the above. So I think this is a > > chance to try to improve the architecture so it makes sense, and then > > figure out if any existing delays should be tuned/reduced. > > > > Ideally, yes, we can avoid making significant system power changes to > > the negative, but I don't think that we should prioritize that above all > > else, as long as it's *possible* to tune the autosuspend delays in a way > > that makes sense. > > Of course it's possible to tune the autosuspend delays. That's what > /sys/.../power/autosuspend_delay_ms is for. Whether it's possible to do > so in a way that makes sense will depend very heavily on the type of > device in question and the way the system is used. One possible > approach might be to have a userspace daemon that tracks all runtime PM > transitions and tries to do some sort of dynamic tuning. How well would > that work? I don't know, but it would be a good research project for > somebody. My point here was that I think Rafael was overly concerned about precisely tuning parent autosuspend delays at the same time as changing the runtime PM core behavior. Yes, they clearly can be tuned in user space; but he was suggesting a phase-in / opt-in approach to this change, because it might harm system power if it's not re-tuned. > > Side note: I think .runtime_idle() should be removed. As far as I can > > tell, it's only used for working around architectural problems in > > runtime PM (like $subject), or for implementing autosuspend without > > actually using autosuspend (usually because they predated autosuspend). > > I have never given much thought to runtime_idle(). When we were doing > the initial implementation of runtime PM I didn't see any use for it, > but Rafael was quite certain that it had its place and the issue wasn't > important enough to argue about. I wouldn't mind a bit if it were > eliminated, but I also don't really mind the extra overhead of the idle > callback pointers and support routines. The overhead isn't my concern. The mental overhead is. There are several rough edges that would be smoother without it, IMO. > > At the same time, it makes it harder to reason about the runtime PM API > > surface -- because currently, .runtime_idle() is a primary reason that > > pm_runtime_put() and pm_runtime_put_autosuspend() are different. > > I wouldn't put it that way. My feeling is that pm_runtime_put() could > be replaced by pm_runtime_put_autosuspend(), since they should behave > identically for devices that don't use autosuspend. Ah, but that's not actually true! pm_runtime_put_autosuspend() skips .runtime_idle(). It (semi)directly calls rpm_suspend(). > (If there are any > devices that _do_ use autosuspend and also use pm_runtime_put() -- well, > I'd say that sounds like either a design error or a simple oversight.) > The reason for having two different routines is mainly historical: > pm_runtime_put_autosuspend() was added some time after pm_runtime_put() > and we didn't want to mess up the existing callers. Maybe this > judgement was a mistake. > > > "Last PM reference" can be a bit subtle; particularly, the child only > > drops a reference on its parent once it *completes* autosuspend (even if > > it was last "busy" much earlier). That means autosuspend delays become > > additive along the hierarchy. Is that desirable? > > That's up to the maintainers of the device and its bus subsystem. They > can arrange the default autosuspend delays any way they like, taking the > children's autosuspend delays into account if they want -- and the user > can always override the defaults. But one thing that does _not_ make > sense would be to have non-additive delays where a parent's delay is > shorter than its child's. > > Your email was fairly long, and I have touched on only a few of the > issues it brought up. Is there anything significant you still want to > discuss? I appreciate all the responses. One particular part I did have questions on, which I'll mostly copy/paste: [Rafael:] > Now, let me acknowledge that some drivers evidently want rpm_idle() to > always set the autosuspend timer instead of suspending the device > right away. That may be because of the "unwanted parent suspends" > problem at hand or for other reasons. Do we have any idea what the "other reasons" could be? For example, because driver writers found it too difficult to find all the appropriate places to call pm_runtime_mark_last_busy()? (Does commit b3db492e8335 ("PM: runtime: Mark last busy stamp in pm_runtime_put_autosuspend()") help? Or should pm_runtime_put() behave similarly?) I see that USB is one of the subsystems where .runtime_idle() calls pm_runtime_mark_last_busy(), and I thought you (Alan) might have some more background on that. Thanks, Brian ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 4/5] PM: runtime: Add tests for last_busy propagation 2026-11-03 22:14 [PATCH v2 0/5] PM: runtime: Improve autosuspend child to parent propagation Brian Norris ` (2 preceding siblings ...) 2026-11-03 22:14 ` [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency Brian Norris @ 2026-11-03 22:14 ` Brian Norris 2026-11-03 23:21 ` Jonathan Cameron 2026-11-03 22:14 ` [PATCH v2 5/5] iio: adc: stm32-adc: Drop runtime_idle() Brian Norris 4 siblings, 1 reply; 22+ messages in thread From: Brian Norris @ 2026-11-03 22:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin, Brian Norris This is a regression test for the problem fixed in "PM: runtime: Propagate last_busy from dependent to dependency". Signed-off-by: Brian Norris <briannorris@chromium.org> --- Changes in v2: * Use device_move() instead of manual re-parenting * fsleep() instead of msleep() drivers/base/power/runtime-test.c | 99 +++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c index 24865ce844fc..00ca3b10543a 100644 --- a/drivers/base/power/runtime-test.c +++ b/drivers/base/power/runtime-test.c @@ -4,6 +4,7 @@ */ #include <linux/cleanup.h> +#include <linux/delay.h> #include <linux/pm_runtime.h> #include <linux/workqueue.h> #include <kunit/device.h> @@ -285,6 +286,102 @@ static void pm_runtime_supplier_suspend_test(struct kunit *test) KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(norpm_supplier)); } +static void pm_runtime_parent_last_used_test(struct kunit *test) +{ + struct device *parent = kunit_device_register(test, DEVICE_NAME "_parent"); + struct device *dev = kunit_device_register(test, DEVICE_NAME); + u64 expiry1, expiry2; + + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent); + + KUNIT_ASSERT_EQ(test, 0, device_move(dev, parent, DPM_ORDER_NONE)); + + pm_runtime_set_active(parent); + pm_runtime_set_active(dev); + pm_runtime_use_autosuspend(parent); + pm_runtime_use_autosuspend(dev); + + pm_runtime_set_autosuspend_delay(dev, 0); + /* Effectively infinite delay (prevent finishing autosuspend during this test). */ + pm_runtime_set_autosuspend_delay(parent, 200000); + + pm_runtime_mark_last_busy(dev); + pm_runtime_mark_last_busy(parent); + + pm_runtime_get_noresume(dev); + + pm_runtime_enable(parent); + pm_runtime_enable(dev); + + expiry1 = pm_runtime_autosuspend_expiration(parent); + KUNIT_EXPECT_NE(test, 0, expiry1); + + /* Make absolutely sure the clock advances. */ + fsleep(1000); + + KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync_autosuspend(dev)); + + KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev)); + + /* Flush, in case any request was pending. */ + flush_workqueue(pm_wq); + KUNIT_EXPECT_FALSE(test, pm_runtime_suspended(parent)); + + expiry2 = pm_runtime_autosuspend_expiration(parent); + KUNIT_EXPECT_NE(test, 0, expiry2); + /* The child's last_busy time should have bumped up the parent's expiration. */ + KUNIT_EXPECT_GT(test, expiry2, expiry1); +} + +static void pm_runtime_supplier_last_used_test(struct kunit *test) +{ + struct device *supplier = kunit_device_register(test, DEVICE_NAME "_supplier"); + struct device *dev = kunit_device_register(test, DEVICE_NAME); + u64 expiry1, expiry2; + + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, supplier); + + KUNIT_ASSERT_NOT_NULL(test, device_link_add(dev, supplier, DL_FLAG_PM_RUNTIME)); + + pm_runtime_set_active(supplier); + pm_runtime_set_active(dev); + pm_runtime_use_autosuspend(supplier); + pm_runtime_use_autosuspend(dev); + + pm_runtime_set_autosuspend_delay(dev, 0); + /* Effectively infinite delay (prevent finishing autosuspend during this test). */ + pm_runtime_set_autosuspend_delay(supplier, 200000); + + pm_runtime_mark_last_busy(dev); + pm_runtime_mark_last_busy(supplier); + + pm_runtime_get_noresume(dev); + + pm_runtime_enable(supplier); + pm_runtime_enable(dev); + + expiry1 = pm_runtime_autosuspend_expiration(supplier); + KUNIT_EXPECT_NE(test, 0, expiry1); + + /* Make absolutely sure the clock advances. */ + fsleep(1000); + + KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync_autosuspend(dev)); + + KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev)); + + /* Flush, in case any request was pending. */ + flush_workqueue(pm_wq); + KUNIT_EXPECT_FALSE(test, pm_runtime_suspended(supplier)); + + expiry2 = pm_runtime_autosuspend_expiration(supplier); + KUNIT_EXPECT_NE(test, 0, expiry2); + /* The consumer's last_busy time should have bumped up the supplier's expiration. */ + KUNIT_EXPECT_GT(test, expiry2, expiry1); +} + static struct kunit_case pm_runtime_test_cases[] = { KUNIT_CASE(pm_runtime_depth_test), KUNIT_CASE(pm_runtime_already_suspended_test), @@ -293,6 +390,8 @@ static struct kunit_case pm_runtime_test_cases[] = { KUNIT_CASE(pm_runtime_error_test), KUNIT_CASE(pm_runtime_probe_active_test), KUNIT_CASE(pm_runtime_supplier_suspend_test), + KUNIT_CASE(pm_runtime_parent_last_used_test), + KUNIT_CASE(pm_runtime_supplier_last_used_test), {} }; -- 2.56.0.398.g882cbad275-goog ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/5] PM: runtime: Add tests for last_busy propagation 2026-11-03 22:14 ` [PATCH v2 4/5] PM: runtime: Add tests for last_busy propagation Brian Norris @ 2026-11-03 23:21 ` Jonathan Cameron 0 siblings, 0 replies; 22+ messages in thread From: Jonathan Cameron @ 2026-11-03 23:21 UTC (permalink / raw) To: Brian Norris Cc: Rafael J. Wysocki, Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin On Tue, 3 Nov 2026 14:14:42 -0800 Brian Norris <briannorris@chromium.org> wrote: > This is a regression test for the problem fixed in "PM: runtime: > Propagate last_busy from dependent to dependency". > > Signed-off-by: Brian Norris <briannorris@chromium.org> Trivial style stuff. > --- > > Changes in v2: > * Use device_move() instead of manual re-parenting > * fsleep() instead of msleep() > > drivers/base/power/runtime-test.c | 99 +++++++++++++++++++++++++++++++ > 1 file changed, 99 insertions(+) > > diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c > index 24865ce844fc..00ca3b10543a 100644 > --- a/drivers/base/power/runtime-test.c > +++ b/drivers/base/power/runtime-test.c > @@ -4,6 +4,7 @@ > */ > > #include <linux/cleanup.h> > +#include <linux/delay.h> > #include <linux/pm_runtime.h> > #include <linux/workqueue.h> > #include <kunit/device.h> > @@ -285,6 +286,102 @@ static void pm_runtime_supplier_suspend_test(struct kunit *test) > KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(norpm_supplier)); > } > > +static void pm_runtime_parent_last_used_test(struct kunit *test) > +{ > + struct device *parent = kunit_device_register(test, DEVICE_NAME "_parent"); > + struct device *dev = kunit_device_register(test, DEVICE_NAME); Similar style comment to below. > + u64 expiry1, expiry2; > + > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent); > + > + KUNIT_ASSERT_EQ(test, 0, device_move(dev, parent, DPM_ORDER_NONE)); > + > + pm_runtime_set_active(parent); > + pm_runtime_set_active(dev); > + pm_runtime_use_autosuspend(parent); > + pm_runtime_use_autosuspend(dev); > + > + pm_runtime_set_autosuspend_delay(dev, 0); > + /* Effectively infinite delay (prevent finishing autosuspend during this test). */ > + pm_runtime_set_autosuspend_delay(parent, 200000); > + > + pm_runtime_mark_last_busy(dev); > + pm_runtime_mark_last_busy(parent); > + > + pm_runtime_get_noresume(dev); > + > + pm_runtime_enable(parent); > + pm_runtime_enable(dev); > + > + expiry1 = pm_runtime_autosuspend_expiration(parent); > + KUNIT_EXPECT_NE(test, 0, expiry1); > + > + /* Make absolutely sure the clock advances. */ > + fsleep(1000); > + > + KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync_autosuspend(dev)); > + > + KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev)); > + > + /* Flush, in case any request was pending. */ > + flush_workqueue(pm_wq); > + KUNIT_EXPECT_FALSE(test, pm_runtime_suspended(parent)); > + > + expiry2 = pm_runtime_autosuspend_expiration(parent); > + KUNIT_EXPECT_NE(test, 0, expiry2); > + /* The child's last_busy time should have bumped up the parent's expiration. */ > + KUNIT_EXPECT_GT(test, expiry2, expiry1); > +} > + > +static void pm_runtime_supplier_last_used_test(struct kunit *test) > +{ > + struct device *supplier = kunit_device_register(test, DEVICE_NAME "_supplier"); > + struct device *dev = kunit_device_register(test, DEVICE_NAME); Having the assignment clear of the error check is never great, but now there are multiple devs, I'd definitely move them. struct device *supplier, *dev; > + u64 expiry1, expiry2; > + supplier = kunit_device_register(test, DEVICE_NAME "_supplier"); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev); dev = kunit_device_register(test, DEVICE_NAME); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, supplier); > + > + KUNIT_ASSERT_NOT_NULL(test, device_link_add(dev, supplier, DL_FLAG_PM_RUNTIME)); > + > + pm_runtime_set_active(supplier); > + pm_runtime_set_active(dev); > + pm_runtime_use_autosuspend(supplier); > + pm_runtime_use_autosuspend(dev); > + > + pm_runtime_set_autosuspend_delay(dev, 0); > + /* Effectively infinite delay (prevent finishing autosuspend during this test). */ > + pm_runtime_set_autosuspend_delay(supplier, 200000); > + > + pm_runtime_mark_last_busy(dev); > + pm_runtime_mark_last_busy(supplier); > + > + pm_runtime_get_noresume(dev); > + > + pm_runtime_enable(supplier); > + pm_runtime_enable(dev); > + > + expiry1 = pm_runtime_autosuspend_expiration(supplier); > + KUNIT_EXPECT_NE(test, 0, expiry1); > + > + /* Make absolutely sure the clock advances. */ > + fsleep(1000); > + > + KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync_autosuspend(dev)); > + > + KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev)); > + > + /* Flush, in case any request was pending. */ > + flush_workqueue(pm_wq); > + KUNIT_EXPECT_FALSE(test, pm_runtime_suspended(supplier)); > + > + expiry2 = pm_runtime_autosuspend_expiration(supplier); > + KUNIT_EXPECT_NE(test, 0, expiry2); > + /* The consumer's last_busy time should have bumped up the supplier's expiration. */ > + KUNIT_EXPECT_GT(test, expiry2, expiry1); > +} > + > static struct kunit_case pm_runtime_test_cases[] = { > KUNIT_CASE(pm_runtime_depth_test), > KUNIT_CASE(pm_runtime_already_suspended_test), > @@ -293,6 +390,8 @@ static struct kunit_case pm_runtime_test_cases[] = { > KUNIT_CASE(pm_runtime_error_test), > KUNIT_CASE(pm_runtime_probe_active_test), > KUNIT_CASE(pm_runtime_supplier_suspend_test), > + KUNIT_CASE(pm_runtime_parent_last_used_test), > + KUNIT_CASE(pm_runtime_supplier_last_used_test), > {} > }; > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 5/5] iio: adc: stm32-adc: Drop runtime_idle() 2026-11-03 22:14 [PATCH v2 0/5] PM: runtime: Improve autosuspend child to parent propagation Brian Norris ` (3 preceding siblings ...) 2026-11-03 22:14 ` [PATCH v2 4/5] PM: runtime: Add tests for last_busy propagation Brian Norris @ 2026-11-03 22:14 ` Brian Norris 4 siblings, 0 replies; 22+ messages in thread From: Brian Norris @ 2026-11-03 22:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Fabrice Gasnier, linux-pm, linux-arm-kernel, Ulf Hansson, linux-kernel, linux-iio, Andy Shevchenko, Jonathan Cameron, David Lechner, Alexandre Torgue, Nuno Sá, Maxime Coquelin, Brian Norris, Fabrice Gasnier, Jonathan Cameron This runtime_idle() implementation was added in commit c537d3457542 ("iio: adc: stm32-adc: fix runtime autosuspend delay when slow polling"), to work around a suboptimal behavior in the runtime PM core. It should no longer be necessary as of "PM: runtime: Propagate last_busy from dependent to dependency". Signed-off-by: Brian Norris <briannorris@chromium.org> Tested-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> Acked-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com> --- I do not have these devices to test. I have tested other drivers, and I've verified this behavior in unit tests as well. (no changes since v1) drivers/iio/adc/stm32-adc-core.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c index c409edd3a827..0193db7a2a7f 100644 --- a/drivers/iio/adc/stm32-adc-core.c +++ b/drivers/iio/adc/stm32-adc-core.c @@ -873,17 +873,10 @@ static int stm32_adc_core_runtime_resume(struct device *dev) return stm32_adc_core_hw_start(dev); } -static int stm32_adc_core_runtime_idle(struct device *dev) -{ - pm_runtime_mark_last_busy(dev); - - return 0; -} - static DEFINE_RUNTIME_DEV_PM_OPS(stm32_adc_core_pm_ops, stm32_adc_core_runtime_suspend, stm32_adc_core_runtime_resume, - stm32_adc_core_runtime_idle); + NULL); static const struct stm32_adc_priv_cfg stm32f4_adc_priv_cfg = { .regs = &stm32f4_adc_common_regs, -- 2.56.0.398.g882cbad275-goog ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2026-11-11 16:42 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-11-03 22:14 [PATCH v2 0/5] PM: runtime: Improve autosuspend child to parent propagation Brian Norris 2026-11-03 22:14 ` [PATCH v2 1/5] PM: runtime: Only queue an idle check for RPM-linked suppliers (part 2) Brian Norris 2026-11-04 14:44 ` Rafael J. Wysocki (Intel) 2026-11-03 22:14 ` [PATCH v2 2/5] PM: runtime: Convert last_busy to atomic64_t Brian Norris 2026-11-04 8:21 ` Joshua Crofts 2026-11-03 22:14 ` [PATCH v2 3/5] PM: runtime: Propagate last_busy from dependent to dependency Brian Norris 2026-11-03 23:17 ` Jonathan Cameron 2026-11-04 0:32 ` Brian Norris 2026-11-04 1:40 ` Jonathan Cameron 2026-11-04 13:34 ` Rafael J. Wysocki (Intel) 2026-11-04 19:31 ` Brian Norris 2026-11-05 11:59 ` Rafael J. Wysocki (Intel) 2026-11-05 20:21 ` Brian Norris 2026-11-05 21:18 ` Rafael J. Wysocki (Intel) 2026-11-06 1:06 ` Brian Norris 2026-11-09 15:24 ` Rafael J. Wysocki (Intel) 2026-11-11 0:04 ` Brian Norris 2026-11-11 3:47 ` Alan Stern 2026-11-11 16:42 ` Brian Norris 2026-11-03 22:14 ` [PATCH v2 4/5] PM: runtime: Add tests for last_busy propagation Brian Norris 2026-11-03 23:21 ` Jonathan Cameron 2026-11-03 22:14 ` [PATCH v2 5/5] iio: adc: stm32-adc: Drop runtime_idle() Brian Norris
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®