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

Rafael J. Wysocki
2024-01-10T12:33:21Z

On Wed, Jan 10, 2024 at 11:37 AM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> wrote:
>
> On Tue, Jan 09, 2024 at 05:59:22PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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@intel.com>
> > ---
> >
> > 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@linux.intel.com>

Thanks!