[PATCH v1 0/3] PM: sleep: Fixes related to aborted suspend handling

[PATCH v1 0/3] PM: sleep: Fixes related to aborted suspend handling

[Rafael J. Wysocki]

Hi Everyone,

Recently merged updates of the PM core have introduced three issues that
manifest themselves when system suspend is aborted and the patches in this
series fix them.

Please refer to the patch changelogs for details.

Many thanks to Chris Bainbridge for reporting the issues and testing the
fixes.

Thanks!

[PATCH v1 1/3] PM: sleep: Fix list splicing in device suspend error paths

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commits aa7a9275ab81 ("PM: sleep: Suspend async parents after suspending
children") and 443046d1ad66 ("PM: sleep: Make suspend of devices more
asynchronous") added list splicing to the error paths of dpm_suspend(),
dpm_suspend_late(), and dpm_noirq_suspend_devices(), but they should
have used the list_splice_init() variant because the emptied list is
used going forward in all of these cases.

Replace list_splice() with list_splice_init() in the code in question as
appropriate.

Fixes: aa7a9275ab81 ("PM: sleep: Suspend async parents after suspending children")
Fixes: 443046d1ad66 ("PM: sleep: Make suspend of devices more asynchronous")
Reported-and-tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1458,7 +1458,7 @@
 			 * Move all devices to the target list to resume them
 			 * properly.
 			 */
-			list_splice(&dpm_late_early_list, &dpm_noirq_list);
+			list_splice_init(&dpm_late_early_list, &dpm_noirq_list);
 			break;
 		}
 	}
@@ -1660,7 +1660,7 @@
 			 * Move all devices to the target list to resume them
 			 * properly.
 			 */
-			list_splice(&dpm_suspended_list, &dpm_late_early_list);
+			list_splice_init(&dpm_suspended_list, &dpm_late_early_list);
 			break;
 		}
 	}
@@ -1953,7 +1953,7 @@
 			 * Move all devices to the target list to resume them
 			 * properly.
 			 */
-			list_splice(&dpm_prepared_list, &dpm_suspended_list);
+			list_splice_init(&dpm_prepared_list, &dpm_suspended_list);
 			break;
 		}
 	}

