mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/5] PM: runtime: Autosuspend race condition fixes + tests
@ 2026-09-29 18:47 Brian Norris
  2026-09-29 18:47 ` [PATCH 1/5] PM: runtime: Cancel pending only during non-transient suspend failure Brian Norris
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Brian Norris @ 2026-09-29 18:47 UTC (permalink / raw)
  To: Rafael J . Wysocki
  Cc: Ulf Hansson, Douglas Anderson, Len Brown, linux-kernel,
	Pavel Machek, Laurent Pinchart, linux-pm, Brian Norris

This series includes a few semi-related bugfixes for race conditions
around autosuspend and asynchronous suspend, as well as KUnit tests.
They don't all have to go together, but because their tests are added to
the same file, it helps to group them to avoid conflicts.

- Brian


Brian Norris (5):
  PM: runtime: Cancel pending only during non-transient suspend failure
  PM: runtime: Avoid racy clock checks for autosuspend-retry
  PM: runtime: Add test for autosuspend-rearm
  PM: runtime: Synchronous suspend when updating autosuspend
  PM: runtime: Add test for pending-autosuspend + teardown

 drivers/base/power/runtime-test.c | 97 +++++++++++++++++++++++++++++++
 drivers/base/power/runtime.c      | 52 +++++++++++------
 include/linux/pm_runtime.h        |  7 ++-
 3 files changed, 138 insertions(+), 18 deletions(-)

-- 
2.56.0.rc1.315.gc6ed9934b7-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/5] PM: runtime: Cancel pending only during non-transient suspend failure
  2026-09-29 18:47 [PATCH 0/5] PM: runtime: Autosuspend race condition fixes + tests Brian Norris
@ 2026-09-29 18:47 ` Brian Norris
  2026-09-29 18:47 ` [PATCH 2/5] PM: runtime: Avoid racy clock checks for autosuspend-retry Brian Norris
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Brian Norris @ 2026-09-29 18:47 UTC (permalink / raw)
  To: Rafael J . Wysocki
  Cc: Ulf Hansson, Douglas Anderson, Len Brown, linux-kernel,
	Pavel Machek, Laurent Pinchart, linux-pm, Brian Norris

If runtime_suspend() fails for transient reasons (EBUSY, EAGAIN), we
intend to allow retrying suspend in many cases. However, on the failure
path we unconditionally cancel any pending timers, which means a suspend
timer scheduled while the callback was running (e.g., via
pm_schedule_suspend()) may be prematurely aborted.

This seems to have inadvertently changed with commit 72263869656d ("PM:
runtime: Unify error handling during suspend and resume"), where the
error-handling logic in rpm_suspend() was subtly changed.

Fixes: 72263869656d ("PM: runtime: Unify error handling during suspend and resume")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 drivers/base/power/runtime.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index bb008dfe85a1..bc5905ffc45f 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -749,8 +749,10 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	dev->power.deferred_resume = false;
 	wake_up_all(&dev->power.wait_queue);
 
-	if (retval != -EAGAIN && retval != -EBUSY)
+	if (retval != -EAGAIN && retval != -EBUSY) {
 		dev->power.runtime_error = retval;
+		pm_runtime_cancel_pending(dev);
+	}
 
 	/*
 	 * On transient errors, if the callback routine failed an autosuspend,
@@ -762,8 +764,6 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	    pm_runtime_autosuspend_expiration(dev) != 0)
 		goto repeat;
 
-	pm_runtime_cancel_pending(dev);
-
 	goto out;
 }
 
-- 
2.56.0.rc1.315.gc6ed9934b7-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/5] PM: runtime: Avoid racy clock checks for autosuspend-retry
  2026-09-29 18:47 [PATCH 0/5] PM: runtime: Autosuspend race condition fixes + tests Brian Norris
  2026-09-29 18:47 ` [PATCH 1/5] PM: runtime: Cancel pending only during non-transient suspend failure Brian Norris
