Received-SPF: pass (google.com: domain of linux-kernel+bounces-22172-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <10423008.nUPlyArG6x@kreacher> <ZZ5zcBBEv7qupIdE@linux.intel.com>
In-Reply-To: <ZZ5zcBBEv7qupIdE@linux.intel.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Wed, 10 Jan 2024 13:33:07 +0100
Message-ID: <CAJZ5v0gp6uETgLNHxDnSd4h_0ois7J2AC7soJJVv18B99GmxcQ@mail.gmail.com>
Subject: Re: [PATCH v1] PM: sleep: Restore asynchronous device resume optimization
To: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>, Linux PM <linux-pm@vger.kernel.org>, 
	Ulf Hansson <ulf.hansson@linaro.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 10, 2024 at 11:37=E2=80=AFAM 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 resum=
e
> > 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
> > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> > --- 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 P=
M 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 subsyst=
ems */
>
> 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 =3D state;
> >
> > +     /*
> > +      * Trigger the resume of "async" devices upfront so they don't ha=
ve 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 =3D 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!