| 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: <CAJZ5v0iELLPYBS6FKmX=DhoyQ2tDq9F9DAzuV0A8etv0dGeJvQ@mail.gmail.com>
Date: Fri, 19 Sep 2025 18:58:50 +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
Hi Brian,
On Wed, Sep 10, 2025 at 10:44 PM Brian Norris <briannorris@...omium.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@...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).
>
> 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.
Powered by blists - more mailing lists