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

[Brian Norris <briannorris@chromium.org>]

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