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

Stanislaw Gruszka
2024-01-10T10:37:40Z

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 ?

> * 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.
Fine if you think otherwise, in general patch LGTM:

Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>