[PATCH v1 0/5] PM: sleep: Improvements of async suspend and resume of devices

[PATCH v1 0/5] PM: sleep: Improvements of async suspend and resume of devices

[Rafael J. Wysocki]

Hi Everyone,

Initially, this was an attempt to address the problems described by
Saravana related to spawning async work for any async device upfront
in the resume path:

https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/

but then I realized that it could be extended to the suspend path and
used for speeding it up, which it really does.

Overall, the idea is that instead of starting an async work item for every
async device upfront, which is not very efficient because the majority of
those devices will not be able to make progress due to dependencies anyway,
the async handling is only started upfront for the devices that are likely
to be able to make progress.  That is, devices without parents in the resume
path and leaf devices (ie. devices without children or consumers) in the
suspend path (the underlying observation here is that devices without parents
are likely to have no suppliers too whereas devices without children that
have consumers are not unheard of).  This allows to reduce the amount of
processing that needs to be done to start with.

Then, after processing every device ("async" or "sync"), "async" processing
is started for some devices that have been "unblocked" by it, which are its
children in the resume path or its parent and its suppliers in the suspend
path.  This allows asynchronous handling to start as soon as it makes sense
without delaying the "async" devices unnecessarily.

Fortunately, the additional plumbing needed to implement this is not
particularly complicated.

The first two patches in the series are preparatory.

Patch [3/5] deals with the resume path for all device resume phases.

Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
the systems in my office the speedup is in the 100 ms range which is around 20%
of the total device resume time).

Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.

Thanks!

[PATCH v1 1/5] PM: sleep: Rename power.async_in_progress to power.work_in_progress

[Rafael J. Wysocki]

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

Rename the async_in_progress field in struct dev_pm_info to
work_in_progress as after subsequent changes it will mean work in
general rather than just async work.

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |   12 ++++++------
 include/linux/pm.h        |    2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -602,7 +602,7 @@
 	reinit_completion(&dev->power.completion);
 
 	if (is_async(dev)) {
-		dev->power.async_in_progress = true;
+		dev->power.work_in_progress = true;
 
 		get_device(dev);
 
@@ -614,9 +614,9 @@
 	/*
 	 * Because async_schedule_dev_nocall() above has returned false or it
 	 * has not been called at all, func() is not running and it is safe to
-	 * update the async_in_progress flag without extra synchronization.
+	 * update the work_in_progress flag without extra synchronization.
 	 */
-	dev->power.async_in_progress = false;
+	dev->power.work_in_progress = false;
 	return false;
 }
 
@@ -736,7 +736,7 @@
 		dev = to_device(dpm_noirq_list.next);
 		list_move_tail(&dev->power.entry, &dpm_late_early_list);
 