@ 2026-09-29 18:47 ` Brian Norris
  2026-10-03  0:27   ` Doug Anderson
  2026-09-29 18:47 ` [PATCH 3/5] PM: runtime: Add test for autosuspend-rearm Brian Norris
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2026-09-29 18:47 UTC (permalink / raw)
  To: Rafael J . Wysocki
  Cc: Ulf Hansson, Douglas Anderson, Len Brown, linux-kernel,
	Pavel Machek, Laurent Pinchart, linux-pm, Brian Norris

The runtime PM docs clearly tell how a .runtime_suspend() implementation
can update the last-busy time (pm_runtime_mark_last_busy()) and return a
transient error code (-EBUSY or -EAGAIN) in order to rearm the
autosuspend timer. However, this check is racy, such that it will not
always rearm the timer properly.

Consider:

 1. runtime_suspend() updates last_busy
 2. long delay (scheduling, etc.)
 3. runtime_suspend() returns -EBUSY
 4. pm_runtime_autosuspend_expiration() looks up the current time
    (ktime_get_mono_fast_ns())
 5. the time from #4 suggests the "expiry" time (based on #1) is already
    in the past
 6. rpm_suspend() quits without either retry or rearm

This violates the documented expectations.

The primary problem is the racy lookup of the current time in #4. We can
avoid that by:

 * Checking whether the expiration time changed before/after the suspend
   attempt, *without* consulting the current clock for past-expiry

 * Improving pm_runtime_mark_last_busy() so that it guards against
   producing the same expiry (e.g., consider a system with a
   low-resolution clock)

Noticed by inspection, and proved out with KUnit tests which follow this
patch.

I see this pattern is utilized in at least a few drivers:

 * blk_pre_runtime_suspend() (block/blk-pm.c)
 * serial_port_runtime_suspend() (drivers/tty/serial/serial_port.c)
 * omap4_keypad_runtime_suspend() (drivers/input/keyboard/omap4-keypad.c)
 * omap_rproc_runtime_suspend() (drivers/remoteproc/omap_remoteproc.c)
 * more...

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
There's a vanishingly small corner case in here still, if:

1) the monotonic clock gets an update while we're suspending

2) there are *two* mark_last_busy() calls

3) the result of those updates (backward in time) and mark_last_busy()
   (forward in time) puts us back at the same expiration.

We could potentially solve this by updating a |last_busy_counter|, and
tracking that instead. I'm not sure if it's worth the complexity though.

There's a similar corner case if somebody calls
pm_runtime_set_autosuspend_delay(), such that a shortened autosuspend
delay lands on the same expiry. This also seems vanishingly unlikely;
but this also could be solved by using a |last_busy_counter|, and
ignoring the delay entirely.

 drivers/base/power/runtime.c | 43 +++++++++++++++++++++++++-----------
 include/linux/pm_runtime.h   |  7 +++++-
 2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index bc5905ffc45f..2a99b7509744 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -162,6 +162,26 @@ static void pm_runtime_cancel_pending(struct device *dev)
 	dev->power.request = RPM_REQ_NONE;
 }
 
+/*
+ * Expiration time without considering whether it has already passed.
+ */
+static u64 __pm_runtime_autosuspend_expiration(struct device *dev)
+{
+	int autosuspend_delay;
+	u64 expires;
+
+	if (!dev->power.use_autosuspend)
+		return 0;
+
+	autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
+	if (autosuspend_delay < 0)
+		return 0;
+
+	expires = READ_ONCE(dev->power.last_busy);
+	expires += (u64)autosuspend_delay * NSEC_PER_MSEC;
+	return expires;
+}
+
 /**
  * pm_runtime_autosuspend_expiration - Get a device's autosuspend-delay expiration time.
  * @dev: Device to handle.
@@ -176,18 +196,10 @@ static void pm_runtime_cancel_pending(struct device *dev)
  */
 u64 pm_runtime_autosuspend_expiration(struct device *dev)
 {
-	int autosuspend_delay;
-	u64 expires;
+	u64 expires = __pm_runtime_autosuspend_expiration(dev);
 
-	if (!dev->power.use_autosuspend)
-		return 0;
-
-	autosuspend_delay = READ_ONCE(dev->power.autosuspend_delay);
-	if (autosuspend_delay < 0)
+	if (!expires)
 		return 0;
-
-	expires  = READ_ONCE(dev->power.last_busy);
-	expires += (u64)autosuspend_delay * NSEC_PER_MSEC;
 	if (expires > ktime_get_mono_fast_ns())
 		return expires;	/* Expires in the future */
 
@@ -586,6 +598,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 {
 	int (*callback)(struct device *);
 	struct device *parent = NULL;
+	u64 old_expires;
 	int retval;
 
 	trace_rpm_suspend(dev, rpmflags);
@@ -689,6 +702,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	}
 
 	__update_runtime_status(dev, RPM_SUSPENDING);
+	old_expires = __pm_runtime_autosuspend_expiration(dev);
 
 	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
 
@@ -760,9 +774,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	 * autosuspend expiration time, automatically reschedule another
 	 * autosuspend.
 	 */
-	if (!dev->power.runtime_error && (rpmflags & RPM_AUTO) &&
-	    pm_runtime_autosuspend_expiration(dev) != 0)
-		goto repeat;
+	if (!dev->power.runtime_error && (rpmflags & RPM_AUTO)) {
+		u64 new_expires = __pm_runtime_autosuspend_expiration(dev);
+
+		if (new_expires && new_expires != old_expires)
+			goto repeat;
+	}
 
 	goto out;
 }
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index 322e3b17f987..424ac9da94ef 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -242,7 +242,12 @@ 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());
+	u64 now = ktime_get_mono_fast_ns();
+
+	if (now == READ_ONCE(dev->power.last_busy))
+		now++;
+
+	WRITE_ONCE(dev->power.last_busy, now);
 }
 
 /**
-- 
2.56.0.rc1.315.gc6ed9934b7-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/5] PM: runtime: Add test for autosuspend-rearm
  2026-09-29 18:47 [PATCH 0/5] PM: runtime: Autosuspend race condition fixes + tests Brian Norris
  2026-09-29 18:47 ` [PATCH 1/5] PM: runtime: Cancel pending only during non-transient suspend failure Brian Norris
  2026-09-29 18:47 ` [PATCH 2/5] PM: runtime: Avoid racy clock checks for autosuspend-retry Brian Norris
@ 2026-09-29 18:47 ` Brian Norris
  2026-09-29 18:47 ` [PATCH 4/5] PM: runtime: Synchronous suspend when updating autosuspend Brian Norris
  2026-09-29 18:47 ` [PATCH 5/5] PM: runtime: Add test for pending-autosuspend + teardown Brian Norris
  4 siblings, 0 replies; 8+ messages in thread
