| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0gGKsR0bVayyTXy1W9FLwVfG1S+gseH7jPKtggzZFNpfA@mail.gmail.com>
Date: Fri, 5 Sep 2025 19:37:38 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Brian Norris <briannorris@...omium.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Pavel Machek <pavel@...nel.org>,
"Rafael J . Wysocki" <rafael.j.wysocki@...el.com>, kunit-dev@...glegroups.com,
Len Brown <lenb@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] PM: runtime: Add basic kunit tests for API contracts
On Fri, Aug 29, 2025 at 2:33 AM Brian Norris <briannorris@...omium.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@...omium.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");
> --
Powered by blists - more mailing lists