Message-ID: <CAJZ5v0hpPOHNYCSTM1bb+p-wyAZkpg+k-huf9f5df9_S8MfvEg@mail.gmail.com>
Date: Fri, 11 Jul 2025 15:38:26 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Tudor Ambarus <tudor.ambarus@...aro.org>
Cc: "Rafael J. Wysocki" <rjw@...ysocki.net>, Linux PM <linux-pm@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Alan Stern <stern@...land.harvard.edu>, 
	Ulf Hansson <ulf.hansson@...aro.org>, Johan Hovold <johan@...nel.org>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, Jon Hunter <jonathanh@...dia.com>, 
	Saravana Kannan <saravanak@...gle.com>, William McVicker <willmcvicker@...gle.com>, 
	Peter Griffin <peter.griffin@...aro.org>, André Draszik <andre.draszik@...aro.org>
Subject: Re: [PATCH v3 1/5] PM: sleep: Resume children after resuming the parent

On Fri, Jul 11, 2025 at 3:08 PM Tudor Ambarus <tudor.ambarus@...aro.org> wrote:
>
>
> Hi, Rafael,
>
> On 3/14/25 12:50 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >
> > According to [1], the handling of device suspend and resume, and
> > particularly the latter, involves unnecessary overhead related to
> > starting new async work items for devices that cannot make progress
> > right away because they have to wait for other devices.
> >
> > To reduce this problem in the resume path, use the observation that
> > starting the async resume of the children of a device after resuming
> > the parent is likely to produce less scheduling and memory management
> > noise than starting it upfront while at the same time it should not
> > increase the resume duration substantially.
> >
> > Accordingly, modify the code to start the async resume of the device's
> > children when the processing of the parent has been completed in each
> > stage of device resume and only start async resume upfront for devices
> > without parents.
> >
> > Also make it check if a given device can be resumed asynchronously
> > before starting the synchronous resume of it in case it will have to
> > wait for another that is already resuming asynchronously.
> >
> > In addition to making the async resume of devices more friendly to
> > systems with relatively less computing resources, this change is also
> > preliminary for analogous changes in the suspend path.
> >
> > On the systems where it has been tested, this change by itself does
> > not affect the overall system resume duration in a measurable way.
> >
> > Link: https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/ [1]
> > Suggested-by: Saravana Kannan <saravanak@...gle.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> I'd like to let you know of a suspend crash that I'm dealing with
> when using the OOT pixel6 drivers on top of v6.16-rc4.

Well, thanks, but there's not much I can do about it.

It is also better to start a new thread in such cases than to reply to
a patch submission.

> Similar to what Jon reported, everything gets back to normal if
> I disable pm_async or if I revert the following patches:
> 443046d1ad66 PM: sleep: Make suspend of devices more asynchronous
> aa7a9275ab81 PM: sleep: Suspend async parents after suspending children
> 0cbef962ce1f PM: sleep: Resume children after resuming the parent
>
> I also reverted their fixes when testing:
> 8887abccf8aa PM: sleep: Add locking to dpm_async_resume_children()
> d46c4c839c20 PM: sleep: Fix power.is_suspended cleanup for direct-complete devices
> 079e8889ad13 PM: sleep: Fix list splicing in device suspend error paths
>
> It seems that the hang happens in dpm_suspend() at
> async_synchronize_full() time after a driver fails to suspend.
> The phone then naturally resets with an APC watchdog.
>
> [  519.142279][ T7917] lwis lwis-eeprom-m24c64x: Can't suspend because eeprom-m24c64x is in use!
> [  519.143556][ T7917] lwis-i2c eeprom@2: PM: dpm_run_callback(): platform_pm_suspend returns -16
> [  519.143872][ T7917] lwis-i2c eeprom@2: PM: platform_pm_suspend returned -16 after 1596 usecs
> [  519.144197][ T7917] lwis-i2c eeprom@2: PM: failed to suspend: error -16
> [  519.144448][ T7917] PM: tudor: dpm_suspend: after while loop, list_empty(&dpm_prepared_list)? 1
> [  519.144779][ T7917] PM: tudor: dpm_suspend: before async_synchronize_full
>
> The extra prints are because of:
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index d9d4fc58bc5a..3efe538c2ec2 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1967,10 +1967,15 @@ int dpm_suspend(pm_message_t state)
>                         break;
>                 }
>         }
> +       pr_err("tudor: %s: after while loop, list_empty(&dpm_prepared_list)? %d\n",
> +              __func__, list_empty(&dpm_prepared_list));
>
>         mutex_unlock(&dpm_list_mtx);
>
> +       pr_err("tudor: %s: before async_synchronize_full\n", __func__);
>         async_synchronize_full();
> +       pr_err("tudor: %s: after async_synchronize_full();\n", __func__);
> +
>         if (!error)
>                 error = async_error;
>
> The synchronous suspend works because its strict, one-by-one ordering
> ensures that device dependencies are met and that no device is suspended
> while another is still using it. The asynchronous suspend fails because
> it creates a race condition where the lwis-eeprom-m24c64x is called for
> suspension before the process using it has been suspended, leading to a
> fatal "device busy" error. Should the failure of a device suspend be
> fatal?

It shouldn't in principle, but it depends on what exactly is involved and how.

It looks like something is blocking on power.completion somewhere.
I'll check the code, maybe a complete() is missing in an error path or
similar.

Thanks!

Powered by blists - more mailing lists