From: Brian Norris @ 2026-09-29 18:47 UTC (permalink / raw)
  To: Rafael J . Wysocki
  Cc: Ulf Hansson, Douglas Anderson, Len Brown, linux-kernel,
	Pavel Machek, Laurent Pinchart, linux-pm, Brian Norris

If runtime_suspend() fails with a transient error (e.g., -EBUSY) +
pm_runtime_mark_last_busy(), we intend to rearm/retry autosuspend. Add a
test for it.

This is a regression test for the bug fixed in "PM: runtime: Avoid racy
clock checks for autosuspend-retry".

Example invocation:

  $ ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_PM=y \
      'pm_runtime*'

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 drivers/base/power/runtime-test.c | 70 +++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c
index 24865ce844fc..3487392e0afd 100644
--- a/drivers/base/power/runtime-test.c
+++ b/drivers/base/power/runtime-test.c
@@ -285,6 +285,75 @@ static void pm_runtime_supplier_suspend_test(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(norpm_supplier));
 }
 
+struct pm_runtime_test_data {
+	int pm_counter;
+};
+
+static int pm_runtime_rearm_test_suspend(struct device *dev)
+{
+	struct pm_runtime_test_data *data = dev_get_drvdata(dev);
+
+	/* Say we're busy the first time. */
+	if (data->pm_counter) {
+		data->pm_counter--;
+		pm_runtime_mark_last_busy(dev);
+
+		/*
+		 * We could also purposely slow this down, to potentially lose
+		 * more races:
+		 *   msleep(1000);
+		 */
+
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
+static int pm_runtime_rearm_test_resume(struct device *dev)
+{
+	struct pm_runtime_test_data *data = dev_get_drvdata(dev);
+
+	data->pm_counter++;
+	return 0;
+}
+
+static const struct dev_pm_ops pm_runtime_rearm_test_ops = {
+	RUNTIME_PM_OPS(pm_runtime_rearm_test_suspend, pm_runtime_rearm_test_resume, NULL)
+};
+
+static void pm_runtime_autosuspend_rearm_test(struct kunit *test)
+{
+	struct device *dev = kunit_device_register(test, DEVICE_NAME);
+	struct pm_runtime_test_data data = {
+		.pm_counter = 1,
+	};
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
+
+	dev_set_drvdata(dev, &data);
+	dev->driver->pm = &pm_runtime_rearm_test_ops;
+
+	pm_runtime_set_active(dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_enable(dev);
+
+	KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
+
+	/*
+	 * .runtime_suspend() should return -EBUSY once, but retry due to
+	 * pm_runtime_mark_last_busy().
+	 */
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_autosuspend(dev));
+
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+	KUNIT_EXPECT_EQ(test, 0, data.pm_counter);
+
+	/* Don't let on-stack drvdata escape this context. */
+	dev->driver->pm = NULL;
+	dev_set_drvdata(dev, NULL);
+}
+
 static struct kunit_case pm_runtime_test_cases[] = {
 	KUNIT_CASE(pm_runtime_depth_test),
 	KUNIT_CASE(pm_runtime_already_suspended_test),
@@ -293,6 +362,7 @@ 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_autosuspend_rearm_test),
 	{}
 };
 
-- 
2.56.0.rc1.315.gc6ed9934b7-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 4/5] PM: runtime: Synchronous suspend when updating autosuspend
  2026-09-29 18:47 [PATCH 0/5] PM: runtime: Autosuspend race condition fixes + tests Brian Norris
                   ` (2 preceding siblings ...)
  2026-09-29 18:47 ` [PATCH 3/5] PM: runtime: Add test for autosuspend-rearm Brian Norris
