[PATCH 1/3] PM: runtime: Add basic kunit tests for API contracts

[PATCH 1/3] PM: runtime: Add basic kunit tests for API contracts

[Brian Norris]

In exploring the various return codes and failure modes of runtime PM
APIs, I found it helpful to verify and codify many of them in unit
tests, especially given that even the kerneldoc can be rather complex to
reason through, and it also has had subtle errors of its own.

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

 drivers/base/Kconfig              |   6 +
 drivers/base/power/Makefile       |   1 +
 drivers/base/power/runtime-test.c | 259 ++++++++++++++++++++++++++++++
 3 files changed, 266 insertions(+)
 create mode 100644 drivers/base/power/runtime-test.c

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 064eb52ff7e2..1786d87b29e2 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -167,6 +167,12 @@ config PM_QOS_KUNIT_TEST
 	depends on KUNIT=y
 	default KUNIT_ALL_TESTS
 
+config PM_RUNTIME_KUNIT_TEST
+	tristate "KUnit Tests for runtime PM" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	depends on PM
+	default KUNIT_ALL_TESTS
+
 config HMEM_REPORTING
 	bool
 	default n
diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index 01f11629d241..2989e42d0161 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -4,5 +4,6 @@ obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o wakeup_stats.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
 obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
 obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o
+obj-$(CONFIG_PM_RUNTIME_KUNIT_TEST) += runtime-test.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c
new file mode 100644
index 000000000000..263c28d5fc50
--- /dev/null
+++ b/drivers/base/power/runtime-test.c
@@ -0,0 +1,259 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2025 Google, Inc.
+ */
+
+#include <linux/cleanup.h>
+#include <linux/pm_runtime.h>
+#include <kunit/device.h>
+#include <kunit/test.h>
+
+#define DEVICE_NAME "pm_runtime_test_device"
+
+static void pm_runtime_depth_test(struct kunit *test)
+{
+	struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+	KUNIT_ASSERT_PTR_NE(test, NULL, dev);
+
+	pm_runtime_enable(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));
+	KUNIT_EXPECT_EQ(test, 1, pm_runtime_get_sync(dev)); /* "already active" */
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+}
+
+/* Test pm_runtime_put() and friends when already suspended. */
+static void pm_runtime_already_suspended_test(struct kunit *test)
+{
+	struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+	KUNIT_ASSERT_PTR_NE(test, NULL, dev);
+
+	pm_runtime_enable(dev);
+
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+	pm_runtime_get_noresume(dev);
+
+	/* Flush, in case the above (non-sync) triggered any work. */
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */
+
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+	/*
+	 * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
+	 * this as -EAGAIN / "runtime PM status change ongoing".
+	 */
+	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put(dev));
+
+	pm_runtime_get_noresume(dev);
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put_sync(dev));
+
+	KUNIT_EXPECT_EQ(test, 1, pm_runtime_suspend(dev));
+	KUNIT_EXPECT_EQ(test, 1, pm_runtime_autosuspend(dev));
+	KUNIT_EXPECT_EQ(test, 1, pm_request_autosuspend(dev));
+
+	pm_runtime_get_noresume(dev);
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+	KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_sync_autosuspend(dev));
+
+	pm_runtime_get_noresume(dev);
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+	KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_autosuspend(dev));
+
+	/* Grab 2 refcounts */
+	pm_runtime_get_noresume(dev);
+	pm_runtime_get_noresume(dev);
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+	/* The first put() sees usage_count 1 */
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_autosuspend(dev));
+	/* The second put() sees usage_count 0 but tells us "already suspended". */
+	KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_autosuspend(dev));
+
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+}
+
+static void pm_runtime_idle_test(struct kunit *test)
+{
+	struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+	KUNIT_ASSERT_PTR_NE(test, NULL, dev);
+
+	pm_runtime_enable(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));
+	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
+	KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
+	pm_runtime_put_noidle(dev);
+	KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_idle(dev));
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
+	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_request_idle(dev));
+}
+
+static void pm_runtime_disabled_test(struct kunit *test)
+{
+	struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+	KUNIT_ASSERT_PTR_NE(test, NULL, dev);
+
+	/* Never called pm_runtime_enable() */
+	KUNIT_EXPECT_FALSE(test, pm_runtime_enabled(dev));
+
+	/* "disabled" is treated as "active" */
+	KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
+	KUNIT_EXPECT_FALSE(test, pm_runtime_suspended(dev));
+
+	/*
+	 * Note: these "fail", but they still acquire/release refcounts, so
+	 * keep them balanced.
+	 */
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get(dev));
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put(dev));
+
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get_sync(dev));
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put_sync(dev));
+
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get(dev));
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put_autosuspend(dev));
+
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_resume_and_get(dev));
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_idle(dev));
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_request_idle(dev));
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_request_resume(dev));
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_request_autosuspend(dev));
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_suspend(dev));
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_resume(dev));
+	KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_autosuspend(dev));
+
+	/* Still active */
+	KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
+}
+
+static void pm_runtime_error_test(struct kunit *test)
+{
+	struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+	KUNIT_ASSERT_PTR_NE(test, NULL, dev);
+
+	pm_runtime_enable(dev);
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+
+	/* Fake a .runtime_resume() error */
+	dev->power.runtime_error = -EIO;
+
+	/*
+	 * Note: these "fail", but they still acquire/release refcounts, so
+	 * keep them balanced.
+	 */
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get(dev));
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put(dev));
+
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get_sync(dev));
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put_sync(dev));
+
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get(dev));
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put_autosuspend(dev));
+
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_resume_and_get(dev));
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_idle(dev));
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_idle(dev));
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_resume(dev));
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_autosuspend(dev));
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_suspend(dev));
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_resume(dev));
+	KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_autosuspend(dev));
+
+	/* Still suspended */
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+	/* Clear error */
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_set_suspended(dev));
+	KUNIT_EXPECT_EQ(test, 0, dev->power.runtime_error);
+	/* Still suspended */
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_get(dev));
+	KUNIT_EXPECT_EQ(test, 1, pm_runtime_barrier(dev)); /* resume was pending */
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_put(dev));
+	pm_runtime_suspend(dev); /* flush the put(), to suspend */
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_get_sync(dev));
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
+
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_get_sync(dev));
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_autosuspend(dev));
+
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_resume_and_get(dev));
+
+	/*
+	 * The following should all "fail" with -EAGAIN (usage is non-zero) or
+	 * 1 (already resumed).
+	 */
+	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
+	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_request_idle(dev));
+	KUNIT_EXPECT_EQ(test, 1, pm_request_resume(dev));
+	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_request_autosuspend(dev));
+	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_suspend(dev));
+	KUNIT_EXPECT_EQ(test, 1, pm_runtime_resume(dev));
+	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_autosuspend(dev));
+
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
+
+	/* Suspended again */
+	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
+}
+
+/*
+ * Explore a typical probe() sequence in which a device marks itself powered,
+ * but doesn't hold any runtime PM reference, so it suspends as soon as it goes
+ * idle.
+ */
+static void pm_runtime_probe_active_test(struct kunit *test)
+{
+	struct device *dev = kunit_device_register(test, DEVICE_NAME);
+
+	KUNIT_ASSERT_PTR_NE(test, NULL, dev);
+
+	KUNIT_EXPECT_TRUE(test, pm_runtime_status_suspended(dev));
+
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_set_active(dev));
+	KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
+
+	pm_runtime_enable(dev);
+	KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
+
+	/* Flush, and ensure we stayed active. */
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev));
+	KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
+
+	/* Ask for idle? Now we suspend. */
+	KUNIT_EXPECT_EQ(test, 0, pm_runtime_idle(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),
+	KUNIT_CASE(pm_runtime_idle_test),
+	KUNIT_CASE(pm_runtime_disabled_test),
+	KUNIT_CASE(pm_runtime_error_test),
+	KUNIT_CASE(pm_runtime_probe_active_test),
+	{}
+};
+
+static struct kunit_suite pm_runtime_test_suite = {
+	.name = "pm_runtime_test_cases",
+	.test_cases = pm_runtime_test_cases,
+};
+
+kunit_test_suite(pm_runtime_test_suite);
+MODULE_DESCRIPTION("Runtime power management unit test suite");
+MODULE_LICENSE("GPL");
-- 
2.51.0.318.gd7df087d1a-goog

[PATCH 2/3] PM: runtime: Make put{,_sync}() return 1 when already suspended

[Brian Norris]

The pm_runtime.h docs say pm_runtime_put() and pm_runtime_put_sync()
return 1 when already suspended, but this is not true -- they return
-EAGAIN. On the other hand, pm_runtime_put_sync_suspend() and
pm_runtime_put_sync_autosuspend() *do* return 1.

This is an artifact of the fact that the former are built on rpm_idle(),
whereas the latter are built on rpm_suspend().

There are precious few pm_runtime_put()/pm_runtime_put_sync() callers
that check the return code at all, but most of them only log errors, and
usually only for negative error codes. None of them should be treating
this as an error, so:

 * at best, this may fix some case where a driver treats this condition
   as an error, when it shouldn't;

 * at worst, this should make no effect; and

 * somewhere in between, we could potentially clear up non-fatal log
   messages.

Fix the pm_runtime_already_suspended_test() while tweaking the behavior.
The test makes a lot more sense when these all return 1 when the device
is already suspended:

    pm_runtime_put(dev);
    pm_runtime_put_sync(dev);
    pm_runtime_suspend(dev);
    pm_runtime_autosuspend(dev);
    pm_request_autosuspend(dev);
    pm_runtime_put_sync_autosuspend(dev);
    pm_runtime_put_autosuspend(dev);

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

 drivers/base/power/runtime-test.c | 8 ++------
 drivers/base/power/runtime.c      | 3 +++
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c
index 263c28d5fc50..1be18e871f1d 100644
--- a/drivers/base/power/runtime-test.c
+++ b/drivers/base/power/runtime-test.c
@@ -43,15 +43,11 @@ static void pm_runtime_already_suspended_test(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */
 
 	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
-	/*
-	 * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
-	 * this as -EAGAIN / "runtime PM status change ongoing".
-	 */
-	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put(dev));
+	KUNIT_EXPECT_EQ(test, 1, pm_runtime_put(dev));
 
 	pm_runtime_get_noresume(dev);
 	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
-	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put_sync(dev));
+	KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_sync(dev));
 
 	KUNIT_EXPECT_EQ(test, 1, pm_runtime_suspend(dev));
 	KUNIT_EXPECT_EQ(test, 1, pm_runtime_autosuspend(dev));
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 3e84dc4122de..17cf111d16aa 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -498,6 +498,9 @@ static int rpm_idle(struct device *dev, int rpmflags)
 	if (retval < 0)
 		;	/* Conditions are wrong. */
 
+	else if ((rpmflags & RPM_GET_PUT) && (retval == 1))
+		;	/* put() is allowed in RPM_SUSPENDED */
+
 	/* Idle notifications are allowed only in the RPM_ACTIVE state. */
 	else if (dev->power.runtime_status != RPM_ACTIVE)
 		retval = -EAGAIN;
-- 
2.51.0.318.gd7df087d1a-goog

Re: [PATCH 2/3] PM: runtime: Make put{,_sync}() return 1 when already suspended

[Sakari Ailus]

Hi Brian,