[PATCH v1 2/3] PM: sleep: Fix power.is_suspended cleanup for direct-complete devices

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit 03f1444016b7 ("PM: sleep: Fix handling devices with direct_complete
set on errors") caused power.is_suspended to be set for devices with
power.direct_complete set, but it forgot to ensure the clearing of that
flag for them in device_resume(), so power.is_suspended is still set for
them during the next system suspend-resume cycle.

If that cycle is aborted in dpm_suspend(), the subsequent invocation of
dpm_resume() will trigger a device_resume() call for every device and
because power.is_suspended is set for the devices in question, they will
not be skipped by device_resume() as expected which causes scary error
messages to be logged (as appropriate).

To address this issue, move the clearing of power.is_suspended in
device_resume() immediately after the power.is_suspended check so it
will be always cleared for all devices processed by that function.

Fixes: 03f1444016b7 ("PM: sleep: Fix handling devices with direct_complete set on errors")
Reported-and-tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -992,6 +992,8 @@
 	if (!dev->power.is_suspended)
 		goto Complete;
 
+	dev->power.is_suspended = false;
+
 	if (dev->power.direct_complete) {
 		/*
 		 * Allow new children to be added under the device after this
@@ -1054,7 +1056,6 @@
 
  End:
 	error = dpm_run_callback(callback, dev, state, info);
-	dev->power.is_suspended = false;
 
 	device_unlock(dev);
 	dpm_watchdog_clear(&wd);

[PATCH v1 3/3] PM: sleep: Add locking to dpm_async_resume_children()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Commit 0cbef962ce1f ("PM: sleep: Resume children after resuming the
parent") introduced a subtle concurrency issue that may lead to a kernel
crash if system suspend is aborted and may also slow down asynchronous
device resume otherwise.

Namely, the initial list walks in dpm_noirq_resume_devices(),
dpm_resume_early(), and dpm_resume() call dpm_clear_async_state() for
every device and attepmt to asynchronously resume it if it has no
children (so it is a "root" device).  The asynchronous resume of a
root device triggers an attempt to asynchronously resume its children
which may take place before calling dpm_clear_async_state() for them
due to the lack of synchronization between dpm_async_resume_children()
and the code calling dpm_clear_async_state().  If this happens, the
dpm_clear_async_state() that comes in late, will clear
power.work_in_progress for the given device after it has been set by
__dpm_async(), so the suspend callback will be allowed to run once
again for the same device during the same transition.  This leads to
a whole range of interesting breakage.

Fortunately, if the suspend transition is not aborted, power.work_in_progress
is set by it for all devices, so dpm_async_resume_children() will not
schedule asynchronous resume for them until dpm_clear_async_state()
clears that flag, but this means missing an opportunity to start the
resume of those devices earlier.

Address the above issue by adding dpm_list_mtx locking to
dpm_async_resume_children(), so it will wait for the entire initial
list walk and the invocation of dpm_clear_async_state() for all devices
to be completed before scheduling any new asynchronous resume callbacks.

Fixes: 0cbef962ce1f ("PM: sleep: Resume children after resuming the parent")
Reported-and-tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |    7 +++++++
 1 file changed, 7 insertions(+)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -638,6 +638,13 @@
 static void dpm_async_resume_children(struct device *dev, async_func_t func)
 {
 	/*
+	 * Prevent racing with dpm_clear_async_state() during initial list
+	 * walks in dpm_noirq_resume_devices(), dpm_resume_early(), and
+	 * dpm_resume().
+	 */
+	guard(mutex)(&dpm_list_mtx);
+
+	/*
 	 * Start processing "async" children of the device unless it's been
 	 * started already for them.
 	 *

Re: [PATCH v1 3/3] PM: sleep: Add locking to dpm_async_resume_children()

[Limonciello, Mario]

On 6/3/25 11:21 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Commit 0cbef962ce1f ("PM: sleep: Resume children after resuming the
> parent") introduced a subtle concurrency issue that may lead to a kernel
> crash if system suspend is aborted and may also slow down asynchronous
> device resume otherwise.
> 
> Namely, the initial list walks in dpm_noirq_resume_devices(),
> dpm_resume_early(), and dpm_resume() call dpm_clear_async_state() for
> every device and attepmt to asynchronously resume it if it has no

s/attepmt/attempts/

> children (so it is a "root" device).  The asynchronous resume of a
> root device triggers an attempt to asynchronously resume its children
> which may take place before calling dpm_clear_async_state() for them
> due to the lack of synchronization between dpm_async_resume_children()
> and the code calling dpm_clear_async_state().  If this happens, the
> dpm_clear_async_state() that comes in late, will clear
> power.work_in_progress for the given device after it has been set by
> __dpm_async(), so the suspend callback will be allowed to run once
> again for the same device during the same transition.  This leads to
> a whole range of interesting breakage.
> 
> Fortunately, if the suspend transition is not aborted, power.work_in_progress
> is set by it for all devices, so dpm_async_resume_children() will not
> schedule asynchronous resume for them until dpm_clear_async_state()
> clears that flag, but this means missing an opportunity to start the
> resume of those devices earlier.
> 
> Address the above issue by adding dpm_list_mtx locking to
> dpm_async_resume_children(), so it will wait for the entire initial
> list walk and the invocation of dpm_clear_async_state() for all devices
> to be completed before scheduling any new asynchronous resume callbacks.
> 
> Fixes: 0cbef962ce1f ("PM: sleep: Resume children after resuming the parent")
> Reported-and-tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>   drivers/base/power/main.c |    7 +++++++
>   1 file changed, 7 insertions(+)
> 
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -638,6 +638,13 @@
>   static void dpm_async_resume_children(struct device *dev, async_func_t func)
>   {
>   	/*
> +	 * Prevent racing with dpm_clear_async_state() during initial list
> +	 * walks in dpm_noirq_resume_devices(), dpm_resume_early(), and
> +	 * dpm_resume().
> +	 */
> +	guard(mutex)(&dpm_list_mtx);
> +
> +	/*
>   	 * Start processing "async" children of the device unless it's been
>   	 * started already for them.
>   	 *
> 
> 
> 

Re: [PATCH v1 3/3] PM: sleep: Add locking to dpm_async_resume_children()

[Rafael J. Wysocki]

On Tue, Jun 3, 2025 at 6:27 PM Limonciello, Mario
<Mario.Limonciello@amd.com> wrote:
>
> On 6/3/25 11:21 AM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Commit 0cbef962ce1f ("PM: sleep: Resume children after resuming the
> > parent") introduced a subtle concurrency issue that may lead to a kernel
> > crash if system suspend is aborted and may also slow down asynchronous
> > device resume otherwise.
> >
> > Namely, the initial list walks in dpm_noirq_resume_devices(),
> > dpm_resume_early(), and dpm_resume() call dpm_clear_async_state() for
> > every device and attepmt to asynchronously resume it if it has no
>
> s/attepmt/attempts/

It is plural, but yes, s/attepmt/attempt/

Will fix when applying, thanks!

Re: [PATCH v1 0/3] PM: sleep: Fixes related to aborted suspend handling

[Limonciello, Mario]

On 6/3/25 11:16 AM, Rafael J. Wysocki wrote:
> Hi Everyone,
> 
> Recently merged updates of the PM core have introduced three issues that
> manifest themselves when system suspend is aborted and the patches in this
> series fix them.
> 
> Please refer to the patch changelogs for details.
> 
> Many thanks to Chris Bainbridge for reporting the issues and testing the
> fixes.
> 
> Thanks!
> 
> 
> 

I found one typo in a commit message in the series, but otherwise it 
looks good.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

Here's a tag to include for closing the issue that this started at as well.

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4280