@ 2026-09-29 18:47 ` Brian Norris
  2026-09-29 18:47 ` [PATCH 5/5] PM: runtime: Add test for pending-autosuspend + teardown Brian Norris
  4 siblings, 0 replies; 8+ messages in thread
From: Brian Norris @ 2026-09-29 18:47 UTC (permalink / raw)
  To: Rafael J . Wysocki
  Cc: Ulf Hansson, Douglas Anderson, Len Brown, linux-kernel,
	Pavel Machek, Laurent Pinchart, linux-pm, Brian Norris

When updating autosuspend settings, we request synchronous autosuspend.
This is sometimes a best-effort action, because we don't know if there
are open reference counts for the device. In other cases, we expect this
to be reliable -- specifically, on the teardown side of
devm_pm_runtime_enable(), which calls pm_runtime_dont_use_autosuspend()
to (among other reasons) ensure the device will immediately suspend if
possible. (See commit b4060db9251f ("PM: runtime: Have
devm_pm_runtime_enable() handle pm_runtime_dont_use_autosuspend()").)

However, synchronous rpm_idle() does not flush pending suspend requests
-- it simply returns -EAGAIN. This is noticeable as a small race
condition in the following sort of scenario:

	/* Last user drops its references, and queues RPM_REQ_AUTOSUSPEND */
	pm_runtime_put_autosuspend(dev);

	/*
	 * Next, driver is unbound:
	 * --> driver remove()
	 *  --> devres teardown
	 *   --> pm_runtime_disable_action()
	 */
	 pm_runtime_dont_use_autosuspend(dev);
	/*
	 * --> rpm_idle(RPM_AUTO) may return -EAGAIN, and the SUSPEND
	 * request may still be pending...
	 */

	// Indeterminate device status: may be left SUSPENDED or ACTIVE,
	// depending on a race condition.
	pm_runtime_disable(dev);

If we use synchronous rpm_suspend() instead, we will properly wait for
an outstanding RPM_REQ_AUTOSUSPEND to quiesce (or else, perform our
own).

This is a safe change, because:

 * drivers that use autosuspend shouldn't care about .runtime_idle()
   (the difference between rpm_idle() and rpm_suspend()) [1]

 * drivers that don't use autosuspend will not reach this code, except
   on the teardown path of devm_pm_runtime_enable()

[1] This isn't actually mentioned in any documentation, but I infer this
    because its dedicated helpers (e.g., pm_runtime_put_autosuspend())
    skip any idle checks. And surveying existing drivers shows that
    those that implement .runtime_idle() behave similarly in their
    .runtime_suspend().

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
See also a regression test in the next patch.

This resolves one racy pitfall when using devm_pm_runtime_enable(), but
it doesn't resolve another pitfall: what if user space forbade runtime
PM (`echo on > /sys/devices/.../power/control`)? In that case, the
device will still remain RPM_ACTIVE on teardown.

Surveying many devm_pm_runtime_enable() users, it's common to fall into
these pitfalls. Commit 2d90ecdfa326 ("ASoC: rockchip: i2s: Use managed
hclk and runtime PM cleanup") shows a rare example that got all the
details right.

I may propose other changes to devm_pm_runtime_enable() later to try to
help those, but I expect them to be a bit more controversial.

 drivers/base/power/runtime.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 2a99b7509744..b0262f187041 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1788,7 +1788,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_irq_safe);
  * @old_use: The former use_autosuspend value.
  *
  * Prevent runtime suspend if the new delay is negative and use_autosuspend is
- * set; otherwise allow it.  Send an idle notification if suspends are allowed.
+ * set; otherwise allow it.  Attempt to suspend the device if suspends are
+ * allowed.
  *
  * This function must be called under dev->power.lock with interrupts disabled.
  */
@@ -1816,7 +1817,7 @@ static void update_autosuspend(struct device *dev, int old_delay, int old_use)
 			atomic_dec(&dev->power.usage_count);
 
 		/* Maybe we can autosuspend now. */
-		rpm_idle(dev, RPM_AUTO);
+		rpm_suspend(dev, RPM_AUTO);
 	}
 }
 