On Thu, Aug 28, 2025 at 05:28:27PM -0700, Brian Norris wrote:
> The pm_runtime.h docs say pm_runtime_put() and pm_runtime_put_sync()
> return 1 when already suspended, but this is not true -- they return
> -EAGAIN. On the other hand, pm_runtime_put_sync_suspend() and
> pm_runtime_put_sync_autosuspend() *do* return 1.
> 
> This is an artifact of the fact that the former are built on rpm_idle(),
> whereas the latter are built on rpm_suspend().
> 
> There are precious few pm_runtime_put()/pm_runtime_put_sync() callers
> that check the return code at all, but most of them only log errors, and
> usually only for negative error codes. None of them should be treating
> this as an error, so:
> 
>  * at best, this may fix some case where a driver treats this condition
>    as an error, when it shouldn't;
> 
>  * at worst, this should make no effect; and
> 
>  * somewhere in between, we could potentially clear up non-fatal log
>    messages.
> 
> Fix the pm_runtime_already_suspended_test() while tweaking the behavior.
> The test makes a lot more sense when these all return 1 when the device
> is already suspended:
> 
>     pm_runtime_put(dev);
>     pm_runtime_put_sync(dev);
>     pm_runtime_suspend(dev);
>     pm_runtime_autosuspend(dev);
>     pm_request_autosuspend(dev);
>     pm_runtime_put_sync_autosuspend(dev);
>     pm_runtime_put_autosuspend(dev);
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 
>  drivers/base/power/runtime-test.c | 8 ++------
>  drivers/base/power/runtime.c      | 3 +++
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c
> index 263c28d5fc50..1be18e871f1d 100644
> --- a/drivers/base/power/runtime-test.c
> +++ b/drivers/base/power/runtime-test.c
> @@ -43,15 +43,11 @@ static void pm_runtime_already_suspended_test(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */
>  
>  	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> -	/*
> -	 * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
> -	 * this as -EAGAIN / "runtime PM status change ongoing".
> -	 */
> -	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put(dev));
> +	KUNIT_EXPECT_EQ(test, 1, pm_runtime_put(dev));
>  
>  	pm_runtime_get_noresume(dev);
>  	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> -	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put_sync(dev));
> +	KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_sync(dev));
>  
>  	KUNIT_EXPECT_EQ(test, 1, pm_runtime_suspend(dev));
>  	KUNIT_EXPECT_EQ(test, 1, pm_runtime_autosuspend(dev));
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 3e84dc4122de..17cf111d16aa 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -498,6 +498,9 @@ static int rpm_idle(struct device *dev, int rpmflags)
>  	if (retval < 0)
>  		;	/* Conditions are wrong. */
>  
> +	else if ((rpmflags & RPM_GET_PUT) && (retval == 1))
> +		;	/* put() is allowed in RPM_SUSPENDED */

Ah, I missed this while reviewing the 3rd patch. Makes sense. Please ignore
my comments regarding the 3rd patch on whether the return value 1 is
applicable.

The latter parentheses are redundant (the former, too, actually, but the
compiler warns so let them be).

> +
>  	/* Idle notifications are allowed only in the RPM_ACTIVE state. */
>  	else if (dev->power.runtime_status != RPM_ACTIVE)
>  		retval = -EAGAIN;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/3] PM: runtime: Make put{,_sync}() return 1 when already suspended

[Rafael J. Wysocki]

On Tue, Sep 2, 2025 at 9:25 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Brian,
>
> On Thu, Aug 28, 2025 at 05:28:27PM -0700, Brian Norris wrote:
> > The pm_runtime.h docs say pm_runtime_put() and pm_runtime_put_sync()
> > return 1 when already suspended, but this is not true -- they return
> > -EAGAIN. On the other hand, pm_runtime_put_sync_suspend() and
> > pm_runtime_put_sync_autosuspend() *do* return 1.
> >
> > This is an artifact of the fact that the former are built on rpm_idle(),
> > whereas the latter are built on rpm_suspend().
> >
> > There are precious few pm_runtime_put()/pm_runtime_put_sync() callers
> > that check the return code at all, but most of them only log errors, and
> > usually only for negative error codes. None of them should be treating
> > this as an error, so:
> >
> >  * at best, this may fix some case where a driver treats this condition
> >    as an error, when it shouldn't;
> >
> >  * at worst, this should make no effect; and
> >
> >  * somewhere in between, we could potentially clear up non-fatal log
> >    messages.
> >
> > Fix the pm_runtime_already_suspended_test() while tweaking the behavior.
> > The test makes a lot more sense when these all return 1 when the device
> > is already suspended:
> >
> >     pm_runtime_put(dev);
> >     pm_runtime_put_sync(dev);
> >     pm_runtime_suspend(dev);
> >     pm_runtime_autosuspend(dev);
> >     pm_request_autosuspend(dev);
> >     pm_runtime_put_sync_autosuspend(dev);
> >     pm_runtime_put_autosuspend(dev);
> >
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> >
> >  drivers/base/power/runtime-test.c | 8 ++------
> >  drivers/base/power/runtime.c      | 3 +++
> >  2 files changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c
> > index 263c28d5fc50..1be18e871f1d 100644
> > --- a/drivers/base/power/runtime-test.c
> > +++ b/drivers/base/power/runtime-test.c
> > @@ -43,15 +43,11 @@ static void pm_runtime_already_suspended_test(struct kunit *test)
> >       KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */
> >
> >       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > -     /*
> > -      * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
> > -      * this as -EAGAIN / "runtime PM status change ongoing".
> > -      */
> > -     KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put(dev));
> > +     KUNIT_EXPECT_EQ(test, 1, pm_runtime_put(dev));
> >
> >       pm_runtime_get_noresume(dev);
> >       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > -     KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put_sync(dev));
> > +     KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_sync(dev));
> >
> >       KUNIT_EXPECT_EQ(test, 1, pm_runtime_suspend(dev));
> >       KUNIT_EXPECT_EQ(test, 1, pm_runtime_autosuspend(dev));
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 3e84dc4122de..17cf111d16aa 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -498,6 +498,9 @@ static int rpm_idle(struct device *dev, int rpmflags)
> >       if (retval < 0)
> >               ;       /* Conditions are wrong. */
> >
> > +     else if ((rpmflags & RPM_GET_PUT) && (retval == 1))
> > +             ;       /* put() is allowed in RPM_SUSPENDED */
>
> Ah, I missed this while reviewing the 3rd patch. Makes sense. Please ignore
> my comments regarding the 3rd patch on whether the return value 1 is
> applicable.
>
> The latter parentheses are redundant (the former, too, actually, but the
> compiler warns so let them be).

Right.

But overall this should work.

> > +
> >       /* Idle notifications are allowed only in the RPM_ACTIVE state. */
> >       else if (dev->power.runtime_status != RPM_ACTIVE)
> >               retval = -EAGAIN;
>
> --

Re: [PATCH 2/3] PM: runtime: Make put{,_sync}() return 1 when already suspended

[Dhruva Gole]

On Aug 28, 2025 at 17:28:27 -0700, Brian Norris wrote:
> The pm_runtime.h docs say pm_runtime_put() and pm_runtime_put_sync()
> return 1 when already suspended, but this is not true -- they return
> -EAGAIN. On the other hand, pm_runtime_put_sync_suspend() and
> pm_runtime_put_sync_autosuspend() *do* return 1.
> 
> This is an artifact of the fact that the former are built on rpm_idle(),
> whereas the latter are built on rpm_suspend().
> 
> There are precious few pm_runtime_put()/pm_runtime_put_sync() callers
> that check the return code at all, but most of them only log errors, and
> usually only for negative error codes. None of them should be treating
> this as an error, so:
> 
>  * at best, this may fix some case where a driver treats this condition
>    as an error, when it shouldn't;
> 
>  * at worst, this should make no effect; and
> 
>  * somewhere in between, we could potentially clear up non-fatal log
>    messages.

Right, just doing a $> git grep -A5 "= pm_runtime_put"    gave me quite a few
callers who actually do check the return codes, and in some cases even
directly backpropagate them! So like you say with this patch best case even
might fix a few cases where it's unnecessary.


> 
> Fix the pm_runtime_already_suspended_test() while tweaking the behavior.
> The test makes a lot more sense when these all return 1 when the device
> is already suspended:
> 
>     pm_runtime_put(dev);
>     pm_runtime_put_sync(dev);
>     pm_runtime_suspend(dev);
>     pm_runtime_autosuspend(dev);
>     pm_request_autosuspend(dev);
>     pm_runtime_put_sync_autosuspend(dev);
>     pm_runtime_put_autosuspend(dev);
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 
>  drivers/base/power/runtime-test.c | 8 ++------
>  drivers/base/power/runtime.c      | 3 +++
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c
> index 263c28d5fc50..1be18e871f1d 100644
> --- a/drivers/base/power/runtime-test.c
> +++ b/drivers/base/power/runtime-test.c
> @@ -43,15 +43,11 @@ static void pm_runtime_already_suspended_test(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */
>  
>  	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> -	/*
> -	 * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
> -	 * this as -EAGAIN / "runtime PM status change ongoing".
> -	 */
> -	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put(dev));
> +	KUNIT_EXPECT_EQ(test, 1, pm_runtime_put(dev));
>  
>  	pm_runtime_get_noresume(dev);
>  	KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> -	KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put_sync(dev));
> +	KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_sync(dev));
>  
>  	KUNIT_EXPECT_EQ(test, 1, pm_runtime_suspend(dev));
>  	KUNIT_EXPECT_EQ(test, 1, pm_runtime_autosuspend(dev));
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 3e84dc4122de..17cf111d16aa 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -498,6 +498,9 @@ static int rpm_idle(struct device *dev, int rpmflags)
>  	if (retval < 0)
>  		;	/* Conditions are wrong. */
>  
> +	else if ((rpmflags & RPM_GET_PUT) && (retval == 1))
> +		;	/* put() is allowed in RPM_SUSPENDED */
> +

No objections from my side,
Reviewed-by: Dhruva Gole <d-gole@ti.com>

-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated

[PATCH 3/3] PM: runtime: Update kerneldoc return codes

[Brian Norris]

APIs based on __pm_runtime_idle() (pm_runtime_idle(), pm_request_idle())
do not return 1 when already suspended. They return -EAGAIN. This is
already covered in the docs, so the entry for "1" is redundant and
conflicting.

(pm_runtime_put() and pm_runtime_put_sync() were previously incorrect,
but that's fixed in "PM: runtime: pm_runtime_put{,_sync}() returns 1
when already suspended", to ensure consistency with APIs like
pm_runtime_put_autosuspend().)

RPM_GET_PUT APIs based on __pm_runtime_suspend() do return 1 when
already suspended, but the language is a little unclear -- it's not
really an "error", so it seems better to list as a clarification before
the 0/success case. Additionally, they only actually return 1 when the
refcount makes it to 0; if the usage_count is still non-zero, we return
0.

pm_runtime_put(), etc., also don't appear at first like they can ever
see "-EAGAIN: Runtime PM usage_count non-zero", because in non-racy
conditions, pm_runtime_put() would drop its reference count, see it's
non-zero, and return early (in __pm_runtime_idle()). However, it's
possible to race with another actor that increments the usage_count
afterward, since rpm_idle() is protected by a separate lock; in such a
case, we may see -EAGAIN.

Because this case is only seen in the presence of concurrent actors, it
makes sense to clarify that this is when "usage_count **became**
non-zero", by way of some racing actor.

