From: Ulf Hansson <ulf.hansson@linaro.org>
To: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Linux PM <linux-pm@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Daniel Lezcano <daniel.lezcano@linaro.org>
Subject: Re: [PATCH 2/2] PM: sleep: Pause cpuidle later and resume it earlier during system transitions
Date: Mon, 25 Oct 2021 16:24:10 +0200 [thread overview]
Message-ID: <CAPDyKFoVGgCNz6AMUpwSwodJvEQN0L1LoaNQ4-A2sDn_K1RfSA@mail.gmail.com> (raw)
In-Reply-To: <2813389.e9J7NaK4W3@kreacher>
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();
> +}
>
>
>
prev parent reply other threads:[~2021-10-25 14:24 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPDyKFoVGgCNz6AMUpwSwodJvEQN0L1LoaNQ4-A2sDn_K1RfSA@mail.gmail.com \
--to=ulf.hansson@linaro.org \
--cc=daniel.lezcano@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).