-- 
2.56.0.rc1.315.gc6ed9934b7-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 5/5] PM: runtime: Add test for pending-autosuspend + teardown
  2026-09-29 18:47 [PATCH 0/5] PM: runtime: Autosuspend race condition fixes + tests Brian Norris
                   ` (3 preceding siblings ...)
  2026-09-29 18:47 ` [PATCH 4/5] PM: runtime: Synchronous suspend when updating autosuspend Brian Norris
@ 2026-09-29 18:47 ` Brian Norris
  4 siblings, 0 replies; 8+ messages in thread
From: Brian Norris @ 2026-09-29 18:47 UTC (permalink / raw)
  To: Rafael J . Wysocki
  Cc: Ulf Hansson, Douglas Anderson, Len Brown, linux-kernel,
	Pavel Machek, Laurent Pinchart, linux-pm, Brian Norris

Add a KUnit regression test for the behavior fixed by the previous patch
("PM: runtime: Synchronous suspend when updating autosuspend"). Note
that the old behavior was racy -- a pending RPM_REQ_AUTOSUSPEND *might*
quiesce, but it wasn't guaranteed. Now the behavior should be non-racy.

Example invocation:

  $ ./tools/testing/kunit/kunit.py run --kconfig_add CONFIG_PM=y \
      'pm_runtime*'

Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 drivers/base/power/runtime-test.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c
index 3487392e0afd..67d637550092 100644
--- a/drivers/base/power/runtime-test.c
+++ b/drivers/base/power/runtime-test.c
@@ -354,6 +354,32 @@ static void pm_runtime_autosuspend_rearm_test(struct kunit *test)
 	dev_set_drvdata(dev, NULL);
 }
 
+static void pm_runtime_pending_teardown_test(struct kunit *test)
+{
+	struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
+
+	pm_runtime_enable(dev);
+	pm_runtime_use_autosuspend(dev);
+
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_get_sync(dev));
+	KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
+
+	/* Queue a RPM_REQ_AUTOSUSPEND (immediately, as autosuspend_delay is 0) */
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_autosuspend(dev));
+
+	/*
+	 * This is expected to quiesce a pending auto-suspend. See especially
+	 * devm_pm_runtime_enable() -> pm_runtime_disable_action().
+	 */
+	pm_runtime_dont_use_autosuspend(dev);
+
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+}
+
 static struct kunit_case pm_runtime_test_cases[] = {
 	KUNIT_CASE(pm_runtime_depth_test),
 	KUNIT_CASE(pm_runtime_already_suspended_test),
@@ -363,6 +389,7 @@ static struct kunit_case pm_runtime_test_cases[] = {
 	KUNIT_CASE(pm_runtime_probe_active_test),
 	KUNIT_CASE(pm_runtime_supplier_suspend_test),
 	KUNIT_CASE(pm_runtime_autosuspend_rearm_test),
+	KUNIT_CASE(pm_runtime_pending_teardown_test),
 	{}
 };
 
-- 
2.56.0.rc1.315.gc6ed9934b7-goog


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/5] PM: runtime: Avoid racy clock checks for autosuspend-retry
  2026-09-29 18:47 ` [PATCH 2/5] PM: runtime: Avoid racy clock checks for autosuspend-retry Brian Norris