Lastly, pm_runtime_put_sync_suspend() duplicated some -EAGAIN language.
Fix that.

Fixes: 271ff96d6066 ("PM: runtime: Document return values of suspend-related API functions")
Link: https://lore.kernel.org/linux-pm/aJ5pkEJuixTaybV4@google.com/
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 include/linux/pm_runtime.h | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index d88d6b6ccf5b..fd17ffe1bc79 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -356,7 +356,6 @@ static inline int pm_runtime_force_resume(struct device *dev) { return -ENXIO; }
  * * -EPERM: Device PM QoS resume latency 0.
  * * -EINPROGRESS: Suspend already in progress.
  * * -ENOSYS: CONFIG_PM not enabled.
- * * 1: Device already suspended.
  * Other values and conditions for the above values are possible as returned by
  * Runtime PM idle and suspend callbacks.
  */
@@ -439,7 +438,6 @@ static inline int pm_runtime_resume(struct device *dev)
  * * -EPERM: Device PM QoS resume latency 0.
  * * -EINPROGRESS: Suspend already in progress.
  * * -ENOSYS: CONFIG_PM not enabled.
- * * 1: Device already suspended.
  */
 static inline int pm_request_idle(struct device *dev)
 {
@@ -540,15 +538,16 @@ static inline int pm_runtime_resume_and_get(struct device *dev)
  * equal to 0, queue up a work item for @dev like in pm_request_idle().
  *
  * Return:
+ * * 1: Usage counts dropped to zero, but device was already suspended.
  * * 0: Success.
  * * -EINVAL: Runtime PM error.
  * * -EACCES: Runtime PM disabled.
- * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
+ * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
+ *            change ongoing.
  * * -EBUSY: Runtime PM child_count non-zero.
  * * -EPERM: Device PM QoS resume latency 0.
  * * -EINPROGRESS: Suspend already in progress.
  * * -ENOSYS: CONFIG_PM not enabled.
- * * 1: Device already suspended.
  */
 static inline int pm_runtime_put(struct device *dev)
 {
@@ -565,15 +564,16 @@ DEFINE_FREE(pm_runtime_put, struct device *, if (_T) pm_runtime_put(_T))
  * equal to 0, queue up a work item for @dev like in pm_request_autosuspend().
  *
  * Return:
+ * * 1: Usage counts dropped to zero, but device was already suspended.
  * * 0: Success.
  * * -EINVAL: Runtime PM error.
  * * -EACCES: Runtime PM disabled.
- * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
+ * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
+ *            change ongoing.
  * * -EBUSY: Runtime PM child_count non-zero.
  * * -EPERM: Device PM QoS resume latency 0.
  * * -EINPROGRESS: Suspend already in progress.
  * * -ENOSYS: CONFIG_PM not enabled.
- * * 1: Device already suspended.
  */
 static inline int __pm_runtime_put_autosuspend(struct device *dev)
 {
@@ -590,15 +590,16 @@ static inline int __pm_runtime_put_autosuspend(struct device *dev)
  * in pm_request_autosuspend().
  *
  * Return:
+ * * 1: Usage counts dropped to zero, but device was already suspended.
  * * 0: Success.
  * * -EINVAL: Runtime PM error.
  * * -EACCES: Runtime PM disabled.
- * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
+ * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
+ *            change ongoing.
  * * -EBUSY: Runtime PM child_count non-zero.
  * * -EPERM: Device PM QoS resume latency 0.
  * * -EINPROGRESS: Suspend already in progress.
  * * -ENOSYS: CONFIG_PM not enabled.
- * * 1: Device already suspended.
  */
 static inline int pm_runtime_put_autosuspend(struct device *dev)
 {
@@ -619,14 +620,15 @@ static inline int pm_runtime_put_autosuspend(struct device *dev)
  * if it returns an error code.
  *
  * Return:
+ * * 1: Usage counts dropped to zero, but device was already suspended.
  * * 0: Success.
  * * -EINVAL: Runtime PM error.
  * * -EACCES: Runtime PM disabled.
- * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
+ * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
+ *            change ongoing.
  * * -EBUSY: Runtime PM child_count non-zero.
  * * -EPERM: Device PM QoS resume latency 0.
  * * -ENOSYS: CONFIG_PM not enabled.
- * * 1: Device already suspended.
  * Other values and conditions for the above values are possible as returned by
  * Runtime PM suspend callbacks.
  */
@@ -646,15 +648,15 @@ static inline int pm_runtime_put_sync(struct device *dev)
  * if it returns an error code.
  *
  * Return:
+ * * 1: Usage counts dropped to zero, but device was already suspended.
  * * 0: Success.
  * * -EINVAL: Runtime PM error.
  * * -EACCES: Runtime PM disabled.
- * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
- * * -EAGAIN: usage_count non-zero or Runtime PM status change ongoing.
+ * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
+ *            change ongoing.
  * * -EBUSY: Runtime PM child_count non-zero.
  * * -EPERM: Device PM QoS resume latency 0.
  * * -ENOSYS: CONFIG_PM not enabled.
- * * 1: Device already suspended.
  * Other values and conditions for the above values are possible as returned by
  * Runtime PM suspend callbacks.
  */
@@ -677,15 +679,16 @@ static inline int pm_runtime_put_sync_suspend(struct device *dev)
  * if it returns an error code.
  *
  * Return:
+ * * 1: Usage counts dropped to zero, but device was already suspended.
  * * 0: Success.
  * * -EINVAL: Runtime PM error.
  * * -EACCES: Runtime PM disabled.
- * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
+ * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
+ *            change ongoing.
  * * -EBUSY: Runtime PM child_count non-zero.
  * * -EPERM: Device PM QoS resume latency 0.
  * * -EINPROGRESS: Suspend already in progress.
  * * -ENOSYS: CONFIG_PM not enabled.
- * * 1: Device already suspended.
  * Other values and conditions for the above values are possible as returned by
  * Runtime PM suspend callbacks.
  */
-- 
2.51.0.318.gd7df087d1a-goog

Re: [PATCH 3/3] PM: runtime: Update kerneldoc return codes

[Sakari Ailus]

Hi Brian,

Thanks for posting this. A few comments below.

On Thu, Aug 28, 2025 at 05:28:28PM -0700, Brian Norris wrote:
> APIs based on __pm_runtime_idle() (pm_runtime_idle(), pm_request_idle())
> do not return 1 when already suspended. They return -EAGAIN. This is
> already covered in the docs, so the entry for "1" is redundant and
> conflicting.
> 
> (pm_runtime_put() and pm_runtime_put_sync() were previously incorrect,
> but that's fixed in "PM: runtime: pm_runtime_put{,_sync}() returns 1
> when already suspended", to ensure consistency with APIs like
> pm_runtime_put_autosuspend().)
> 
> RPM_GET_PUT APIs based on __pm_runtime_suspend() do return 1 when
> already suspended, but the language is a little unclear -- it's not
> really an "error", so it seems better to list as a clarification before
> the 0/success case. Additionally, they only actually return 1 when the
> refcount makes it to 0; if the usage_count is still non-zero, we return
> 0.
> 
> pm_runtime_put(), etc., also don't appear at first like they can ever
> see "-EAGAIN: Runtime PM usage_count non-zero", because in non-racy
> conditions, pm_runtime_put() would drop its reference count, see it's
> non-zero, and return early (in __pm_runtime_idle()). However, it's
> possible to race with another actor that increments the usage_count
> afterward, since rpm_idle() is protected by a separate lock; in such a
> case, we may see -EAGAIN.
> 
> Because this case is only seen in the presence of concurrent actors, it
> makes sense to clarify that this is when "usage_count **became**
> non-zero", by way of some racing actor.
> 
> Lastly, pm_runtime_put_sync_suspend() duplicated some -EAGAIN language.
> Fix that.
> 
> Fixes: 271ff96d6066 ("PM: runtime: Document return values of suspend-related API functions")
> Link: https://lore.kernel.org/linux-pm/aJ5pkEJuixTaybV4@google.com/
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 
>  include/linux/pm_runtime.h | 33 ++++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index d88d6b6ccf5b..fd17ffe1bc79 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -356,7 +356,6 @@ static inline int pm_runtime_force_resume(struct device *dev) { return -ENXIO; }
>   * * -EPERM: Device PM QoS resume latency 0.
>   * * -EINPROGRESS: Suspend already in progress.
>   * * -ENOSYS: CONFIG_PM not enabled.
> - * * 1: Device already suspended.
>   * Other values and conditions for the above values are possible as returned by
>   * Runtime PM idle and suspend callbacks.
>   */
> @@ -439,7 +438,6 @@ static inline int pm_runtime_resume(struct device *dev)
>   * * -EPERM: Device PM QoS resume latency 0.
>   * * -EINPROGRESS: Suspend already in progress.
>   * * -ENOSYS: CONFIG_PM not enabled.
> - * * 1: Device already suspended.
>   */
>  static inline int pm_request_idle(struct device *dev)
>  {
> @@ -540,15 +538,16 @@ static inline int pm_runtime_resume_and_get(struct device *dev)
>   * equal to 0, queue up a work item for @dev like in pm_request_idle().
>   *
>   * Return:
> + * * 1: Usage counts dropped to zero, but device was already suspended.

Does this actually happen? pm_runtime_put() calls __pm_runtime_idle() that
doesn't appear to return 1 in any case.

>   * * 0: Success.
>   * * -EINVAL: Runtime PM error.
>   * * -EACCES: Runtime PM disabled.
> - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> + *            change ongoing.
>   * * -EBUSY: Runtime PM child_count non-zero.
>   * * -EPERM: Device PM QoS resume latency 0.
>   * * -EINPROGRESS: Suspend already in progress.
>   * * -ENOSYS: CONFIG_PM not enabled.
> - * * 1: Device already suspended.
>   */
>  static inline int pm_runtime_put(struct device *dev)
>  {
> @@ -565,15 +564,16 @@ DEFINE_FREE(pm_runtime_put, struct device *, if (_T) pm_runtime_put(_T))
>   * equal to 0, queue up a work item for @dev like in pm_request_autosuspend().
>   *
>   * Return:
> + * * 1: Usage counts dropped to zero, but device was already suspended.
>   * * 0: Success.

"usage_count" and "usage counter" is being used in kernel-doc already, I'd
use either of the two. "usage_count" refers directly to the field in struct
dev_pm_info (and is used a few lines below). Same elsewhere.

>   * * -EINVAL: Runtime PM error.
>   * * -EACCES: Runtime PM disabled.
> - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> + *            change ongoing.
>   * * -EBUSY: Runtime PM child_count non-zero.
>   * * -EPERM: Device PM QoS resume latency 0.
>   * * -EINPROGRESS: Suspend already in progress.
>   * * -ENOSYS: CONFIG_PM not enabled.
> - * * 1: Device already suspended.
>   */
>  static inline int __pm_runtime_put_autosuspend(struct device *dev)
>  {
> @@ -590,15 +590,16 @@ static inline int __pm_runtime_put_autosuspend(struct device *dev)
>   * in pm_request_autosuspend().
>   *
>   * Return:
> + * * 1: Usage counts dropped to zero, but device was already suspended.
>   * * 0: Success.
>   * * -EINVAL: Runtime PM error.
>   * * -EACCES: Runtime PM disabled.
> - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> + *            change ongoing.
>   * * -EBUSY: Runtime PM child_count non-zero.
>   * * -EPERM: Device PM QoS resume latency 0.
>   * * -EINPROGRESS: Suspend already in progress.
>   * * -ENOSYS: CONFIG_PM not enabled.
> - * * 1: Device already suspended.
>   */
>  static inline int pm_runtime_put_autosuspend(struct device *dev)
>  {
> @@ -619,14 +620,15 @@ static inline int pm_runtime_put_autosuspend(struct device *dev)
>   * if it returns an error code.
>   *
>   * Return:
> + * * 1: Usage counts dropped to zero, but device was already suspended.
>   * * 0: Success.

Does this happen (pm_runtime_put_sync() calls __pm_runtime_idle())?

>   * * -EINVAL: Runtime PM error.
>   * * -EACCES: Runtime PM disabled.
> - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> + *            change ongoing.
>   * * -EBUSY: Runtime PM child_count non-zero.
>   * * -EPERM: Device PM QoS resume latency 0.
>   * * -ENOSYS: CONFIG_PM not enabled.
> - * * 1: Device already suspended.
>   * Other values and conditions for the above values are possible as returned by
>   * Runtime PM suspend callbacks.
>   */
> @@ -646,15 +648,15 @@ static inline int pm_runtime_put_sync(struct device *dev)
>   * if it returns an error code.
>   *
>   * Return:
> + * * 1: Usage counts dropped to zero, but device was already suspended.
>   * * 0: Success.
>   * * -EINVAL: Runtime PM error.
>   * * -EACCES: Runtime PM disabled.
> - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> - * * -EAGAIN: usage_count non-zero or Runtime PM status change ongoing.
> + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> + *            change ongoing.
>   * * -EBUSY: Runtime PM child_count non-zero.
>   * * -EPERM: Device PM QoS resume latency 0.
>   * * -ENOSYS: CONFIG_PM not enabled.
> - * * 1: Device already suspended.
>   * Other values and conditions for the above values are possible as returned by
>   * Runtime PM suspend callbacks.
>   */
> @@ -677,15 +679,16 @@ static inline int pm_runtime_put_sync_suspend(struct device *dev)
>   * if it returns an error code.
>   *
>   * Return:
> + * * 1: Usage counts dropped to zero, but device was already suspended.
>   * * 0: Success.
>   * * -EINVAL: Runtime PM error.
>   * * -EACCES: Runtime PM disabled.
> - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> + *            change ongoing.
>   * * -EBUSY: Runtime PM child_count non-zero.
>   * * -EPERM: Device PM QoS resume latency 0.
>   * * -EINPROGRESS: Suspend already in progress.
>   * * -ENOSYS: CONFIG_PM not enabled.
> - * * 1: Device already suspended.
>   * Other values and conditions for the above values are possible as returned by
>   * Runtime PM suspend callbacks.
>   */

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 3/3] PM: runtime: Update kerneldoc return codes

[Rafael J. Wysocki]

On Tue, Sep 2, 2025 at 9:18 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Brian,
>
> Thanks for posting this. A few comments below.
>
> On Thu, Aug 28, 2025 at 05:28:28PM -0700, Brian Norris wrote:
> > APIs based on __pm_runtime_idle() (pm_runtime_idle(), pm_request_idle())
> > do not return 1 when already suspended. They return -EAGAIN. This is
> > already covered in the docs, so the entry for "1" is redundant and
> > conflicting.
> >
> > (pm_runtime_put() and pm_runtime_put_sync() were previously incorrect,
> > but that's fixed in "PM: runtime: pm_runtime_put{,_sync}() returns 1
> > when already suspended", to ensure consistency with APIs like
> > pm_runtime_put_autosuspend().)
> >
> > RPM_GET_PUT APIs based on __pm_runtime_suspend() do return 1 when
> > already suspended, but the language is a little unclear -- it's not
> > really an "error", so it seems better to list as a clarification before
> > the 0/success case. Additionally, they only actually return 1 when the
> > refcount makes it to 0; if the usage_count is still non-zero, we return
> > 0.
> >
> > pm_runtime_put(), etc., also don't appear at first like they can ever
> > see "-EAGAIN: Runtime PM usage_count non-zero", because in non-racy
> > conditions, pm_runtime_put() would drop its reference count, see it's
> > non-zero, and return early (in __pm_runtime_idle()). However, it's
> > possible to race with another actor that increments the usage_count
> > afterward, since rpm_idle() is protected by a separate lock; in such a
> > case, we may see -EAGAIN.
> >
> > Because this case is only seen in the presence of concurrent actors, it
> > makes sense to clarify that this is when "usage_count **became**
> > non-zero", by way of some racing actor.
> >
> > Lastly, pm_runtime_put_sync_suspend() duplicated some -EAGAIN language.
> > Fix that.
> >
> > Fixes: 271ff96d6066 ("PM: runtime: Document return values of suspend-related API functions")
> > Link: https://lore.kernel.org/linux-pm/aJ5pkEJuixTaybV4@google.com/
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> >
> >  include/linux/pm_runtime.h | 33 ++++++++++++++++++---------------
> >  1 file changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> > index d88d6b6ccf5b..fd17ffe1bc79 100644
> > --- a/include/linux/pm_runtime.h
> > +++ b/include/linux/pm_runtime.h
> > @@ -356,7 +356,6 @@ static inline int pm_runtime_force_resume(struct device *dev) { return -ENXIO; }
> >   * * -EPERM: Device PM QoS resume latency 0.
> >   * * -EINPROGRESS: Suspend already in progress.
> >   * * -ENOSYS: CONFIG_PM not enabled.
> > - * * 1: Device already suspended.
> >   * Other values and conditions for the above values are possible as returned by
> >   * Runtime PM idle and suspend callbacks.
> >   */
> > @@ -439,7 +438,6 @@ static inline int pm_runtime_resume(struct device *dev)
> >   * * -EPERM: Device PM QoS resume latency 0.
> >   * * -EINPROGRESS: Suspend already in progress.
> >   * * -ENOSYS: CONFIG_PM not enabled.
> > - * * 1: Device already suspended.
> >   */
> >  static inline int pm_request_idle(struct device *dev)
> >  {
> > @@ -540,15 +538,16 @@ static inline int pm_runtime_resume_and_get(struct device *dev)
> >   * equal to 0, queue up a work item for @dev like in pm_request_idle().
> >   *
> >   * Return:
> > + * * 1: Usage counts dropped to zero, but device was already suspended.
>
> Does this actually happen? pm_runtime_put() calls __pm_runtime_idle() that
> doesn't appear to return 1 in any case.

Except when it calls rpm_suspend() that may return 1.

> >   * * 0: Success.
> >   * * -EINVAL: Runtime PM error.
> >   * * -EACCES: Runtime PM disabled.
> > - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> > + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> > + *            change ongoing.
> >   * * -EBUSY: Runtime PM child_count non-zero.
> >   * * -EPERM: Device PM QoS resume latency 0.
> >   * * -EINPROGRESS: Suspend already in progress.
> >   * * -ENOSYS: CONFIG_PM not enabled.
> > - * * 1: Device already suspended.
> >   */
> >  static inline int pm_runtime_put(struct device *dev)
> >  {
> > @@ -565,15 +564,16 @@ DEFINE_FREE(pm_runtime_put, struct device *, if (_T) pm_runtime_put(_T))
> >   * equal to 0, queue up a work item for @dev like in pm_request_autosuspend().
> >   *
> >   * Return:
> > + * * 1: Usage counts dropped to zero, but device was already suspended.
> >   * * 0: Success.
>
> "usage_count" and "usage counter" is being used in kernel-doc already, I'd
> use either of the two. "usage_count" refers directly to the field in struct
> dev_pm_info (and is used a few lines below). Same elsewhere.

"Usage counter", please!

> >   * * -EINVAL: Runtime PM error.
> >   * * -EACCES: Runtime PM disabled.
> > - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> > + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> > + *            change ongoing.
> >   * * -EBUSY: Runtime PM child_count non-zero.
> >   * * -EPERM: Device PM QoS resume latency 0.
> >   * * -EINPROGRESS: Suspend already in progress.
> >   * * -ENOSYS: CONFIG_PM not enabled.
> > - * * 1: Device already suspended.
> >   */
> >  static inline int __pm_runtime_put_autosuspend(struct device *dev)
> >  {
> > @@ -590,15 +590,16 @@ static inline int __pm_runtime_put_autosuspend(struct device *dev)
> >   * in pm_request_autosuspend().
> >   *
> >   * Return:
> > + * * 1: Usage counts dropped to zero, but device was already suspended.
> >   * * 0: Success.
> >   * * -EINVAL: Runtime PM error.
> >   * * -EACCES: Runtime PM disabled.
> > - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> > + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> > + *            change ongoing.
> >   * * -EBUSY: Runtime PM child_count non-zero.
> >   * * -EPERM: Device PM QoS resume latency 0.
> >   * * -EINPROGRESS: Suspend already in progress.
> >   * * -ENOSYS: CONFIG_PM not enabled.
> > - * * 1: Device already suspended.
> >   */
> >  static inline int pm_runtime_put_autosuspend(struct device *dev)
> >  {
> > @@ -619,14 +620,15 @@ static inline int pm_runtime_put_autosuspend(struct device *dev)
> >   * if it returns an error code.
> >   *
> >   * Return:
> > + * * 1: Usage counts dropped to zero, but device was already suspended.
> >   * * 0: Success.
>
> Does this happen (pm_runtime_put_sync() calls __pm_runtime_idle())?

It does (see above).

> >   * * -EINVAL: Runtime PM error.
> >   * * -EACCES: Runtime PM disabled.
> > - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> > + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> > + *            change ongoing.
> >   * * -EBUSY: Runtime PM child_count non-zero.
> >   * * -EPERM: Device PM QoS resume latency 0.
> >   * * -ENOSYS: CONFIG_PM not enabled.
> > - * * 1: Device already suspended.
> >   * Other values and conditions for the above values are possible as returned by
> >   * Runtime PM suspend callbacks.
> >   */
> > @@ -646,15 +648,15 @@ static inline int pm_runtime_put_sync(struct device *dev)
> >   * if it returns an error code.
> >   *
> >   * Return:
> > + * * 1: Usage counts dropped to zero, but device was already suspended.
> >   * * 0: Success.
> >   * * -EINVAL: Runtime PM error.
> >   * * -EACCES: Runtime PM disabled.
> > - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> > - * * -EAGAIN: usage_count non-zero or Runtime PM status change ongoing.
> > + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> > + *            change ongoing.
> >   * * -EBUSY: Runtime PM child_count non-zero.
> >   * * -EPERM: Device PM QoS resume latency 0.
> >   * * -ENOSYS: CONFIG_PM not enabled.
> > - * * 1: Device already suspended.
> >   * Other values and conditions for the above values are possible as returned by
> >   * Runtime PM suspend callbacks.
> >   */
> > @@ -677,15 +679,16 @@ static inline int pm_runtime_put_sync_suspend(struct device *dev)
> >   * if it returns an error code.
> >   *
> >   * Return:
> > + * * 1: Usage counts dropped to zero, but device was already suspended.
> >   * * 0: Success.
> >   * * -EINVAL: Runtime PM error.
> >   * * -EACCES: Runtime PM disabled.
> > - * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> > + * * -EAGAIN: Runtime PM usage_count became non-zero or Runtime PM status
> > + *            change ongoing.
> >   * * -EBUSY: Runtime PM child_count non-zero.
> >   * * -EPERM: Device PM QoS resume latency 0.
> >   * * -EINPROGRESS: Suspend already in progress.
> >   * * -ENOSYS: CONFIG_PM not enabled.
> > - * * 1: Device already suspended.
> >   * Other values and conditions for the above values are possible as returned by
> >   * Runtime PM suspend callbacks.
> >   */
>
> --

Re: [PATCH 1/3] PM: runtime: Add basic kunit tests for API contracts

[Rafael J. Wysocki]

On Fri, Aug 29, 2025 at 2:33 AM Brian Norris <briannorris@chromium.org> wrote:
>
> In exploring the various return codes and failure modes of runtime PM
> APIs, I found it helpful to verify and codify many of them in unit
> tests, especially given that even the kerneldoc can be rather complex to
> reason through, and it also has had subtle errors of its own.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

This is nice in general, but I have a couple of questions/comments (see below).

> ---
>
>  drivers/base/Kconfig              |   6 +
>  drivers/base/power/Makefile       |   1 +
>  drivers/base/power/runtime-test.c | 259 ++++++++++++++++++++++++++++++
>  3 files changed, 266 insertions(+)
>  create mode 100644 drivers/base/power/runtime-test.c
>
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 064eb52ff7e2..1786d87b29e2 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -167,6 +167,12 @@ config PM_QOS_KUNIT_TEST
>         depends on KUNIT=y
>         default KUNIT_ALL_TESTS
>
> +config PM_RUNTIME_KUNIT_TEST
> +       tristate "KUnit Tests for runtime PM" if !KUNIT_ALL_TESTS
> +       depends on KUNIT
> +       depends on PM
> +       default KUNIT_ALL_TESTS
> +
>  config HMEM_REPORTING
>         bool
>         default n
> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index 01f11629d241..2989e42d0161 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -4,5 +4,6 @@ obj-$(CONFIG_PM_SLEEP)  += main.o wakeup.o wakeup_stats.o
>  obj-$(CONFIG_PM_TRACE_RTC)     += trace.o
>  obj-$(CONFIG_HAVE_CLK) += clock_ops.o
>  obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o
> +obj-$(CONFIG_PM_RUNTIME_KUNIT_TEST) += runtime-test.o
>
>  ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c
> new file mode 100644
> index 000000000000..263c28d5fc50
> --- /dev/null
> +++ b/drivers/base/power/runtime-test.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2025 Google, Inc.
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/pm_runtime.h>
> +#include <kunit/device.h>
> +#include <kunit/test.h>
> +
> +#define DEVICE_NAME "pm_runtime_test_device"
> +
> +static void pm_runtime_depth_test(struct kunit *test)
> +{
> +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> +
> +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> +
> +       pm_runtime_enable(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));
> +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_get_sync(dev)); /* "already active" */
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> +}
> +
> +/* Test pm_runtime_put() and friends when already suspended. */
> +static void pm_runtime_already_suspended_test(struct kunit *test)
> +{
> +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> +
> +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> +
> +       pm_runtime_enable(dev);
> +
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> +       pm_runtime_get_noresume(dev);
> +
> +       /* Flush, in case the above (non-sync) triggered any work. */
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */

Why do you run pm_runtime_barrier(dev) here?  It is guaranteed that no
requests are pending at this point.

> +
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));

This has already been tested above.

> +       /*
> +        * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
> +        * this as -EAGAIN / "runtime PM status change ongoing".

No, this means "Conditions are not suitable, but may change".

> +        */
> +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put(dev));
> +
> +       pm_runtime_get_noresume(dev);
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));

This has been tested already twice and why would it change?

> +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put_sync(dev));
> +
> +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_suspend(dev));
> +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_autosuspend(dev));
> +       KUNIT_EXPECT_EQ(test, 1, pm_request_autosuspend(dev));
> +
> +       pm_runtime_get_noresume(dev);
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));

