* [PATCH 0/2] PM: sleep: Rework cpuidle handling during system-wide PM transitions @ 2021-10-22 16:02 Rafael J. Wysocki 2021-10-22 16:04 ` [PATCH 1/2] PM: suspend: Do not pause cpuidle in the suspend-to-idle path Rafael J. Wysocki 2021-10-22 16:07 ` [PATCH 2/2] PM: sleep: Pause cpuidle later and resume it earlier during system transitions Rafael J. Wysocki 0 siblings, 2 replies; 5+ messages in thread From: Rafael J. Wysocki @ 2021-10-22 16:02 UTC (permalink / raw) To: Linux PM; +Cc: LKML, Ulf Hansson, Daniel Lezcano Hi All, Currently, cpuidle is paused before the last phase of suspending device (suspend_noirq), but first it doesn't make sense to pause it for suspend-to-idle, because suspend-to-idle resumes it anyway later, and then it need not be paused while suspending devices anyway. Patch [1/2] removes the pausing and resuming of cpuidle from the suspend-to-idle path and patch [1/2] combines these operations with the disabling and enabling of secondary CPUs (which does not take place in the suspend-to-idle path), respectively. Thanks! ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] PM: suspend: Do not pause cpuidle in the suspend-to-idle path 2021-10-22 16:02 [PATCH 0/2] PM: sleep: Rework cpuidle handling during system-wide PM transitions Rafael J. Wysocki @ 2021-10-22 16:04 ` Rafael J. Wysocki 2021-10-25 14:23 ` Ulf Hansson 2021-10-22 16:07 ` [PATCH 2/2] PM: sleep: Pause cpuidle later and resume it earlier during system transitions Rafael J. Wysocki 1 sibling, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2021-10-22 16:04 UTC (permalink / raw) To: Linux PM; +Cc: LKML, Ulf Hansson, Daniel Lezcano From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> It is pointless to pause cpuidle in the suspend-to-idle path, because it is going to be resumed in the same path later and pausing it does not serve any particular purpose in that case. Rework the code to avoid doing that. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/base/power/main.c | 11 ++++++----- kernel/power/suspend.c | 8 ++++++-- 2 files changed, 12 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 @@ -747,8 +747,6 @@ void dpm_resume_noirq(pm_message_t state resume_device_irqs(); device_wakeup_disarm_wake_irqs(); - - cpuidle_resume(); } /** @@ -881,6 +879,7 @@ void dpm_resume_early(pm_message_t state void dpm_resume_start(pm_message_t state) { dpm_resume_noirq(state); + cpuidle_resume(); dpm_resume_early(state); } EXPORT_SYMBOL_GPL(dpm_resume_start); @@ -1336,8 +1335,6 @@ int dpm_suspend_noirq(pm_message_t state { int ret; - cpuidle_pause(); - device_wakeup_arm_wake_irqs(); suspend_device_irqs(); @@ -1521,9 +1518,13 @@ int dpm_suspend_end(pm_message_t state) if (error) goto out; + cpuidle_pause(); + error = dpm_suspend_noirq(state); - if (error) + if (error) { + cpuidle_resume(); dpm_resume_early(resume_event(state)); + } out: dpm_show_time(starttime, state, error, "end"); Index: linux-pm/kernel/power/suspend.c =================================================================== --- linux-pm.orig/kernel/power/suspend.c +++ linux-pm/kernel/power/suspend.c @@ -97,7 +97,6 @@ static void s2idle_enter(void) raw_spin_unlock_irq(&s2idle_lock); cpus_read_lock(); - cpuidle_resume(); /* Push all the CPUs into the idle loop. */ wake_up_all_idle_cpus(); @@ -105,7 +104,6 @@ static void s2idle_enter(void) swait_event_exclusive(s2idle_wait_head, s2idle_state == S2IDLE_STATE_WAKE); - cpuidle_pause(); cpus_read_unlock(); raw_spin_lock_irq(&s2idle_lock); @@ -405,6 +403,9 @@ static int suspend_enter(suspend_state_t if (error) goto Devices_early_resume; + if (state != PM_SUSPEND_TO_IDLE) + cpuidle_pause(); + error = dpm_suspend_noirq(PMSG_SUSPEND); if (error) { pr_err("noirq suspend of devices failed\n"); @@ -459,6 +460,9 @@ static int suspend_enter(suspend_state_t dpm_resume_noirq(PMSG_RESUME); Platform_early_resume: + if (state != PM_SUSPEND_TO_IDLE) + cpuidle_resume(); + platform_resume_early(state); Devices_early_resume: ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] PM: suspend: Do not pause cpuidle in the suspend-to-idle path 2021-10-22 16:04 ` [PATCH 1/2] PM: suspend: Do not pause cpuidle in the suspend-to-idle path Rafael J. Wysocki @ 2021-10-25 14:23 ` Ulf Hansson 0 siblings, 0 replies; 5+ messages in thread From: Ulf Hansson @ 2021-10-25 14:23 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Linux PM, LKML, Daniel Lezcano On Fri, 22 Oct 2021 at 18:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > It is pointless to pause cpuidle in the suspend-to-idle path, > because it is going to be resumed in the same path later and > pausing it does not serve any particular purpose in that case. > > Rework the code to avoid doing that. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Tested-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/base/power/main.c | 11 ++++++----- > kernel/power/suspend.c | 8 ++++++-- > 2 files changed, 12 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 > @@ -747,8 +747,6 @@ void dpm_resume_noirq(pm_message_t state > > resume_device_irqs(); > device_wakeup_disarm_wake_irqs(); > - > - cpuidle_resume(); > } > > /** > @@ -881,6 +879,7 @@ void dpm_resume_early(pm_message_t state > void dpm_resume_start(pm_message_t state) > { > dpm_resume_noirq(state); > + cpuidle_resume(); > dpm_resume_early(state); > } > EXPORT_SYMBOL_GPL(dpm_resume_start); > @@ -1336,8 +1335,6 @@ int dpm_suspend_noirq(pm_message_t state > { > int ret; > > - cpuidle_pause(); > - > device_wakeup_arm_wake_irqs(); > suspend_device_irqs(); > > @@ -1521,9 +1518,13 @@ int dpm_suspend_end(pm_message_t state) > if (error) > goto out; > > + cpuidle_pause(); > + > error = dpm_suspend_noirq(state); > - if (error) > + if (error) { > + cpuidle_resume(); > dpm_resume_early(resume_event(state)); > + } > > out: > dpm_show_time(starttime, state, error, "end"); > Index: linux-pm/kernel/power/suspend.c > =================================================================== > --- linux-pm.orig/kernel/power/suspend.c > +++ linux-pm/kernel/power/suspend.c > @@ -97,7 +97,6 @@ static void s2idle_enter(void) > raw_spin_unlock_irq(&s2idle_lock); > > cpus_read_lock(); > - cpuidle_resume(); > > /* Push all the CPUs into the idle loop. */ > wake_up_all_idle_cpus(); > @@ -105,7 +104,6 @@ static void s2idle_enter(void) > swait_event_exclusive(s2idle_wait_head, > s2idle_state == S2IDLE_STATE_WAKE); > > - cpuidle_pause(); > cpus_read_unlock(); > > raw_spin_lock_irq(&s2idle_lock); > @@ -405,6 +403,9 @@ static int suspend_enter(suspend_state_t > if (error) > goto Devices_early_resume; > > + if (state != PM_SUSPEND_TO_IDLE) > + cpuidle_pause(); > + > error = dpm_suspend_noirq(PMSG_SUSPEND); > if (error) { > pr_err("noirq suspend of devices failed\n"); > @@ -459,6 +460,9 @@ static int suspend_enter(suspend_state_t > dpm_resume_noirq(PMSG_RESUME); > > Platform_early_resume: > + if (state != PM_SUSPEND_TO_IDLE) > + cpuidle_resume(); > + > platform_resume_early(state); > > Devices_early_resume: > > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] PM: sleep: Pause cpuidle later and resume it earlier during system transitions 2021-10-22 16:02 [PATCH 0/2] PM: sleep: Rework cpuidle handling during system-wide PM transitions Rafael J. Wysocki 2021-10-22 16:04 ` [PATCH 1/2] PM: suspend: Do not pause cpuidle in the suspend-to-idle path Rafael J. Wysocki @ 2021-10-22 16:07 ` Rafael J. Wysocki 2021-10-25 14:24 ` Ulf Hansson 1 sibling, 1 reply; 5+ messages in thread From: Rafael J. Wysocki @ 2021-10-22 16:07 UTC (permalink / raw) To: Linux PM; +Cc: LKML, Ulf Hansson, Daniel Lezcano From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Commit 8651f97bd951 ("PM / cpuidle: System resume hang fix with cpuidle") that introduced cpuidle pausing during system suspend did that to work around a platform firmware issue causing systems to hang during resume if CPUs were allowed to enter idle states in the system suspend and resume code paths. However, pausing cpuidle before the last phase of suspending devices is the source of an otherwise arbitrary difference between the suspend-to-idle path and other system suspend variants, so it is cleaner to do that later, before taking secondary CPUs offline (it is still safer to take secondary CPUs offline with cpuidle paused, though). Modify the code accordingly, but in order to avoid code duplication, introduce new wrapper functions, pm_sleep_disable_secondary_cpus() and pm_sleep_enable_secondary_cpus(), to combine cpuidle_pause() and cpuidle_resume(), respectively, with the handling of secondary CPUs during system-wide transitions to sleep states. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/base/power/main.c | 8 +------- kernel/power/hibernate.c | 12 +++++++----- kernel/power/power.h | 14 ++++++++++++++ kernel/power/suspend.c | 10 ++-------- 4 files changed, 24 insertions(+), 20 deletions(-) Index: linux-pm/kernel/power/suspend.c =================================================================== --- linux-pm.orig/kernel/power/suspend.c +++ linux-pm/kernel/power/suspend.c @@ -403,9 +403,6 @@ static int suspend_enter(suspend_state_t if (error) goto Devices_early_resume; - if (state != PM_SUSPEND_TO_IDLE) - cpuidle_pause(); - error = dpm_suspend_noirq(PMSG_SUSPEND); if (error) { pr_err("noirq suspend of devices failed\n"); @@ -423,7 +420,7 @@ static int suspend_enter(suspend_state_t goto Platform_wake; } - error = suspend_disable_secondary_cpus(); + error = pm_sleep_disable_secondary_cpus(); if (error || suspend_test(TEST_CPUS)) goto Enable_cpus; @@ -453,16 +450,13 @@ static int suspend_enter(suspend_state_t BUG_ON(irqs_disabled()); Enable_cpus: - suspend_enable_secondary_cpus(); + pm_sleep_enable_secondary_cpus(); Platform_wake: platform_resume_noirq(state); dpm_resume_noirq(PMSG_RESUME); Platform_early_resume: - if (state != PM_SUSPEND_TO_IDLE) - cpuidle_resume(); - platform_resume_early(state); Devices_early_resume: Index: linux-pm/kernel/power/hibernate.c =================================================================== --- linux-pm.orig/kernel/power/hibernate.c +++ linux-pm/kernel/power/hibernate.c @@ -300,7 +300,7 @@ static int create_image(int platform_mod if (error || hibernation_test(TEST_PLATFORM)) goto Platform_finish; - error = suspend_disable_secondary_cpus(); + error = pm_sleep_disable_secondary_cpus(); if (error || hibernation_test(TEST_CPUS)) goto Enable_cpus; @@ -342,7 +342,7 @@ static int create_image(int platform_mod local_irq_enable(); Enable_cpus: - suspend_enable_secondary_cpus(); + pm_sleep_enable_secondary_cpus(); /* Allow architectures to do nosmt-specific post-resume dances */ if (!in_suspend) @@ -466,6 +466,8 @@ static int resume_target_kernel(bool pla if (error) goto Cleanup; + cpuidle_pause(); + error = hibernate_resume_nonboot_cpu_disable(); if (error) goto Enable_cpus; @@ -509,7 +511,7 @@ static int resume_target_kernel(bool pla local_irq_enable(); Enable_cpus: - suspend_enable_secondary_cpus(); + pm_sleep_enable_secondary_cpus(); Cleanup: platform_restore_cleanup(platform_mode); @@ -587,7 +589,7 @@ int hibernation_platform_enter(void) if (error) goto Platform_finish; - error = suspend_disable_secondary_cpus(); + error = pm_sleep_disable_secondary_cpus(); if (error) goto Enable_cpus; @@ -609,7 +611,7 @@ int hibernation_platform_enter(void) local_irq_enable(); Enable_cpus: - suspend_enable_secondary_cpus(); + pm_sleep_enable_secondary_cpus(); Platform_finish: hibernation_ops->finish(); Index: linux-pm/drivers/base/power/main.c =================================================================== --- linux-pm.orig/drivers/base/power/main.c +++ linux-pm/drivers/base/power/main.c @@ -32,7 +32,6 @@ #include <linux/suspend.h> #include <trace/events/power.h> #include <linux/cpufreq.h> -#include <linux/cpuidle.h> #include <linux/devfreq.h> #include <linux/timer.h> @@ -879,7 +878,6 @@ void dpm_resume_early(pm_message_t state void dpm_resume_start(pm_message_t state) { dpm_resume_noirq(state); - cpuidle_resume(); dpm_resume_early(state); } EXPORT_SYMBOL_GPL(dpm_resume_start); @@ -1518,13 +1516,9 @@ int dpm_suspend_end(pm_message_t state) if (error) goto out; - cpuidle_pause(); - error = dpm_suspend_noirq(state); - if (error) { - cpuidle_resume(); + if (error) dpm_resume_early(resume_event(state)); - } out: dpm_show_time(starttime, state, error, "end"); Index: linux-pm/kernel/power/power.h =================================================================== --- linux-pm.orig/kernel/power/power.h +++ linux-pm/kernel/power/power.h @@ -4,6 +4,8 @@ #include <linux/utsname.h> #include <linux/freezer.h> #include <linux/compiler.h> +#include <linux/cpu.h> +#include <linux/cpuidle.h> struct swsusp_info { struct new_utsname uts; @@ -310,3 +312,15 @@ extern int pm_wake_lock(const char *buf) extern int pm_wake_unlock(const char *buf); #endif /* !CONFIG_PM_WAKELOCKS */ + +static inline int pm_sleep_disable_secondary_cpus(void) +{ + cpuidle_pause(); + return suspend_disable_secondary_cpus(); +} + +static inline void pm_sleep_enable_secondary_cpus(void) +{ + suspend_enable_secondary_cpus(); + cpuidle_resume(); +} ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] PM: sleep: Pause cpuidle later and resume it earlier during system transitions 2021-10-22 16:07 ` [PATCH 2/2] PM: sleep: Pause cpuidle later and resume it earlier during system transitions Rafael J. Wysocki @ 2021-10-25 14:24 ` Ulf Hansson 0 siblings, 0 replies; 5+ messages in thread From: Ulf Hansson @ 2021-10-25 14:24 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: Linux PM, LKML, Daniel Lezcano On Fri, 22 Oct 2021 at 18:08, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Commit 8651f97bd951 ("PM / cpuidle: System resume hang fix with > cpuidle") that introduced cpuidle pausing during system suspend > did that to work around a platform firmware issue causing systems > to hang during resume if CPUs were allowed to enter idle states > in the system suspend and resume code paths. > > However, pausing cpuidle before the last phase of suspending > devices is the source of an otherwise arbitrary difference between > the suspend-to-idle path and other system suspend variants, so it is > cleaner to do that later, before taking secondary CPUs offline (it > is still safer to take secondary CPUs offline with cpuidle paused, > though). > > Modify the code accordingly, but in order to avoid code duplication, > introduce new wrapper functions, pm_sleep_disable_secondary_cpus() > and pm_sleep_enable_secondary_cpus(), to combine cpuidle_pause() > and cpuidle_resume(), respectively, with the handling of secondary > CPUs during system-wide transitions to sleep states. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Tested-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/base/power/main.c | 8 +------- > kernel/power/hibernate.c | 12 +++++++----- > kernel/power/power.h | 14 ++++++++++++++ > kernel/power/suspend.c | 10 ++-------- > 4 files changed, 24 insertions(+), 20 deletions(-) > > Index: linux-pm/kernel/power/suspend.c > =================================================================== > --- linux-pm.orig/kernel/power/suspend.c > +++ linux-pm/kernel/power/suspend.c > @@ -403,9 +403,6 @@ static int suspend_enter(suspend_state_t > if (error) > goto Devices_early_resume; > > - if (state != PM_SUSPEND_TO_IDLE) > - cpuidle_pause(); > - > error = dpm_suspend_noirq(PMSG_SUSPEND); > if (error) { > pr_err("noirq suspend of devices failed\n"); > @@ -423,7 +420,7 @@ static int suspend_enter(suspend_state_t > goto Platform_wake; > } > > - error = suspend_disable_secondary_cpus(); > + error = pm_sleep_disable_secondary_cpus(); > if (error || suspend_test(TEST_CPUS)) > goto Enable_cpus; > > @@ -453,16 +450,13 @@ static int suspend_enter(suspend_state_t > BUG_ON(irqs_disabled()); > > Enable_cpus: > - suspend_enable_secondary_cpus(); > + pm_sleep_enable_secondary_cpus(); > > Platform_wake: > platform_resume_noirq(state); > dpm_resume_noirq(PMSG_RESUME); > > Platform_early_resume: > - if (state != PM_SUSPEND_TO_IDLE) > - cpuidle_resume(); > - > platform_resume_early(state); > > Devices_early_resume: > Index: linux-pm/kernel/power/hibernate.c > =================================================================== > --- linux-pm.orig/kernel/power/hibernate.c > +++ linux-pm/kernel/power/hibernate.c > @@ -300,7 +300,7 @@ static int create_image(int platform_mod > if (error || hibernation_test(TEST_PLATFORM)) > goto Platform_finish; > > - error = suspend_disable_secondary_cpus(); > + error = pm_sleep_disable_secondary_cpus(); > if (error || hibernation_test(TEST_CPUS)) > goto Enable_cpus; > > @@ -342,7 +342,7 @@ static int create_image(int platform_mod > local_irq_enable(); > > Enable_cpus: > - suspend_enable_secondary_cpus(); > + pm_sleep_enable_secondary_cpus(); > > /* Allow architectures to do nosmt-specific post-resume dances */ > if (!in_suspend) > @@ -466,6 +466,8 @@ static int resume_target_kernel(bool pla > if (error) > goto Cleanup; > > + cpuidle_pause(); > + > error = hibernate_resume_nonboot_cpu_disable(); > if (error) > goto Enable_cpus; > @@ -509,7 +511,7 @@ static int resume_target_kernel(bool pla > local_irq_enable(); > > Enable_cpus: > - suspend_enable_secondary_cpus(); > + pm_sleep_enable_secondary_cpus(); > > Cleanup: > platform_restore_cleanup(platform_mode); > @@ -587,7 +589,7 @@ int hibernation_platform_enter(void) > if (error) > goto Platform_finish; > > - error = suspend_disable_secondary_cpus(); > + error = pm_sleep_disable_secondary_cpus(); > if (error) > goto Enable_cpus; > > @@ -609,7 +611,7 @@ int hibernation_platform_enter(void) > local_irq_enable(); > > Enable_cpus: > - suspend_enable_secondary_cpus(); > + pm_sleep_enable_secondary_cpus(); > > Platform_finish: > hibernation_ops->finish(); > Index: linux-pm/drivers/base/power/main.c > =================================================================== > --- linux-pm.orig/drivers/base/power/main.c > +++ linux-pm/drivers/base/power/main.c > @@ -32,7 +32,6 @@ > #include <linux/suspend.h> > #include <trace/events/power.h> > #include <linux/cpufreq.h> > -#include <linux/cpuidle.h> > #include <linux/devfreq.h> > #include <linux/timer.h> > > @@ -879,7 +878,6 @@ void dpm_resume_early(pm_message_t state > void dpm_resume_start(pm_message_t state) > { > dpm_resume_noirq(state); > - cpuidle_resume(); > dpm_resume_early(state); > } > EXPORT_SYMBOL_GPL(dpm_resume_start); > @@ -1518,13 +1516,9 @@ int dpm_suspend_end(pm_message_t state) > if (error) > goto out; > > - cpuidle_pause(); > - > error = dpm_suspend_noirq(state); > - if (error) { > - cpuidle_resume(); > + if (error) > dpm_resume_early(resume_event(state)); > - } > > out: > dpm_show_time(starttime, state, error, "end"); > Index: linux-pm/kernel/power/power.h > =================================================================== > --- linux-pm.orig/kernel/power/power.h > +++ linux-pm/kernel/power/power.h > @@ -4,6 +4,8 @@ > #include <linux/utsname.h> > #include <linux/freezer.h> > #include <linux/compiler.h> > +#include <linux/cpu.h> > +#include <linux/cpuidle.h> > > struct swsusp_info { > struct new_utsname uts; > @@ -310,3 +312,15 @@ extern int pm_wake_lock(const char *buf) > extern int pm_wake_unlock(const char *buf); > > #endif /* !CONFIG_PM_WAKELOCKS */ > + > +static inline int pm_sleep_disable_secondary_cpus(void) > +{ > + cpuidle_pause(); > + return suspend_disable_secondary_cpus(); > +} > + > +static inline void pm_sleep_enable_secondary_cpus(void) > +{ > + suspend_enable_secondary_cpus(); > + cpuidle_resume(); > +} > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-25 14:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-22 16:02 [PATCH 0/2] PM: sleep: Rework cpuidle handling during system-wide PM transitions Rafael J. Wysocki 2021-10-22 16:04 ` [PATCH 1/2] PM: suspend: Do not pause cpuidle in the suspend-to-idle path Rafael J. Wysocki 2021-10-25 14:23 ` Ulf Hansson 2021-10-22 16:07 ` [PATCH 2/2] PM: sleep: Pause cpuidle later and resume it earlier during system transitions Rafael J. Wysocki 2021-10-25 14:24 ` Ulf Hansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).