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!
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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;
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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;
On Mon, Jan 22, 2024 at 12:25:45PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Stanislaw Gruszka <[email protected]>
> ---
> 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;
>
>
>