[PATCH v1 00/12] PM: sleep: Fix up suspend stats handling and clean up core code

[PATCH v1 00/12] PM: sleep: Fix up suspend stats handling and clean up core code

[Rafael J. Wysocki]

Hi Everyone,

This series of patches modifies the core system-wide suspend resume of
devices to get it more internally consistent (between the suspend and
resume parts) and fixes up the handling of suspend statistics in it.

The functional changes made by it are expected to be limited to the
statistics handling part.

Please refer to individual patch changelogs for details.

Thanks!

[PATCH v1 01/12] PM: sleep: Simplify dpm_suspended_list walk in dpm_resume()

[Rafael J. Wysocki]

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

Notice that devices can be moved to dpm_prepared_list before running
their resume callbacks, in analogy with dpm_noirq_resume_devices() and
dpm_resume_early(), because doing so will not affect the final ordering
of that list.

Namely, if a device is the first dpm_suspended_list entry while
dpm_list_mtx is held, it has not been removed so far and it cannot be
removed until dpm_list_mtx is released, so moving it to dpm_prepared_list
at that point is valid.  If it is removed later, while its resume
callback is running, it will be deleted from dpm_prepared_list without
changing the ordering of the other devices in that list.

Accordingly, rearrange the while () loop in dpm_resume() to move
devices to dpm_prepared_list before running their resume callbacks and
implify the locking and device reference counting in it.

No intentional functional impact.

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

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1017,25 +1017,19 @@ void dpm_resume(pm_message_t state)
 
 	while (!list_empty(&dpm_suspended_list)) {
 		dev = to_device(dpm_suspended_list.next);
-
-		get_device(dev);
+		list_move_tail(&dev->power.entry, &dpm_prepared_list);
 
 		if (!dev->power.async_in_progress) {
+			get_device(dev);
+
 			mutex_unlock(&dpm_list_mtx);
 
 			device_resume(dev, state, false);
 
+			put_device(dev);
+
 			mutex_lock(&dpm_list_mtx);
 		}
-
-		if (!list_empty(&dev->power.entry))
-			list_move_tail(&dev->power.entry, &dpm_prepared_list);
-
-		mutex_unlock(&dpm_list_mtx);
-
-		put_device(dev);
-
-		mutex_lock(&dpm_list_mtx);
 	}
 	mutex_unlock(&dpm_list_mtx);
 	async_synchronize_full();

Re: [PATCH v1 01/12] PM: sleep: Simplify dpm_suspended_list walk in dpm_resume()

[Stanislaw Gruszka]

On Mon, Jan 22, 2024 at 12:22:38PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Notice that devices can be moved to dpm_prepared_list before running
> their resume callbacks, in analogy with dpm_noirq_resume_devices() and
> dpm_resume_early(), because doing so will not affect the final ordering
> of that list.
> 
> Namely, if a device is the first dpm_suspended_list entry while
> dpm_list_mtx is held, it has not been removed so far and it cannot be
> removed until dpm_list_mtx is released, so moving it to dpm_prepared_list
> at that point is valid.  If it is removed later, while its resume
> callback is running, it will be deleted from dpm_prepared_list without
> changing the ordering of the other devices in that list.
> 
> Accordingly, rearrange the while () loop in dpm_resume() to move
> devices to dpm_prepared_list before running their resume callbacks and
> implify the locking and device reference counting in it.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  drivers/base/power/main.c |   16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -1017,25 +1017,19 @@ void dpm_resume(pm_message_t state)
>  
>  	while (!list_empty(&dpm_suspended_list)) {
>  		dev = to_device(dpm_suspended_list.next);
> -
> -		get_device(dev);
> +		list_move_tail(&dev->power.entry, &dpm_prepared_list);
>  
>  		if (!dev->power.async_in_progress) {
> +			get_device(dev);
> +
>  			mutex_unlock(&dpm_list_mtx);
>  
>  			device_resume(dev, state, false);
>  
> +			put_device(dev);
> +
>  			mutex_lock(&dpm_list_mtx);
>  		}
> -
> -		if (!list_empty(&dev->power.entry))
> -			list_move_tail(&dev->power.entry, &dpm_prepared_list);
> -
> -		mutex_unlock(&dpm_list_mtx);
> -
> -		put_device(dev);
> -
> -		mutex_lock(&dpm_list_mtx);
>  	}
>  	mutex_unlock(&dpm_list_mtx);
>  	async_synchronize_full();
> 
> 
> 

[PATCH v1 02/12] PM: sleep: Relocate two device PM core functions

[Rafael J. Wysocki]

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

Move is_async() and dpm_async_fn() in the PM core to a more suitable
place.

No functional impact.

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

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -578,6 +578,35 @@ bool dev_pm_skip_resume(struct device *d
 	return !dev->power.must_resume;
 }
 
