Re: [PATCH v1] PM: sleep: Restore asynchronous device resume optimization

[Rafael J. Wysocki]

On Wed, Jan 10, 2024 at 11:37 AM Stanislaw Gruszka
<stanislaw.gruszka@xxxxxxxxxxxxxxx> wrote:
>
> On Tue, Jan 09, 2024 at 05:59:22PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> >
> > Before commit 7839d0078e0d ("PM: sleep: Fix possible deadlocks in core
> > system-wide PM code"), the resume of devices that were allowed to resume
> > asynchronously was scheduled before starting the resume of the other
> > devices, so the former did not have to wait for the latter unless
> > functional dependencies were present.
> >
> > Commit 7839d0078e0d removed that optimization in order to address a
> > correctness issue, but it can be restored with the help of a new device
> > power management flag, so do that now.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > ---
> >
> > I said I'd probably do this in 6.9, but then I thought more about it
> > and now I think it would be nice to have 6.8-rc1 without a suspend
> > performance regression and the change is relatively straightforward,
> > so here it goes.
> >
> > ---
> >  drivers/base/power/main.c |  117 +++++++++++++++++++++++++---------------------
> >  include/linux/pm.h        |    1
> >  2 files changed, 65 insertions(+), 53 deletions(-)
> >
> > Index: linux-pm/include/linux/pm.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/pm.h
> > +++ linux-pm/include/linux/pm.h
> > @@ -681,6 +681,7 @@ struct dev_pm_info {
> >       bool                    wakeup_path:1;
> >       bool                    syscore:1;
> >       bool                    no_pm_callbacks:1;      /* Owned by the PM core */
> > +     bool                    in_progress:1;  /* Owned by the PM core */
> >       unsigned int            must_resume:1;  /* Owned by the PM core */
> >       unsigned int            may_skip_resume:1;      /* Set by subsystems */
>
> Not related to the patch, just question: why some types here are
> unsigned int :1 others bool :1 ?

No particular reason.

I think I will change them all to bool in the future.

> >   * dpm_resume_early - Execute "early resume" callbacks for all devices.
> >   * @state: PM transition of the system being carried out.
> > @@ -845,18 +845,28 @@ void dpm_resume_early(pm_message_t state
> >       mutex_lock(&dpm_list_mtx);
> >       pm_transition = state;
> >
> > +     /*
> > +      * Trigger the resume of "async" devices upfront so they don't have to
> > +      * wait for the "non-async" ones they don't depend on.
> > +      */
> > +     list_for_each_entry(dev, &dpm_late_early_list, power.entry)
> > +             dpm_async_fn(dev, async_resume_early);
> > +
> >       while (!list_empty(&dpm_late_early_list)) {
> >               dev = to_device(dpm_late_early_list.next);
> > -             get_device(dev);
> >               list_move_tail(&dev->power.entry, &dpm_suspended_list);
> >
> > -             mutex_unlock(&dpm_list_mtx);
> > +             if (!dev->power.in_progress) {
>
> I would consider different naming just to make clear this
> is regarding async call, in_progress looks too generic for me.

OK, what about async_in_progress?

> Fine if you think otherwise, in general patch LGTM:
>
> Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx>

Thanks!