-		if (!dev->power.async_in_progress) {
+		if (!dev->power.work_in_progress) {
 			get_device(dev);
 
 			mutex_unlock(&dpm_list_mtx);
@@ -876,7 +876,7 @@
 		dev = to_device(dpm_late_early_list.next);
 		list_move_tail(&dev->power.entry, &dpm_suspended_list);
 
-		if (!dev->power.async_in_progress) {
+		if (!dev->power.work_in_progress) {
 			get_device(dev);
 
 			mutex_unlock(&dpm_list_mtx);
@@ -1042,7 +1042,7 @@
 		dev = to_device(dpm_suspended_list.next);
 		list_move_tail(&dev->power.entry, &dpm_prepared_list);
 
-		if (!dev->power.async_in_progress) {
+		if (!dev->power.work_in_progress) {
 			get_device(dev);
 
 			mutex_unlock(&dpm_list_mtx);
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -679,7 +679,7 @@
 	bool			wakeup_path:1;
 	bool			syscore:1;
 	bool			no_pm_callbacks:1;	/* Owned by the PM core */
-	bool			async_in_progress:1;	/* Owned by the PM core */
+	bool			work_in_progress:1;	/* Owned by the PM core */
 	bool			smart_suspend:1;	/* Owned by the PM core */
 	bool			must_resume:1;		/* Owned by the PM core */
 	bool			may_skip_resume:1;	/* Set by subsystems */

[PATCH v1 2/5] PM: sleep: Rearrange dpm_async_fn() and async state clearing

[Rafael J. Wysocki]

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

In preparation for subsequent changes, move the power.completion
reinitialization along with clearing power.work_in_progress into a
separate function called dpm_clear_async_state() and rearrange
dpm_async_fn() to get rid of unnecessary indentation.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |   44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -599,27 +599,34 @@
 
 static bool dpm_async_fn(struct device *dev, async_func_t func)
 {
-	reinit_completion(&dev->power.completion);
+	if (!is_async(dev))
+		return false;
 
-	if (is_async(dev)) {
-		dev->power.work_in_progress = true;
+	dev->power.work_in_progress = true;
 
-		get_device(dev);
+	get_device(dev);
 
-		if (async_schedule_dev_nocall(func, dev))
-			return true;
+	if (async_schedule_dev_nocall(func, dev))
+		return true;
+
+	put_device(dev);
 
-		put_device(dev);
-	}
 	/*
-	 * Because async_schedule_dev_nocall() above has returned false or it
-	 * has not been called at all, func() is not running and it is safe to
-	 * update the work_in_progress flag without extra synchronization.
+	 * async_schedule_dev_nocall() above has returned false, so func() is
+	 * not running and it is safe to update power.work_in_progress without
+	 * extra synchronization.
 	 */
 	dev->power.work_in_progress = false;
+
 	return false;
 }
 
+static void dpm_clear_async_state(struct device *dev)
+{
+	reinit_completion(&dev->power.completion);
+	dev->power.work_in_progress = false;
+}
+
 /**
  * device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
@@ -729,8 +736,10 @@
 	 * 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_noirq_list, power.entry)
+	list_for_each_entry(dev, &dpm_noirq_list, power.entry) {
+		dpm_clear_async_state(dev);
 		dpm_async_fn(dev, async_resume_noirq);
+	}
 
 	while (!list_empty(&dpm_noirq_list)) {
 		dev = to_device(dpm_noirq_list.next);
@@ -869,8 +878,10 @@
 	 * 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)
+	list_for_each_entry(dev, &dpm_late_early_list, power.entry) {
+		dpm_clear_async_state(dev);
 		dpm_async_fn(dev, async_resume_early);
+	}
 
 	while (!list_empty(&dpm_late_early_list)) {
 		dev = to_device(dpm_late_early_list.next);
@@ -1035,8 +1046,10 @@
 	 * 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_suspended_list, power.entry)
+	list_for_each_entry(dev, &dpm_suspended_list, power.entry) {
+		dpm_clear_async_state(dev);
 		dpm_async_fn(dev, async_resume);
+	}
 
 	while (!list_empty(&dpm_suspended_list)) {
 		dev = to_device(dpm_suspended_list.next);
@@ -1314,6 +1327,7 @@
 
 		list_move(&dev->power.entry, &dpm_noirq_list);
 
+		dpm_clear_async_state(dev);
 		if (dpm_async_fn(dev, async_suspend_noirq))
 			continue;
 
@@ -1491,6 +1505,7 @@
 
 		list_move(&dev->power.entry, &dpm_late_early_list);
 
+		dpm_clear_async_state(dev);
 		if (dpm_async_fn(dev, async_suspend_late))
 			continue;
 
@@ -1758,6 +1773,7 @@
 
 		list_move(&dev->power.entry, &dpm_suspended_list);
 
+		dpm_clear_async_state(dev);
 		if (dpm_async_fn(dev, async_suspend))
 			continue;
 

[PATCH v1 3/5] PM: sleep: Resume children right after resuming the parent

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.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]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |   72 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 9 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -621,12 +621,41 @@
 	return false;
 }
 
+static int dpm_async_unless_in_progress(struct device *dev, void *fn)
+{
+	async_func_t func = fn;
+
+	if (!dev->power.work_in_progress)
+		dpm_async_fn(dev, func);
+
+	return 0;
+}
+
+static void dpm_async_resume_children(struct device *dev, async_func_t func)
+{
+	mutex_lock(&dpm_list_mtx);
+
+	/*
+	 * Start processing "async" children of the device unless it's been
+	 * started already for them.
+	 *
+	 * This could have been done for the device's "async" consumers too, but
+	 * they either need to wait for their parents or the processing has
+	 * already started for them after their parents were processed.
+	 */
+	device_for_each_child(dev, func, dpm_async_unless_in_progress);
+
+	mutex_unlock(&dpm_list_mtx);
+}
+
 static void dpm_clear_async_state(struct device *dev)
 {
 	reinit_completion(&dev->power.completion);
 	dev->power.work_in_progress = false;
 }
 
+static void async_resume_noirq(void *data, async_cookie_t cookie);
+
 /**
  * device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
@@ -710,6 +739,8 @@
 		dpm_save_failed_dev(dev_name(dev));
 		pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
 	}
+
+	dpm_async_resume_children(dev, async_resume_noirq);
 }
 
 static void async_resume_noirq(void *data, async_cookie_t cookie)
@@ -733,19 +764,24 @@
 	mutex_lock(&dpm_list_mtx);
 
 	/*
-	 * Trigger the resume of "async" devices upfront so they don't have to
-	 * wait for the "non-async" ones they don't depend on.
+	 * Start processing "async" devices without parents upfront so they
+	 * don't wait for the "sync" devices they don't depend on.
 	 */
 	list_for_each_entry(dev, &dpm_noirq_list, power.entry) {
 		dpm_clear_async_state(dev);
-		dpm_async_fn(dev, async_resume_noirq);
+		if (!dev->parent)
+			dpm_async_fn(dev, async_resume_noirq);
 	}
 
 	while (!list_empty(&dpm_noirq_list)) {
 		dev = to_device(dpm_noirq_list.next);
 		list_move_tail(&dev->power.entry, &dpm_late_early_list);
 
+		dpm_async_unless_in_progress(dev, async_resume_noirq);
+
 		if (!dev->power.work_in_progress) {
+			dev->power.work_in_progress = true;
+
 			get_device(dev);
 
 			mutex_unlock(&dpm_list_mtx);
@@ -781,6 +817,8 @@
 	device_wakeup_disarm_wake_irqs();
 }
 
+static void async_resume_early(void *data, async_cookie_t cookie);
+
 /**
  * device_resume_early - Execute an "early resume" callback for given device.
  * @dev: Device to handle.
@@ -848,6 +886,8 @@
 		dpm_save_failed_dev(dev_name(dev));
 		pm_dev_err(dev, state, async ? " async early" : " early", error);
 	}
+
+	dpm_async_resume_children(dev, async_resume_early);
 }
 
 static void async_resume_early(void *data, async_cookie_t cookie)
@@ -875,19 +915,24 @@
 	mutex_lock(&dpm_list_mtx);
 
 	/*
-	 * Trigger the resume of "async" devices upfront so they don't have to
-	 * wait for the "non-async" ones they don't depend on.
+	 * Start processing "async" devices without parents upfront so they
+	 * don't wait for the "sync" devices they don't depend on.
 	 */
 	list_for_each_entry(dev, &dpm_late_early_list, power.entry) {
 		dpm_clear_async_state(dev);
-		dpm_async_fn(dev, async_resume_early);
+		if (!dev->parent)
+			dpm_async_fn(dev, async_resume_early);
 	}
 
 	while (!list_empty(&dpm_late_early_list)) {
 		dev = to_device(dpm_late_early_list.next);
 		list_move_tail(&dev->power.entry, &dpm_suspended_list);
 
+		dpm_async_unless_in_progress(dev, async_resume_early);
+
 		if (!dev->power.work_in_progress) {
+			dev->power.work_in_progress = true;
+
 			get_device(dev);
 
 			mutex_unlock(&dpm_list_mtx);
@@ -919,6 +964,8 @@
 }
 EXPORT_SYMBOL_GPL(dpm_resume_start);
 
+static void async_resume(void *data, async_cookie_t cookie);
+
 /**
  * device_resume - Execute "resume" callbacks for given device.
  * @dev: Device to handle.
@@ -1012,6 +1059,8 @@
 		dpm_save_failed_dev(dev_name(dev));
 		pm_dev_err(dev, state, async ? " async" : "", error);
 	}
+
+	dpm_async_resume_children(dev, async_resume);
 }
 
 static void async_resume(void *data, async_cookie_t cookie)
@@ -1043,19 +1092,24 @@
 	mutex_lock(&dpm_list_mtx);
 
 	/*
-	 * Trigger the resume of "async" devices upfront so they don't have to
-	 * wait for the "non-async" ones they don't depend on.
+	 * Start processing "async" devices without parents upfront so they
+	 * don't wait for the "sync" devices they don't depend on.
 	 */
 	list_for_each_entry(dev, &dpm_suspended_list, power.entry) {
 		dpm_clear_async_state(dev);
-		dpm_async_fn(dev, async_resume);
+		if (!dev->parent)
+			dpm_async_fn(dev, async_resume);
 	}
 
 	while (!list_empty(&dpm_suspended_list)) {
 		dev = to_device(dpm_suspended_list.next);
 		list_move_tail(&dev->power.entry, &dpm_prepared_list);
 
+		dpm_async_unless_in_progress(dev, async_resume);
+
 		if (!dev->power.work_in_progress) {
+			dev->power.work_in_progress = true;
+
 			get_device(dev);
 
 			mutex_unlock(&dpm_list_mtx);

Re: [PATCH v1 3/5] PM: sleep: Resume children right after resuming the parent

[Saravana Kannan]

Sorry for the delay, I wanted to double, triple, multiple check my
replies to make sure I didn't get it wrong. I hope I didn't.

On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.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]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/main.c |   72 ++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 9 deletions(-)
>
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -621,12 +621,41 @@
>         return false;
>  }
>
> +static int dpm_async_unless_in_progress(struct device *dev, void *fn)
> +{
> +       async_func_t func = fn;
> +
> +       if (!dev->power.work_in_progress)
> +               dpm_async_fn(dev, func);
> +
> +       return 0;
> +}
> +
> +static void dpm_async_resume_children(struct device *dev, async_func_t func)
> +{
> +       mutex_lock(&dpm_list_mtx);
> +
> +       /*
> +        * Start processing "async" children of the device unless it's been
> +        * started already for them.
> +        *
> +        * This could have been done for the device's "async" consumers too, but
> +        * they either need to wait for their parents or the processing has
> +        * already started for them after their parents were processed.
> +        */
> +       device_for_each_child(dev, func, dpm_async_unless_in_progress);

Is there a reason you aren't resuming the consumers? Or checking that
the children won't block on any suppliers?

Not dealing with device links might be ok for systems where there
aren't a lot of device links, but in DT based systems with fw_devlink
this patch will not make much of a difference. There'll still be a ton
of "sleep and try again" issues because of the supplier/consumer
dependencies. And when you include device links, it's a dependency
graph and no longer a dependency tree. So things become a bit more
complicated.

Also, not taking device links isn't consideration when kicking off
async work is not just about additional sleep/wake cycles, but it'll
also cause more thread creation because blocked "async" worker threads
will quickly use up the worker thread pool and more threads need to be
created.

> +
> +       mutex_unlock(&dpm_list_mtx);
> +}
> +
>  static void dpm_clear_async_state(struct device *dev)
>  {
>         reinit_completion(&dev->power.completion);
>         dev->power.work_in_progress = false;
>  }
>
> +static void async_resume_noirq(void *data, async_cookie_t cookie);
> +
>  /**
>   * device_resume_noirq - Execute a "noirq resume" callback for given device.
>   * @dev: Device to handle.
> @@ -710,6 +739,8 @@
>                 dpm_save_failed_dev(dev_name(dev));
>                 pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
>         }
> +
> +       dpm_async_resume_children(dev, async_resume_noirq);

Similar comment here.

>  }
>
>  static void async_resume_noirq(void *data, async_cookie_t cookie)
> @@ -733,19 +764,24 @@
>         mutex_lock(&dpm_list_mtx);
>
>         /*
> -        * Trigger the resume of "async" devices upfront so they don't have to
> -        * wait for the "non-async" ones they don't depend on.
> +        * Start processing "async" devices without parents upfront so they
> +        * don't wait for the "sync" devices they don't depend on.
>          */
>         list_for_each_entry(dev, &dpm_noirq_list, power.entry) {
>                 dpm_clear_async_state(dev);
> -               dpm_async_fn(dev, async_resume_noirq);
> +               if (!dev->parent)
> +                       dpm_async_fn(dev, async_resume_noirq);
>         }
>
>         while (!list_empty(&dpm_noirq_list)) {
>                 dev = to_device(dpm_noirq_list.next);
>                 list_move_tail(&dev->power.entry, &dpm_late_early_list);
>
> +               dpm_async_unless_in_progress(dev, async_resume_noirq);
> +
>                 if (!dev->power.work_in_progress) {
> +                       dev->power.work_in_progress = true;
> +
>                         get_device(dev);
>
>                         mutex_unlock(&dpm_list_mtx);
> @@ -781,6 +817,8 @@
>         device_wakeup_disarm_wake_irqs();
>  }
>
> +static void async_resume_early(void *data, async_cookie_t cookie);
> +
>  /**
>   * device_resume_early - Execute an "early resume" callback for given device.
>   * @dev: Device to handle.
> @@ -848,6 +886,8 @@
>                 dpm_save_failed_dev(dev_name(dev));
>                 pm_dev_err(dev, state, async ? " async early" : " early", error);
>         }
> +
> +       dpm_async_resume_children(dev, async_resume_early);

Similar comment here.

>  }
>
>  static void async_resume_early(void *data, async_cookie_t cookie)
> @@ -875,19 +915,24 @@
>         mutex_lock(&dpm_list_mtx);
>
>         /*
> -        * Trigger the resume of "async" devices upfront so they don't have to
> -        * wait for the "non-async" ones they don't depend on.
> +        * Start processing "async" devices without parents upfront so they
> +        * don't wait for the "sync" devices they don't depend on.
>          */
>         list_for_each_entry(dev, &dpm_late_early_list, power.entry) {
>                 dpm_clear_async_state(dev);

I initially thought that there could be a race here, but it's not the
case because you are using the dpm_list_mtx here. However, since you
are grabbing the dpm_list_mtx lock before you loop through and kick
off the async threads for child devices it'll cause a lot of
unnecessary lock contention/waiting. Which is what this series is
trying to avoid in the first place.

If you take device links into consideration (which we need to do for
this patch series to improve suspend/resume for DT based systems),
there is an even higher chance of racing (or more lock contention)
because multiple threads might attempt to resume the same device.
Which is why in my patch series, I try to use the device links read
lock than using the single dpm_list_mtx mutex.

Thanks,
Saravana

> -               dpm_async_fn(dev, async_resume_early);
> +               if (!dev->parent)
> +                       dpm_async_fn(dev, async_resume_early);
>         }
>
>         while (!list_empty(&dpm_late_early_list)) {
>                 dev = to_device(dpm_late_early_list.next);
>                 list_move_tail(&dev->power.entry, &dpm_suspended_list);
>
> +               dpm_async_unless_in_progress(dev, async_resume_early);
> +
>                 if (!dev->power.work_in_progress) {
> +                       dev->power.work_in_progress = true;
> +
>                         get_device(dev);
>
>                         mutex_unlock(&dpm_list_mtx);
> @@ -919,6 +964,8 @@
>  }
>  EXPORT_SYMBOL_GPL(dpm_resume_start);
>
> +static void async_resume(void *data, async_cookie_t cookie);
> +
>  /**
>   * device_resume - Execute "resume" callbacks for given device.
>   * @dev: Device to handle.
> @@ -1012,6 +1059,8 @@
>                 dpm_save_failed_dev(dev_name(dev));
>                 pm_dev_err(dev, state, async ? " async" : "", error);
>         }
> +
> +       dpm_async_resume_children(dev, async_resume);
>  }
>
>  static void async_resume(void *data, async_cookie_t cookie)
> @@ -1043,19 +1092,24 @@
>         mutex_lock(&dpm_list_mtx);
>
>         /*
> -        * Trigger the resume of "async" devices upfront so they don't have to
> -        * wait for the "non-async" ones they don't depend on.
> +        * Start processing "async" devices without parents upfront so they
> +        * don't wait for the "sync" devices they don't depend on.
>          */
>         list_for_each_entry(dev, &dpm_suspended_list, power.entry) {
>                 dpm_clear_async_state(dev);
> -               dpm_async_fn(dev, async_resume);
> +               if (!dev->parent)
> +                       dpm_async_fn(dev, async_resume);
>         }
>
>         while (!list_empty(&dpm_suspended_list)) {
>                 dev = to_device(dpm_suspended_list.next);
>                 list_move_tail(&dev->power.entry, &dpm_prepared_list);
>
> +               dpm_async_unless_in_progress(dev, async_resume);
> +
>                 if (!dev->power.work_in_progress) {
> +                       dev->power.work_in_progress = true;
> +
>                         get_device(dev);
>
>                         mutex_unlock(&dpm_list_mtx);
>
>
>

Re: [PATCH v1 3/5] PM: sleep: Resume children right after resuming the parent

[Rafael J. Wysocki]

On Thu, Mar 13, 2025 at 2:47 AM Saravana Kannan <saravanak@google.com> wrote:
>
> Sorry for the delay, I wanted to double, triple, multiple check my
> replies to make sure I didn't get it wrong. I hope I didn't.
>
> On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.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]
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/main.c |   72 ++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 63 insertions(+), 9 deletions(-)
> >
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -621,12 +621,41 @@
> >         return false;
> >  }
> >
> > +static int dpm_async_unless_in_progress(struct device *dev, void *fn)
> > +{
> > +       async_func_t func = fn;
> > +
> > +       if (!dev->power.work_in_progress)
> > +               dpm_async_fn(dev, func);
> > +
> > +       return 0;
> > +}
> > +
> > +static void dpm_async_resume_children(struct device *dev, async_func_t func)
> > +{
> > +       mutex_lock(&dpm_list_mtx);
> > +
> > +       /*
> > +        * Start processing "async" children of the device unless it's been
> > +        * started already for them.
> > +        *
> > +        * This could have been done for the device's "async" consumers too, but
> > +        * they either need to wait for their parents or the processing has
> > +        * already started for them after their parents were processed.
> > +        */
> > +       device_for_each_child(dev, func, dpm_async_unless_in_progress);
>
> Is there a reason you aren't resuming the consumers? Or checking that
> the children won't block on any suppliers?

This is deliberate and the above comment is about it.

At this point, all of the consumers either still need to wait for
their parents, in which case it is not useful to queue them up, or
their parent's have already completed the resume and started their
processing, if they are async.

Checking if children won't block can be added here, but then (a) the
code will need to track their dependencies and (b) walking the
consumers would become necessary which is extra overhead.

Arguably, there are no devices with tons of children, so adding
several async work items that will just wait until they get unblocked
here should not add too much overhead.

> Not dealing with device links might be ok for systems where there
> aren't a lot of device links, but in DT based systems with fw_devlink
> this patch will not make much of a difference. There'll still be a ton
> of "sleep and try again" issues because of the supplier/consumer
> dependencies.

That IMO really needs to be evaluated because it all depends on how
much it takes to resume individual devices.

> And when you include device links, it's a dependency graph and no longer
> a dependency tree. So things become a bit more complicated.

They are, but all I said above still holds I believe.

> Also, not taking device links isn't consideration when kicking off
> async work is not just about additional sleep/wake cycles, but it'll
> also cause more thread creation because blocked "async" worker threads
> will quickly use up the worker thread pool and more threads need to be
> created.

Again, this needs to be evaluated and an actual system.  It would have
been the case if devices had taken no time to actually resume, but it
is far from reality AFAICS.

[skip]

> >  }
> >
> >  static void async_resume_early(void *data, async_cookie_t cookie)
> > @@ -875,19 +915,24 @@
> >         mutex_lock(&dpm_list_mtx);
> >
> >         /*
> > -        * Trigger the resume of "async" devices upfront so they don't have to
> > -        * wait for the "non-async" ones they don't depend on.
> > +        * Start processing "async" devices without parents upfront so they
> > +        * don't wait for the "sync" devices they don't depend on.
> >          */
> >         list_for_each_entry(dev, &dpm_late_early_list, power.entry) {
> >                 dpm_clear_async_state(dev);
>
> I initially thought that there could be a race here, but it's not the
> case because you are using the dpm_list_mtx here. However, since you
> are grabbing the dpm_list_mtx lock before you loop through and kick
> off the async threads for child devices it'll cause a lot of
> unnecessary lock contention/waiting.

I don't think this is what happens here.  It is only processing the
devices without parents which are not too many.

> Which is what this series is trying to avoid in the first place.
>
> If you take device links into consideration (which we need to do for
> this patch series to improve suspend/resume for DT based systems),
> there is an even higher chance of racing (or more lock contention)
> because multiple threads might attempt to resume the same device.
> Which is why in my patch series, I try to use the device links read
> lock than using the single dpm_list_mtx mutex.

I think that you are talking about acquiring dpm_list_mtx in
dpm_async_resume_children().

The reason for acquiring it there is dpm_async_unless_in_progress()
and particularly the power.work_in_progress check in it and the
invocation of dpm_async_fn() which manipulates power.work_in_progress.

If dpm_list_mtx is too contentious for this, the device's power.lock
can be used for protecting the power.work_in_progress accesses in
principle, but that would mean more complexity in the code.

This is unrelated to whether or not device links are walked in
dpm_async_resume_children() (well, if they were, it would need to be
renamed), though.

[PATCH v1 4/5] PM: sleep: Start suspending parents and suppliers after subordinate suspend

[Rafael J. Wysocki]

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

In analogy with the previous change affecting the resume path,
make device_suspend() start the async suspend of the device's parent
and suppliers after the device itself has been processed and make
dpm_suspend() start processing "async" leaf devices (that is, devices
without children or consumers) upfront because they don't need to wait
for any other devices.

On the Dell XPS13 9360 in my office, this change reduces the total
duration of device suspend by approximately 100 ms (over 20%).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |   73 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 69 insertions(+), 4 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1237,6 +1237,49 @@
 
 /*------------------------- Suspend routines -------------------------*/
 
+static bool dpm_leaf_device(struct device *dev)
+{
+	struct device *child;
+
+	lockdep_assert_held(&dpm_list_mtx);
+
+	child = device_find_any_child(dev);
+	if (child) {
+		put_device(child);
+
+		return false;
+	}
+
+	/*
+	 * Since this function is required to run under dpm_list_mtx, the
+	 * list_empty() below will only return true if the device's list of
+	 * consumers is actually empty before calling it.
+	 */
+	return list_empty(&dev->links.consumers);
+}
+
+static void dpm_async_suspend_superior(struct device *dev, async_func_t func)
+{
+	struct device_link *link;
+
+	mutex_lock(&dpm_list_mtx);
+
+	/* Start processing the device's parent if it is "async". */
+	if (dev->parent)
+		dpm_async_unless_in_progress(dev->parent, func);
+
+	/*
+	 * Start processing the device's "async" suppliers.
+	 *
+	 * The dpm_list_mtx locking is sufficient for this.
+	 */
+	list_for_each_entry_rcu(link, &dev->links.consumers, s_node)
+		if (READ_ONCE(link->status) != DL_STATE_DORMANT)
+			dpm_async_unless_in_progress(link->consumer, func);
+
+	mutex_unlock(&dpm_list_mtx);
+}
+
 /**
  * resume_event - Return a "resume" message for given "suspend" sleep state.
  * @sleep_state: PM message representing a sleep state.
@@ -1663,6 +1706,8 @@
 	device_links_read_unlock(idx);
 }
 
+static void async_suspend(void *data, async_cookie_t cookie);
+
 /**
  * device_suspend - Execute "suspend" callbacks for given device.
  * @dev: Device to handle.
@@ -1791,7 +1836,13 @@
 
 	complete_all(&dev->power.completion);
 	TRACE_SUSPEND(error);
-	return error;
+
+	if (error || async_error)
+		return error;
+
+	dpm_async_suspend_superior(dev, async_suspend);
+
+	return 0;
 }
 
 static void async_suspend(void *data, async_cookie_t cookie)
@@ -1809,6 +1860,7 @@
 int dpm_suspend(pm_message_t state)
 {
 	ktime_t starttime = ktime_get();
+	struct device *dev;
 	int error = 0;
 
 	trace_suspend_resume(TPS("dpm_suspend"), state.event, true);
@@ -1822,15 +1874,28 @@
 
 	mutex_lock(&dpm_list_mtx);
 
+	/*
+	 * Start processing "async" leaf devices upfront because they don't need
+	 * to wait.
+	 */
+	list_for_each_entry_reverse(dev, &dpm_prepared_list, power.entry) {
+		dpm_clear_async_state(dev);
+		if (dpm_leaf_device(dev))
+			dpm_async_fn(dev, async_suspend);
+	}
+
 	while (!list_empty(&dpm_prepared_list)) {
-		struct device *dev = to_device(dpm_prepared_list.prev);
+		dev = to_device(dpm_prepared_list.prev);
 
 		list_move(&dev->power.entry, &dpm_suspended_list);
 
-		dpm_clear_async_state(dev);
-		if (dpm_async_fn(dev, async_suspend))
+		dpm_async_unless_in_progress(dev, async_suspend);
+
+		if (dev->power.work_in_progress)
 			continue;
 
+		dev->power.work_in_progress = true;
+
 		get_device(dev);
 
 		mutex_unlock(&dpm_list_mtx);

Re: [PATCH v1 4/5] PM: sleep: Start suspending parents and suppliers after subordinate suspend

[Saravana Kannan]

On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In analogy with the previous change affecting the resume path,
> make device_suspend() start the async suspend of the device's parent
> and suppliers after the device itself has been processed and make
> dpm_suspend() start processing "async" leaf devices (that is, devices
> without children or consumers) upfront because they don't need to wait
> for any other devices.
>
> On the Dell XPS13 9360 in my office, this change reduces the total
> duration of device suspend by approximately 100 ms (over 20%).
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/main.c |   73 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 69 insertions(+), 4 deletions(-)
>
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1237,6 +1237,49 @@
>
>  /*------------------------- Suspend routines -------------------------*/
>
> +static bool dpm_leaf_device(struct device *dev)
> +{
> +       struct device *child;
> +
> +       lockdep_assert_held(&dpm_list_mtx);
> +
> +       child = device_find_any_child(dev);
> +       if (child) {
> +               put_device(child);
> +
> +               return false;
> +       }
> +
> +       /*
> +        * Since this function is required to run under dpm_list_mtx, the
> +        * list_empty() below will only return true if the device's list of
> +        * consumers is actually empty before calling it.
> +        */
> +       return list_empty(&dev->links.consumers);
> +}
> +
> +static void dpm_async_suspend_superior(struct device *dev, async_func_t func)
> +{
> +       struct device_link *link;
> +
> +       mutex_lock(&dpm_list_mtx);
> +
> +       /* Start processing the device's parent if it is "async". */
> +       if (dev->parent)
> +               dpm_async_unless_in_progress(dev->parent, func);
> +
> +       /*
> +        * Start processing the device's "async" suppliers.
> +        *
> +        * The dpm_list_mtx locking is sufficient for this.
> +        */

Why is dpm_list_mtx sufficient? Is it because you are assuming no
driver is trying to change the device links during suspend/resume? Or
is there some other reason? That sounds a bit risky. Is it because if
you do, you'll hit a AB-BA deadlock or at least a lockdep warning?
Also, if we can use the device links read locks, we won't block the
other readers -- so, less contention.

> +       list_for_each_entry_rcu(link, &dev->links.consumers, s_node)
> +               if (READ_ONCE(link->status) != DL_STATE_DORMANT)
> +                       dpm_async_unless_in_progress(link->consumer, func);

This will still queue a lot of devices that can't suspend yet.

Curious, how many devices do you have in the system where you are testing this?

-Saravana

> +
> +       mutex_unlock(&dpm_list_mtx);
> +}
> +
>  /**
>   * resume_event - Return a "resume" message for given "suspend" sleep state.
>   * @sleep_state: PM message representing a sleep state.
> @@ -1663,6 +1706,8 @@
>         device_links_read_unlock(idx);
>  }
>
> +static void async_suspend(void *data, async_cookie_t cookie);
> +
>  /**
>   * device_suspend - Execute "suspend" callbacks for given device.
>   * @dev: Device to handle.
> @@ -1791,7 +1836,13 @@
>
>         complete_all(&dev->power.completion);
>         TRACE_SUSPEND(error);
> -       return error;
> +
> +       if (error || async_error)
> +               return error;
> +
> +       dpm_async_suspend_superior(dev, async_suspend);
> +
> +       return 0;
>  }
>
>  static void async_suspend(void *data, async_cookie_t cookie)
> @@ -1809,6 +1860,7 @@
>  int dpm_suspend(pm_message_t state)
>  {
>         ktime_t starttime = ktime_get();
> +       struct device *dev;
>         int error = 0;
>
>         trace_suspend_resume(TPS("dpm_suspend"), state.event, true);
> @@ -1822,15 +1874,28 @@
>
>         mutex_lock(&dpm_list_mtx);
>
> +       /*
> +        * Start processing "async" leaf devices upfront because they don't need
> +        * to wait.
> +        */
> +       list_for_each_entry_reverse(dev, &dpm_prepared_list, power.entry) {
> +               dpm_clear_async_state(dev);
> +               if (dpm_leaf_device(dev))
> +                       dpm_async_fn(dev, async_suspend);
> +       }
> +
>         while (!list_empty(&dpm_prepared_list)) {
> -               struct device *dev = to_device(dpm_prepared_list.prev);
> +               dev = to_device(dpm_prepared_list.prev);
>
>                 list_move(&dev->power.entry, &dpm_suspended_list);
>
> -               dpm_clear_async_state(dev);
> -               if (dpm_async_fn(dev, async_suspend))
> +               dpm_async_unless_in_progress(dev, async_suspend);
> +
> +               if (dev->power.work_in_progress)
>                         continue;
>
> +               dev->power.work_in_progress = true;
> +
>                 get_device(dev);
>
>                 mutex_unlock(&dpm_list_mtx);
>
>
>

Re: [PATCH v1 4/5] PM: sleep: Start suspending parents and suppliers after subordinate suspend

[Rafael J. Wysocki]

On Thu, Mar 13, 2025 at 2:47 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > In analogy with the previous change affecting the resume path,
> > make device_suspend() start the async suspend of the device's parent
> > and suppliers after the device itself has been processed and make
> > dpm_suspend() start processing "async" leaf devices (that is, devices
> > without children or consumers) upfront because they don't need to wait
> > for any other devices.
> >
> > On the Dell XPS13 9360 in my office, this change reduces the total
> > duration of device suspend by approximately 100 ms (over 20%).
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/main.c |   73 +++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 69 insertions(+), 4 deletions(-)
> >
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -1237,6 +1237,49 @@
> >
> >  /*------------------------- Suspend routines -------------------------*/
> >
> > +static bool dpm_leaf_device(struct device *dev)
> > +{
> > +       struct device *child;
> > +
> > +       lockdep_assert_held(&dpm_list_mtx);
> > +
> > +       child = device_find_any_child(dev);
> > +       if (child) {
> > +               put_device(child);
> > +
> > +               return false;
> > +       }
> > +
> > +       /*
> > +        * Since this function is required to run under dpm_list_mtx, the
> > +        * list_empty() below will only return true if the device's list of
> > +        * consumers is actually empty before calling it.
> > +        */
> > +       return list_empty(&dev->links.consumers);
> > +}
> > +
> > +static void dpm_async_suspend_superior(struct device *dev, async_func_t func)
> > +{
> > +       struct device_link *link;
> > +
> > +       mutex_lock(&dpm_list_mtx);
> > +
> > +       /* Start processing the device's parent if it is "async". */
> > +       if (dev->parent)
> > +               dpm_async_unless_in_progress(dev->parent, func);
> > +
> > +       /*
> > +        * Start processing the device's "async" suppliers.
> > +        *
> > +        * The dpm_list_mtx locking is sufficient for this.
> > +        */
>
> Why is dpm_list_mtx sufficient? Is it because you are assuming no
> driver is trying to change the device links during suspend/resume? Or
> is there some other reason?

dpm_list_mtx is acquired in device_link_add(), so no new links can be
added while this code is running, and list_del_rcu() is safe with
respect to list_for_each_entry_rcu() according to its kerneldoc
comment.

Worst case it will start async processing for a device that is going
away which should be handled cleanly.

> That sounds a bit risky. Is it because if
> you do, you'll hit a AB-BA deadlock or at least a lockdep warning?
> Also, if we can use the device links read locks, we won't block the
> other readers -- so, less contention.

Readers are not blocked regardless, writers are.

> > +       list_for_each_entry_rcu(link, &dev->links.consumers, s_node)
> > +               if (READ_ONCE(link->status) != DL_STATE_DORMANT)
> > +                       dpm_async_unless_in_progress(link->consumer, func);

Oh, the above is actually broken.  It should be

list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node)
        if (READ_ONCE(link->status) != DL_STATE_DORMANT)
                dpm_async_unless_in_progress(link->supplier, func);

shouldn't it?

I need to fix this.

> This will still queue a lot of devices that can't suspend yet.

I'm not sure what you mean by "a lot"?  This is only going to queue
the suppliers of this particular device.  How many of those could be
there?

> Curious, how many devices do you have in the system where you are testing this?

Around 1500 device objects, and the majority of them have parents and
children, but there are only a few device links.

Thanks!

[PATCH v1 5/5] PM: sleep: Make late and noirq suspend of devices more asynchronous

[Rafael J. Wysocki]

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

In analogy with previous changes, make device_suspend_late() and
device_suspend_noirq() start the async suspend of the device's parent
and suppliers after the device itself has been processed and make
dpm_suspend_late() and dpm_noirq_suspend_devices() start processing
"async" leaf devices (that is, devices without children or consumers)
upfront because they don't need to wait for any other devices.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |   60 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 52 insertions(+), 8 deletions(-)

--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1317,6 +1317,8 @@
 	device_links_read_unlock(idx);
 }
 
+static void async_suspend_noirq(void *data, async_cookie_t cookie);
+
 /**
  * device_suspend_noirq - Execute a "noirq suspend" callback for given device.
  * @dev: Device to handle.
@@ -1396,7 +1398,13 @@
 Complete:
 	complete_all(&dev->power.completion);
 	TRACE_SUSPEND(error);
-	return error;
+
+	if (error || async_error)
+		return error;
+
+	dpm_async_suspend_superior(dev, async_suspend_noirq);
+
+	return 0;
 }
 
 static void async_suspend_noirq(void *data, async_cookie_t cookie)
@@ -1410,6 +1418,7 @@
 static int dpm_noirq_suspend_devices(pm_message_t state)
 {
 	ktime_t starttime = ktime_get();
+	struct device *dev;
 	int error = 0;
 
 	trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, true);
@@ -1419,15 +1428,28 @@
 
 	mutex_lock(&dpm_list_mtx);
 
+	/*
+	 * Start processing "async" leaf devices upfront because they don't need
+	 * to wait.
+	 */
+	list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry) {
+		dpm_clear_async_state(dev);
+		if (dpm_leaf_device(dev))
+			dpm_async_fn(dev, async_suspend_noirq);
+	}
+
 	while (!list_empty(&dpm_late_early_list)) {
-		struct device *dev = to_device(dpm_late_early_list.prev);
+		dev = to_device(dpm_late_early_list.prev);
 
 		list_move(&dev->power.entry, &dpm_noirq_list);
 
-		dpm_clear_async_state(dev);
-		if (dpm_async_fn(dev, async_suspend_noirq))
+		dpm_async_unless_in_progress(dev, async_suspend_noirq);
+
+		if (dev->power.work_in_progress)
 			continue;
 
+		dev->power.work_in_progress = true;
+
 		get_device(dev);
 
 		mutex_unlock(&dpm_list_mtx);
@@ -1492,6 +1514,8 @@
 	spin_unlock_irq(&parent->power.lock);
 }
 
+static void async_suspend_late(void *data, async_cookie_t cookie);
+
 /**
  * device_suspend_late - Execute a "late suspend" callback for given device.
  * @dev: Device to handle.
@@ -1568,7 +1592,13 @@
 Complete:
 	TRACE_SUSPEND(error);
 	complete_all(&dev->power.completion);
-	return error;
+
+	if (error || async_error)
+		return error;
+
+	dpm_async_suspend_superior(dev, async_suspend_late);
+
+	return 0;
 }
 
 static void async_suspend_late(void *data, async_cookie_t cookie)
@@ -1586,6 +1616,7 @@
 int dpm_suspend_late(pm_message_t state)
 {
 	ktime_t starttime = ktime_get();
+	struct device *dev;
 	int error = 0;
 
 	trace_suspend_resume(TPS("dpm_suspend_late"), state.event, true);
@@ -1597,15 +1628,28 @@
 
 	mutex_lock(&dpm_list_mtx);
 
+	/*
+	 * Start processing "async" leaf devices upfront because they don't need
+	 * to wait.
+	 */
+	list_for_each_entry_reverse(dev, &dpm_suspended_list, power.entry) {
+		dpm_clear_async_state(dev);
+		if (dpm_leaf_device(dev))
+			dpm_async_fn(dev, async_suspend_late);
+	}
+
 	while (!list_empty(&dpm_suspended_list)) {
-		struct device *dev = to_device(dpm_suspended_list.prev);
+		dev = to_device(dpm_suspended_list.prev);
 
 		list_move(&dev->power.entry, &dpm_late_early_list);
 
-		dpm_clear_async_state(dev);
-		if (dpm_async_fn(dev, async_suspend_late))
+		dpm_async_unless_in_progress(dev, async_suspend_late);
+
+		if (dev->power.work_in_progress)
 			continue;
 
+		dev->power.work_in_progress = true;
+
 		get_device(dev);
 
 		mutex_unlock(&dpm_list_mtx);

Re: [PATCH v1 5/5] PM: sleep: Make late and noirq suspend of devices more asynchronous

[Saravana Kannan]

On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> In analogy with previous changes, make device_suspend_late() and
> device_suspend_noirq() start the async suspend of the device's parent
> and suppliers after the device itself has been processed and make
> dpm_suspend_late() and dpm_noirq_suspend_devices() start processing
> "async" leaf devices (that is, devices without children or consumers)
> upfront because they don't need to wait for any other devices.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/main.c |   60 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 52 insertions(+), 8 deletions(-)
>
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1317,6 +1317,8 @@
>         device_links_read_unlock(idx);
>  }
>
> +static void async_suspend_noirq(void *data, async_cookie_t cookie);
> +
>  /**
>   * device_suspend_noirq - Execute a "noirq suspend" callback for given device.
>   * @dev: Device to handle.
> @@ -1396,7 +1398,13 @@
>  Complete:
>         complete_all(&dev->power.completion);
>         TRACE_SUSPEND(error);
> -       return error;
> +
> +       if (error || async_error)
> +               return error;
> +
> +       dpm_async_suspend_superior(dev, async_suspend_noirq);
> +
> +       return 0;
>  }
>
>  static void async_suspend_noirq(void *data, async_cookie_t cookie)
> @@ -1410,6 +1418,7 @@
>  static int dpm_noirq_suspend_devices(pm_message_t state)
>  {
>         ktime_t starttime = ktime_get();
> +       struct device *dev;
>         int error = 0;
>
>         trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, true);
> @@ -1419,15 +1428,28 @@
>
>         mutex_lock(&dpm_list_mtx);
>
> +       /*
> +        * Start processing "async" leaf devices upfront because they don't need
> +        * to wait.
> +        */
> +       list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry) {
> +               dpm_clear_async_state(dev);
> +               if (dpm_leaf_device(dev))
> +                       dpm_async_fn(dev, async_suspend_noirq);
> +       }
> +
>         while (!list_empty(&dpm_late_early_list)) {
> -               struct device *dev = to_device(dpm_late_early_list.prev);
> +               dev = to_device(dpm_late_early_list.prev);
>
>                 list_move(&dev->power.entry, &dpm_noirq_list);

This issue is present in the previous patch too, but it's easier for
me to point it out here. Suspend abort will break now.

For example, say the devices are suspended in the order A -> B -> C ->
D -> E if everything was sync.

Case 1: Fully sync devices
If C aborts, only A and B will be in the dpm_noirq_list. When we try
to undo the suspend, we just resume devices in dpm_noirq_list and that
just resumes A and B.

Case 2: Only C is sync.
When C aborts, A, B, D and E could have finished suspending. But only
A and B will be in the dpm_noirq_list. When we try to undo the
suspend, we just resume devices in dpm_noirq_list and that just
resumes A and B. D and E never get resumed.

My fix for this is to move all devices to dpm_noirq_list if a suspend
aborts and then using the existing
is_suspended/is_noirq_suspended/is_late_suspended flags to skip over
devices that haven't been suspended. That works nicely except in
is_suspended and I tried to fix it in [2]. But you had an issue with
[2] that I didn't fully understand and I meant to dig deeper and fix.
But I didn't get around to it as I got swamped with other work.

[2] - https://lore.kernel.org/linux-pm/20241114220921.2529905-2-saravanak@google.com/

Thanks,
Saravana



>
> -               dpm_clear_async_state(dev);
> -               if (dpm_async_fn(dev, async_suspend_noirq))
> +               dpm_async_unless_in_progress(dev, async_suspend_noirq);
> +
> +               if (dev->power.work_in_progress)
>                         continue;
>
> +               dev->power.work_in_progress = true;
> +
>                 get_device(dev);
>
>                 mutex_unlock(&dpm_list_mtx);
> @@ -1492,6 +1514,8 @@
>         spin_unlock_irq(&parent->power.lock);
>  }
>
> +static void async_suspend_late(void *data, async_cookie_t cookie);
> +
>  /**
>   * device_suspend_late - Execute a "late suspend" callback for given device.
>   * @dev: Device to handle.
> @@ -1568,7 +1592,13 @@
>  Complete:
>         TRACE_SUSPEND(error);
>         complete_all(&dev->power.completion);
> -       return error;
> +
> +       if (error || async_error)
> +               return error;
> +
> +       dpm_async_suspend_superior(dev, async_suspend_late);
> +
> +       return 0;
>  }
>
>  static void async_suspend_late(void *data, async_cookie_t cookie)
> @@ -1586,6 +1616,7 @@
>  int dpm_suspend_late(pm_message_t state)
>  {
>         ktime_t starttime = ktime_get();
> +       struct device *dev;
>         int error = 0;
>
>         trace_suspend_resume(TPS("dpm_suspend_late"), state.event, true);
> @@ -1597,15 +1628,28 @@
>
>         mutex_lock(&dpm_list_mtx);
>
> +       /*
> +        * Start processing "async" leaf devices upfront because they don't need
> +        * to wait.
> +        */
> +       list_for_each_entry_reverse(dev, &dpm_suspended_list, power.entry) {
> +               dpm_clear_async_state(dev);
> +               if (dpm_leaf_device(dev))
> +                       dpm_async_fn(dev, async_suspend_late);
> +       }
> +
>         while (!list_empty(&dpm_suspended_list)) {
> -               struct device *dev = to_device(dpm_suspended_list.prev);
> +               dev = to_device(dpm_suspended_list.prev);
>
>                 list_move(&dev->power.entry, &dpm_late_early_list);
>
> -               dpm_clear_async_state(dev);
> -               if (dpm_async_fn(dev, async_suspend_late))
> +               dpm_async_unless_in_progress(dev, async_suspend_late);
> +
> +               if (dev->power.work_in_progress)
>                         continue;
>
> +               dev->power.work_in_progress = true;
> +
>                 get_device(dev);
>
>                 mutex_unlock(&dpm_list_mtx);
>
>
>

Re: [PATCH v1 5/5] PM: sleep: Make late and noirq suspend of devices more asynchronous

[Rafael J. Wysocki]

On Thu, Mar 13, 2025 at 2:47 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > In analogy with previous changes, make device_suspend_late() and
> > device_suspend_noirq() start the async suspend of the device's parent
> > and suppliers after the device itself has been processed and make
> > dpm_suspend_late() and dpm_noirq_suspend_devices() start processing
> > "async" leaf devices (that is, devices without children or consumers)
> > upfront because they don't need to wait for any other devices.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/main.c |   60 +++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 52 insertions(+), 8 deletions(-)
> >
> > --- a/drivers/base/power/main.c
> > +++ b/drivers/base/power/main.c
> > @@ -1317,6 +1317,8 @@
> >         device_links_read_unlock(idx);
> >  }
> >
> > +static void async_suspend_noirq(void *data, async_cookie_t cookie);
> > +
> >  /**
> >   * device_suspend_noirq - Execute a "noirq suspend" callback for given device.
> >   * @dev: Device to handle.
> > @@ -1396,7 +1398,13 @@
> >  Complete:
> >         complete_all(&dev->power.completion);
> >         TRACE_SUSPEND(error);
> > -       return error;
> > +
> > +       if (error || async_error)
> > +               return error;
> > +
> > +       dpm_async_suspend_superior(dev, async_suspend_noirq);
> > +
> > +       return 0;
> >  }
> >
> >  static void async_suspend_noirq(void *data, async_cookie_t cookie)
> > @@ -1410,6 +1418,7 @@
> >  static int dpm_noirq_suspend_devices(pm_message_t state)
> >  {
> >         ktime_t starttime = ktime_get();
> > +       struct device *dev;
> >         int error = 0;
> >
> >         trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, true);
> > @@ -1419,15 +1428,28 @@
> >
> >         mutex_lock(&dpm_list_mtx);
> >
> > +       /*
> > +        * Start processing "async" leaf devices upfront because they don't need
> > +        * to wait.
> > +        */
> > +       list_for_each_entry_reverse(dev, &dpm_late_early_list, power.entry) {
> > +               dpm_clear_async_state(dev);
> > +               if (dpm_leaf_device(dev))
> > +                       dpm_async_fn(dev, async_suspend_noirq);
> > +       }
> > +
> >         while (!list_empty(&dpm_late_early_list)) {
> > -               struct device *dev = to_device(dpm_late_early_list.prev);
> > +               dev = to_device(dpm_late_early_list.prev);
> >
> >                 list_move(&dev->power.entry, &dpm_noirq_list);
>
> This issue is present in the previous patch too, but it's easier for
> me to point it out here. Suspend abort will break now.
>
> For example, say the devices are suspended in the order A -> B -> C ->
> D -> E if everything was sync.
>
> Case 1: Fully sync devices
> If C aborts, only A and B will be in the dpm_noirq_list. When we try
> to undo the suspend, we just resume devices in dpm_noirq_list and that
> just resumes A and B.
>
> Case 2: Only C is sync.
> When C aborts, A, B, D and E could have finished suspending. But only
> A and B will be in the dpm_noirq_list. When we try to undo the
> suspend, we just resume devices in dpm_noirq_list and that just
> resumes A and B. D and E never get resumed.
>
> My fix for this is to move all devices to dpm_noirq_list if a suspend
> aborts and then using the existing
> is_suspended/is_noirq_suspended/is_late_suspended flags to skip over
> devices that haven't been suspended. That works nicely except in
> is_suspended and I tried to fix it in [2]. But you had an issue with
> [2] that I didn't fully understand and I meant to dig deeper and fix.
> But I didn't get around to it as I got swamped with other work.
>
> [2] - https://lore.kernel.org/linux-pm/20241114220921.2529905-2-saravanak@google.com/

I hope that my last message in this thread clarifies this a bit.

Anyway, yes, this is a bug and yes, it can be addressed by just
continuing to move devices to the target list on errors until all of
them get moved and then look at
is_suspended/is_noirq_suspended/is_late_suspended.

I'll post a fix for the direct-complete handling in device_suspend()
and an update of the last three patches in this series with this issue
addressed.

Re: [PATCH v1 0/5] PM: sleep: Improvements of async suspend and resume of devices

[Saravana Kannan]

On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Everyone,
>
> Initially, this was an attempt to address the problems described by
> Saravana related to spawning async work for any async device upfront
> in the resume path:
>
> https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
>
> but then I realized that it could be extended to the suspend path and
> used for speeding it up, which it really does.

Btw, maybe I didn't  word it correctly, but my patch series was meant
to speed up the non-async case too.

I was going to get around sending a v2 of my series, but was caught up
with some other work. But I'm okay if you want to finish up my effort
-- less work for me and I can focus on the other aspects of suspend :)

Maybe add a Suggested-by: to the patches?

I definitely want to review the series, but very busy this week with
some other work. I'll get to this next week for sure.

Thanks,
Saravana

>
> Overall, the idea is that instead of starting an async work item for every
> async device upfront, which is not very efficient because the majority of
> those devices will not be able to make progress due to dependencies anyway,
> the async handling is only started upfront for the devices that are likely
> to be able to make progress.  That is, devices without parents in the resume
> path and leaf devices (ie. devices without children or consumers) in the
> suspend path (the underlying observation here is that devices without parents
> are likely to have no suppliers too whereas devices without children that
> have consumers are not unheard of).  This allows to reduce the amount of
> processing that needs to be done to start with.
>
> Then, after processing every device ("async" or "sync"), "async" processing
> is started for some devices that have been "unblocked" by it, which are its
> children in the resume path or its parent and its suppliers in the suspend
> path.  This allows asynchronous handling to start as soon as it makes sense
> without delaying the "async" devices unnecessarily.
>
> Fortunately, the additional plumbing needed to implement this is not
> particularly complicated.
>
> The first two patches in the series are preparatory.
>
> Patch [3/5] deals with the resume path for all device resume phases.
>
> Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
> the systems in my office the speedup is in the 100 ms range which is around 20%
> of the total device resume time).
>
> Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.
>
> Thanks!
>
>
>

Re: [PATCH v1 0/5] PM: sleep: Improvements of async suspend and resume of devices

[Rafael J. Wysocki]

On Thu, Feb 27, 2025 at 4:45 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > Hi Everyone,
> >
> > Initially, this was an attempt to address the problems described by
> > Saravana related to spawning async work for any async device upfront
> > in the resume path:
> >
> > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> >
> > but then I realized that it could be extended to the suspend path and
> > used for speeding it up, which it really does.
>
> Btw, maybe I didn't  word it correctly, but my patch series was meant
> to speed up the non-async case too.

If "the non-async case" means the case with "async" suspend/resume
disabled entirely, I don't think that the ordering in which devices
are processed can be changed just because there are no known
dependencies.

> I was going to get around sending a v2 of my series, but was caught up
> with some other work. But I'm okay if you want to finish up my effort
> -- less work for me and I can focus on the other aspects of suspend :)
>
> Maybe add a Suggested-by: to the patches?

Yeah, I can do that.

> I definitely want to review the series, but very busy this week with
> some other work. I'll get to this next week for sure.

That should be fine.

> > Overall, the idea is that instead of starting an async work item for every
> > async device upfront, which is not very efficient because the majority of
> > those devices will not be able to make progress due to dependencies anyway,
> > the async handling is only started upfront for the devices that are likely
> > to be able to make progress.  That is, devices without parents in the resume
> > path and leaf devices (ie. devices without children or consumers) in the
> > suspend path (the underlying observation here is that devices without parents
> > are likely to have no suppliers too whereas devices without children that
> > have consumers are not unheard of).  This allows to reduce the amount of
> > processing that needs to be done to start with.
> >
> > Then, after processing every device ("async" or "sync"), "async" processing
> > is started for some devices that have been "unblocked" by it, which are its
> > children in the resume path or its parent and its suppliers in the suspend
> > path.  This allows asynchronous handling to start as soon as it makes sense
> > without delaying the "async" devices unnecessarily.
> >
> > Fortunately, the additional plumbing needed to implement this is not
> > particularly complicated.
> >
> > The first two patches in the series are preparatory.
> >
> > Patch [3/5] deals with the resume path for all device resume phases.
> >
> > Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
> > the systems in my office the speedup is in the 100 ms range which is around 20%
> > of the total device resume time).
> >
> > Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.
> >
> > Thanks!

Re: [PATCH v1 0/5] PM: sleep: Improvements of async suspend and resume of devices

[Saravana Kannan]

On Thu, Feb 27, 2025 at 8:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 27, 2025 at 4:45 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > Hi Everyone,
> > >
> > > Initially, this was an attempt to address the problems described by
> > > Saravana related to spawning async work for any async device upfront
> > > in the resume path:
> > >
> > > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> > >
> > > but then I realized that it could be extended to the suspend path and
> > > used for speeding it up, which it really does.
> >
> > Btw, maybe I didn't  word it correctly, but my patch series was meant
> > to speed up the non-async case too.
>
> If "the non-async case" means the case with "async" suspend/resume
> disabled entirely, I don't think that the ordering in which devices
> are processed can be changed just because there are no known
> dependencies.
>
> > I was going to get around sending a v2 of my series, but was caught up
> > with some other work. But I'm okay if you want to finish up my effort
> > -- less work for me and I can focus on the other aspects of suspend :)
> >
> > Maybe add a Suggested-by: to the patches?
>
> Yeah, I can do that.
>
> > I definitely want to review the series, but very busy this week with
> > some other work. I'll get to this next week for sure.
>
> That should be fine.

Hi Rafael,

I looked at the full series and it has at least one bug and a few gaps
that I address in mine. And those are what make my patches have a
higher diff. Can we just continue with my series instead? That'll be
easier to explain and more forward for me than reviewing your patch
and making sure it covers everything I already tried to deal with.

You partially reviewed my patches, if you can give me more details on
my patch 1 and what else you want me to do for the rest of them, I'd
be happy to do that.

Thanks,
Saravana

>
> > > Overall, the idea is that instead of starting an async work item for every
> > > async device upfront, which is not very efficient because the majority of
> > > those devices will not be able to make progress due to dependencies anyway,
> > > the async handling is only started upfront for the devices that are likely
> > > to be able to make progress.  That is, devices without parents in the resume
> > > path and leaf devices (ie. devices without children or consumers) in the
> > > suspend path (the underlying observation here is that devices without parents
> > > are likely to have no suppliers too whereas devices without children that
> > > have consumers are not unheard of).  This allows to reduce the amount of
> > > processing that needs to be done to start with.
> > >
> > > Then, after processing every device ("async" or "sync"), "async" processing
> > > is started for some devices that have been "unblocked" by it, which are its
> > > children in the resume path or its parent and its suppliers in the suspend
> > > path.  This allows asynchronous handling to start as soon as it makes sense
> > > without delaying the "async" devices unnecessarily.
> > >
> > > Fortunately, the additional plumbing needed to implement this is not
> > > particularly complicated.
> > >
> > > The first two patches in the series are preparatory.
> > >
> > > Patch [3/5] deals with the resume path for all device resume phases.
> > >
> > > Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
> > > the systems in my office the speedup is in the 100 ms range which is around 20%
> > > of the total device resume time).
> > >
> > > Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.
> > >
> > > Thanks!

Re: [PATCH v1 0/5] PM: sleep: Improvements of async suspend and resume of devices

[Rafael J. Wysocki]

On Sun, Mar 9, 2025 at 11:38 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Thu, Feb 27, 2025 at 8:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Feb 27, 2025 at 4:45 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > Hi Everyone,
> > > >
> > > > Initially, this was an attempt to address the problems described by
> > > > Saravana related to spawning async work for any async device upfront
> > > > in the resume path:
> > > >
> > > > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> > > >
> > > > but then I realized that it could be extended to the suspend path and
> > > > used for speeding it up, which it really does.
> > >
> > > Btw, maybe I didn't  word it correctly, but my patch series was meant
> > > to speed up the non-async case too.
> >
> > If "the non-async case" means the case with "async" suspend/resume
> > disabled entirely, I don't think that the ordering in which devices
> > are processed can be changed just because there are no known
> > dependencies.
> >
> > > I was going to get around sending a v2 of my series, but was caught up
> > > with some other work. But I'm okay if you want to finish up my effort
> > > -- less work for me and I can focus on the other aspects of suspend :)
> > >
> > > Maybe add a Suggested-by: to the patches?
> >
> > Yeah, I can do that.
> >
> > > I definitely want to review the series, but very busy this week with
> > > some other work. I'll get to this next week for sure.
> >
> > That should be fine.
>
> Hi Rafael,
>
> I looked at the full series and it has at least one bug and a few gaps
> that I address in mine.

What bug?

You need to tell me specifically because I'm not aware of any bugs in
this series and unless you tell me what it is and I agree that it is a
bug, I have no reason to believe that there are any.

As for the gaps, there are obvious differences between this patch
series and your work and it would be kind of nice to explain why they
matter in practice, in your view.

> And those are what make my patches have a
> higher diff. Can we just continue with my series instead?

Of course you are free to send a new version of it, but it is unlikely
to be a sufficient replacement for constructive feedback.

Re: [PATCH v1 0/5] PM: sleep: Improvements of async suspend and resume of devices

[Saravana Kannan]

On Mon, Mar 10, 2025 at 9:01 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Sun, Mar 9, 2025 at 11:38 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Thu, Feb 27, 2025 at 8:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Feb 27, 2025 at 4:45 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > >
> > > > > Hi Everyone,
> > > > >
> > > > > Initially, this was an attempt to address the problems described by
> > > > > Saravana related to spawning async work for any async device upfront
> > > > > in the resume path:
> > > > >
> > > > > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> > > > >
> > > > > but then I realized that it could be extended to the suspend path and
> > > > > used for speeding it up, which it really does.
> > > >
> > > > Btw, maybe I didn't  word it correctly, but my patch series was meant
> > > > to speed up the non-async case too.
> > >
> > > If "the non-async case" means the case with "async" suspend/resume
> > > disabled entirely, I don't think that the ordering in which devices
> > > are processed can be changed just because there are no known
> > > dependencies.
> > >
> > > > I was going to get around sending a v2 of my series, but was caught up
> > > > with some other work. But I'm okay if you want to finish up my effort
> > > > -- less work for me and I can focus on the other aspects of suspend :)
> > > >
> > > > Maybe add a Suggested-by: to the patches?
> > >
> > > Yeah, I can do that.
> > >
> > > > I definitely want to review the series, but very busy this week with
> > > > some other work. I'll get to this next week for sure.
> > >
> > > That should be fine.
> >
> > Hi Rafael,
> >
> > I looked at the full series and it has at least one bug and a few gaps
> > that I address in mine.
>
> What bug?
>
> You need to tell me specifically because I'm not aware of any bugs in
> this series and unless you tell me what it is and I agree that it is a
> bug, I have no reason to believe that there are any.
>
> As for the gaps, there are obvious differences between this patch
> series and your work and it would be kind of nice to explain why they
> matter in practice, in your view.

Sure, I'll do this. But it just felt like an inefficient way to get to
close to where my series is. Instead of you just saying you don't like
about my series and giving me some feedback on how to fix it.

> > And those are what make my patches have a
> > higher diff. Can we just continue with my series instead?
>
> Of course you are free to send a new version of it, but it is unlikely
> to be a sufficient replacement for constructive feedback.

Ok, I'll point out the issues I see in this series and hopefully you
can point out the issues in my series and we can move forward with
mine if you agree with the additional issues my series is working
through.

Thanks,
Saravana

Re: [PATCH v1 0/5] PM: sleep: Improvements of async suspend and resume of devices

[Rafael J. Wysocki]

On Mon, Mar 10, 2025 at 9:31 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Mar 10, 2025 at 9:01 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Sun, Mar 9, 2025 at 11:38 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Thu, Feb 27, 2025 at 8:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Thu, Feb 27, 2025 at 4:45 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > On Tue, Feb 25, 2025 at 8:46 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > >
> > > > > > Hi Everyone,
> > > > > >
> > > > > > Initially, this was an attempt to address the problems described by
> > > > > > Saravana related to spawning async work for any async device upfront
> > > > > > in the resume path:
> > > > > >
> > > > > > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> > > > > >
> > > > > > but then I realized that it could be extended to the suspend path and
> > > > > > used for speeding it up, which it really does.
> > > > >
> > > > > Btw, maybe I didn't  word it correctly, but my patch series was meant
> > > > > to speed up the non-async case too.
> > > >
> > > > If "the non-async case" means the case with "async" suspend/resume
> > > > disabled entirely, I don't think that the ordering in which devices
> > > > are processed can be changed just because there are no known
> > > > dependencies.
> > > >
> > > > > I was going to get around sending a v2 of my series, but was caught up
> > > > > with some other work. But I'm okay if you want to finish up my effort
> > > > > -- less work for me and I can focus on the other aspects of suspend :)
> > > > >
> > > > > Maybe add a Suggested-by: to the patches?
> > > >
> > > > Yeah, I can do that.
> > > >
> > > > > I definitely want to review the series, but very busy this week with
> > > > > some other work. I'll get to this next week for sure.
> > > >
> > > > That should be fine.
> > >
> > > Hi Rafael,
> > >
> > > I looked at the full series and it has at least one bug and a few gaps
> > > that I address in mine.
> >
> > What bug?
> >
> > You need to tell me specifically because I'm not aware of any bugs in
> > this series and unless you tell me what it is and I agree that it is a
> > bug, I have no reason to believe that there are any.
> >
> > As for the gaps, there are obvious differences between this patch
> > series and your work and it would be kind of nice to explain why they
> > matter in practice, in your view.
>
> Sure, I'll do this.

OK

> But it just felt like an inefficient way to get to close to where my series is.

I'm not sure where it is TBH.

> Instead of you just saying you don't like
> about my series and giving me some feedback on how to fix it.

You got feedback on it:

https://lore.kernel.org/linux-pm/CAJZ5v0grG7eSJ7_c73i9-bXaFhm5rfE2WmxtR6yLB-MGkd7sVg@mail.gmail.com/

And no response.

Also here:

https://lore.kernel.org/linux-pm/CAJZ5v0g9A1pZ5FjPAjdLY5ybNmefnBVVMJM7h3czW38p1fTfqQ@mail.gmail.com/

And there was a bunch of feedback from other people (and 0-day) on the
last patch.

> > > And those are what make my patches have a
> > > higher diff. Can we just continue with my series instead?
> >
> > Of course you are free to send a new version of it, but it is unlikely
> > to be a sufficient replacement for constructive feedback.
>
> Ok, I'll point out the issues I see in this series and hopefully you
> can point out the issues in my series and we can move forward with
> mine if you agree with the additional issues my series is working
> through.

Sure, but please address the feedback so far and send a new version.

Re: [PATCH v1 0/5] PM: sleep: Improvements of async suspend and resume of devices

[Ulf Hansson]

On Tue, 25 Feb 2025 at 17:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi Everyone,
>
> Initially, this was an attempt to address the problems described by
> Saravana related to spawning async work for any async device upfront
> in the resume path:
>
> https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
>
> but then I realized that it could be extended to the suspend path and
> used for speeding it up, which it really does.
>
> Overall, the idea is that instead of starting an async work item for every
> async device upfront, which is not very efficient because the majority of
> those devices will not be able to make progress due to dependencies anyway,
> the async handling is only started upfront for the devices that are likely
> to be able to make progress.  That is, devices without parents in the resume
> path and leaf devices (ie. devices without children or consumers) in the
> suspend path (the underlying observation here is that devices without parents
> are likely to have no suppliers too whereas devices without children that
> have consumers are not unheard of).  This allows to reduce the amount of
> processing that needs to be done to start with.
>
> Then, after processing every device ("async" or "sync"), "async" processing
> is started for some devices that have been "unblocked" by it, which are its
> children in the resume path or its parent and its suppliers in the suspend
> path.  This allows asynchronous handling to start as soon as it makes sense
> without delaying the "async" devices unnecessarily.
>
> Fortunately, the additional plumbing needed to implement this is not
> particularly complicated.

Thanks for the detailed description! Overall, the approach makes
perfect sense to me too!

I am certainly interested to hear Saravana's thoughts around this too.

>
> The first two patches in the series are preparatory.

For these two, feel free to add:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

>
> Patch [3/5] deals with the resume path for all device resume phases.
>
> Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
> the systems in my office the speedup is in the 100 ms range which is around 20%
> of the total device resume time).
>
> Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.