There's no way by which it could change above.

> +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_sync_autosuspend(dev));
> +
> +       pm_runtime_get_noresume(dev);
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_autosuspend(dev));
> +
> +       /* Grab 2 refcounts */
> +       pm_runtime_get_noresume(dev);
> +       pm_runtime_get_noresume(dev);
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> +       /* The first put() sees usage_count 1 */
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_autosuspend(dev));
> +       /* The second put() sees usage_count 0 but tells us "already suspended". */
> +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_autosuspend(dev));
> +
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));

Again, there is no way this can change in the whole test.

> +}
> +
> +static void pm_runtime_idle_test(struct kunit *test)
> +{
> +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> +
> +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> +
> +       pm_runtime_enable(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));
> +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> +       pm_runtime_put_noidle(dev);
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_idle(dev));
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
> +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_request_idle(dev));
> +}
> +
> +static void pm_runtime_disabled_test(struct kunit *test)
> +{
> +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> +
> +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> +
> +       /* Never called pm_runtime_enable() */
> +       KUNIT_EXPECT_FALSE(test, pm_runtime_enabled(dev));
> +
> +       /* "disabled" is treated as "active" */
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> +       KUNIT_EXPECT_FALSE(test, pm_runtime_suspended(dev));
> +
> +       /*
> +        * Note: these "fail", but they still acquire/release refcounts, so
> +        * keep them balanced.
> +        */
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get(dev));
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put(dev));
> +
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get_sync(dev));
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put_sync(dev));
> +
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get(dev));
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put_autosuspend(dev));
> +
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_resume_and_get(dev));
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_idle(dev));
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_request_idle(dev));
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_request_resume(dev));
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_request_autosuspend(dev));
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_suspend(dev));
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_resume(dev));
> +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_autosuspend(dev));
> +
> +       /* Still active */