@ 2026-10-03  0:27   ` Doug Anderson
  2026-10-05 18:59     ` Brian Norris
  0 siblings, 1 reply; 8+ messages in thread
From: Doug Anderson @ 2026-10-03  0:27 UTC (permalink / raw)
  To: Brian Norris
  Cc: Rafael J . Wysocki, Ulf Hansson, Len Brown, linux-kernel,
	Pavel Machek, Laurent Pinchart, linux-pm

Hi,

On Tue, Sep 29, 2026 at 11:47 AM Brian Norris <briannorris@chromium.org> wrote:
>
> @@ -242,7 +242,12 @@ 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());
> +       u64 now = ktime_get_mono_fast_ns();
> +
> +       if (now == READ_ONCE(dev->power.last_busy))
> +               now++;
> +
> +       WRITE_ONCE(dev->power.last_busy, now);

While we definitely want to fix the problem identified in this patch,
the proposed logic doesn't sit right with me. Let's say that
'power.last_busy" starts out as a given value, let's say "1020". Now,
while the clock hasn't ticked you call pm_runtime_mark_last_busy(). It
detects that 1020 == 1020 so it sets the time to 1021 so it's
different. Now you call pm_runtime_mark_last_busy() again when the
clock hasn't ticked. Now 1020 != 1021, so it goes back to 1020. This
could cause the whole heuristic to fail, can't it?

Maybe I'm just worrying about something that can't happen, but the
logic still seems odd.

It felt to me like we could just have a "bool". We set it to false
before we call runtime_suspend() and we check it after
runtime_suspend() returns. If the bool is set then we know they called
pm_runtime_mark_last_busy(). There shouldn't even be any weird
problems with weakly ordered memory since this boolean should always
be cleared, set, and checked in the same thread, right?

