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

[Stanislaw Gruszka]

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 ?

>   * 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@xxxxxxxxxxxxxxx>