Still disabled rather.

> +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> +}
> +
> +static void pm_runtime_error_test(struct kunit *test)
> +{
> +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> +
> +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> +
> +       pm_runtime_enable(dev);
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> +
> +       /* Fake a .runtime_resume() error */
> +       dev->power.runtime_error = -EIO;
> +
> +       /*
> +        * Note: these "fail", but they still acquire/release refcounts, so
> +        * keep them balanced.
> +        */
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get(dev));
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put(dev));
> +
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get_sync(dev));
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put_sync(dev));
> +
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get(dev));
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put_autosuspend(dev));
> +
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_resume_and_get(dev));
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_idle(dev));
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_idle(dev));
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_resume(dev));
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_autosuspend(dev));
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_suspend(dev));
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_resume(dev));
> +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_autosuspend(dev));
> +
> +       /* Still suspended */

Error is still pending.

> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> +       /* Clear error */
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_set_suspended(dev));
> +       KUNIT_EXPECT_EQ(test, 0, dev->power.runtime_error);
> +       /* Still suspended */
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> +
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_get(dev));
> +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_barrier(dev)); /* resume was pending */
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put(dev));
> +       pm_runtime_suspend(dev); /* flush the put(), to suspend */
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> +
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_get_sync(dev));
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
> +
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_get_sync(dev));
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_autosuspend(dev));
> +
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_resume_and_get(dev));
> +
> +       /*
> +        * The following should all "fail" with -EAGAIN (usage is non-zero) or
> +        * 1 (already resumed).

The return value of 1 doesn't count as a failure.

> +        */
> +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
> +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_request_idle(dev));
> +       KUNIT_EXPECT_EQ(test, 1, pm_request_resume(dev));
> +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_request_autosuspend(dev));
> +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_suspend(dev));
> +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_resume(dev));
> +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_autosuspend(dev));
> +
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
> +
> +       /* Suspended again */
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> +}
> +
> +/*
> + * Explore a typical probe() sequence in which a device marks itself powered,
> + * but doesn't hold any runtime PM reference, so it suspends as soon as it goes
> + * idle.
> + */
> +static void pm_runtime_probe_active_test(struct kunit *test)
> +{
> +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> +
> +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> +
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_status_suspended(dev));
> +
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_set_active(dev));
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> +
> +       pm_runtime_enable(dev);
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> +
> +       /* Flush, and ensure we stayed active. */

There's nothing to flush though.

> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev));
> +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> +
> +       /* Ask for idle? Now we suspend. */
> +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_idle(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),
> +       KUNIT_CASE(pm_runtime_idle_test),
> +       KUNIT_CASE(pm_runtime_disabled_test),
> +       KUNIT_CASE(pm_runtime_error_test),
> +       KUNIT_CASE(pm_runtime_probe_active_test),
> +       {}
> +};
> +
> +static struct kunit_suite pm_runtime_test_suite = {
> +       .name = "pm_runtime_test_cases",
> +       .test_cases = pm_runtime_test_cases,
> +};
> +
> +kunit_test_suite(pm_runtime_test_suite);
> +MODULE_DESCRIPTION("Runtime power management unit test suite");
> +MODULE_LICENSE("GPL");
> --

Re: [PATCH 1/3] PM: runtime: Add basic kunit tests for API contracts

[Brian Norris]

Hi Rafael,

On Fri, Sep 05, 2025 at 07:37:38PM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 29, 2025 at 2:33 AM Brian Norris <briannorris@chromium.org> wrote:
> >
> > In exploring the various return codes and failure modes of runtime PM
> > APIs, I found it helpful to verify and codify many of them in unit
> > tests, especially given that even the kerneldoc can be rather complex to
> > reason through, and it also has had subtle errors of its own.
> >
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> 
> This is nice in general, but I have a couple of questions/comments (see below).

Thanks for looking. There's certainly some matter of opinion on how
exactly to test things, and I'm still getting up to speed on some of the
runtime PM API details, so I appreciate the care you've given.

Replies inline.

