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] [day] [month] [year] [list]

Message-ID: <CAJZ5v0i0_r_=rsPzLmST7cZtGjHCP73t9aoXdVFa81J9nJmzsQ@mail.gmail.com>
Date: Wed, 24 Sep 2025 19:34:31 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Brian Norris <briannorris@...omium.org>
Cc: 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 Wed, Sep 24, 2025 at 7:32 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> 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.

s/none/all/ (sorry)