I will try to have a closer look at patch 3->5 later in the week.

Kind regards
Uffe

Re: [PATCH v1 0/5] PM: sleep: Improvements of async suspend and resume of devices

[Rafael J. Wysocki]

On Mon, Mar 3, 2025 at 1:07 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Tue, 25 Feb 2025 at 17:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > Hi Everyone,
> >
> > Initially, this was an attempt to address the problems described by
> > Saravana related to spawning async work for any async device upfront
> > in the resume path:
> >
> > https://lore.kernel.org/linux-pm/20241114220921.2529905-1-saravanak@google.com/
> >
> > but then I realized that it could be extended to the suspend path and
> > used for speeding it up, which it really does.
> >
> > Overall, the idea is that instead of starting an async work item for every
> > async device upfront, which is not very efficient because the majority of
> > those devices will not be able to make progress due to dependencies anyway,
> > the async handling is only started upfront for the devices that are likely
> > to be able to make progress.  That is, devices without parents in the resume
> > path and leaf devices (ie. devices without children or consumers) in the
> > suspend path (the underlying observation here is that devices without parents
> > are likely to have no suppliers too whereas devices without children that
> > have consumers are not unheard of).  This allows to reduce the amount of
> > processing that needs to be done to start with.
> >
> > Then, after processing every device ("async" or "sync"), "async" processing
> > is started for some devices that have been "unblocked" by it, which are its
> > children in the resume path or its parent and its suppliers in the suspend
> > path.  This allows asynchronous handling to start as soon as it makes sense
> > without delaying the "async" devices unnecessarily.
> >
> > Fortunately, the additional plumbing needed to implement this is not
> > particularly complicated.
>
> Thanks for the detailed description! Overall, the approach makes
> perfect sense to me too!
>
> I am certainly interested to hear Saravana's thoughts around this too.
>
> >
> > The first two patches in the series are preparatory.
>
> For these two, feel free to add:
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> >
> > Patch [3/5] deals with the resume path for all device resume phases.
> >
> > Patch [4/5] optimizes the "suspend" phase which has the most visible effect (on
> > the systems in my office the speedup is in the 100 ms range which is around 20%
> > of the total device resume time).
> >
> > Patch [5/5] extend this to the "suspend late" and "suspend noirq" phases.
>
> I will try to have a closer look at patch 3->5 later in the week.

Thank you and thanks for all of the other reviews!