> > ---
> >
> >  drivers/base/Kconfig              |   6 +
> >  drivers/base/power/Makefile       |   1 +
> >  drivers/base/power/runtime-test.c | 259 ++++++++++++++++++++++++++++++
> >  3 files changed, 266 insertions(+)
> >  create mode 100644 drivers/base/power/runtime-test.c
> >
> > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> > index 064eb52ff7e2..1786d87b29e2 100644
> > --- a/drivers/base/Kconfig
> > +++ b/drivers/base/Kconfig
> > @@ -167,6 +167,12 @@ config PM_QOS_KUNIT_TEST
> >         depends on KUNIT=y
> >         default KUNIT_ALL_TESTS
> >
> > +config PM_RUNTIME_KUNIT_TEST
> > +       tristate "KUnit Tests for runtime PM" if !KUNIT_ALL_TESTS
> > +       depends on KUNIT
> > +       depends on PM
> > +       default KUNIT_ALL_TESTS
> > +
> >  config HMEM_REPORTING
> >         bool
> >         default n
> > diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> > index 01f11629d241..2989e42d0161 100644
> > --- a/drivers/base/power/Makefile
> > +++ b/drivers/base/power/Makefile
> > @@ -4,5 +4,6 @@ obj-$(CONFIG_PM_SLEEP)  += main.o wakeup.o wakeup_stats.o
> >  obj-$(CONFIG_PM_TRACE_RTC)     += trace.o
> >  obj-$(CONFIG_HAVE_CLK) += clock_ops.o
> >  obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o
> > +obj-$(CONFIG_PM_RUNTIME_KUNIT_TEST) += runtime-test.o
> >
> >  ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> > diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c
> > new file mode 100644
> > index 000000000000..263c28d5fc50
> > --- /dev/null
> > +++ b/drivers/base/power/runtime-test.c
> > @@ -0,0 +1,259 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2025 Google, Inc.
> > + */
> > +
> > +#include <linux/cleanup.h>
> > +#include <linux/pm_runtime.h>
> > +#include <kunit/device.h>
> > +#include <kunit/test.h>
> > +
> > +#define DEVICE_NAME "pm_runtime_test_device"
> > +
> > +static void pm_runtime_depth_test(struct kunit *test)
> > +{
> > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > +
> > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > +
> > +       pm_runtime_enable(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));
> > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_get_sync(dev)); /* "already active" */
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > +}
> > +
> > +/* Test pm_runtime_put() and friends when already suspended. */
> > +static void pm_runtime_already_suspended_test(struct kunit *test)
> > +{
> > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > +
> > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > +
> > +       pm_runtime_enable(dev);
> > +
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > +       pm_runtime_get_noresume(dev);
> > +
> > +       /* Flush, in case the above (non-sync) triggered any work. */
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */
> 
> Why do you run pm_runtime_barrier(dev) here?  It is guaranteed that no
> requests are pending at this point.

I suppose my thought is as somewhat of an outsider, that's not really
familiar with exactly how each API is supposed to work. So without
looking into the details of the implementation, it's not clear to me
that a "get_noresume()" will never queue any work. Admittedly, that's a
pretty weak reason.

OTOH, it does serve to test the 0 side of the API contract:

"""
 * 1, if there was a resume request pending and the device had to be woken up,
 * 0, otherwise
"""

So IMO, it's a reasonable thing to run in this test, although I probably
should drop the "Flush" comment.

> > +
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> 
> This has already been tested above.

I'm not really an expert on unit testing and style, but the whole point
of this test (named "already_suspended") is that we're testing each
operation when the device is suspended. So it's many series of:

1. set up some precondition
2. assert that the device is (still) suspended
3. test that an API returns the expected value for "already suspended"

Even if #1/#3 aren't likely to affect #2 for a later sequence, it seems
like a good pattern to actually test that this continues to remain true
each time. If the test changes in the future such that we perform
something different in #1, we might find ourselves not testing "already
suspended" behavior in #3.

Alternatively, I could split each #1/#2/#3 sequence into its own
subtest, but that might get a little excessive.

Anyway, like I said, it's probably some matter of opinion/style. I can
drop some of these checks if you still think they have no place here.

> > +       /*
> > +        * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
> > +        * this as -EAGAIN / "runtime PM status change ongoing".
> 
> No, this means "Conditions are not suitable, but may change".

I'm just quoting the API docs for put():

"""
* * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
"""

If that's the wrong language, then we should update the API doc. At any
rate, I'm not sure what's "unsuitable" about a suspended device when we
call put(). It's not unsuitable -- it's already in the target state!

Notably, I'm also changing this behavior in patch 2, since I think it's
an API bug. And the comment then goes away.

> > +        */
> > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put(dev));
> > +
> > +       pm_runtime_get_noresume(dev);
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> 
> This has been tested already twice and why would it change?

Addressed above. I can drop it if you think it's excessive.

> > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put_sync(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_suspend(dev));
> > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_autosuspend(dev));
> > +       KUNIT_EXPECT_EQ(test, 1, pm_request_autosuspend(dev));
> > +
> > +       pm_runtime_get_noresume(dev);
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> 
> There's no way by which it could change above.

Same.

> > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_sync_autosuspend(dev));
> > +
> > +       pm_runtime_get_noresume(dev);
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_autosuspend(dev));
> > +
> > +       /* Grab 2 refcounts */
> > +       pm_runtime_get_noresume(dev);
> > +       pm_runtime_get_noresume(dev);
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > +       /* The first put() sees usage_count 1 */
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_autosuspend(dev));
> > +       /* The second put() sees usage_count 0 but tells us "already suspended". */
> > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_autosuspend(dev));
> > +
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> 
> Again, there is no way this can change in the whole test.

Same.

> > +}
> > +
> > +static void pm_runtime_idle_test(struct kunit *test)
> > +{
> > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > +
> > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > +
> > +       pm_runtime_enable(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));
> > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > +       pm_runtime_put_noidle(dev);
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_idle(dev));
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
> > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_request_idle(dev));
> > +}
> > +
> > +static void pm_runtime_disabled_test(struct kunit *test)
> > +{
> > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > +
> > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > +
> > +       /* Never called pm_runtime_enable() */
> > +       KUNIT_EXPECT_FALSE(test, pm_runtime_enabled(dev));
> > +
> > +       /* "disabled" is treated as "active" */
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > +       KUNIT_EXPECT_FALSE(test, pm_runtime_suspended(dev));
> > +
> > +       /*
> > +        * Note: these "fail", but they still acquire/release refcounts, so
> > +        * keep them balanced.
> > +        */
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get(dev));
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get_sync(dev));
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put_sync(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get(dev));
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put_autosuspend(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_resume_and_get(dev));
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_idle(dev));
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_request_idle(dev));
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_request_resume(dev));
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_request_autosuspend(dev));
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_suspend(dev));
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_resume(dev));
> > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_autosuspend(dev));
> > +
> > +       /* Still active */
> 
> Still disabled rather.

Ack, will change.

> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > +}
> > +
> > +static void pm_runtime_error_test(struct kunit *test)
> > +{
> > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > +
> > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > +
> > +       pm_runtime_enable(dev);
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > +
> > +       /* Fake a .runtime_resume() error */
> > +       dev->power.runtime_error = -EIO;
> > +
> > +       /*
> > +        * Note: these "fail", but they still acquire/release refcounts, so
> > +        * keep them balanced.
> > +        */
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get(dev));
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get_sync(dev));
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put_sync(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get(dev));
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put_autosuspend(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_resume_and_get(dev));
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_idle(dev));
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_idle(dev));
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_resume(dev));
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_autosuspend(dev));
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_suspend(dev));
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_resume(dev));
> > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_autosuspend(dev));
> > +
> > +       /* Still suspended */
> 
> Error is still pending.

Your statement is true, but I'm not quite sure what you're suggesting.
Are you suggesting I should

	KUNIT_EXPECT_EQ(test, -EIO, dev->power.runtime_error);

?

Or are you suggesting I change the comment?

I'm thinking I'll do both.

> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > +       /* Clear error */
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_set_suspended(dev));
> > +       KUNIT_EXPECT_EQ(test, 0, dev->power.runtime_error);
> > +       /* Still suspended */
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_get(dev));
> > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_barrier(dev)); /* resume was pending */
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put(dev));
> > +       pm_runtime_suspend(dev); /* flush the put(), to suspend */
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_get_sync(dev));
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_get_sync(dev));
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_autosuspend(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_resume_and_get(dev));
> > +
> > +       /*
> > +        * The following should all "fail" with -EAGAIN (usage is non-zero) or
> > +        * 1 (already resumed).
> 
> The return value of 1 doesn't count as a failure.

Hehe, sure. I suppose that's also a matter of unclear docs, because for
some of these, the kerneldoc specifically says 0 is success, while 1
doesn't really say whether it's success or failure. One has to infer
that "already resumed" is essentially "success" when requesting a
resume.

I'll try to tweak the language.

> > +        */
> > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
> > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_request_idle(dev));
> > +       KUNIT_EXPECT_EQ(test, 1, pm_request_resume(dev));
> > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_request_autosuspend(dev));
> > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_suspend(dev));
> > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_resume(dev));
> > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_autosuspend(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
> > +
> > +       /* Suspended again */
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > +}
> > +
> > +/*
> > + * Explore a typical probe() sequence in which a device marks itself powered,
> > + * but doesn't hold any runtime PM reference, so it suspends as soon as it goes
> > + * idle.
> > + */
> > +static void pm_runtime_probe_active_test(struct kunit *test)
> > +{
> > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > +
> > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > +
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_status_suspended(dev));
> > +
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_set_active(dev));
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > +
> > +       pm_runtime_enable(dev);
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > +
> > +       /* Flush, and ensure we stayed active. */
> 
> There's nothing to flush though.

Ack. I'll reword.

Brian

> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev));
> > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > +
> > +       /* Ask for idle? Now we suspend. */
> > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_idle(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),
> > +       KUNIT_CASE(pm_runtime_idle_test),
> > +       KUNIT_CASE(pm_runtime_disabled_test),
> > +       KUNIT_CASE(pm_runtime_error_test),
> > +       KUNIT_CASE(pm_runtime_probe_active_test),
> > +       {}
> > +};
> > +
> > +static struct kunit_suite pm_runtime_test_suite = {
> > +       .name = "pm_runtime_test_cases",
> > +       .test_cases = pm_runtime_test_cases,
> > +};
> > +
> > +kunit_test_suite(pm_runtime_test_suite);
> > +MODULE_DESCRIPTION("Runtime power management unit test suite");
> > +MODULE_LICENSE("GPL");
> > --

Re: [PATCH 1/3] PM: runtime: Add basic kunit tests for API contracts

[Rafael J. Wysocki]

Hi Brian,