+static bool is_async(struct device *dev)
+{
+	return dev->power.async_suspend && pm_async_enabled
+		&& !pm_trace_is_enabled();
+}
+
+static bool dpm_async_fn(struct device *dev, async_func_t func)
+{
+	reinit_completion(&dev->power.completion);
+
+	if (is_async(dev)) {
+		dev->power.async_in_progress = true;
+
+		get_device(dev);
+
+		if (async_schedule_dev_nocall(func, dev))
+			return true;
+
+		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 async_in_progress flag without extra synchronization.
+	 */
+	dev->power.async_in_progress = false;
+	return false;
+}
+
 /**
  * device_resume_noirq - Execute a "noirq resume" callback for given device.
  * @dev: Device to handle.
@@ -664,35 +693,6 @@ Out:
 	}
 }
 
-static bool is_async(struct device *dev)
-{
-	return dev->power.async_suspend && pm_async_enabled
-		&& !pm_trace_is_enabled();
-}
-
-static bool dpm_async_fn(struct device *dev, async_func_t func)
-{
-	reinit_completion(&dev->power.completion);
-
-	if (is_async(dev)) {
-		dev->power.async_in_progress = true;
-
-		get_device(dev);
-
-		if (async_schedule_dev_nocall(func, dev))
-			return true;
-
-		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 async_in_progress flag without extra synchronization.
-	 */
-	dev->power.async_in_progress = false;
-	return false;
-}
-
 static void async_resume_noirq(void *data, async_cookie_t cookie)
 {
 	struct device *dev = data;

Re: [PATCH v1 02/12] PM: sleep: Relocate two device PM core functions

[Stanislaw Gruszka]

On Mon, Jan 22, 2024 at 12:24:21PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Move is_async() and dpm_async_fn() in the PM core to a more suitable
> place.
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  drivers/base/power/main.c |   58 +++++++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 29 deletions(-)
> 
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -578,6 +578,35 @@ bool dev_pm_skip_resume(struct device *d
>  	return !dev->power.must_resume;
>  }
>  
> +static bool is_async(struct device *dev)
> +{
> +	return dev->power.async_suspend && pm_async_enabled
> +		&& !pm_trace_is_enabled();
> +}
> +
> +static bool dpm_async_fn(struct device *dev, async_func_t func)
> +{
> +	reinit_completion(&dev->power.completion);
> +
> +	if (is_async(dev)) {
> +		dev->power.async_in_progress = true;
> +
> +		get_device(dev);
> +
> +		if (async_schedule_dev_nocall(func, dev))
> +			return true;
> +
> +		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 async_in_progress flag without extra synchronization.
> +	 */
> +	dev->power.async_in_progress = false;
> +	return false;
> +}
> +
>  /**
>   * device_resume_noirq - Execute a "noirq resume" callback for given device.
>   * @dev: Device to handle.
> @@ -664,35 +693,6 @@ Out:
>  	}
>  }
>  
> -static bool is_async(struct device *dev)
> -{
> -	return dev->power.async_suspend && pm_async_enabled
> -		&& !pm_trace_is_enabled();
> -}
> -
> -static bool dpm_async_fn(struct device *dev, async_func_t func)
> -{
> -	reinit_completion(&dev->power.completion);
> -
> -	if (is_async(dev)) {
> -		dev->power.async_in_progress = true;
> -
> -		get_device(dev);
> -
> -		if (async_schedule_dev_nocall(func, dev))
> -			return true;
> -
> -		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 async_in_progress flag without extra synchronization.
> -	 */
> -	dev->power.async_in_progress = false;
> -	return false;
> -}
> -
>  static void async_resume_noirq(void *data, async_cookie_t cookie)
>  {
>  	struct device *dev = data;
> 
> 
> 
> 

[PATCH v1 03/12] PM: sleep: stats: Use array of suspend step names

[Rafael J. Wysocki]

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

Replace suspend_step_name() in the suspend statistics code with an array
of suspend step names which has fewer lines of code and less overhead.

While at it, remove two unnecessary line breaks in suspend_stats_show()
for a more consistent code layout.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 include/linux/suspend.h |    3 ++-
 kernel/power/main.c     |   48 +++++++++++++++++-------------------------------
 2 files changed, 19 insertions(+), 32 deletions(-)

Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -41,7 +41,8 @@ typedef int __bitwise suspend_state_t;
 #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
 
 enum suspend_stat_step {
-	SUSPEND_FREEZE = 1,
+	SUSPEND_NONE = 0,
+	SUSPEND_FREEZE,
 	SUSPEND_PREPARE,
 	SUSPEND_SUSPEND,
 	SUSPEND_SUSPEND_LATE,
Index: linux-pm/kernel/power/main.c
===================================================================
--- linux-pm.orig/kernel/power/main.c
+++ linux-pm/kernel/power/main.c
@@ -319,25 +319,17 @@ static ssize_t pm_test_store(struct kobj
 power_attr(pm_test);
 #endif /* CONFIG_PM_SLEEP_DEBUG */
 
-static char *suspend_step_name(enum suspend_stat_step step)
-{
-	switch (step) {
-	case SUSPEND_FREEZE:
-		return "freeze";
-	case SUSPEND_PREPARE:
-		return "prepare";
-	case SUSPEND_SUSPEND:
-		return "suspend";
-	case SUSPEND_SUSPEND_NOIRQ:
-		return "suspend_noirq";
-	case SUSPEND_RESUME_NOIRQ:
-		return "resume_noirq";
-	case SUSPEND_RESUME:
-		return "resume";
-	default:
-		return "";
-	}
-}
+static const char * const suspend_step_names[] = {
+	[SUSPEND_NONE] = "",
+	[SUSPEND_FREEZE] = "freeze",
+	[SUSPEND_PREPARE] = "prepare",
+	[SUSPEND_SUSPEND] = "suspend",
+	[SUSPEND_SUSPEND_LATE] = "suspend_late",
+	[SUSPEND_SUSPEND_NOIRQ] = "suspend_noirq",
+	[SUSPEND_RESUME_NOIRQ] = "resume_noirq",
+	[SUSPEND_RESUME_EARLY] = "resume_early",
+	[SUSPEND_RESUME] = "resume",
+};
 
 #define suspend_attr(_name, format_str)				\
 static ssize_t _name##_show(struct kobject *kobj,		\
@@ -392,16 +384,14 @@ static struct kobj_attribute last_failed
 static ssize_t last_failed_step_show(struct kobject *kobj,
 		struct kobj_attribute *attr, char *buf)
 {
-	int index;
 	enum suspend_stat_step step;
-	char *last_failed_step = NULL;
+	int index;
 
 	index = suspend_stats.last_failed_step + REC_FAILED_NUM - 1;
 	index %= REC_FAILED_NUM;
 	step = suspend_stats.failed_steps[index];
-	last_failed_step = suspend_step_name(step);
 
-	return sprintf(buf, "%s\n", last_failed_step);
+	return sprintf(buf, "%s\n", suspend_step_names[step]);
 }
 static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
 
@@ -477,26 +467,22 @@ static int suspend_stats_show(struct seq
 	for (i = 1; i < REC_FAILED_NUM; i++) {
 		index = last_dev + REC_FAILED_NUM - i;
 		index %= REC_FAILED_NUM;
-		seq_printf(s, "\t\t\t%-s\n",
-			suspend_stats.failed_devs[index]);
+		seq_printf(s, "\t\t\t%-s\n", suspend_stats.failed_devs[index]);
 	}
 	seq_printf(s,	"  last_failed_errno:\t%-d\n",
 			suspend_stats.errno[last_errno]);
 	for (i = 1; i < REC_FAILED_NUM; i++) {
 		index = last_errno + REC_FAILED_NUM - i;
 		index %= REC_FAILED_NUM;
-		seq_printf(s, "\t\t\t%-d\n",
-			suspend_stats.errno[index]);
+		seq_printf(s, "\t\t\t%-d\n", suspend_stats.errno[index]);
 	}
 	seq_printf(s,	"  last_failed_step:\t%-s\n",
-			suspend_step_name(
-				suspend_stats.failed_steps[last_step]));
+		   suspend_step_names[suspend_stats.failed_steps[last_step]]);
 	for (i = 1; i < REC_FAILED_NUM; i++) {
 		index = last_step + REC_FAILED_NUM - i;
 		index %= REC_FAILED_NUM;
 		seq_printf(s, "\t\t\t%-s\n",
-			suspend_step_name(
-				suspend_stats.failed_steps[index]));
+			   suspend_step_names[suspend_stats.failed_steps[index]]);
 	}
 
 	return 0;

Re: [PATCH v1 03/12] PM: sleep: stats: Use array of suspend step names

[Stanislaw Gruszka]

On Mon, Jan 22, 2024 at 12:25:45PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Replace suspend_step_name() in the suspend statistics code with an array
> of suspend step names which has fewer lines of code and less overhead.
> 
> While at it, remove two unnecessary line breaks in suspend_stats_show()
> for a more consistent code layout.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  include/linux/suspend.h |    3 ++-
>  kernel/power/main.c     |   48 +++++++++++++++++-------------------------------
>  2 files changed, 19 insertions(+), 32 deletions(-)
> 
> Index: linux-pm/include/linux/suspend.h
> ===================================================================
> --- linux-pm.orig/include/linux/suspend.h
> +++ linux-pm/include/linux/suspend.h
> @@ -41,7 +41,8 @@ typedef int __bitwise suspend_state_t;
>  #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
>  
>  enum suspend_stat_step {
> -	SUSPEND_FREEZE = 1,
> +	SUSPEND_NONE = 0,
> +	SUSPEND_FREEZE,
>  	SUSPEND_PREPARE,
>  	SUSPEND_SUSPEND,
>  	SUSPEND_SUSPEND_LATE,
> Index: linux-pm/kernel/power/main.c
> ===================================================================
> --- linux-pm.orig/kernel/power/main.c
> +++ linux-pm/kernel/power/main.c
> @@ -319,25 +319,17 @@ static ssize_t pm_test_store(struct kobj
>  power_attr(pm_test);
>  #endif /* CONFIG_PM_SLEEP_DEBUG */
>  
> -static char *suspend_step_name(enum suspend_stat_step step)
> -{
> -	switch (step) {
> -	case SUSPEND_FREEZE:
> -		return "freeze";
> -	case SUSPEND_PREPARE:
> -		return "prepare";
> -	case SUSPEND_SUSPEND:
> -		return "suspend";
> -	case SUSPEND_SUSPEND_NOIRQ:
> -		return "suspend_noirq";
> -	case SUSPEND_RESUME_NOIRQ:
> -		return "resume_noirq";
> -	case SUSPEND_RESUME:
> -		return "resume";
> -	default:
> -		return "";
> -	}
> -}
> +static const char * const suspend_step_names[] = {
> +	[SUSPEND_NONE] = "",
> +	[SUSPEND_FREEZE] = "freeze",
> +	[SUSPEND_PREPARE] = "prepare",
> +	[SUSPEND_SUSPEND] = "suspend",
> +	[SUSPEND_SUSPEND_LATE] = "suspend_late",
> +	[SUSPEND_SUSPEND_NOIRQ] = "suspend_noirq",
> +	[SUSPEND_RESUME_NOIRQ] = "resume_noirq",
> +	[SUSPEND_RESUME_EARLY] = "resume_early",
> +	[SUSPEND_RESUME] = "resume",
> +};
>  
>  #define suspend_attr(_name, format_str)				\
>  static ssize_t _name##_show(struct kobject *kobj,		\
> @@ -392,16 +384,14 @@ static struct kobj_attribute last_failed
>  static ssize_t last_failed_step_show(struct kobject *kobj,
>  		struct kobj_attribute *attr, char *buf)
>  {
> -	int index;
>  	enum suspend_stat_step step;
> -	char *last_failed_step = NULL;
> +	int index;
>  
>  	index = suspend_stats.last_failed_step + REC_FAILED_NUM - 1;
>  	index %= REC_FAILED_NUM;
>  	step = suspend_stats.failed_steps[index];
> -	last_failed_step = suspend_step_name(step);
>  
> -	return sprintf(buf, "%s\n", last_failed_step);
> +	return sprintf(buf, "%s\n", suspend_step_names[step]);
>  }
>  static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
>  
> @@ -477,26 +467,22 @@ static int suspend_stats_show(struct seq
>  	for (i = 1; i < REC_FAILED_NUM; i++) {
>  		index = last_dev + REC_FAILED_NUM - i;
>  		index %= REC_FAILED_NUM;
> -		seq_printf(s, "\t\t\t%-s\n",
> -			suspend_stats.failed_devs[index]);
> +		seq_printf(s, "\t\t\t%-s\n", suspend_stats.failed_devs[index]);
>  	}
>  	seq_printf(s,	"  last_failed_errno:\t%-d\n",
>  			suspend_stats.errno[last_errno]);
>  	for (i = 1; i < REC_FAILED_NUM; i++) {
>  		index = last_errno + REC_FAILED_NUM - i;
>  		index %= REC_FAILED_NUM;
> -		seq_printf(s, "\t\t\t%-d\n",
> -			suspend_stats.errno[index]);
> +		seq_printf(s, "\t\t\t%-d\n", suspend_stats.errno[index]);
>  	}
>  	seq_printf(s,	"  last_failed_step:\t%-s\n",
> -			suspend_step_name(
> -				suspend_stats.failed_steps[last_step]));
> +		   suspend_step_names[suspend_stats.failed_steps[last_step]]);
>  	for (i = 1; i < REC_FAILED_NUM; i++) {
>  		index = last_step + REC_FAILED_NUM - i;
>  		index %= REC_FAILED_NUM;
>  		seq_printf(s, "\t\t\t%-s\n",
> -			suspend_step_name(
> -				suspend_stats.failed_steps[index]));
> +			   suspend_step_names[suspend_stats.failed_steps[index]]);
>  	}
>  
>  	return 0;
> 
> 
> 

[PATCH v1 04/12] PM: sleep: stats: Use an array of step failure counters

[Rafael J. Wysocki]

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

Instead of using a set of individual struct suspend_stats fields
representing suspend step failure counters, use an array of counters
indexed by enum suspend_stat_step for this purpose, which allows
dpm_save_failed_step() to increment the appropriate counter
automatically, so that its callers don't need to do that directly.

It also allows suspend_stats_show() to carry out a loop over the
counters array to print their values.

Because the counters cannot become negative, use unsigned int for
representing them.

The only user-observable impact of this change is a different
ordering of entries in the suspend_stats debugfs file which is not
expected to matter.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/main.c |   22 ++++++++-----------
 include/linux/suspend.h   |   13 +++--------
 kernel/power/main.c       |   51 ++++++++++++++++++++++++----------------------
 kernel/power/suspend.c    |    1 
 4 files changed, 40 insertions(+), 47 deletions(-)

Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -49,20 +49,14 @@ enum suspend_stat_step {
 	SUSPEND_SUSPEND_NOIRQ,
 	SUSPEND_RESUME_NOIRQ,
 	SUSPEND_RESUME_EARLY,
-	SUSPEND_RESUME
+	SUSPEND_RESUME,
+	SUSPEND_NR_STEPS
 };
 
 struct suspend_stats {
+	unsigned int step_failures[SUSPEND_NR_STEPS];
 	int	success;
 	int	fail;
-	int	failed_freeze;
-	int	failed_prepare;
-	int	failed_suspend;
-	int	failed_suspend_late;
-	int	failed_suspend_noirq;
-	int	failed_resume;
-	int	failed_resume_early;
-	int	failed_resume_noirq;
 #define	REC_FAILED_NUM	2
 	int	last_failed_dev;
 	char	failed_devs[REC_FAILED_NUM][40];
@@ -95,6 +89,7 @@ static inline void dpm_save_failed_errno
 
 static inline void dpm_save_failed_step(enum suspend_stat_step step)
 {
+	suspend_stats.step_failures[step]++;
 	suspend_stats.failed_steps[suspend_stats.last_failed_step] = step;
 	suspend_stats.last_failed_step++;
 	suspend_stats.last_failed_step %= REC_FAILED_NUM;
Index: linux-pm/kernel/power/main.c
===================================================================
--- linux-pm.orig/kernel/power/main.c
+++ linux-pm/kernel/power/main.c
@@ -341,18 +341,28 @@ static struct kobj_attribute _name = __A
 
 suspend_attr(success, "%d\n");
 suspend_attr(fail, "%d\n");
-suspend_attr(failed_freeze, "%d\n");
-suspend_attr(failed_prepare, "%d\n");
-suspend_attr(failed_suspend, "%d\n");
-suspend_attr(failed_suspend_late, "%d\n");
-suspend_attr(failed_suspend_noirq, "%d\n");
-suspend_attr(failed_resume, "%d\n");
-suspend_attr(failed_resume_early, "%d\n");
-suspend_attr(failed_resume_noirq, "%d\n");
 suspend_attr(last_hw_sleep, "%llu\n");
 suspend_attr(total_hw_sleep, "%llu\n");
 suspend_attr(max_hw_sleep, "%llu\n");
 
+#define suspend_step_attr(_name, step)		\
+static ssize_t _name##_show(struct kobject *kobj,		\
+		struct kobj_attribute *attr, char *buf)		\
+{								\
+	return sprintf(buf, "%u\n",				\
+		       suspend_stats.step_failures[step]);	\
+}								\
+static struct kobj_attribute _name = __ATTR_RO(_name)
+
+suspend_step_attr(failed_freeze, SUSPEND_FREEZE);
+suspend_step_attr(failed_prepare, SUSPEND_PREPARE);
+suspend_step_attr(failed_suspend, SUSPEND_SUSPEND);
+suspend_step_attr(failed_suspend_late, SUSPEND_SUSPEND_LATE);
+suspend_step_attr(failed_suspend_noirq, SUSPEND_SUSPEND_NOIRQ);
+suspend_step_attr(failed_resume, SUSPEND_RESUME);
+suspend_step_attr(failed_resume_early, SUSPEND_RESUME_EARLY);
+suspend_step_attr(failed_resume_noirq, SUSPEND_RESUME_NOIRQ);
+
 static ssize_t last_failed_dev_show(struct kobject *kobj,
 		struct kobj_attribute *attr, char *buf)
 {
@@ -439,6 +449,7 @@ static const struct attribute_group susp
 static int suspend_stats_show(struct seq_file *s, void *unused)
 {
 	int i, index, last_dev, last_errno, last_step;
+	enum suspend_stat_step step;
 
 	last_dev = suspend_stats.last_failed_dev + REC_FAILED_NUM - 1;
 	last_dev %= REC_FAILED_NUM;
@@ -446,22 +457,14 @@ static int suspend_stats_show(struct seq
 	last_errno %= REC_FAILED_NUM;
 	last_step = suspend_stats.last_failed_step + REC_FAILED_NUM - 1;
 	last_step %= REC_FAILED_NUM;
-	seq_printf(s, "%s: %d\n%s: %d\n%s: %d\n%s: %d\n%s: %d\n"
-			"%s: %d\n%s: %d\n%s: %d\n%s: %d\n%s: %d\n",
-			"success", suspend_stats.success,
-			"fail", suspend_stats.fail,
-			"failed_freeze", suspend_stats.failed_freeze,
-			"failed_prepare", suspend_stats.failed_prepare,
-			"failed_suspend", suspend_stats.failed_suspend,
-			"failed_suspend_late",
-				suspend_stats.failed_suspend_late,
-			"failed_suspend_noirq",
-				suspend_stats.failed_suspend_noirq,
-			"failed_resume", suspend_stats.failed_resume,
-			"failed_resume_early",
-				suspend_stats.failed_resume_early,
-			"failed_resume_noirq",
-				suspend_stats.failed_resume_noirq);
+
+	seq_printf(s, "success: %d\nfail: %d\n",
+		   suspend_stats.success, suspend_stats.fail);
+
+	for (step = SUSPEND_FREEZE; step < SUSPEND_NR_STEPS; step++)
+		seq_printf(s, "failed_%s: %u\n", suspend_step_names[step],
+			   suspend_stats.step_failures[step]);
+
 	seq_printf(s,	"failures:\n  last_failed_dev:\t%-s\n",
 			suspend_stats.failed_devs[last_dev]);
 	for (i = 1; i < REC_FAILED_NUM; i++) {
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -367,7 +367,6 @@ static int suspend_prepare(suspend_state
 	if (!error)
 		return 0;
 
-	suspend_stats.failed_freeze++;
 	dpm_save_failed_step(SUSPEND_FREEZE);
 	pm_notifier_call_chain(PM_POST_SUSPEND);
  Restore:
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -686,7 +686,6 @@ Out:
 	TRACE_RESUME(error);
 
 	if (error) {
-		suspend_stats.failed_resume_noirq++;
 		dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
 		dpm_save_failed_dev(dev_name(dev));
 		pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
@@ -817,7 +816,6 @@ Out:
 	complete_all(&dev->power.completion);
 
 	if (error) {
-		suspend_stats.failed_resume_early++;
 		dpm_save_failed_step(SUSPEND_RESUME_EARLY);
 		dpm_save_failed_dev(dev_name(dev));
 		pm_dev_err(dev, state, async ? " async early" : " early", error);
@@ -974,7 +972,6 @@ static void device_resume(struct device
 	TRACE_RESUME(error);
 
 	if (error) {
-		suspend_stats.failed_resume++;
 		dpm_save_failed_step(SUSPEND_RESUME);
 		dpm_save_failed_dev(dev_name(dev));
 		pm_dev_err(dev, state, async ? " async" : "", error);
@@ -1323,10 +1320,9 @@ static int dpm_noirq_suspend_devices(pm_
 	if (!error)
 		error = async_error;
 
-	if (error) {
-		suspend_stats.failed_suspend_noirq++;
+	if (error)
 		dpm_save_failed_step(SUSPEND_SUSPEND_NOIRQ);
-	}
+
 	dpm_show_time(starttime, state, error, "noirq");
 	trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, false);
 	return error;
@@ -1509,8 +1505,8 @@ int dpm_suspend_late(pm_message_t state)
 	async_synchronize_full();
 	if (!error)
 		error = async_error;
+
 	if (error) {
-		suspend_stats.failed_suspend_late++;
 		dpm_save_failed_step(SUSPEND_SUSPEND_LATE);
 		dpm_resume_early(resume_event(state));
 	}
@@ -1789,10 +1785,10 @@ int dpm_suspend(pm_message_t state)
 	async_synchronize_full();
 	if (!error)
 		error = async_error;
-	if (error) {
-		suspend_stats.failed_suspend++;
+
+	if (error)
 		dpm_save_failed_step(SUSPEND_SUSPEND);
-	}
+
 	dpm_show_time(starttime, state, error, NULL);
 	trace_suspend_resume(TPS("dpm_suspend"), state.event, false);
 	return error;
@@ -1943,11 +1939,11 @@ int dpm_suspend_start(pm_message_t state
 	int error;
 
 	error = dpm_prepare(state);
-	if (error) {
-		suspend_stats.failed_prepare++;
+	if (error)
 		dpm_save_failed_step(SUSPEND_PREPARE);
-	} else
+	else
 		error = dpm_suspend(state);
+
 	dpm_show_time(starttime, state, error, "start");
 	return error;
 }

Re: [PATCH v1 04/12] PM: sleep: stats: Use an array of step failure counters

[Stanislaw Gruszka]

On Mon, Jan 22, 2024 at 12:27:21PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Instead of using a set of individual struct suspend_stats fields
> representing suspend step failure counters, use an array of counters
> indexed by enum suspend_stat_step for this purpose, which allows
> dpm_save_failed_step() to increment the appropriate counter
> automatically, so that its callers don't need to do that directly.
> 
> It also allows suspend_stats_show() to carry out a loop over the
> counters array to print their values.
> 
> Because the counters cannot become negative, use unsigned int for
> representing them.
> 
> The only user-observable impact of this change is a different
> ordering of entries in the suspend_stats debugfs file which is not
> expected to matter.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  drivers/base/power/main.c |   22 ++++++++-----------
>  include/linux/suspend.h   |   13 +++--------
>  kernel/power/main.c       |   51 ++++++++++++++++++++++++----------------------
>  kernel/power/suspend.c    |    1 
>  4 files changed, 40 insertions(+), 47 deletions(-)
> 
> Index: linux-pm/include/linux/suspend.h
> ===================================================================
> --- linux-pm.orig/include/linux/suspend.h
> +++ linux-pm/include/linux/suspend.h
> @@ -49,20 +49,14 @@ enum suspend_stat_step {
>  	SUSPEND_SUSPEND_NOIRQ,
>  	SUSPEND_RESUME_NOIRQ,
>  	SUSPEND_RESUME_EARLY,
> -	SUSPEND_RESUME
> +	SUSPEND_RESUME,
> +	SUSPEND_NR_STEPS
>  };
>  
>  struct suspend_stats {
> +	unsigned int step_failures[SUSPEND_NR_STEPS];
>  	int	success;
>  	int	fail;
> -	int	failed_freeze;
> -	int	failed_prepare;
> -	int	failed_suspend;
> -	int	failed_suspend_late;
> -	int	failed_suspend_noirq;
> -	int	failed_resume;
> -	int	failed_resume_early;
> -	int	failed_resume_noirq;
>  #define	REC_FAILED_NUM	2
>  	int	last_failed_dev;
>  	char	failed_devs[REC_FAILED_NUM][40];
> @@ -95,6 +89,7 @@ static inline void dpm_save_failed_errno
>  
>  static inline void dpm_save_failed_step(enum suspend_stat_step step)
>  {
> +	suspend_stats.step_failures[step]++;
>  	suspend_stats.failed_steps[suspend_stats.last_failed_step] = step;
>  	suspend_stats.last_failed_step++;
>  	suspend_stats.last_failed_step %= REC_FAILED_NUM;
> Index: linux-pm/kernel/power/main.c
> ===================================================================
> --- linux-pm.orig/kernel/power/main.c
> +++ linux-pm/kernel/power/main.c
> @@ -341,18 +341,28 @@ static struct kobj_attribute _name = __A
>  
>  suspend_attr(success, "%d\n");
>  suspend_attr(fail, "%d\n");
> -suspend_attr(failed_freeze, "%d\n");
> -suspend_attr(failed_prepare, "%d\n");
> -suspend_attr(failed_suspend, "%d\n");
> -suspend_attr(failed_suspend_late, "%d\n");
> -suspend_attr(failed_suspend_noirq, "%d\n");
> -suspend_attr(failed_resume, "%d\n");
> -suspend_attr(failed_resume_early, "%d\n");
> -suspend_attr(failed_resume_noirq, "%d\n");
>  suspend_attr(last_hw_sleep, "%llu\n");
>  suspend_attr(total_hw_sleep, "%llu\n");
>  suspend_attr(max_hw_sleep, "%llu\n");
>  
> +#define suspend_step_attr(_name, step)		\
> +static ssize_t _name##_show(struct kobject *kobj,		\
> +		struct kobj_attribute *attr, char *buf)		\
> +{								\
> +	return sprintf(buf, "%u\n",				\
> +		       suspend_stats.step_failures[step]);	\
> +}								\
> +static struct kobj_attribute _name = __ATTR_RO(_name)
> +
> +suspend_step_attr(failed_freeze, SUSPEND_FREEZE);
> +suspend_step_attr(failed_prepare, SUSPEND_PREPARE);
> +suspend_step_attr(failed_suspend, SUSPEND_SUSPEND);
> +suspend_step_attr(failed_suspend_late, SUSPEND_SUSPEND_LATE);
> +suspend_step_attr(failed_suspend_noirq, SUSPEND_SUSPEND_NOIRQ);
> +suspend_step_attr(failed_resume, SUSPEND_RESUME);
> +suspend_step_attr(failed_resume_early, SUSPEND_RESUME_EARLY);
> +suspend_step_attr(failed_resume_noirq, SUSPEND_RESUME_NOIRQ);
> +
>  static ssize_t last_failed_dev_show(struct kobject *kobj,
>  		struct kobj_attribute *attr, char *buf)
>  {
> @@ -439,6 +449,7 @@ static const struct attribute_group susp
>  static int suspend_stats_show(struct seq_file *s, void *unused)
>  {
>  	int i, index, last_dev, last_errno, last_step;
> +	enum suspend_stat_step step;
>  
>  	last_dev = suspend_stats.last_failed_dev + REC_FAILED_NUM - 1;
>  	last_dev %= REC_FAILED_NUM;
> @@ -446,22 +457,14 @@ static int suspend_stats_show(struct seq
>  	last_errno %= REC_FAILED_NUM;
>  	last_step = suspend_stats.last_failed_step + REC_FAILED_NUM - 1;
>  	last_step %= REC_FAILED_NUM;
> -	seq_printf(s, "%s: %d\n%s: %d\n%s: %d\n%s: %d\n%s: %d\n"
> -			"%s: %d\n%s: %d\n%s: %d\n%s: %d\n%s: %d\n",
> -			"success", suspend_stats.success,
> -			"fail", suspend_stats.fail,
> -			"failed_freeze", suspend_stats.failed_freeze,
> -			"failed_prepare", suspend_stats.failed_prepare,
> -			"failed_suspend", suspend_stats.failed_suspend,
> -			"failed_suspend_late",
> -				suspend_stats.failed_suspend_late,
> -			"failed_suspend_noirq",
> -				suspend_stats.failed_suspend_noirq,
> -			"failed_resume", suspend_stats.failed_resume,
> -			"failed_resume_early",
> -				suspend_stats.failed_resume_early,
> -			"failed_resume_noirq",
> -				suspend_stats.failed_resume_noirq);
> +
> +	seq_printf(s, "success: %d\nfail: %d\n",
> +		   suspend_stats.success, suspend_stats.fail);
> +
> +	for (step = SUSPEND_FREEZE; step < SUSPEND_NR_STEPS; step++)
> +		seq_printf(s, "failed_%s: %u\n", suspend_step_names[step],
> +			   suspend_stats.step_failures[step]);
> +
>  	seq_printf(s,	"failures:\n  last_failed_dev:\t%-s\n",
>  			suspend_stats.failed_devs[last_dev]);
>  	for (i = 1; i < REC_FAILED_NUM; i++) {
> Index: linux-pm/kernel/power/suspend.c
> ===================================================================
> --- linux-pm.orig/kernel/power/suspend.c
> +++ linux-pm/kernel/power/suspend.c
> @@ -367,7 +367,6 @@ static int suspend_prepare(suspend_state
>  	if (!error)
>  		return 0;
>  
> -	suspend_stats.failed_freeze++;
>  	dpm_save_failed_step(SUSPEND_FREEZE);
>  	pm_notifier_call_chain(PM_POST_SUSPEND);
>   Restore:
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -686,7 +686,6 @@ Out:
>  	TRACE_RESUME(error);
>  
>  	if (error) {
> -		suspend_stats.failed_resume_noirq++;
>  		dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
>  		dpm_save_failed_dev(dev_name(dev));
>  		pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
> @@ -817,7 +816,6 @@ Out:
>  	complete_all(&dev->power.completion);
>  
>  	if (error) {
> -		suspend_stats.failed_resume_early++;
>  		dpm_save_failed_step(SUSPEND_RESUME_EARLY);
>  		dpm_save_failed_dev(dev_name(dev));
>  		pm_dev_err(dev, state, async ? " async early" : " early", error);
> @@ -974,7 +972,6 @@ static void device_resume(struct device
>  	TRACE_RESUME(error);
>  
>  	if (error) {
> -		suspend_stats.failed_resume++;
>  		dpm_save_failed_step(SUSPEND_RESUME);
>  		dpm_save_failed_dev(dev_name(dev));
>  		pm_dev_err(dev, state, async ? " async" : "", error);
> @@ -1323,10 +1320,9 @@ static int dpm_noirq_suspend_devices(pm_
>  	if (!error)
>  		error = async_error;
>  
> -	if (error) {
> -		suspend_stats.failed_suspend_noirq++;
> +	if (error)
>  		dpm_save_failed_step(SUSPEND_SUSPEND_NOIRQ);
> -	}
> +
>  	dpm_show_time(starttime, state, error, "noirq");
>  	trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, false);
>  	return error;
> @@ -1509,8 +1505,8 @@ int dpm_suspend_late(pm_message_t state)
>  	async_synchronize_full();
>  	if (!error)
>  		error = async_error;
> +
>  	if (error) {
> -		suspend_stats.failed_suspend_late++;
>  		dpm_save_failed_step(SUSPEND_SUSPEND_LATE);
>  		dpm_resume_early(resume_event(state));
>  	}
> @@ -1789,10 +1785,10 @@ int dpm_suspend(pm_message_t state)
>  	async_synchronize_full();
>  	if (!error)
>  		error = async_error;
> -	if (error) {
> -		suspend_stats.failed_suspend++;
> +
> +	if (error)
>  		dpm_save_failed_step(SUSPEND_SUSPEND);
> -	}
> +
>  	dpm_show_time(starttime, state, error, NULL);
>  	trace_suspend_resume(TPS("dpm_suspend"), state.event, false);
>  	return error;
> @@ -1943,11 +1939,11 @@ int dpm_suspend_start(pm_message_t state
>  	int error;
>  
>  	error = dpm_prepare(state);
> -	if (error) {
> -		suspend_stats.failed_prepare++;
> +	if (error)
>  		dpm_save_failed_step(SUSPEND_PREPARE);
> -	} else
> +	else
>  		error = dpm_suspend(state);
> +
>  	dpm_show_time(starttime, state, error, "start");
>  	return error;
>  }
> 
> 
> 
> 

[PATCH v1 05/12] PM: sleep: stats: Use step_failures[0] as a counter of successful cycles

[Rafael J. Wysocki]

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

The first (index 0) cell of the step_failures[] array in struct
suspend_stats introduced previously can be used as a counter of
successful suspend-resume cycles instead of the separate "success"
field in it, so do that.

While at it, change the type of the "fail" field in struct
suspend_stats to unsigned int, because it cannot be negative.

No intentional functional impact.

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

Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -55,8 +55,7 @@ enum suspend_stat_step {
 
 struct suspend_stats {
 	unsigned int step_failures[SUSPEND_NR_STEPS];
-	int	success;
-	int	fail;
+	unsigned int fail;
 #define	REC_FAILED_NUM	2
 	int	last_failed_dev;
 	char	failed_devs[REC_FAILED_NUM][40];
Index: linux-pm/kernel/power/main.c
===================================================================
--- linux-pm.orig/kernel/power/main.c
+++ linux-pm/kernel/power/main.c
@@ -339,8 +339,7 @@ static ssize_t _name##_show(struct kobje
 }								\
 static struct kobj_attribute _name = __ATTR_RO(_name)
 
-suspend_attr(success, "%d\n");
-suspend_attr(fail, "%d\n");
+suspend_attr(fail, "%u\n");
 suspend_attr(last_hw_sleep, "%llu\n");
 suspend_attr(total_hw_sleep, "%llu\n");
 suspend_attr(max_hw_sleep, "%llu\n");
@@ -354,6 +353,7 @@ static ssize_t _name##_show(struct kobje
 }								\
 static struct kobj_attribute _name = __ATTR_RO(_name)
 
+suspend_step_attr(success, SUSPEND_NONE);
 suspend_step_attr(failed_freeze, SUSPEND_FREEZE);
 suspend_step_attr(failed_prepare, SUSPEND_PREPARE);
 suspend_step_attr(failed_suspend, SUSPEND_SUSPEND);
@@ -458,8 +458,9 @@ static int suspend_stats_show(struct seq
 	last_step = suspend_stats.last_failed_step + REC_FAILED_NUM - 1;
 	last_step %= REC_FAILED_NUM;
 
-	seq_printf(s, "success: %d\nfail: %d\n",
-		   suspend_stats.success, suspend_stats.fail);
+	seq_printf(s, "success: %u\nfail: %u\n",
+		   suspend_stats.step_failures[SUSPEND_NONE],
+		   suspend_stats.fail);
 
 	for (step = SUSPEND_FREEZE; step < SUSPEND_NR_STEPS; step++)
 		seq_printf(s, "failed_%s: %u\n", suspend_step_names[step],
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -620,7 +620,7 @@ int pm_suspend(suspend_state_t state)
 		suspend_stats.fail++;
 		dpm_save_failed_errno(error);
 	} else {
-		suspend_stats.success++;
+		suspend_stats.step_failures[SUSPEND_NONE]++;
 	}
 	pr_info("suspend exit\n");
 	return error;

Re: [PATCH v1 05/12] PM: sleep: stats: Use step_failures[0] as a counter of successful cycles

[Stanislaw Gruszka]

On Mon, Jan 22, 2024 at 12:29:11PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The first (index 0) cell of the step_failures[] array in struct
> suspend_stats introduced previously can be used as a counter of
> successful suspend-resume cycles instead of the separate "success"
> field in it, so do that.
> 
> While at it, change the type of the "fail" field in struct
> suspend_stats to unsigned int, because it cannot be negative.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  include/linux/suspend.h |    3 +--
>  kernel/power/main.c     |    9 +++++----
>  kernel/power/suspend.c  |    2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> Index: linux-pm/include/linux/suspend.h
> ===================================================================
> --- linux-pm.orig/include/linux/suspend.h
> +++ linux-pm/include/linux/suspend.h
> @@ -55,8 +55,7 @@ enum suspend_stat_step {
>  
>  struct suspend_stats {
>  	unsigned int step_failures[SUSPEND_NR_STEPS];
> -	int	success;
<snip>
> -		   suspend_stats.success, suspend_stats.fail);
> +	seq_printf(s, "success: %u\nfail: %u\n",
> +		   suspend_stats.step_failures[SUSPEND_NONE],
> +		   suspend_stats.fail);
>  
>  	for (step = SUSPEND_FREEZE; step < SUSPEND_NR_STEPS; step++)
>  		seq_printf(s, "failed_%s: %u\n", suspend_step_names[step],
> Index: linux-pm/kernel/power/suspend.c
> ===================================================================
> --- linux-pm.orig/kernel/power/suspend.c
> +++ linux-pm/kernel/power/suspend.c
> @@ -620,7 +620,7 @@ int pm_suspend(suspend_state_t state)
>  		suspend_stats.fail++;
>  		dpm_save_failed_errno(error);
>  	} else {
> -		suspend_stats.success++;
> +		suspend_stats.step_failures[SUSPEND_NONE]++;

This looks confusing for me. I think would be better keep
success field and just remove SUSPEND_NONE from the 
suspend_stat_step and suspend_stat_names. Actually do
not introduce it, SUSPEND_NONE does not seems to be necessary
(SUSPEND_FREEZE can be 0).

Regards
Stanislaw
 

Re: [PATCH v1 05/12] PM: sleep: stats: Use step_failures[0] as a counter of successful cycles

[Rafael J. Wysocki]

On Thu, Jan 25, 2024 at 8:52 AM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> wrote:
>
> On Mon, Jan 22, 2024 at 12:29:11PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The first (index 0) cell of the step_failures[] array in struct
> > suspend_stats introduced previously can be used as a counter of
> > successful suspend-resume cycles instead of the separate "success"
> > field in it, so do that.
> >
> > While at it, change the type of the "fail" field in struct
> > suspend_stats to unsigned int, because it cannot be negative.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  include/linux/suspend.h |    3 +--
> >  kernel/power/main.c     |    9 +++++----
> >  kernel/power/suspend.c  |    2 +-
> >  3 files changed, 7 insertions(+), 7 deletions(-)
> >
> > Index: linux-pm/include/linux/suspend.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/suspend.h
> > +++ linux-pm/include/linux/suspend.h
> > @@ -55,8 +55,7 @@ enum suspend_stat_step {
> >
> >  struct suspend_stats {
> >       unsigned int step_failures[SUSPEND_NR_STEPS];
> > -     int     success;
> <snip>
> > -                suspend_stats.success, suspend_stats.fail);
> > +     seq_printf(s, "success: %u\nfail: %u\n",
> > +                suspend_stats.step_failures[SUSPEND_NONE],
> > +                suspend_stats.fail);
> >
> >       for (step = SUSPEND_FREEZE; step < SUSPEND_NR_STEPS; step++)
> >               seq_printf(s, "failed_%s: %u\n", suspend_step_names[step],
> > Index: linux-pm/kernel/power/suspend.c
> > ===================================================================
> > --- linux-pm.orig/kernel/power/suspend.c
> > +++ linux-pm/kernel/power/suspend.c
> > @@ -620,7 +620,7 @@ int pm_suspend(suspend_state_t state)
> >               suspend_stats.fail++;
> >               dpm_save_failed_errno(error);
> >       } else {
> > -             suspend_stats.success++;
> > +             suspend_stats.step_failures[SUSPEND_NONE]++;
>
> This looks confusing for me. I think would be better keep
> success field and just remove SUSPEND_NONE from the
> suspend_stat_step and suspend_stat_names. Actually do
> not introduce it, SUSPEND_NONE does not seems to be necessary
> (SUSPEND_FREEZE can be 0).

OK

I'll need to rearrange the series for that somewhat except for the
first two patches.

I guess it's OK to retain the R-by tags?

Thanks for all of the reviews!

Re: [PATCH v1 05/12] PM: sleep: stats: Use step_failures[0] as a counter of successful cycles

[Stanislaw Gruszka]

On Thu, Jan 25, 2024 at 04:11:08PM +0100, Rafael J. Wysocki wrote:
> On Thu, Jan 25, 2024 at 8:52 AM Stanislaw Gruszka
> <stanislaw.gruszka@linux.intel.com> wrote:
> >
> > On Mon, Jan 22, 2024 at 12:29:11PM +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > The first (index 0) cell of the step_failures[] array in struct
> > > suspend_stats introduced previously can be used as a counter of
> > > successful suspend-resume cycles instead of the separate "success"
> > > field in it, so do that.
> > >
> > > While at it, change the type of the "fail" field in struct
> > > suspend_stats to unsigned int, because it cannot be negative.
> > >
> > > No intentional functional impact.
> > >
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  include/linux/suspend.h |    3 +--
> > >  kernel/power/main.c     |    9 +++++----
> > >  kernel/power/suspend.c  |    2 +-
> > >  3 files changed, 7 insertions(+), 7 deletions(-)
> > >
> > > Index: linux-pm/include/linux/suspend.h
> > > ===================================================================
> > > --- linux-pm.orig/include/linux/suspend.h
> > > +++ linux-pm/include/linux/suspend.h
> > > @@ -55,8 +55,7 @@ enum suspend_stat_step {
> > >
> > >  struct suspend_stats {
> > >       unsigned int step_failures[SUSPEND_NR_STEPS];
> > > -     int     success;
> > <snip>
> > > -                suspend_stats.success, suspend_stats.fail);
> > > +     seq_printf(s, "success: %u\nfail: %u\n",
> > > +                suspend_stats.step_failures[SUSPEND_NONE],
> > > +                suspend_stats.fail);
> > >
> > >       for (step = SUSPEND_FREEZE; step < SUSPEND_NR_STEPS; step++)
> > >               seq_printf(s, "failed_%s: %u\n", suspend_step_names[step],
> > > Index: linux-pm/kernel/power/suspend.c
> > > ===================================================================
> > > --- linux-pm.orig/kernel/power/suspend.c
> > > +++ linux-pm/kernel/power/suspend.c
> > > @@ -620,7 +620,7 @@ int pm_suspend(suspend_state_t state)
> > >               suspend_stats.fail++;
> > >               dpm_save_failed_errno(error);
> > >       } else {
> > > -             suspend_stats.success++;
> > > +             suspend_stats.step_failures[SUSPEND_NONE]++;
> >
> > This looks confusing for me. I think would be better keep
> > success field and just remove SUSPEND_NONE from the
> > suspend_stat_step and suspend_stat_names. Actually do
> > not introduce it, SUSPEND_NONE does not seems to be necessary
> > (SUSPEND_FREEZE can be 0).
> 
> OK
> 
> I'll need to rearrange the series for that somewhat except for the
> first two patches.

I wouldn't mind to skip this change and just remove SUSPEND_NONE
in separate patch.

> I guess it's OK to retain the R-by tags?

Yes, is OK.
 
Regards
Stanislaw

[PATCH v1 06/12] PM: sleep: stats: Define suspend_stats next to the code using it

[Rafael J. Wysocki]

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

It is not necessary to define struct suspend_stats in a header file and the
suspend_stats variable in the core device system-wide PM code.  They both
can be defined in kernel/power/main.c, next to the sysfs and debugfs code
accessing suspend_stats, which can be static.

Modify the code in question in accordance with the above observation and
replace the static inline functions manipulating suspend_stats with
regular ones defined in kernel/power/main.c.

While at it, move the enum suspend_stat_step to the end of suspend.h which
is a more suitable place for it.

No intentional functional impact.

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

Index: linux-pm/include/linux/suspend.h
===================================================================
--- linux-pm.orig/include/linux/suspend.h
+++ linux-pm/include/linux/suspend.h
@@ -40,60 +40,6 @@ typedef int __bitwise suspend_state_t;
 #define PM_SUSPEND_MIN		PM_SUSPEND_TO_IDLE
 #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
 
-enum suspend_stat_step {
-	SUSPEND_NONE = 0,
-	SUSPEND_FREEZE,
-	SUSPEND_PREPARE,
-	SUSPEND_SUSPEND,
-	SUSPEND_SUSPEND_LATE,
-	SUSPEND_SUSPEND_NOIRQ,
-	SUSPEND_RESUME_NOIRQ,
-	SUSPEND_RESUME_EARLY,
-	SUSPEND_RESUME,
-	SUSPEND_NR_STEPS
-};
-
-struct suspend_stats {
-	unsigned int step_failures[SUSPEND_NR_STEPS];
-	unsigned int fail;
-#define	REC_FAILED_NUM	2
-	int	last_failed_dev;
-	char	failed_devs[REC_FAILED_NUM][40];
-	int	last_failed_errno;
-	int	errno[REC_FAILED_NUM];
-	int	last_failed_step;
-	u64	last_hw_sleep;
-	u64	total_hw_sleep;
-	u64	max_hw_sleep;
-	enum suspend_stat_step	failed_steps[REC_FAILED_NUM];
-};
-
-extern struct suspend_stats suspend_stats;
-
-static inline void dpm_save_failed_dev(const char *name)
-{
-	strscpy(suspend_stats.failed_devs[suspend_stats.last_failed_dev],
-		name,
-		sizeof(suspend_stats.failed_devs[0]));
-	suspend_stats.last_failed_dev++;
-	suspend_stats.last_failed_dev %= REC_FAILED_NUM;
-}
-
-static inline void dpm_save_failed_errno(int err)
-{
-	suspend_stats.errno[suspend_stats.last_failed_errno] = err;
-	suspend_stats.last_failed_errno++;
-	suspend_stats.last_failed_errno %= REC_FAILED_NUM;
-}
-
-static inline void dpm_save_failed_step(enum suspend_stat_step step)
-{
-	suspend_stats.step_failures[step]++;
-	suspend_stats.failed_steps[suspend_stats.last_failed_step] = step;
-	suspend_stats.last_failed_step++;
-	suspend_stats.last_failed_step %= REC_FAILED_NUM;
-}
-
 /**
  * struct platform_suspend_ops - Callbacks for managing platform dependent
  *	system sleep states.
@@ -621,4 +567,20 @@ static inline void queue_up_suspend_work
 
 #endif /* !CONFIG_PM_AUTOSLEEP */
 
+enum suspend_stat_step {
+	SUSPEND_NONE = 0,
+	SUSPEND_FREEZE,
+	SUSPEND_PREPARE,
+	SUSPEND_SUSPEND,
+	SUSPEND_SUSPEND_LATE,
+	SUSPEND_SUSPEND_NOIRQ,
+	SUSPEND_RESUME_NOIRQ,
+	SUSPEND_RESUME_EARLY,
+	SUSPEND_RESUME,
+	SUSPEND_NR_STEPS
+};
+
+void dpm_save_failed_dev(const char *name);
+void dpm_save_failed_step(enum suspend_stat_step step);
+
 #endif /* _LINUX_SUSPEND_H */
Index: linux-pm/kernel/power/main.c
===================================================================
--- linux-pm.orig/kernel/power/main.c
+++ linux-pm/kernel/power/main.c
@@ -95,19 +95,6 @@ int unregister_pm_notifier(struct notifi
 }
 EXPORT_SYMBOL_GPL(unregister_pm_notifier);
 
-void pm_report_hw_sleep_time(u64 t)
-{
-	suspend_stats.last_hw_sleep = t;
-	suspend_stats.total_hw_sleep += t;
-}
-EXPORT_SYMBOL_GPL(pm_report_hw_sleep_time);
-
-void pm_report_max_hw_sleep(u64 t)
-{
-	suspend_stats.max_hw_sleep = t;
-}
-EXPORT_SYMBOL_GPL(pm_report_max_hw_sleep);
-
 int pm_notifier_call_chain_robust(unsigned long val_up, unsigned long val_down)
 {
 	int ret;
@@ -319,6 +306,67 @@ static ssize_t pm_test_store(struct kobj
 power_attr(pm_test);
 #endif /* CONFIG_PM_SLEEP_DEBUG */
 
+#define REC_FAILED_NUM	2
+
+struct suspend_stats {
+	unsigned int step_failures[SUSPEND_NR_STEPS];
+	unsigned int fail;
+	int last_failed_dev;
+	char failed_devs[REC_FAILED_NUM][40];
+	int last_failed_errno;
+	int errno[REC_FAILED_NUM];
+	int last_failed_step;
+	u64 last_hw_sleep;
+	u64 total_hw_sleep;
+	u64 max_hw_sleep;
+	enum suspend_stat_step failed_steps[REC_FAILED_NUM];
+};
+
+static struct suspend_stats suspend_stats;
+
+void dpm_save_failed_dev(const char *name)
+{
+	strscpy(suspend_stats.failed_devs[suspend_stats.last_failed_dev],
+		name, sizeof(suspend_stats.failed_devs[0]));
+	suspend_stats.last_failed_dev++;
+	suspend_stats.last_failed_dev %= REC_FAILED_NUM;
+}
+
+void dpm_save_failed_step(enum suspend_stat_step step)
+{
+	suspend_stats.step_failures[step]++;
+	suspend_stats.failed_steps[suspend_stats.last_failed_step] = step;
+	suspend_stats.last_failed_step++;
+	suspend_stats.last_failed_step %= REC_FAILED_NUM;
+}
+
+void dpm_save_errno(int err)
+{
+	if (!err) {
+		suspend_stats.step_failures[SUSPEND_NONE]++;
+		return;
+	}
+
+	suspend_stats.fail++;
+
+	suspend_stats.errno[suspend_stats.last_failed_errno] = err;
+	suspend_stats.last_failed_errno++;
+	suspend_stats.last_failed_errno %= REC_FAILED_NUM;
+}
+
+void pm_report_hw_sleep_time(u64 t)
+{
+	suspend_stats.last_hw_sleep = t;
+	suspend_stats.total_hw_sleep += t;
+}
+EXPORT_SYMBOL_GPL(pm_report_hw_sleep_time);
+
+void pm_report_max_hw_sleep(u64 t)
+{
+	suspend_stats.max_hw_sleep = t;
+}
+EXPORT_SYMBOL_GPL(pm_report_max_hw_sleep);
+
 static const char * const suspend_step_names[] = {
 	[SUSPEND_NONE] = "",
 	[SUSPEND_FREEZE] = "freeze",
Index: linux-pm/kernel/power/power.h
===================================================================
--- linux-pm.orig/kernel/power/power.h
+++ linux-pm/kernel/power/power.h
@@ -327,3 +327,5 @@ static inline void pm_sleep_enable_secon
 	suspend_enable_secondary_cpus();
 	cpuidle_resume();
 }
+
+void dpm_save_errno(int err);
Index: linux-pm/kernel/power/suspend.c
===================================================================
--- linux-pm.orig/kernel/power/suspend.c
+++ linux-pm/kernel/power/suspend.c
@@ -616,12 +616,7 @@ int pm_suspend(suspend_state_t state)
 
 	pr_info("suspend entry (%s)\n", mem_sleep_labels[state]);
 	error = enter_state(state);
-	if (error) {
-		suspend_stats.fail++;
-		dpm_save_failed_errno(error);
-	} else {
-		suspend_stats.step_failures[SUSPEND_NONE]++;
-	}
+	dpm_save_errno(error);
 	pr_info("suspend exit\n");
 	return error;
 }
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -60,7 +60,6 @@ static LIST_HEAD(dpm_suspended_list);
 static LIST_HEAD(dpm_late_early_list);
 static LIST_HEAD(dpm_noirq_list);
 
-struct suspend_stats suspend_stats;
 static DEFINE_MUTEX(dpm_list_mtx);
 static pm_message_t pm_transition;
 

Re: [PATCH v1 06/12] PM: sleep: stats: Define suspend_stats next to the code using it

[Stanislaw Gruszka]

On Mon, Jan 22, 2024 at 12:31:20PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> It is not necessary to define struct suspend_stats in a header file and the
> suspend_stats variable in the core device system-wide PM code.  They both
> can be defined in kernel/power/main.c, next to the sysfs and debugfs code
> accessing suspend_stats, which can be static.
> 
> Modify the code in question in accordance with the above observation and
> replace the static inline functions manipulating suspend_stats with
> regular ones defined in kernel/power/main.c.
> 
> While at it, move the enum suspend_stat_step to the end of suspend.h which
> is a more suitable place for it.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  drivers/base/power/main.c |    1 
>  include/linux/suspend.h   |   70 +++++++++----------------------------------
>  kernel/power/main.c       |   74 +++++++++++++++++++++++++++++++++++++---------
>  kernel/power/power.h      |    2 +
>  kernel/power/suspend.c    |    7 ----
>  5 files changed, 80 insertions(+), 74 deletions(-)
> 
> Index: linux-pm/include/linux/suspend.h
> ===================================================================
> --- linux-pm.orig/include/linux/suspend.h
> +++ linux-pm/include/linux/suspend.h
> @@ -40,60 +40,6 @@ typedef int __bitwise suspend_state_t;
>  #define PM_SUSPEND_MIN		PM_SUSPEND_TO_IDLE
>  #define PM_SUSPEND_MAX		((__force suspend_state_t) 4)
>  
> -enum suspend_stat_step {
> -	SUSPEND_NONE = 0,
> -	SUSPEND_FREEZE,
> -	SUSPEND_PREPARE,
> -	SUSPEND_SUSPEND,
> -	SUSPEND_SUSPEND_LATE,
> -	SUSPEND_SUSPEND_NOIRQ,
> -	SUSPEND_RESUME_NOIRQ,
> -	SUSPEND_RESUME_EARLY,
> -	SUSPEND_RESUME,
> -	SUSPEND_NR_STEPS
> -};
> -
> -struct suspend_stats {
> -	unsigned int step_failures[SUSPEND_NR_STEPS];
> -	unsigned int fail;
> -#define	REC_FAILED_NUM	2
> -	int	last_failed_dev;
> -	char	failed_devs[REC_FAILED_NUM][40];
> -	int	last_failed_errno;
> -	int	errno[REC_FAILED_NUM];
> -	int	last_failed_step;
> -	u64	last_hw_sleep;
> -	u64	total_hw_sleep;
> -	u64	max_hw_sleep;
> -	enum suspend_stat_step	failed_steps[REC_FAILED_NUM];
> -};
> -
> -extern struct suspend_stats suspend_stats;
> -
> -static inline void dpm_save_failed_dev(const char *name)
> -{
> -	strscpy(suspend_stats.failed_devs[suspend_stats.last_failed_dev],
> -		name,
> -		sizeof(suspend_stats.failed_devs[0]));
> -	suspend_stats.last_failed_dev++;
> -	suspend_stats.last_failed_dev %= REC_FAILED_NUM;
> -}
> -
> -static inline void dpm_save_failed_errno(int err)
> -{
> -	suspend_stats.errno[suspend_stats.last_failed_errno] = err;
> -	suspend_stats.last_failed_errno++;
> -	suspend_stats.last_failed_errno %= REC_FAILED_NUM;
> -}
> -
> -static inline void dpm_save_failed_step(enum suspend_stat_step step)
> -{
> -	suspend_stats.step_failures[step]++;
> -	suspend_stats.failed_steps[suspend_stats.last_failed_step] = step;
> -	suspend_stats.last_failed_step++;
> -	suspend_stats.last_failed_step %= REC_FAILED_NUM;
> -}
> -
>  /**
>   * struct platform_suspend_ops - Callbacks for managing platform dependent
>   *	system sleep states.
> @@ -621,4 +567,20 @@ static inline void queue_up_suspend_work
>  
>  #endif /* !CONFIG_PM_AUTOSLEEP */
>  
> +enum suspend_stat_step {
> +	SUSPEND_NONE = 0,
> +	SUSPEND_FREEZE,
> +	SUSPEND_PREPARE,
> +	SUSPEND_SUSPEND,
> +	SUSPEND_SUSPEND_LATE,
> +	SUSPEND_SUSPEND_NOIRQ,
> +	SUSPEND_RESUME_NOIRQ,
> +	SUSPEND_RESUME_EARLY,
> +	SUSPEND_RESUME,
> +	SUSPEND_NR_STEPS
> +};
> +
> +void dpm_save_failed_dev(const char *name);
> +void dpm_save_failed_step(enum suspend_stat_step step);
> +
>  #endif /* _LINUX_SUSPEND_H */
> Index: linux-pm/kernel/power/main.c
> ===================================================================
> --- linux-pm.orig/kernel/power/main.c
> +++ linux-pm/kernel/power/main.c
> @@ -95,19 +95,6 @@ int unregister_pm_notifier(struct notifi
>  }
>  EXPORT_SYMBOL_GPL(unregister_pm_notifier);
>  
> -void pm_report_hw_sleep_time(u64 t)
> -{
> -	suspend_stats.last_hw_sleep = t;
> -	suspend_stats.total_hw_sleep += t;
> -}
> -EXPORT_SYMBOL_GPL(pm_report_hw_sleep_time);
> -
> -void pm_report_max_hw_sleep(u64 t)
> -{
> -	suspend_stats.max_hw_sleep = t;
> -}
> -EXPORT_SYMBOL_GPL(pm_report_max_hw_sleep);
> -
>  int pm_notifier_call_chain_robust(unsigned long val_up, unsigned long val_down)
>  {
>  	int ret;
> @@ -319,6 +306,67 @@ static ssize_t pm_test_store(struct kobj
>  power_attr(pm_test);
>  #endif /* CONFIG_PM_SLEEP_DEBUG */
>  
> +#define REC_FAILED_NUM	2
> +
> +struct suspend_stats {
> +	unsigned int step_failures[SUSPEND_NR_STEPS];
> +	unsigned int fail;
> +	int last_failed_dev;
> +	char failed_devs[REC_FAILED_NUM][40];
> +	int last_failed_errno;
> +	int errno[REC_FAILED_NUM];
> +	int last_failed_step;
> +	u64 last_hw_sleep;
> +	u64 total_hw_sleep;
> +	u64 max_hw_sleep;
> +	enum suspend_stat_step failed_steps[REC_FAILED_NUM];
> +};
> +
> +static struct suspend_stats suspend_stats;
> +
> +void dpm_save_failed_dev(const char *name)
> +{
> +	strscpy(suspend_stats.failed_devs[suspend_stats.last_failed_dev],
> +		name, sizeof(suspend_stats.failed_devs[0]));
> +	suspend_stats.last_failed_dev++;
> +	suspend_stats.last_failed_dev %= REC_FAILED_NUM;
> +}
> +
> +void dpm_save_failed_step(enum suspend_stat_step step)
> +{
> +	suspend_stats.step_failures[step]++;
> +	suspend_stats.failed_steps[suspend_stats.last_failed_step] = step;
> +	suspend_stats.last_failed_step++;
> +	suspend_stats.last_failed_step %= REC_FAILED_NUM;
> +}
> +
> +void dpm_save_errno(int err)
> +{
> +	if (!err) {
> +		suspend_stats.step_failures[SUSPEND_NONE]++;
> +		return;
> +	}
> +
> +	suspend_stats.fail++;
> +
> +	suspend_stats.errno[suspend_stats.last_failed_errno] = err;
> +	suspend_stats.last_failed_errno++;
> +	suspend_stats.last_failed_errno %= REC_FAILED_NUM;
> +}
> +
> +void pm_report_hw_sleep_time(u64 t)
> +{
> +	suspend_stats.last_hw_sleep = t;
> +	suspend_stats.total_hw_sleep += t;
> +}
> +EXPORT_SYMBOL_GPL(pm_report_hw_sleep_time);
> +
> +void pm_report_max_hw_sleep(u64 t)
> +{
> +	suspend_stats.max_hw_sleep = t;
> +}
> +EXPORT_SYMBOL_GPL(pm_report_max_hw_sleep);
> +
>  static const char * const suspend_step_names[] = {
>  	[SUSPEND_NONE] = "",
>  	[SUSPEND_FREEZE] = "freeze",
> Index: linux-pm/kernel/power/power.h
> ===================================================================
> --- linux-pm.orig/kernel/power/power.h
> +++ linux-pm/kernel/power/power.h
> @@ -327,3 +327,5 @@ static inline void pm_sleep_enable_secon
>  	suspend_enable_secondary_cpus();
>  	cpuidle_resume();
>  }
> +
> +void dpm_save_errno(int err);
> Index: linux-pm/kernel/power/suspend.c
> ===================================================================
> --- linux-pm.orig/kernel/power/suspend.c
> +++ linux-pm/kernel/power/suspend.c
> @@ -616,12 +616,7 @@ int pm_suspend(suspend_state_t state)
>  
>  	pr_info("suspend entry (%s)\n", mem_sleep_labels[state]);
>  	error = enter_state(state);
> -	if (error) {
> -		suspend_stats.fail++;
> -		dpm_save_failed_errno(error);
> -	} else {
> -		suspend_stats.step_failures[SUSPEND_NONE]++;
> -	}
> +	dpm_save_errno(error);
>  	pr_info("suspend exit\n");
>  	return error;
>  }
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -60,7 +60,6 @@ static LIST_HEAD(dpm_suspended_list);
>  static LIST_HEAD(dpm_late_early_list);
>  static LIST_HEAD(dpm_noirq_list);
>  
> -struct suspend_stats suspend_stats;
>  static DEFINE_MUTEX(dpm_list_mtx);
>  static pm_message_t pm_transition;
>  
> 
> 
> 
> 

[PATCH v1 07/12] PM: sleep: stats: Call dpm_save_failed_step() at most once per phase

[Rafael J. Wysocki]

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

If the handling of two or more devices fails in one suspend-resume
phase, it should be counted once in the statistics which is not
guaranteed to happen during system-wide resume of devices due to
the possible asynchronous execution of device callbacks.

Address this by using the async_error static variable during system-wide
device resume to indicate that there has been a device resume error and
the given suspend-resume phase should be counted as failing.

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

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -685,7 +685,7 @@ Out:
 	TRACE_RESUME(error);
 
 	if (error) {
-		dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
+		async_error = error;
 		dpm_save_failed_dev(dev_name(dev));
 		pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
 	}
@@ -705,6 +705,9 @@ static void dpm_noirq_resume_devices(pm_
 	ktime_t starttime = ktime_get();
 
 	trace_suspend_resume(TPS("dpm_resume_noirq"), state.event, true);
+
+	async_error = 0;
+
 	mutex_lock(&dpm_list_mtx);
 	pm_transition = state;
 
@@ -734,6 +737,9 @@ static void dpm_noirq_resume_devices(pm_
 	mutex_unlock(&dpm_list_mtx);
 	async_synchronize_full();
 	dpm_show_time(starttime, state, 0, "noirq");
+	if (async_error)
+		dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
+
 	trace_suspend_resume(TPS("dpm_resume_noirq"), state.event, false);
 }
 
@@ -815,7 +821,7 @@ Out:
 	complete_all(&dev->power.completion);
 
 	if (error) {
-		dpm_save_failed_step(SUSPEND_RESUME_EARLY);
+		async_error = error;
 		dpm_save_failed_dev(dev_name(dev));
 		pm_dev_err(dev, state, async ? " async early" : " early", error);
 	}
@@ -839,6 +845,9 @@ void dpm_resume_early(pm_message_t state
 	ktime_t starttime = ktime_get();
 
 	trace_suspend_resume(TPS("dpm_resume_early"), state.event, true);
+
+	async_error = 0;
+
 	mutex_lock(&dpm_list_mtx);
 	pm_transition = state;
 
@@ -868,6 +877,9 @@ void dpm_resume_early(pm_message_t state
 	mutex_unlock(&dpm_list_mtx);
 	async_synchronize_full();
 	dpm_show_time(starttime, state, 0, "early");
+	if (async_error)
+		dpm_save_failed_step(SUSPEND_RESUME_EARLY);
+
 	trace_suspend_resume(TPS("dpm_resume_early"), state.event, false);
 }
 
@@ -971,7 +983,7 @@ static void device_resume(struct device
 	TRACE_RESUME(error);
 
 	if (error) {
-		dpm_save_failed_step(SUSPEND_RESUME);
+		async_error = error;
 		dpm_save_failed_dev(dev_name(dev));
 		pm_dev_err(dev, state, async ? " async" : "", error);
 	}
@@ -1030,6 +1042,8 @@ void dpm_resume(pm_message_t state)
 	mutex_unlock(&dpm_list_mtx);
 	async_synchronize_full();
 	dpm_show_time(starttime, state, 0, NULL);
+	if (async_error)
+		dpm_save_failed_step(SUSPEND_RESUME);
 
 	cpufreq_resume();
 	devfreq_resume();

[PATCH v1 08/12] PM: sleep: stats: Use locking in dpm_save_failed_dev()

[Rafael J. Wysocki]

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

Because dpm_save_failed_dev() may be called simultaneously by multiple
failing device PM functions, the state of the suspend_stats fields
updated by it may become inconsistent.

Prevent that from happening by using a lock in dpm_save_failed_dev().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 kernel/power/main.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: linux-pm/kernel/power/main.c
===================================================================
--- linux-pm.orig/kernel/power/main.c
+++ linux-pm/kernel/power/main.c
@@ -323,13 +323,18 @@ struct suspend_stats {
 };
 
 static struct suspend_stats suspend_stats;
+static DEFINE_MUTEX(suspend_stats_lock);
 
 void dpm_save_failed_dev(const char *name)
 {
+	mutex_lock(&suspend_stats_lock);
+
 	strscpy(suspend_stats.failed_devs[suspend_stats.last_failed_dev],
 		name, sizeof(suspend_stats.failed_devs[0]));
 	suspend_stats.last_failed_dev++;
 	suspend_stats.last_failed_dev %= REC_FAILED_NUM;
+
+	mutex_unlock(&suspend_stats_lock);
 }
 
 void dpm_save_failed_step(enum suspend_stat_step step)

Re: [PATCH v1 08/12] PM: sleep: stats: Use locking in dpm_save_failed_dev()

[Stanislaw Gruszka]

On Mon, Jan 22, 2024 at 12:33:53PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Because dpm_save_failed_dev() may be called simultaneously by multiple
> failing device PM functions, the state of the suspend_stats fields
> updated by it may become inconsistent.
> 
> Prevent that from happening by using a lock in dpm_save_failed_dev().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  kernel/power/main.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> Index: linux-pm/kernel/power/main.c
> ===================================================================
> --- linux-pm.orig/kernel/power/main.c
> +++ linux-pm/kernel/power/main.c
> @@ -323,13 +323,18 @@ struct suspend_stats {
>  };
>  
>  static struct suspend_stats suspend_stats;
> +static DEFINE_MUTEX(suspend_stats_lock);
>  
>  void dpm_save_failed_dev(const char *name)
>  {
> +	mutex_lock(&suspend_stats_lock);
> +
>  	strscpy(suspend_stats.failed_devs[suspend_stats.last_failed_dev],
>  		name, sizeof(suspend_stats.failed_devs[0]));
>  	suspend_stats.last_failed_dev++;
>  	suspend_stats.last_failed_dev %= REC_FAILED_NUM;
> +
> +	mutex_unlock(&suspend_stats_lock);
>  }
>  
>  void dpm_save_failed_step(enum suspend_stat_step step)
> 
> 
> 
> 

[PATCH v1 09/12] PM: sleep: stats: Log errors right after running suspend callbacks

[Rafael J. Wysocki]

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

The error logging and failure statistics updates are carried out in two
places in each system-wide device suspend phase, which is unnecessary
code duplication, so do that in one place in each phase, right after
invoking device suspend callbacks.

While at it, add "noirq" or "late" to the "async" string printed when
the failing device callback in the "noirq" or "late" suspend phase,
respectively, was run asynchronously.

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

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1244,6 +1244,8 @@ Run:
 	error = dpm_run_callback(callback, dev, state, info);
 	if (error) {
 		async_error = error;
+		dpm_save_failed_dev(dev_name(dev));
+		pm_dev_err(dev, state, async ? " async noirq" : " noirq", error);
 		goto Complete;
 	}
 
@@ -1273,14 +1275,8 @@ Complete:
 static void async_suspend_noirq(void *data, async_cookie_t cookie)
 {
 	struct device *dev = data;
-	int error;
-
-	error = __device_suspend_noirq(dev, pm_transition, true);
-	if (error) {
-		dpm_save_failed_dev(dev_name(dev));
-		pm_dev_err(dev, pm_transition, " async", error);
-	}
 
+	__device_suspend_noirq(dev, pm_transition, true);
 	put_device(dev);
 }
 
@@ -1312,12 +1308,8 @@ static int dpm_noirq_suspend_devices(pm_
 
 		mutex_lock(&dpm_list_mtx);
 
-		if (error) {
-			pm_dev_err(dev, state, " noirq", error);
-			dpm_save_failed_dev(dev_name(dev));
-		} else if (!list_empty(&dev->power.entry)) {
+		if (!error && !list_empty(&dev->power.entry))
 			list_move(&dev->power.entry, &dpm_noirq_list);
-		}
 
 		mutex_unlock(&dpm_list_mtx);
 
@@ -1437,6 +1429,8 @@ Run:
 	error = dpm_run_callback(callback, dev, state, info);
 	if (error) {
 		async_error = error;
+		dpm_save_failed_dev(dev_name(dev));
+		pm_dev_err(dev, state, async ? " async late" : " late", error);
 		goto Complete;
 	}
 	dpm_propagate_wakeup_to_parent(dev);
@@ -1453,13 +1447,8 @@ Complete:
 static void async_suspend_late(void *data, async_cookie_t cookie)
 {
 	struct device *dev = data;
-	int error;
 
-	error = __device_suspend_late(dev, pm_transition, true);
-	if (error) {
-		dpm_save_failed_dev(dev_name(dev));
-		pm_dev_err(dev, pm_transition, " async", error);
-	}
+	__device_suspend_late(dev, pm_transition, true);
 	put_device(dev);
 }
 
@@ -1500,11 +1489,6 @@ int dpm_suspend_late(pm_message_t state)
 		if (!list_empty(&dev->power.entry))
 			list_move(&dev->power.entry, &dpm_late_early_list);
 
-		if (error) {
-			pm_dev_err(dev, state, " late", error);
-			dpm_save_failed_dev(dev_name(dev));
-		}
-
 		mutex_unlock(&dpm_list_mtx);
 
 		put_device(dev);
@@ -1719,8 +1703,11 @@ static int __device_suspend(struct devic
 	dpm_watchdog_clear(&wd);
 
  Complete:
-	if (error)
+	if (error) {
 		async_error = error;
+		dpm_save_failed_dev(dev_name(dev));
+		pm_dev_err(dev, state, async ? " async" : "", error);
+	}
 
 	complete_all(&dev->power.completion);
 	TRACE_SUSPEND(error);
@@ -1730,14 +1717,8 @@ static int __device_suspend(struct devic
 static void async_suspend(void *data, async_cookie_t cookie)
 {
 	struct device *dev = data;
-	int error;
-
-	error = __device_suspend(dev, pm_transition, true);
-	if (error) {
-		dpm_save_failed_dev(dev_name(dev));
-		pm_dev_err(dev, pm_transition, " async", error);
-	}
 
+	__device_suspend(dev, pm_transition, true);
 	put_device(dev);
 }
 
@@ -1778,12 +1759,8 @@ int dpm_suspend(pm_message_t state)
 
 		mutex_lock(&dpm_list_mtx);
 
-		if (error) {
-			pm_dev_err(dev, state, "", error);
-			dpm_save_failed_dev(dev_name(dev));
-		} else if (!list_empty(&dev->power.entry)) {
+		if (!error && !list_empty(&dev->power.entry))
 			list_move(&dev->power.entry, &dpm_suspended_list);
-		}
 
 		mutex_unlock(&dpm_list_mtx);
 

[PATCH v1 10/12] PM: sleep: Move some assignments from under a lock

[Rafael J. Wysocki]

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

The async_error and pm_transition variables are set under dpm_list_mtx
in multiple places in the system-wide device PM core code, which is
unnecessary and confusing, so rearrange the code so that the variables
in question are set before acquiring the lock.

While at it, add some empty code lines around locking to improve the
consistency of the code.

No intentional functional impact.

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

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -707,9 +707,9 @@ static void dpm_noirq_resume_devices(pm_
 	trace_suspend_resume(TPS("dpm_resume_noirq"), state.event, true);
 
 	async_error = 0;
+	pm_transition = state;
 
 	mutex_lock(&dpm_list_mtx);
-	pm_transition = state;
 
 	/*
 	 * Trigger the resume of "async" devices upfront so they don't have to
@@ -847,9 +847,9 @@ void dpm_resume_early(pm_message_t state
 	trace_suspend_resume(TPS("dpm_resume_early"), state.event, true);
 
 	async_error = 0;
+	pm_transition = state;
 
 	mutex_lock(&dpm_list_mtx);
-	pm_transition = state;
 
 	/*
 	 * Trigger the resume of "async" devices upfront so they don't have to
@@ -1012,10 +1012,11 @@ void dpm_resume(pm_message_t state)
 	trace_suspend_resume(TPS("dpm_resume"), state.event, true);
 	might_sleep();
 
-	mutex_lock(&dpm_list_mtx);
 	pm_transition = state;
 	async_error = 0;
 
+	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.
@@ -1294,10 +1295,12 @@ static int dpm_noirq_suspend_devices(pm_
 	int error = 0;
 
 	trace_suspend_resume(TPS("dpm_suspend_noirq"), state.event, true);
-	mutex_lock(&dpm_list_mtx);
+
 	pm_transition = state;
 	async_error = 0;
 
+	mutex_lock(&dpm_list_mtx);
+
 	while (!list_empty(&dpm_late_early_list)) {
 		struct device *dev = to_device(dpm_late_early_list.prev);
 
@@ -1320,7 +1323,9 @@ static int dpm_noirq_suspend_devices(pm_
 		if (error || async_error)
 			break;
 	}
+
 	mutex_unlock(&dpm_list_mtx);
+
 	async_synchronize_full();
 	if (!error)
 		error = async_error;
@@ -1470,11 +1475,14 @@ int dpm_suspend_late(pm_message_t state)
 	int error = 0;
 
 	trace_suspend_resume(TPS("dpm_suspend_late"), state.event, true);
-	wake_up_all_idle_cpus();
-	mutex_lock(&dpm_list_mtx);
+
 	pm_transition = state;
 	async_error = 0;
 
+	wake_up_all_idle_cpus();
+
+	mutex_lock(&dpm_list_mtx);
+
 	while (!list_empty(&dpm_suspended_list)) {
 		struct device *dev = to_device(dpm_suspended_list.prev);
 
@@ -1498,7 +1506,9 @@ int dpm_suspend_late(pm_message_t state)
 		if (error || async_error)
 			break;
 	}
+
 	mutex_unlock(&dpm_list_mtx);
+
 	async_synchronize_full();
 	if (!error)
 		error = async_error;
@@ -1745,9 +1755,11 @@ int dpm_suspend(pm_message_t state)
 	devfreq_suspend();
 	cpufreq_suspend();
 
-	mutex_lock(&dpm_list_mtx);
 	pm_transition = state;
 	async_error = 0;
+
+	mutex_lock(&dpm_list_mtx);
+
 	while (!list_empty(&dpm_prepared_list)) {
 		struct device *dev = to_device(dpm_prepared_list.prev);
 
@@ -1771,7 +1783,9 @@ int dpm_suspend(pm_message_t state)
 		if (error || async_error)
 			break;
 	}
+
 	mutex_unlock(&dpm_list_mtx);
+
 	async_synchronize_full();
 	if (!error)
 		error = async_error;

[PATCH v1 11/12] PM: sleep: Move devices to new lists earlier in each suspend phase

[Rafael J. Wysocki]

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

During a system-wide suspend of devices, dpm_noirq_suspend_devices(),
dpm_suspend_late() and dpm_suspend() move devices from one list to
another.  They do it with each device after its PM callback in the
given suspend phase has run or has been scheduled for asynchronous
execution, in case it is deleted from the current list in the
meantime.

However, devices can be moved to a new list before invoking their PM
callbacks (which usually is the case for the devices whose callbacks
are executed asynchronously anyway), because doing so does not affect
the ordering of that list.  In either case, each device is moved to
the new list after the previous device has been moved to it or gone
away, and if a device is removed, it does not matter which list it is
in at that point, because deleting an entry from a list does not change
the ordering of the other entries in it.

Accordingly, modify the functions mentioned above to move devices to
new lists without waiting for their PM callbacks to run regardless of
whether or not they run asynchronously.

No intentional functional impact.

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

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1304,18 +1304,12 @@ static int dpm_noirq_suspend_devices(pm_
 	while (!list_empty(&dpm_late_early_list)) {
 		struct device *dev = to_device(dpm_late_early_list.prev);
 
+		list_move(&dev->power.entry, &dpm_noirq_list);
 		get_device(dev);
 		mutex_unlock(&dpm_list_mtx);
 
 		error = device_suspend_noirq(dev);
 
-		mutex_lock(&dpm_list_mtx);
-
-		if (!error && !list_empty(&dev->power.entry))
-			list_move(&dev->power.entry, &dpm_noirq_list);
-
-		mutex_unlock(&dpm_list_mtx);
-
 		put_device(dev);
 
 		mutex_lock(&dpm_list_mtx);
@@ -1486,19 +1480,13 @@ int dpm_suspend_late(pm_message_t state)
 	while (!list_empty(&dpm_suspended_list)) {
 		struct device *dev = to_device(dpm_suspended_list.prev);
 
+		list_move(&dev->power.entry, &dpm_late_early_list);
 		get_device(dev);
 
 		mutex_unlock(&dpm_list_mtx);
 
 		error = device_suspend_late(dev);
 
-		mutex_lock(&dpm_list_mtx);
-
-		if (!list_empty(&dev->power.entry))
-			list_move(&dev->power.entry, &dpm_late_early_list);
-
-		mutex_unlock(&dpm_list_mtx);
-
 		put_device(dev);
 
 		mutex_lock(&dpm_list_mtx);
@@ -1763,19 +1751,13 @@ int dpm_suspend(pm_message_t state)
 	while (!list_empty(&dpm_prepared_list)) {
 		struct device *dev = to_device(dpm_prepared_list.prev);
 
+		list_move(&dev->power.entry, &dpm_suspended_list);
 		get_device(dev);
 
 		mutex_unlock(&dpm_list_mtx);
 
 		error = device_suspend(dev);
 
-		mutex_lock(&dpm_list_mtx);
-
-		if (!error && !list_empty(&dev->power.entry))
-			list_move(&dev->power.entry, &dpm_suspended_list);
-
-		mutex_unlock(&dpm_list_mtx);
-
 		put_device(dev);
 
 		mutex_lock(&dpm_list_mtx);

Re: [PATCH v1 11/12] PM: sleep: Move devices to new lists earlier in each suspend phase

[Stanislaw Gruszka]

On Mon, Jan 22, 2024 at 12:42:46PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> During a system-wide suspend of devices, dpm_noirq_suspend_devices(),
> dpm_suspend_late() and dpm_suspend() move devices from one list to
> another.  They do it with each device after its PM callback in the
> given suspend phase has run or has been scheduled for asynchronous
> execution, in case it is deleted from the current list in the
> meantime.
> 
> However, devices can be moved to a new list before invoking their PM
> callbacks (which usually is the case for the devices whose callbacks
> are executed asynchronously anyway), because doing so does not affect
> the ordering of that list.  In either case, each device is moved to
> the new list after the previous device has been moved to it or gone
> away, and if a device is removed, it does not matter which list it is
> in at that point, because deleting an entry from a list does not change
> the ordering of the other entries in it.
> 
> Accordingly, modify the functions mentioned above to move devices to
> new lists without waiting for their PM callbacks to run regardless of
> whether or not they run asynchronously.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  drivers/base/power/main.c |   24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -1304,18 +1304,12 @@ static int dpm_noirq_suspend_devices(pm_
>  	while (!list_empty(&dpm_late_early_list)) {
>  		struct device *dev = to_device(dpm_late_early_list.prev);
>  
> +		list_move(&dev->power.entry, &dpm_noirq_list);
>  		get_device(dev);
>  		mutex_unlock(&dpm_list_mtx);
>  
>  		error = device_suspend_noirq(dev);
>  
> -		mutex_lock(&dpm_list_mtx);
> -
> -		if (!error && !list_empty(&dev->power.entry))
> -			list_move(&dev->power.entry, &dpm_noirq_list);
> -
> -		mutex_unlock(&dpm_list_mtx);
> -
>  		put_device(dev);
>  
>  		mutex_lock(&dpm_list_mtx);
> @@ -1486,19 +1480,13 @@ int dpm_suspend_late(pm_message_t state)
>  	while (!list_empty(&dpm_suspended_list)) {
>  		struct device *dev = to_device(dpm_suspended_list.prev);
>  
> +		list_move(&dev->power.entry, &dpm_late_early_list);
>  		get_device(dev);
>  
>  		mutex_unlock(&dpm_list_mtx);
>  
>  		error = device_suspend_late(dev);
>  
> -		mutex_lock(&dpm_list_mtx);
> -
> -		if (!list_empty(&dev->power.entry))
> -			list_move(&dev->power.entry, &dpm_late_early_list);
> -
> -		mutex_unlock(&dpm_list_mtx);
> -
>  		put_device(dev);
>  
>  		mutex_lock(&dpm_list_mtx);
> @@ -1763,19 +1751,13 @@ int dpm_suspend(pm_message_t state)
>  	while (!list_empty(&dpm_prepared_list)) {
>  		struct device *dev = to_device(dpm_prepared_list.prev);
>  
> +		list_move(&dev->power.entry, &dpm_suspended_list);
>  		get_device(dev);
>  
>  		mutex_unlock(&dpm_list_mtx);
>  
>  		error = device_suspend(dev);
>  
> -		mutex_lock(&dpm_list_mtx);
> -
> -		if (!error && !list_empty(&dev->power.entry))
> -			list_move(&dev->power.entry, &dpm_suspended_list);
> -
> -		mutex_unlock(&dpm_list_mtx);
> -
>  		put_device(dev);
>  
>  		mutex_lock(&dpm_list_mtx);
> 
> 
> 
> 

[PATCH v1 12/12] PM: sleep: Call dpm_async_fn() directly in each suspend phase

[Rafael J. Wysocki]

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

Simplify the system-wide suspend of devices by invoking dpm_async_fn()
directly from the main loop in each suspend phase instead of using an
additional wrapper function for running it.

No intentional functional impact.

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

Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1192,7 +1192,7 @@ static void dpm_superior_set_must_resume
 }
 
 /**
- * __device_suspend_noirq - Execute a "noirq suspend" callback for given device.
+ * device_suspend_noirq - Execute a "noirq suspend" callback for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being suspended asynchronously.
@@ -1200,7 +1200,7 @@ static void dpm_superior_set_must_resume
  * The driver of @dev will not receive interrupts while this function is being
  * executed.
  */
-static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
+static int device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
 {
 	pm_callback_t callback = NULL;
 	const char *info = NULL;
@@ -1277,18 +1277,10 @@ static void async_suspend_noirq(void *da
 {
 	struct device *dev = data;
 
-	__device_suspend_noirq(dev, pm_transition, true);
+	device_suspend_noirq(dev, pm_transition, true);
 	put_device(dev);
 }
 
-static int device_suspend_noirq(struct device *dev)
-{
-	if (dpm_async_fn(dev, async_suspend_noirq))
-		return 0;
-
-	return __device_suspend_noirq(dev, pm_transition, false);
-}
-
 static int dpm_noirq_suspend_devices(pm_message_t state)
 {
 	ktime_t starttime = ktime_get();
@@ -1305,10 +1297,15 @@ static int dpm_noirq_suspend_devices(pm_
 		struct device *dev = to_device(dpm_late_early_list.prev);
 
 		list_move(&dev->power.entry, &dpm_noirq_list);
+
+		if (dpm_async_fn(dev, async_suspend_noirq))
+			continue;
+
 		get_device(dev);
+
 		mutex_unlock(&dpm_list_mtx);
 
-		error = device_suspend_noirq(dev);
+		error = device_suspend_noirq(dev, state, false);
 
 		put_device(dev);
 
@@ -1369,14 +1366,14 @@ static void dpm_propagate_wakeup_to_pare
 }
 
 /**
- * __device_suspend_late - Execute a "late suspend" callback for given device.
+ * device_suspend_late - Execute a "late suspend" callback for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being suspended asynchronously.
  *
  * Runtime PM is disabled for @dev while this function is being executed.
  */
-static int __device_suspend_late(struct device *dev, pm_message_t state, bool async)
+static int device_suspend_late(struct device *dev, pm_message_t state, bool async)
 {
 	pm_callback_t callback = NULL;
 	const char *info = NULL;
@@ -1447,18 +1444,10 @@ static void async_suspend_late(void *dat
 {
 	struct device *dev = data;
 
-	__device_suspend_late(dev, pm_transition, true);
+	device_suspend_late(dev, pm_transition, true);
 	put_device(dev);
 }
 
-static int device_suspend_late(struct device *dev)
-{
-	if (dpm_async_fn(dev, async_suspend_late))
-		return 0;
-
-	return __device_suspend_late(dev, pm_transition, false);
-}
-
 /**
  * dpm_suspend_late - Execute "late suspend" callbacks for all devices.
  * @state: PM transition of the system being carried out.
@@ -1481,11 +1470,15 @@ int dpm_suspend_late(pm_message_t state)
 		struct device *dev = to_device(dpm_suspended_list.prev);
 
 		list_move(&dev->power.entry, &dpm_late_early_list);
+
+		if (dpm_async_fn(dev, async_suspend_late))
+			continue;
+
 		get_device(dev);
 
 		mutex_unlock(&dpm_list_mtx);
 
-		error = device_suspend_late(dev);
+		error = device_suspend_late(dev, state, false);
 
 		put_device(dev);
 
@@ -1582,12 +1575,12 @@ static void dpm_clear_superiors_direct_c
 }
 
 /**
- * __device_suspend - Execute "suspend" callbacks for given device.
+ * device_suspend - Execute "suspend" callbacks for given device.
  * @dev: Device to handle.
  * @state: PM transition of the system being carried out.
  * @async: If true, the device is being suspended asynchronously.
  */
-static int __device_suspend(struct device *dev, pm_message_t state, bool async)
+static int device_suspend(struct device *dev, pm_message_t state, bool async)
 {
 	pm_callback_t callback = NULL;
 	const char *info = NULL;
@@ -1716,18 +1709,10 @@ static void async_suspend(void *data, as
 {
 	struct device *dev = data;
 
-	__device_suspend(dev, pm_transition, true);
+	device_suspend(dev, pm_transition, true);
 	put_device(dev);
 }
 
-static int device_suspend(struct device *dev)
-{
-	if (dpm_async_fn(dev, async_suspend))
-		return 0;
-
-	return __device_suspend(dev, pm_transition, false);
-}
-
 /**
  * dpm_suspend - Execute "suspend" callbacks for all non-sysdev devices.
  * @state: PM transition of the system being carried out.
@@ -1752,11 +1737,15 @@ int dpm_suspend(pm_message_t state)
 		struct device *dev = to_device(dpm_prepared_list.prev);
 
 		list_move(&dev->power.entry, &dpm_suspended_list);
+
+		if (dpm_async_fn(dev, async_suspend))
+			continue;
+
 		get_device(dev);
 
 		mutex_unlock(&dpm_list_mtx);
 
-		error = device_suspend(dev);
+		error = device_suspend(dev, state, false);
 
 		put_device(dev);