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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0jnPsVrULF9+S-e+HvT+bik=+WA7FfXzFg5vfO8WhTy9Q@mail.gmail.com>
Date: Wed, 24 Sep 2025 19:32:37 +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 Tue, Sep 23, 2025 at 11:51 PM Brian Norris <briannorris@...omium.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@...omium.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@...omium.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.