-Doug

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/5] PM: runtime: Avoid racy clock checks for autosuspend-retry
  2026-10-03  0:27   ` Doug Anderson
@ 2026-10-05 18:59     ` Brian Norris
  0 siblings, 0 replies; 8+ messages in thread
From: Brian Norris @ 2026-10-05 18:59 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Rafael J . Wysocki, Ulf Hansson, Len Brown, linux-kernel,
	Pavel Machek, Laurent Pinchart, linux-pm

On Fri, Oct 02, 2026 at 05:27:23PM -0700, Doug Anderson wrote:
> On Tue, Sep 29, 2026 at 11:47 AM Brian Norris <briannorris@chromium.org> wrote:
> >
> > @@ -242,7 +242,12 @@ 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());
> > +       u64 now = ktime_get_mono_fast_ns();
> > +
> > +       if (now == READ_ONCE(dev->power.last_busy))
> > +               now++;
> > +
> > +       WRITE_ONCE(dev->power.last_busy, now);
> 
> While we definitely want to fix the problem identified in this patch,
> the proposed logic doesn't sit right with me. Let's say that
> 'power.last_busy" starts out as a given value, let's say "1020". Now,
> while the clock hasn't ticked you call pm_runtime_mark_last_busy(). It
> detects that 1020 == 1020 so it sets the time to 1021 so it's
> different. Now you call pm_runtime_mark_last_busy() again when the
> clock hasn't ticked. Now 1020 != 1021, so it goes back to 1020. This
> could cause the whole heuristic to fail, can't it?
> 
> Maybe I'm just worrying about something that can't happen, but the
> logic still seems odd.

Thanks for pointing this all out. The above solution was trying to be
too clever for its own good, and left a different set of holes. It's
definitely odd, and I think it points toward:

1) either we somehow have to be even more clever or

2) we really need an additional state variable.

I don't think #1 is a good idea.

And #2 was what I was pointing toward below the "---" fold, mentioning a
|last_busy_counter|. I thought we could avoid it, but I believe I'm
wrong.

> It felt to me like we could just have a "bool". We set it to false
> before we call runtime_suspend() and we check it after
> runtime_suspend() returns. If the bool is set then we know they called
> pm_runtime_mark_last_busy().

Yes, I think it doesn't even need to be a counter -- just a bool is
probably fine.

> There shouldn't even be any weird
> problems with weakly ordered memory since this boolean should always
> be cleared, set, and checked in the same thread, right?

Well, it *can* be set in other threads (there's intentionally no locking
on pm_runtime_mark_last_busy()), but as long as the setters we care
about are in-thread, I think the only problem we'd invite is reading a
false "last_busy was set", and performing a spurious retry.

But potentially-spurious retry is already baked into this protocol.

I'll let this sit for a bit in case somebody else has other
thoughts/suggestions, but otherwise, I'll probably end up adding a
"last_busy_updated" field for v2.

Brian

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-10-05 18:59 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-29 18:47 [PATCH 0/5] PM: runtime: Autosuspend race condition fixes + tests Brian Norris
2026-09-29 18:47 ` [PATCH 1/5] PM: runtime: Cancel pending only during non-transient suspend failure Brian Norris
2026-09-29 18:47 ` [PATCH 2/5] PM: runtime: Avoid racy clock checks for autosuspend-retry Brian Norris
2026-10-03  0:27   ` Doug Anderson
2026-10-05 18:59     ` Brian Norris
2026-09-29 18:47 ` [PATCH 3/5] PM: runtime: Add test for autosuspend-rearm Brian Norris
2026-09-29 18:47 ` [PATCH 4/5] PM: runtime: Synchronous suspend when updating autosuspend Brian Norris
2026-09-29 18:47 ` [PATCH 5/5] PM: runtime: Add test for pending-autosuspend + teardown 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®