Re: [PATCH v2] PM: sleep: core: Clear is_prepared if no_pm_callbacks is true before checking power.syscore

[Xu Yang <xu.yang_2@nxp.com>]

Hi Rafael,

On Fri, Feb 21, 2025 at 09:49:11PM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 13, 2025 at 10:33 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Feb 13, 2025 at 11:58 AM Xu Yang <xu.yang_2@nxp.com> wrote:
> > >
> > > Currently, if power.no_callbacks is true for a device, device_prepare()
> > > will also set power.direct_complete to true. When device_resume() check
> > > power.direct_complete, setting power.is_prepared will be skipped if it
> > > can directly complete. This will cause a warning when add new devices
> > > during resume() stage.
> > >
> > > Although power.is_prepared should be cleared in complete() state, commit
> > > (f76b168b6f11 PM: Rename dev_pm_info.in_suspend to is_prepared) allow
> > > clear it in earlier resume() stage. However, we need also set is_prepared
> > > to false before checking syscore if the device has no pm callbacks.
> > >
> > > Take USB as example:
> > > The usb_interface is such a device which setting power.no_callbacks to
> > > true. Then if the user call usb_set_interface() during resume() stage,
> > > the kernel will print below warning since the system will create and
> > > add ep devices.
> > >
> > > [  186.461414] usb 1-1: reset high-speed USB device number 3 using ci_hdrc
> > > [  187.102681]  ep_81: PM: parent 1-1:1.1 should not be sleeping
> > > [  187.105010] PM: resume devices took 0.936 seconds
> > >
> > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > >
> > > ---
> > > v2: clear is_prepared before check syscore as suggested by Rafael
> > > ---
> > >  drivers/base/power/main.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> > > index 40e1d8d8a589..5e69cfaec661 100644
> > > --- a/drivers/base/power/main.c
> > > +++ b/drivers/base/power/main.c
> > > @@ -926,6 +926,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)
> > >         TRACE_DEVICE(dev);
> > >         TRACE_RESUME(0);
> > >
> > > +       if (dev->power.no_pm_callbacks)
> > > +               dev->power.is_prepared = false;
> > > +
> > >         if (dev->power.syscore)
> > >                 goto Complete;
> > >
> > > --
> >
> > As I've already said in the other thread, my initial feedback was
> > based on an oversimplified view of this code, and the problem will
> > need to be addressed differently.
> >
> > Sorry about that.
> 
> Unfortunately, I don't think that this can cover all of the corner
> cases.  Something is going to escape, this way or another.

Agree. Although I don't know how the corner case looks, it seems like
a risk in the future.

> 
> To minimize the number of things that may escape, I would first treat
> direct_complete as a special case, so only clear power.is_prepared
> early if direct_complete is set.
> 
> Then, clear it only for devices with power.no_pm_callbacks set, that is
> 
>     if (dev->power.direct_complete) {
>         /* Add explanatory comment here */
>         if (dev->power.no_pm_callbacks)
>             dev->power.is_prepared = false;
> 
>         /* Match the pm_runtime_disable() in device_suspend(). */
>         pm_runtime_enable(dev);
>         goto Complete;
>     }

Thank you for the analysis and suggestions! Combined checking direct_complete
and no_pm_callbacks seems like a better solution and this will precisely cover
the issue I reported. I will adjust the patch and send v3 later.

> 
> This is not perfect because ideally the device with no callbacks
> should wait for its parent and suppliers (if any) to resume before new
> children can be added under it safely (in case the children depend on
> the devices that it depends on) and is_prepared may be cleared before
> that happens, but those new children may be added by the parent's
> resume callback and they won't appear if the parent is not ready
> anyway.

Understand.

Thanks,
Xu Yang