PM: sleep: Fix up suspend stats handling and clean up core suspend code
Subsystems (4)
v2 cover PM: sleep: Fix up suspend stats handling and clean up core suspend code
Hi Everyone,
This is a v2 of
https://lore.kernel.org/linux-pm/5760158.DvuYhMxLoT@kreacher
except for the first two patches that have been queued up for 6.9 already.
The original series description still applies:
Quoted 6 lines
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.
and the differences from the v1 are minor.
Please refer to individual patch changelogs for details.
Thanks!
PATCH v2 1/10 PM: sleep: stats: Use array of suspend step names
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() and adjust some white space in there to the kernel coding style for a more consistent code layout.
No intentional functional impact.
---
v1 -> v2: Added R-by from Stanislaw.
--- include/linux/suspend.h | 3 +- kernel/power/main.c | 50 +++++++++++++++++------------------------------- 2 files changed, 20 insertions(+), 33 deletions(-)
include/linux/suspend.h +2 -1
@@ -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 ,
kernel/power/main.c +18 -32
89 lines not on this page · raw message
PATCH v2 2/10 PM: sleep: stats: Use an array of step failure counters
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
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.
---
v1 -> v2:
* Use one cell less in suspend_stats.step_failures[] to avoid introducing an unused array cell (Stanislaw).
@Stanislaw: This is different from setting SUSPEND_FREEZE to 0, because that would complicate printing in the sysfs attributes and the debugfs code, so I've not added the R-by.
--- drivers/base/power/main.c | 22 ++++++++----------- include/linux/suspend.h | 12 +++------- kernel/power/main.c | 51 ++++++++++++++++++++++++---------------------- kernel/power/suspend.c | 1 4 files changed, 40 insertions(+), 46 deletions(-)
include/linux/suspend.h +4 -8
@@ -52,17 +52,12 @@ enum suspend_stat_step {SUSPEND_RESUME }; +#define SUSPEND_NR_STEPS SUSPEND_RESUME +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 2int last_failed_dev;char failed_devs[REC_FAILED_NUM ][40 ];
@@ -95,6 +90,7 @@ static inline void dpm_save_failed_errnostatic inline void dpm_save_failed_step (enum suspend_stat_step step) { + suspend_stats.step_failures[step-1 ]++; suspend_stats.failed_steps[suspend_stats.last_failed_step] = step; suspend_stats.last_failed_step++; suspend_stats.last_failed_step %=REC_FAILED_NUM ;
kernel/power/main.c +27 -24
73 lines not on this page · raw message
kernel/power/suspend.c +0 -1
@@ -367,7 +367,6 @@ static int suspend_prepare(suspend_stateif (!error)return 0 ; - suspend_stats.failed_freeze++;dpm_save_failed_step (SUSPEND_FREEZE );pm_notifier_call_chain (PM_POST_SUSPEND ); Restore:
drivers/base/power/main.c +9 -13
@@ -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 deviceTRACE_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 stateint 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 v2 02/10] PM: sleep: stats: Use an array of step failure counters
Quoted 21 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Instead of using a set of individual struct
suspend_statsfields representing suspend step failure counters, use an array of counters indexed by enumsuspend_stat_stepfor this purpose, which allowsdpm_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_statsdebugfs file which is not expected to matter.Signed-off-by Rafael J. Wysocki <[email protected]>---
Quoted 7 lines
v1 -> v2:
* Use one cell less in suspend_stats.step_failures[] to avoid introducing an unused array cell (Stanislaw).@Stanislaw: This is different from setting
SUSPEND_FREEZEto 0, because that would complicate printing in the sysfs attributes and the debugfs code, so I've not added the R-by.
LGTM.
kernel/power/main.c static int suspend_stats_show(struct seq · line 466
Quoted 3 lines
kernel/power/main.c +3
-0+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-1 ]);
Consider (in separate patch) removing SUSPEND_NONE from suspend_step_names[] and use step-1 for it as well.
Regards
Stanislaw
Rafael J. Wysocki
maintainerRe: [PATCH v2 02/10] PM: sleep: stats: Use an array of step failure counters
Quoted 33 lines · Stanislaw Gruszka
Quoted 21 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Instead of using a set of individual struct
suspend_statsfields representing suspend step failure counters, use an array of counters indexed by enumsuspend_stat_stepfor this purpose, which allowsdpm_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_statsdebugfs file which is not expected to matter.Signed-off-by Rafael J. Wysocki <[email protected]>---
Quoted 7 lines
v1 -> v2:
* Use one cell less in suspend_stats.step_failures[] to avoid introducing an unused array cell (Stanislaw).@Stanislaw: This is different from setting
SUSPEND_FREEZEto 0, because that would complicate printing in the sysfs attributes and the debugfs code, so I've not added the R-by.LGTM.
Quoted 8 lines
Reviewed-by Stanislaw Gruszka <[email protected]>kernel/power/main.c static int suspend_stats_show(struct seq · line 466
Quoted 3 lines
kernel/power/main.c +3
-0+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-1 ]);Consider (in separate patch) removing
SUSPEND_NONEfrom suspend_step_names[] and use step-1 for it as well.
SUSPEND_NONE is handy for printing an empty string when there are no suspend-resume errors.
PATCH v2 3/10 PM: sleep: stats: Use unsigned int for success and failure counters
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
Change the type of the "success" and "fail" fields in struct suspend_stats to unsigned int, because they cannot be negative.
No intentional functional impact.
---
v1 -> v2: New patch.
--- include/linux/suspend.h | 4 ++-- kernel/power/main.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)
include/linux/suspend.h +2 -2
@@ -56,8 +56,8 @@ enum suspend_stat_step {struct suspend_stats {unsigned int step_failures[SUSPEND_NR_STEPS ]; -int success; -int fail; +unsigned int success; +unsigned int fail;#define REC_FAILED_NUM 2int last_failed_dev;char failed_devs[REC_FAILED_NUM ][40 ];
kernel/power/main.c +3 -3
@@ -339,8 +339,8 @@ 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 (success,"%u\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" );
@@ -458,7 +458,7 @@ 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" , +seq_printf (s,"success: %u\nfail: %u\n" , suspend_stats.success, suspend_stats.fail);for (step =SUSPEND_FREEZE ; step <=SUSPEND_NR_STEPS ; step++)
Re: [PATCH v2 03/10] PM: sleep: stats: Use unsigned int for success and failure counters
Quoted 8 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
Change the type of the "success" and "fail" fields in struct
suspend_statsto unsigned int, because they cannot be negative.No intentional functional impact.
Signed-off-by Rafael J. Wysocki <[email protected]>
Quoted 52 lines
---
v1 -> v2: New patch.
--- include/linux/suspend.h | 4 ++-- kernel/power/main.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-)Index:
linux-pm/include/linux/suspend.h
===================================================================include/linux/suspend.h +2
-2@@ -56,8 +56,8 @@ enum suspend_stat_step {struct suspend_stats {unsigned int step_failures[SUSPEND_NR_STEPS ]; -int success; -int fail; +unsigned int success; +unsigned int fail;#define REC_FAILED_NUM 2int last_failed_dev;char failed_devs[REC_FAILED_NUM ][40 ];Index:
linux-pm/kernel/power/main.c
===================================================================kernel/power/main.c +3
-3@@ -339,8 +339,8 @@ 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 (success,"%u\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" );@@ -458,7 +458,7 @@ 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" , +seq_printf (s,"success: %u\nfail: %u\n" , suspend_stats.success, suspend_stats.fail);for (step =SUSPEND_FREEZE ; step <=SUSPEND_NR_STEPS ; step++)
PATCH v2 4/10 PM: sleep: stats: Define suspend_stats next to the code using it
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
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.
---
v1 -> v2:
* Take the modifications of patches [2-3/10] into account.
--- drivers/base/power/main.c | 1 include/linux/suspend.h | 71 +++++++++--------------------------------- kernel/power/main.c | 76 ++++++++++++++++++++++++++++++++++++++-------- kernel/power/power.h | 2 + kernel/power/suspend.c | 7 ---- 5 files changed, 81 insertions(+), 76 deletions(-)
include/linux/suspend.h +15 -56
@@ -40,62 +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 -}; - -#define SUSPEND_NR_STEPS SUSPEND_RESUME - -struct suspend_stats { -unsigned int step_failures[SUSPEND_NR_STEPS ]; -unsigned int success; -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-1 ]++; - 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.
@@ -623,4 +567,19 @@ 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 +}; + +void dpm_save_failed_dev (const char *name); +void dpm_save_failed_step (enum suspend_stat_step step); +#endif /* _LINUX_SUSPEND_H */
kernel/power/main.c +63 -13
88 lines not on this page · raw message
kernel/power/power.h +2 -0
@@ -327,3 +327,5 @@ static inline void pm_sleep_enable_seconsuspend_enable_secondary_cpus ();cpuidle_resume (); } + +void dpm_save_errno (int err);
kernel/power/suspend.c +1 -6
@@ -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.success++; - } +dpm_save_errno (error);pr_info ("suspend exit\n" );return error; }
drivers/base/power/main.c +0 -1
@@ -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 v2 5/10 PM: sleep: stats: Call dpm_save_failed_step() at most once per phase
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
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.
---
v1 -> v2: No changes.
--- drivers/base/power/main.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
drivers/base/power/main.c +17 -3
@@ -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 statektime_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 statemutex_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 deviceTRACE_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 ();
Re: [PATCH v2 05/10] PM: sleep: stats: Call dpm_save_failed_step() at most once per phase
Quoted 7 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
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.
Quoted 5 lines
Address this by using the
async_errorstatic 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 <[email protected]>
Quoted 91 lines
---
v1 -> v2: No changes.
--- drivers/base/power/main.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)Index:
linux-pm/drivers/base/power/main.c
===================================================================drivers/base/power/main.c +17
-3@@ -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 statektime_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 statemutex_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 deviceTRACE_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 v2 6/10 PM: sleep: stats: Use locking in dpm_save_failed_dev()
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
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().
---
v1 -> v2: Added R-by from Stanislaw.
--- kernel/power/main.c | 5 +++++ 1 file changed, 5 insertions(+)
kernel/power/main.c +5 -0
@@ -325,13 +325,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 v2 7/10 PM: sleep: stats: Log errors right after running suspend callbacks
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
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.
--- drivers/base/power/main.c | 49 ++++++++++++---------------------------------- 1 file changed, 13 insertions(+), 36 deletions(-)
drivers/base/power/main.c +13 -36
@@ -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 devicdpm_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 devicstatic 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);
Re: [PATCH v2 07/10] PM: sleep: stats: Log errors right after running suspend callbacks
Quoted 7 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
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.
Quoted 5 lines
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 <[email protected]>
Quoted 129 lines
--- drivers/base/power/main.c | 49 ++++++++++++---------------------------------- 1 file changed, 13 insertions(+), 36 deletions(-)Index:
linux-pm/drivers/base/power/main.c
===================================================================drivers/base/power/main.c +13
-36@@ -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 devicdpm_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 devicstatic 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 v2 8/10 PM: sleep: Move some assignments from under a lock
In reply to Rafael J. WysockiFrom: 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.
---
v1 -> v2: No changes.
--- drivers/base/power/main.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)
drivers/base/power/main.c +21 -7
@@ -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 statetrace_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;
Re: [PATCH v2 08/10] PM: sleep: Move some assignments from under a lock
Quoted 7 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
The
async_errorandpm_transitionvariables are set underdpm_list_mtxin 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.
Quoted 6 lines
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]>
Quoted 125 lines
---
v1 -> v2: No changes.
--- drivers/base/power/main.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)Index:
linux-pm/drivers/base/power/main.c
===================================================================drivers/base/power/main.c +21
-7@@ -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 statetrace_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 v2 9/10 PM: sleep: Move devices to new lists earlier in each suspend phase
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
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.
---
v1 -> v2: Added R-by from Stanislaw.
--- drivers/base/power/main.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-)
drivers/base/power/main.c +3 -21
@@ -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 v2 10/10 PM: sleep: Call dpm_async_fn() directly in each suspend phase
In reply to Rafael J. WysockiFrom: Rafael J. Wysocki <[email protected]>
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.
---
v1 -> v2: No changes.
--- drivers/base/power/main.c | 61 ++++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 36 deletions(-)
drivers/base/power/main.c +25 -36
@@ -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);
Re: [PATCH v2 10/10] PM: sleep: Call dpm_async_fn() directly in each suspend phase
Quoted 9 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
Quoted 176 lines
---
v1 -> v2: No changes.
--- drivers/base/power/main.c | 61 ++++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 36 deletions(-)Index:
linux-pm/drivers/base/power/main.c
===================================================================drivers/base/power/main.c +25
-36@@ -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);
Quoted 16 lines · Rafael J. Wysocki
Hi Everyone,
This is a v2 of
https://lore.kernel.org/linux-pm/5760158.DvuYhMxLoT@kreacher
except for the first two patches that have been queued up for 6.9 already.
The original series description still applies:
Quoted 4 lines
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.
Quoted 8 lines
Quoted 2 lines
The functional changes made by it are expected to be limited to the statistics handling part.
and the differences from the v1 are minor.
Please refer to individual patch changelogs for details.
Thanks!
I have looked through the series and it looks very nice to me!
Feel free to add:
Kind regards
Uffe
Rafael J. Wysocki
maintainerQuoted 23 lines · Ulf Hansson
Quoted 21 lines · Rafael J. Wysocki
Hi Everyone,
This is a v2 of
https://lore.kernel.org/linux-pm/5760158.DvuYhMxLoT@kreacher
except for the first two patches that have been queued up for 6.9 already.
The original series description still applies:
Quoted 6 lines
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.
and the differences from the v1 are minor.
Quoted 8 lines
Quoted 3 lines
Please refer to individual patch changelogs for details.
Thanks!
I have looked through the series and it looks very nice to me!
Feel free to add:
Reviewed-by Ulf Hansson <[email protected]>
Thank you!