On Wed, Sep 10, 2025 at 10:44 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Hi Rafael,
>
> On Fri, Sep 05, 2025 at 07:37:38PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Aug 29, 2025 at 2:33 AM Brian Norris <briannorris@chromium.org> wrote:
> > >
> > > In exploring the various return codes and failure modes of runtime PM
> > > APIs, I found it helpful to verify and codify many of them in unit
> > > tests, especially given that even the kerneldoc can be rather complex to
> > > reason through, and it also has had subtle errors of its own.
> > >
> > > Signed-off-by: Brian Norris <briannorris@chromium.org>
> >
> > This is nice in general, but I have a couple of questions/comments (see below).
>
> Thanks for looking. There's certainly some matter of opinion on how
> exactly to test things, and I'm still getting up to speed on some of the
> runtime PM API details, so I appreciate the care you've given.
>
> Replies inline.
>
> > > ---
> > >
> > >  drivers/base/Kconfig              |   6 +
> > >  drivers/base/power/Makefile       |   1 +
> > >  drivers/base/power/runtime-test.c | 259 ++++++++++++++++++++++++++++++
> > >  3 files changed, 266 insertions(+)
> > >  create mode 100644 drivers/base/power/runtime-test.c
> > >
> > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> > > index 064eb52ff7e2..1786d87b29e2 100644
> > > --- a/drivers/base/Kconfig
> > > +++ b/drivers/base/Kconfig
> > > @@ -167,6 +167,12 @@ config PM_QOS_KUNIT_TEST
> > >         depends on KUNIT=y
> > >         default KUNIT_ALL_TESTS
> > >
> > > +config PM_RUNTIME_KUNIT_TEST
> > > +       tristate "KUnit Tests for runtime PM" if !KUNIT_ALL_TESTS
> > > +       depends on KUNIT
> > > +       depends on PM
> > > +       default KUNIT_ALL_TESTS
> > > +
> > >  config HMEM_REPORTING
> > >         bool
> > >         default n
> > > diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> > > index 01f11629d241..2989e42d0161 100644
> > > --- a/drivers/base/power/Makefile
> > > +++ b/drivers/base/power/Makefile
> > > @@ -4,5 +4,6 @@ obj-$(CONFIG_PM_SLEEP)  += main.o wakeup.o wakeup_stats.o
> > >  obj-$(CONFIG_PM_TRACE_RTC)     += trace.o
> > >  obj-$(CONFIG_HAVE_CLK) += clock_ops.o
> > >  obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o
> > > +obj-$(CONFIG_PM_RUNTIME_KUNIT_TEST) += runtime-test.o
> > >
> > >  ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> > > diff --git a/drivers/base/power/runtime-test.c b/drivers/base/power/runtime-test.c
> > > new file mode 100644
> > > index 000000000000..263c28d5fc50
> > > --- /dev/null
> > > +++ b/drivers/base/power/runtime-test.c
> > > @@ -0,0 +1,259 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2025 Google, Inc.
> > > + */
> > > +
> > > +#include <linux/cleanup.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <kunit/device.h>
> > > +#include <kunit/test.h>
> > > +
> > > +#define DEVICE_NAME "pm_runtime_test_device"
> > > +
> > > +static void pm_runtime_depth_test(struct kunit *test)
> > > +{
> > > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > > +
> > > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > > +
> > > +       pm_runtime_enable(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));
> > > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_get_sync(dev)); /* "already active" */
> > > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
> > > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_sync(dev));
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > > +}
> > > +
> > > +/* Test pm_runtime_put() and friends when already suspended. */
> > > +static void pm_runtime_already_suspended_test(struct kunit *test)
> > > +{
> > > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > > +
> > > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > > +
> > > +       pm_runtime_enable(dev);
> > > +
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > > +       pm_runtime_get_noresume(dev);
> > > +
> > > +       /* Flush, in case the above (non-sync) triggered any work. */
> > > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */
> >
> > Why do you run pm_runtime_barrier(dev) here?  It is guaranteed that no
> > requests are pending at this point.
>
> I suppose my thought is as somewhat of an outsider, that's not really
> familiar with exactly how each API is supposed to work. So without
> looking into the details of the implementation, it's not clear to me
> that a "get_noresume()" will never queue any work. Admittedly, that's a
> pretty weak reason.
>
> OTOH, it does serve to test the 0 side of the API contract:
>
> """
>  * 1, if there was a resume request pending and the device had to be woken up,
>  * 0, otherwise
> """
>
> So IMO, it's a reasonable thing to run in this test, although I probably
> should drop the "Flush" comment.

Yeah, changing the comment would help.

> > > +
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> >
> > This has already been tested above.
>
> I'm not really an expert on unit testing and style, but the whole point
> of this test (named "already_suspended") is that we're testing each
> operation when the device is suspended. So it's many series of:
>
> 1. set up some precondition
> 2. assert that the device is (still) suspended
> 3. test that an API returns the expected value for "already suspended"
>
> Even if #1/#3 aren't likely to affect #2 for a later sequence, it seems
> like a good pattern to actually test that this continues to remain true
> each time. If the test changes in the future such that we perform
> something different in #1, we might find ourselves not testing "already
> suspended" behavior in #3.
>
> Alternatively, I could split each #1/#2/#3 sequence into its own
> subtest, but that might get a little excessive.
>
> Anyway, like I said, it's probably some matter of opinion/style. I can
> drop some of these checks if you still think they have no place here.

I would do just two of them, one at the beginning and one at the end.
It should be an invariant for everything in between.

> > > +       /*
> > > +        * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
> > > +        * this as -EAGAIN / "runtime PM status change ongoing".
> >
> > No, this means "Conditions are not suitable, but may change".
>
> I'm just quoting the API docs for put():
>
> """
> * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> """
>
> If that's the wrong language, then we should update the API doc. At any
> rate, I'm not sure what's "unsuitable" about a suspended device when we
> call put(). It's not unsuitable -- it's already in the target state!
>
> Notably, I'm also changing this behavior in patch 2, since I think it's
> an API bug. And the comment then goes away.

Yeah, so I'd prefer to change this particular thing entirely,
especially in the face of

https://lore.kernel.org/linux-pm/5049058.31r3eYUQgx@rafael.j.wysocki/

which quite obviously doesn't take the return value of
pm_runtime_put() and pm_runtime_put_sutosuspend() into account.

I would like these two functions to be void.

Of course, there are existing users that check their return values,
but I'm not sure how much of a real advantage from doing that is.  At
least some of those users appear to not exactly know what they are
doing.

> > > +        */
> > > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put(dev));
> > > +
> > > +       pm_runtime_get_noresume(dev);
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> >
> > This has been tested already twice and why would it change?
>
> Addressed above. I can drop it if you think it's excessive.
>
> > > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_put_sync(dev));
> > > +
> > > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_suspend(dev));
> > > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_autosuspend(dev));
> > > +       KUNIT_EXPECT_EQ(test, 1, pm_request_autosuspend(dev));
> > > +
> > > +       pm_runtime_get_noresume(dev);
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> >
> > There's no way by which it could change above.
>
> Same.
>
> > > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_sync_autosuspend(dev));
> > > +
> > > +       pm_runtime_get_noresume(dev);
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_autosuspend(dev));
> > > +
> > > +       /* Grab 2 refcounts */
> > > +       pm_runtime_get_noresume(dev);
> > > +       pm_runtime_get_noresume(dev);
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > > +       /* The first put() sees usage_count 1 */
> > > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_put_autosuspend(dev));
> > > +       /* The second put() sees usage_count 0 but tells us "already suspended". */
> > > +       KUNIT_EXPECT_EQ(test, 1, pm_runtime_put_autosuspend(dev));
> > > +
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> >
> > Again, there is no way this can change in the whole test.
>
> Same.
>
> > > +}
> > > +
> > > +static void pm_runtime_idle_test(struct kunit *test)
> > > +{
> > > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > > +
> > > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > > +
> > > +       pm_runtime_enable(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));
> > > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > > +       pm_runtime_put_noidle(dev);
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_idle(dev));
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_runtime_idle(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EAGAIN, pm_request_idle(dev));
> > > +}
> > > +
> > > +static void pm_runtime_disabled_test(struct kunit *test)
> > > +{
> > > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > > +
> > > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > > +
> > > +       /* Never called pm_runtime_enable() */
> > > +       KUNIT_EXPECT_FALSE(test, pm_runtime_enabled(dev));
> > > +
> > > +       /* "disabled" is treated as "active" */
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > > +       KUNIT_EXPECT_FALSE(test, pm_runtime_suspended(dev));
> > > +
> > > +       /*
> > > +        * Note: these "fail", but they still acquire/release refcounts, so
> > > +        * keep them balanced.
> > > +        */
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put(dev));
> > > +
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get_sync(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put_sync(dev));
> > > +
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_get(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_put_autosuspend(dev));
> > > +
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_resume_and_get(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_idle(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_request_idle(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_request_resume(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_request_autosuspend(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_suspend(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_resume(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EACCES, pm_runtime_autosuspend(dev));
> > > +
> > > +       /* Still active */
> >
> > Still disabled rather.
>
> Ack, will change.
>
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_active(dev));
> > > +}
> > > +
> > > +static void pm_runtime_error_test(struct kunit *test)
> > > +{
> > > +       struct device *dev = kunit_device_register(test, DEVICE_NAME);
> > > +
> > > +       KUNIT_ASSERT_PTR_NE(test, NULL, dev);
> > > +
> > > +       pm_runtime_enable(dev);
> > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > > +
> > > +       /* Fake a .runtime_resume() error */
> > > +       dev->power.runtime_error = -EIO;
> > > +
> > > +       /*
> > > +        * Note: these "fail", but they still acquire/release refcounts, so
> > > +        * keep them balanced.
> > > +        */
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put(dev));
> > > +
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get_sync(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put_sync(dev));
> > > +
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_get(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_put_autosuspend(dev));
> > > +
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_resume_and_get(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_idle(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_idle(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_resume(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_request_autosuspend(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_suspend(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_resume(dev));
> > > +       KUNIT_EXPECT_EQ(test, -EINVAL, pm_runtime_autosuspend(dev));
> > > +
> > > +       /* Still suspended */
> >
> > Error is still pending.
>
> Your statement is true, but I'm not quite sure what you're suggesting.
> Are you suggesting I should
>
>         KUNIT_EXPECT_EQ(test, -EIO, dev->power.runtime_error);
>
> ?
>
> Or are you suggesting I change the comment?

Change the comment.

> I'm thinking I'll do both.

That will work too.

Re: [PATCH 1/3] PM: runtime: Add basic kunit tests for API contracts

[Brian Norris]

Hi Rafael,

On Fri, Sep 19, 2025 at 06:58:50PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 10, 2025 at 10:44 PM Brian Norris <briannorris@chromium.org> wrote:
> > On Fri, Sep 05, 2025 at 07:37:38PM +0200, Rafael J. Wysocki wrote:
> > > On Fri, Aug 29, 2025 at 2:33 AM Brian Norris <briannorris@chromium.org> wrote:
> > > > +       /* Flush, in case the above (non-sync) triggered any work. */
> > > > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */
> > >
> > > Why do you run pm_runtime_barrier(dev) here?  It is guaranteed that no
> > > requests are pending at this point.
> >
...
> > So IMO, it's a reasonable thing to run in this test, although I probably
> > should drop the "Flush" comment.
> 
> Yeah, changing the comment would help.

Will do.

> > > > +
> > > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > >
> > > This has already been tested above.
...
> > Anyway, like I said, it's probably some matter of opinion/style. I can
> > drop some of these checks if you still think they have no place here.
> 
> I would do just two of them, one at the beginning and one at the end.
> It should be an invariant for everything in between.

Ack.

> > > > +       /*
> > > > +        * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
> > > > +        * this as -EAGAIN / "runtime PM status change ongoing".
> > >
> > > No, this means "Conditions are not suitable, but may change".
> >
> > I'm just quoting the API docs for put():
> >
> > """
> > * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> > """
> >
> > If that's the wrong language, then we should update the API doc. At any
> > rate, I'm not sure what's "unsuitable" about a suspended device when we
> > call put(). It's not unsuitable -- it's already in the target state!
> >
> > Notably, I'm also changing this behavior in patch 2, since I think it's
> > an API bug. And the comment then goes away.
> 
> Yeah, so I'd prefer to change this particular thing entirely,
> especially in the face of
> 
> https://lore.kernel.org/linux-pm/5049058.31r3eYUQgx@rafael.j.wysocki/
> 
> which quite obviously doesn't take the return value of
> pm_runtime_put() and pm_runtime_put_sutosuspend() into account.
> 
> I would like these two functions to be void.

Sure, I think inspecting put() return codes is generally a bad idea.
'void' would be cool with me, although maybe a bit impractical now,
considering how many users look at the current return code. So at a
minimum, I'd separate "make 'em void" from my "document and test the
API" work.

Really, I'm mostly looking at this area because I have to support driver
developers trying to learn how to use the runtime PM API, and they
wonder about the return codes. So if they exist, I'd at least like them
to make sense.

Anyway, for the particulars of this test: I can try to adapt the comment
language a bit. But are you suggesting I shouldn't even try patch 2,
which fixes the pm_runtime_put() return codes?

> Of course, there are existing users that check their return values,
> but I'm not sure how much of a real advantage from doing that is.  At
> least some of those users appear to not exactly know what they are
> doing.
> 

...

> > > > +static void pm_runtime_error_test(struct kunit *test)
> > > > +{
...

> > > > +       /* Still suspended */
> > >
> > > Error is still pending.
> >
> > Your statement is true, but I'm not quite sure what you're suggesting.
> > Are you suggesting I should
> >
> >         KUNIT_EXPECT_EQ(test, -EIO, dev->power.runtime_error);
> >
> > ?
> >
> > Or are you suggesting I change the comment?
> 
> Change the comment.
> 
> > I'm thinking I'll do both.
> 
> That will work too.

Ack.

Brian

Re: [PATCH 1/3] PM: runtime: Add basic kunit tests for API contracts

[Rafael J. Wysocki]

Hi Brian,

On Tue, Sep 23, 2025 at 11:51 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Hi Rafael,
>
> On Fri, Sep 19, 2025 at 06:58:50PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Sep 10, 2025 at 10:44 PM Brian Norris <briannorris@chromium.org> wrote:
> > > On Fri, Sep 05, 2025 at 07:37:38PM +0200, Rafael J. Wysocki wrote:
> > > > On Fri, Aug 29, 2025 at 2:33 AM Brian Norris <briannorris@chromium.org> wrote:
> > > > > +       /* Flush, in case the above (non-sync) triggered any work. */
> > > > > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */
> > > >
> > > > Why do you run pm_runtime_barrier(dev) here?  It is guaranteed that no
> > > > requests are pending at this point.
> > >
> ...
> > > So IMO, it's a reasonable thing to run in this test, although I probably
> > > should drop the "Flush" comment.
> >
> > Yeah, changing the comment would help.
>
> Will do.
>
> > > > > +
> > > > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > > >
> > > > This has already been tested above.
> ...
> > > Anyway, like I said, it's probably some matter of opinion/style. I can
> > > drop some of these checks if you still think they have no place here.
> >
> > I would do just two of them, one at the beginning and one at the end.
> > It should be an invariant for everything in between.
>
> Ack.
>
> > > > > +       /*
> > > > > +        * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
> > > > > +        * this as -EAGAIN / "runtime PM status change ongoing".
> > > >
> > > > No, this means "Conditions are not suitable, but may change".
> > >
> > > I'm just quoting the API docs for put():
> > >
> > > """
> > > * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> > > """
> > >
> > > If that's the wrong language, then we should update the API doc. At any
> > > rate, I'm not sure what's "unsuitable" about a suspended device when we
> > > call put(). It's not unsuitable -- it's already in the target state!
> > >
> > > Notably, I'm also changing this behavior in patch 2, since I think it's
> > > an API bug. And the comment then goes away.
> >
> > Yeah, so I'd prefer to change this particular thing entirely,
> > especially in the face of
> >
> > https://lore.kernel.org/linux-pm/5049058.31r3eYUQgx@rafael.j.wysocki/
> >
> > which quite obviously doesn't take the return value of
> > pm_runtime_put() and pm_runtime_put_sutosuspend() into account.
> >
> > I would like these two functions to be void.
>
> Sure, I think inspecting put() return codes is generally a bad idea.
> 'void' would be cool with me, although maybe a bit impractical now,
> considering how many users look at the current return code.

For pm_runtime_put() it's not that bad.  I have ~20 patches changing
all of the code looking at its return value to stop doing that.

Interestingly enough, there's only one piece of that code (USB core)
doing anything remotely useful with that return value.  Everything
else is just garbage IMV.

> So at a minimum, I'd separate "make 'em void" from my "document and test the
> API" work.

But you can just skip them.

> Really, I'm mostly looking at this area because I have to support driver
> developers trying to learn how to use the runtime PM API, and they
> wonder about the return codes. So if they exist, I'd at least like them
> to make sense.

Sure.

That said, as far as pm_runtime_put() and pm_runtime_put_autosuspend()
are concerned, you may as well just say "discard their return values,
you don't want to have to deal with them, and never ever pass them
verbatim to the callers of your code".

> Anyway, for the particulars of this test: I can try to adapt the comment
> language a bit. But are you suggesting I shouldn't even try patch 2,
> which fixes the pm_runtime_put() return codes?

Not really.

> > Of course, there are existing users that check their return values,
> > but I'm not sure how much of a real advantage from doing that is.

Well, see above. :-)

>  At least some of those users appear to not exactly know what they are
> doing.

Almost none of them do nonsense.

Re: [PATCH 1/3] PM: runtime: Add basic kunit tests for API contracts

[Rafael J. Wysocki]

On Wed, Sep 24, 2025 at 7:32 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> Hi Brian,
>
> On Tue, Sep 23, 2025 at 11:51 PM Brian Norris <briannorris@chromium.org> wrote:
> >
> > Hi Rafael,
> >
> > On Fri, Sep 19, 2025 at 06:58:50PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Sep 10, 2025 at 10:44 PM Brian Norris <briannorris@chromium.org> wrote:
> > > > On Fri, Sep 05, 2025 at 07:37:38PM +0200, Rafael J. Wysocki wrote:
> > > > > On Fri, Aug 29, 2025 at 2:33 AM Brian Norris <briannorris@chromium.org> wrote:
> > > > > > +       /* Flush, in case the above (non-sync) triggered any work. */
> > > > > > +       KUNIT_EXPECT_EQ(test, 0, pm_runtime_barrier(dev)); /* no wakeup needed */
> > > > >
> > > > > Why do you run pm_runtime_barrier(dev) here?  It is guaranteed that no
> > > > > requests are pending at this point.
> > > >
> > ...
> > > > So IMO, it's a reasonable thing to run in this test, although I probably
> > > > should drop the "Flush" comment.
> > >
> > > Yeah, changing the comment would help.
> >
> > Will do.
> >
> > > > > > +
> > > > > > +       KUNIT_EXPECT_TRUE(test, pm_runtime_suspended(dev));
> > > > >
> > > > > This has already been tested above.
> > ...
> > > > Anyway, like I said, it's probably some matter of opinion/style. I can
> > > > drop some of these checks if you still think they have no place here.
> > >
> > > I would do just two of them, one at the beginning and one at the end.
> > > It should be an invariant for everything in between.
> >
> > Ack.
> >
> > > > > > +       /*
> > > > > > +        * We never actually left RPM_SUSPENDED, but rpm_idle() still treats
> > > > > > +        * this as -EAGAIN / "runtime PM status change ongoing".
> > > > >
> > > > > No, this means "Conditions are not suitable, but may change".
> > > >
> > > > I'm just quoting the API docs for put():
> > > >
> > > > """
> > > > * * -EAGAIN: Runtime PM usage_count non-zero or Runtime PM status change ongoing.
> > > > """
> > > >
> > > > If that's the wrong language, then we should update the API doc. At any
> > > > rate, I'm not sure what's "unsuitable" about a suspended device when we
> > > > call put(). It's not unsuitable -- it's already in the target state!
> > > >
> > > > Notably, I'm also changing this behavior in patch 2, since I think it's
> > > > an API bug. And the comment then goes away.
> > >
> > > Yeah, so I'd prefer to change this particular thing entirely,
> > > especially in the face of
> > >
> > > https://lore.kernel.org/linux-pm/5049058.31r3eYUQgx@rafael.j.wysocki/
> > >
> > > which quite obviously doesn't take the return value of
> > > pm_runtime_put() and pm_runtime_put_sutosuspend() into account.
> > >
> > > I would like these two functions to be void.
> >
> > Sure, I think inspecting put() return codes is generally a bad idea.
> > 'void' would be cool with me, although maybe a bit impractical now,
> > considering how many users look at the current return code.
>
> For pm_runtime_put() it's not that bad.  I have ~20 patches changing
> all of the code looking at its return value to stop doing that.
>
> Interestingly enough, there's only one piece of that code (USB core)
> doing anything remotely useful with that return value.  Everything
> else is just garbage IMV.
>
> > So at a minimum, I'd separate "make 'em void" from my "document and test the
> > API" work.
>
> But you can just skip them.
>
> > Really, I'm mostly looking at this area because I have to support driver
> > developers trying to learn how to use the runtime PM API, and they
> > wonder about the return codes. So if they exist, I'd at least like them
> > to make sense.
>
> Sure.
>
> That said, as far as pm_runtime_put() and pm_runtime_put_autosuspend()
> are concerned, you may as well just say "discard their return values,
> you don't want to have to deal with them, and never ever pass them
> verbatim to the callers of your code".
>
> > Anyway, for the particulars of this test: I can try to adapt the comment
> > language a bit. But are you suggesting I shouldn't even try patch 2,
> > which fixes the pm_runtime_put() return codes?
>
> Not really.
>
> > > Of course, there are existing users that check their return values,
> > > but I'm not sure how much of a real advantage from doing that is.
>
> Well, see above. :-)
>
> >  At least some of those users appear to not exactly know what they are
> > doing.
>
> Almost none of them do nonsense.

s/none/all/ (sorry)

Re: [PATCH 1/3] PM: runtime: Add basic kunit tests for API contracts

[Brian Norris]

On Wed, Sep 24, 2025 at 07:34:31PM +0200, Rafael J. Wysocki wrote:
> On Wed, Sep 24, 2025 at 7:32 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Sep 23, 2025 at 11:51 PM Brian Norris <briannorris@chromium.org> wrote:
> > > On Fri, Sep 19, 2025 at 06:58:50PM +0200, Rafael J. Wysocki wrote:
> > > > Yeah, so I'd prefer to change this particular thing entirely,
> > > > especially in the face of
> > > >
> > > > https://lore.kernel.org/linux-pm/5049058.31r3eYUQgx@rafael.j.wysocki/
> > > >
> > > > which quite obviously doesn't take the return value of
> > > > pm_runtime_put() and pm_runtime_put_sutosuspend() into account.
> > > >
> > > > I would like these two functions to be void.
> > >
> > > Sure, I think inspecting put() return codes is generally a bad idea.
> > > 'void' would be cool with me, although maybe a bit impractical now,
> > > considering how many users look at the current return code.
> >
> > For pm_runtime_put() it's not that bad.  I have ~20 patches changing
> > all of the code looking at its return value to stop doing that.
> >
> > Interestingly enough, there's only one piece of that code (USB core)
> > doing anything remotely useful with that return value.  Everything
> > else is just garbage IMV.
> >
> > > So at a minimum, I'd separate "make 'em void" from my "document and test the
> > > API" work.
> >
> > But you can just skip them.
> >
> > > Really, I'm mostly looking at this area because I have to support driver
> > > developers trying to learn how to use the runtime PM API, and they
> > > wonder about the return codes. So if they exist, I'd at least like them
> > > to make sense.
> >
> > Sure.
> >
> > That said, as far as pm_runtime_put() and pm_runtime_put_autosuspend()
> > are concerned, you may as well just say "discard their return values,
> > you don't want to have to deal with them, and never ever pass them
> > verbatim to the callers of your code".

Sounds reasonable.

I'll drop any unit test expectations for pm_runtime_put() and
pm_runtime_put_autosuspend() return codes. But I'll leave
pm_runtime_put_sync().

> > > Anyway, for the particulars of this test: I can try to adapt the comment
> > > language a bit. But are you suggesting I shouldn't even try patch 2,
> > > which fixes the pm_runtime_put() return codes?
> >
> > Not really.
> >
> > > > Of course, there are existing users that check their return values,
> > > > but I'm not sure how much of a real advantage from doing that is.
> >
> > Well, see above. :-)
> >
> > >  At least some of those users appear to not exactly know what they are
> > > doing.
> >
> > Almost none of them do nonsense.
> 
> s/none/all/ (sorry)

Ha, thanks for the clarification :)

Brian