* [PATCH] PM: s2idle: Introduce syscore callbacks in s2idle flow @ 2020-09-01 7:04 Claude Yen 2020-09-01 7:04 ` [PATCH 1/1] " Claude Yen 2020-09-01 11:57 ` [PATCH] " Rafael J. Wysocki 0 siblings, 2 replies; 7+ messages in thread From: Claude Yen @ 2020-09-01 7:04 UTC (permalink / raw) To: Rafael J. Wysocki, Daniel Lezcano, Frederic Weisbecker, Thomas Gleixner, Ingo Molnar, Matthias Brugger, Bjorn Andersson, Greg Kroah-Hartman, Stephen Boyd, Douglas Anderson Cc: wsd_upstream, linux-mediatek, linux-kernel, linux-arm-kernel, linux-pm This series based on 5.9-rc1 This patch makes s2idle call existing syscore callbacks. Currently, when s2idle is selected as system suspend method, callbacks hooked by register_syscore_ops() will not be triggered. This may induce unexpected results. For example, sched_clock_suspend() was added to s2idle flow in commit 3f2552f7e9c5 ("timers/sched_clock: Prevent generic sched_clock wrap caused by tick_freeze()") to fix clock wrap problem. However, sched_clock_suspend() is originally registered in syscore callback. With this patch, if another syscore callback is needed in s2idle, additional migration effort could be saved. *** BLURB HERE *** claude.yen (1): PM: s2idle: Introduce syscore callbacks in s2idle flow drivers/cpuidle/cpuidle.c | 36 ++++++++++++++++++++++++++++++++---- kernel/cpu_pm.c | 17 +++++++++++++++++ kernel/time/tick-common.c | 17 ++--------------- 3 files changed, 51 insertions(+), 19 deletions(-) -- 1.7.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/1] PM: s2idle: Introduce syscore callbacks in s2idle flow 2020-09-01 7:04 [PATCH] PM: s2idle: Introduce syscore callbacks in s2idle flow Claude Yen @ 2020-09-01 7:04 ` Claude Yen 2020-09-01 7:40 ` Stephen Boyd 2020-09-01 11:57 ` [PATCH] " Rafael J. Wysocki 1 sibling, 1 reply; 7+ messages in thread From: Claude Yen @ 2020-09-01 7:04 UTC (permalink / raw) To: Rafael J. Wysocki, Daniel Lezcano, Frederic Weisbecker, Thomas Gleixner, Ingo Molnar, Matthias Brugger, Bjorn Andersson, Greg Kroah-Hartman, Stephen Boyd, Douglas Anderson Cc: wsd_upstream, linux-pm, claude.yen, linux-kernel, linux-mediatek, linux-arm-kernel From: "claude.yen" <claude.yen@mediatek.com> Adding syscore callbacks to s2idle makes the behavior of s2idle become more similar to Suspend-to-Ram (S2R) and reduces potential porting effort. tick_freeze() in s2idle flow calls sched_clock_suspend() and timekeeping_suspend(), which both functions are also registered as syscore callback. sched_clock_suspend() introduced in commit 3f2552f7e9c5 ("timers/sched_clock: Prevent generic sched_clock wrap caused by tick_freeze()") is added due to clock wrap issue. By adding syscore callbacks in s2idle, if there is any syscore callbacks also required in s2idle, additional porting effort could be saved. Besides, in both S2R and s2idle, before the system entering low power state, there is a state when only one cpu alive with interrupt-disabled, which is syscore callback designed for. Adding syscore callbacks in s2idle is feasible option. Scenarios to call syscore callback: S2R: one cpu alive when nonboot cpus are hotplug-ed off s2idle: one cpu alive when other cpus have enter idle state Signed-off-by: claude.yen <claude.yen@mediatek.com> --- drivers/cpuidle/cpuidle.c | 36 ++++++++++++++++++++++++++++++++---- kernel/cpu_pm.c | 17 +++++++++++++++++ kernel/time/tick-common.c | 17 ++--------------- 3 files changed, 51 insertions(+), 19 deletions(-) diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 8719731..be22174 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -23,6 +23,9 @@ #include <linux/suspend.h> #include <linux/tick.h> #include <trace/events/power.h> +#include <linux/cpumask.h> +#include <linux/spinlock.h> +#include <linux/syscore_ops.h> #include "cpuidle.h" @@ -35,6 +38,8 @@ static int enabled_devices; static int off __read_mostly; static int initialized __read_mostly; +static struct cpumask s2idle_cpumask; +static DEFINE_SPINLOCK(s2idle_spinlock); int cpuidle_disabled(void) { @@ -137,15 +142,27 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv, struct cpuidle_device *dev, int index) { ktime_t time_start, time_end; + int cpu; time_start = ns_to_ktime(local_clock()); + cpu = smp_processor_id(); + + tick_freeze(); /* - * trace_suspend_resume() called by tick_freeze() for the last CPU + * trace_suspend_resume() called by syscore_suepnd() for the last CPU * executing it contains RCU usage regarded as invalid in the idle * context, so tell RCU about that. */ - RCU_NONIDLE(tick_freeze()); + spin_lock(&s2idle_spinlock); + cpumask_set_cpu(cpu, &s2idle_cpumask); + if (cpumask_weight(&s2idle_cpumask) == num_online_cpus()) { + system_state = SYSTEM_SUSPEND; + RCU_NONIDLE(syscore_suspend()); + } + + spin_unlock(&s2idle_spinlock); + /* * The state used here cannot be a "coupled" one, because the "coupled" * cpuidle mechanism enables interrupts and doing that with timekeeping @@ -154,12 +171,21 @@ static void enter_s2idle_proper(struct cpuidle_driver *drv, stop_critical_timings(); drv->states[index].enter_s2idle(dev, drv, index); WARN_ON(!irqs_disabled()); + /* - * timekeeping_resume() that will be called by tick_unfreeze() for the + * timekeeping_resume() that will be called by syscore_reume() for the * first CPU executing it calls functions containing RCU read-side * critical sections, so tell RCU about that. */ - RCU_NONIDLE(tick_unfreeze()); + spin_lock(&s2idle_spinlock); + if (cpumask_weight(&s2idle_cpumask) == num_online_cpus()) { + RCU_NONIDLE(syscore_resume()); + system_state = SYSTEM_RUNNING; + } + cpumask_clear_cpu(cpu, &s2idle_cpumask); + spin_unlock(&s2idle_spinlock); + + tick_unfreeze(); start_critical_timings(); time_end = ns_to_ktime(local_clock()); @@ -745,6 +771,8 @@ static int __init cpuidle_init(void) if (cpuidle_disabled()) return -ENODEV; + cpumask_clear(&s2idle_cpumask); + return cpuidle_add_interface(cpu_subsys.dev_root); } diff --git a/kernel/cpu_pm.c b/kernel/cpu_pm.c index 44a2593..6c2f5ce 100644 --- a/kernel/cpu_pm.c +++ b/kernel/cpu_pm.c @@ -12,6 +12,7 @@ #include <linux/notifier.h> #include <linux/spinlock.h> #include <linux/syscore_ops.h> +#include <linux/suspend.h> static ATOMIC_NOTIFIER_HEAD(cpu_pm_notifier_chain); @@ -172,6 +173,14 @@ static int cpu_pm_suspend(void) { int ret; +#ifdef CONFIG_SUSPEND + /* + * Skip cpu_pm_suspend in s2idle flow because cpu_pm notifier callbacks + * are triggered in idle framework + */ + if (s2idle_state != S2IDLE_STATE_NONE) + return 0; +#endif ret = cpu_pm_enter(); if (ret) return ret; @@ -182,6 +191,14 @@ static int cpu_pm_suspend(void) static void cpu_pm_resume(void) { +#ifdef CONFIG_SUSPEND + /* + * Skip cpu_pm_resume in s2idle flow because cpu_pm notifier callbacks + * are triggered in idle framework + */ + if (s2idle_state != S2IDLE_STATE_NONE) + return; +#endif cpu_cluster_pm_exit(); cpu_pm_exit(); } diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 6c9c342..19feeed 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -528,15 +528,8 @@ void tick_freeze(void) raw_spin_lock(&tick_freeze_lock); tick_freeze_depth++; - if (tick_freeze_depth == num_online_cpus()) { - trace_suspend_resume(TPS("timekeeping_freeze"), - smp_processor_id(), true); - system_state = SYSTEM_SUSPEND; - sched_clock_suspend(); - timekeeping_suspend(); - } else { + if (tick_freeze_depth < num_online_cpus()) tick_suspend_local(); - } raw_spin_unlock(&tick_freeze_lock); } @@ -554,13 +547,7 @@ void tick_unfreeze(void) { raw_spin_lock(&tick_freeze_lock); - if (tick_freeze_depth == num_online_cpus()) { - timekeeping_resume(); - sched_clock_resume(); - system_state = SYSTEM_RUNNING; - trace_suspend_resume(TPS("timekeeping_freeze"), - smp_processor_id(), false); - } else { + if (tick_freeze_depth < num_online_cpus()) { touch_softlockup_watchdog(); tick_resume_local(); } -- 1.7.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] PM: s2idle: Introduce syscore callbacks in s2idle flow 2020-09-01 7:04 ` [PATCH 1/1] " Claude Yen @ 2020-09-01 7:40 ` Stephen Boyd 2020-09-03 3:43 ` Claude.Yen 0 siblings, 1 reply; 7+ messages in thread From: Stephen Boyd @ 2020-09-01 7:40 UTC (permalink / raw) To: Bjorn Andersson, Claude Yen, Daniel Lezcano, Douglas Anderson, Frederic Weisbecker, Greg Kroah-Hartman, Ingo Molnar, Matthias Brugger, Rafael J. Wysocki, Thomas Gleixner Cc: wsd_upstream, linux-pm, claude.yen, linux-kernel, linux-mediatek, linux-arm-kernel Quoting Claude Yen (2020-09-01 00:04:19) > From: "claude.yen" <claude.yen@mediatek.com> > > Adding syscore callbacks to s2idle makes the behavior of s2idle become > more similar to Suspend-to-Ram (S2R) and reduces potential porting > effort. > > tick_freeze() in s2idle flow calls sched_clock_suspend() and > timekeeping_suspend(), which both functions are also registered as > syscore callback. sched_clock_suspend() introduced in > commit 3f2552f7e9c5 ("timers/sched_clock: Prevent generic sched_clock > wrap caused by tick_freeze()") is added due to clock wrap issue. > By adding syscore callbacks in s2idle, if there is any syscore callbacks > also required in s2idle, additional porting effort could be saved. > > Besides, in both S2R and s2idle, before the system entering low power > state, there is a state when only one cpu alive with interrupt-disabled, > which is syscore callback designed for. Adding syscore callbacks in > s2idle is feasible option. > > Scenarios to call syscore callback: > S2R: one cpu alive when nonboot cpus are hotplug-ed off > s2idle: one cpu alive when other cpus have enter idle state > Do you have syscore ops that can't be converted to something else like CPU_PM notifier? At this point most syscore code that is important has been converted so I don't see much benefit for this patch. If anything, it will prevent conversions to code that works for both cases. > > Signed-off-by: claude.yen <claude.yen@mediatek.com> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] PM: s2idle: Introduce syscore callbacks in s2idle flow 2020-09-01 7:40 ` Stephen Boyd @ 2020-09-03 3:43 ` Claude.Yen 0 siblings, 0 replies; 7+ messages in thread From: Claude.Yen @ 2020-09-03 3:43 UTC (permalink / raw) To: Stephen Boyd Cc: wsd_upstream, linux-pm, Frederic Weisbecker, Daniel Lezcano, Rafael J.Wysocki, Douglas Anderson, Bjorn Andersson, linux-mediatek, linux-arm-kernel, Greg Kroah-Hartman, Matthias Brugger, Thomas Gleixner, Ingo Molnar, linux-kernel On Tue, 2020-09-01 at 00:40 -0700, Stephen Boyd wrote: > Quoting Claude Yen (2020-09-01 00:04:19) > > From: "claude.yen" <claude.yen@mediatek.com> > > > > Adding syscore callbacks to s2idle makes the behavior of s2idle become > > more similar to Suspend-to-Ram (S2R) and reduces potential porting > > effort. > > > > tick_freeze() in s2idle flow calls sched_clock_suspend() and > > timekeeping_suspend(), which both functions are also registered as > > syscore callback. sched_clock_suspend() introduced in > > commit 3f2552f7e9c5 ("timers/sched_clock: Prevent generic sched_clock > > wrap caused by tick_freeze()") is added due to clock wrap issue. > > By adding syscore callbacks in s2idle, if there is any syscore callbacks > > also required in s2idle, additional porting effort could be saved. > > > > Besides, in both S2R and s2idle, before the system entering low power > > state, there is a state when only one cpu alive with interrupt-disabled, > > which is syscore callback designed for. Adding syscore callbacks in > > s2idle is feasible option. > > > > Scenarios to call syscore callback: > > S2R: one cpu alive when nonboot cpus are hotplug-ed off > > s2idle: one cpu alive when other cpus have enter idle state > > > > Do you have syscore ops that can't be converted to something else like > CPU_PM notifier? At this point most syscore code that is important has > been converted so I don't see much benefit for this patch. If anything, > it will prevent conversions to code that works for both cases. In Mediatek SoC's implementation, battery voltage and remaining capacity are tracked in syscore callback when system going to suspend. The reason to hook callback in syscore is that all devices are suspended, and thus battery output become steady and suitable for measurement. Such callback is not suitable for CPU_PM notifier. If s2idle is applied, the callback to track battery status is not triggered due to syscore is not called by current design. Regards, Claude _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PM: s2idle: Introduce syscore callbacks in s2idle flow 2020-09-01 7:04 [PATCH] PM: s2idle: Introduce syscore callbacks in s2idle flow Claude Yen 2020-09-01 7:04 ` [PATCH 1/1] " Claude Yen @ 2020-09-01 11:57 ` Rafael J. Wysocki 2020-09-03 2:14 ` Claude.Yen 1 sibling, 1 reply; 7+ messages in thread From: Rafael J. Wysocki @ 2020-09-01 11:57 UTC (permalink / raw) To: Claude Yen Cc: wsd_upstream, Linux PM, Frederic Weisbecker, Daniel Lezcano, Rafael J. Wysocki, Douglas Anderson, Bjorn Andersson, moderated list:ARM/Mediatek SoC..., Linux ARM, Greg Kroah-Hartman, Matthias Brugger, Thomas Gleixner, Stephen Boyd, Ingo Molnar, Linux Kernel Mailing List On Tue, Sep 1, 2020 at 9:05 AM Claude Yen <claude.yen@mediatek.com> wrote: > > This series based on 5.9-rc1 > This patch makes s2idle call existing syscore callbacks. Currently, > when s2idle is selected as system suspend method, callbacks hooked > by register_syscore_ops() will not be triggered. This may induce > unexpected results. They are not executed by design. > For example, sched_clock_suspend() was added to s2idle flow in > commit 3f2552f7e9c5 ("timers/sched_clock: Prevent generic sched_clock > wrap caused by tick_freeze()") to fix clock wrap problem. However, > sched_clock_suspend() is originally registered in syscore callback. I'm not sure why this matters here. > With this patch, if another syscore callback is needed in s2idle, > additional migration effort could be saved. s2idle cannot execute syscore callbacks, because it doesn' take non-boot CPUs offline and it won't do that. Thanks! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PM: s2idle: Introduce syscore callbacks in s2idle flow 2020-09-01 11:57 ` [PATCH] " Rafael J. Wysocki @ 2020-09-03 2:14 ` Claude.Yen 2020-09-10 12:55 ` Rafael J. Wysocki 0 siblings, 1 reply; 7+ messages in thread From: Claude.Yen @ 2020-09-03 2:14 UTC (permalink / raw) To: Rafael J. Wysocki Cc: wsd_upstream, Linux PM, Frederic Weisbecker, Daniel Lezcano, Rafael J. Wysocki, Douglas Anderson, Bjorn Andersson, moderated list:ARM/Mediatek SoC..., Linux ARM, Greg Kroah-Hartman, Matthias Brugger, Thomas Gleixner, Stephen Boyd, Ingo Molnar, Linux Kernel Mailing List On Tue, 2020-09-01 at 13:57 +0200, Rafael J. Wysocki wrote: > On Tue, Sep 1, 2020 at 9:05 AM Claude Yen <claude.yen@mediatek.com> wrote: > > > > This series based on 5.9-rc1 > > This patch makes s2idle call existing syscore callbacks. Currently, > > when s2idle is selected as system suspend method, callbacks hooked > > by register_syscore_ops() will not be triggered. This may induce > > unexpected results. > > They are not executed by design. > > > For example, sched_clock_suspend() was added to s2idle flow in > > commit 3f2552f7e9c5 ("timers/sched_clock: Prevent generic sched_clock > > wrap caused by tick_freeze()") to fix clock wrap problem. However, > > sched_clock_suspend() is originally registered in syscore callback. > > I'm not sure why this matters here. If functions in syscore callbacks are needed in s2idle, explicit migration is needed like commit 3f2552f7e9c5 ("timers/sched_clock: Prevent generic sched_clock wrap caused by tick_freeze()"). Thus, I am wondering if such effort could be saved. > > With this patch, if another syscore callback is needed in s2idle, > > additional migration effort could be saved. > > s2idle cannot execute syscore callbacks, because it doesn' take > non-boot CPUs offline and it won't do that. > > Thanks! Yes, the current design of syscore callback needs non-boot CPUs offline. Considering the following case: in s2idle flow, there is a status that only one CPU is alive and other CPUs have enter deepest idle state. This situation is similar to getting non-boot CPUs offline, though all CPUs are online from kernel's perspective. Reply from Stephen mentioned that if an operation is needed in both S2R and s2idle, CPU_PM notifier can be utilized. In my opinion, CPU_PM notifier is particularly for CPU entering idle state. In contrast, syscore callback is for system going low power state. There exists semantic difference between these two callbacks. Could the current design of syscore callback be re-designed as system-wide suspend callback? Proposed suspend flow in this patch: Freeze tasks | V Device suspend callbacks | |-------------s2idle---------- | | V | Disable nonboot CPUs Is this CPU last core to enter idle? | | V |------------- syscore callbacks | | | No Yes V | | platform suspend V V enter idle syscore callback | V enter idle Regards, Claude _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] PM: s2idle: Introduce syscore callbacks in s2idle flow 2020-09-03 2:14 ` Claude.Yen @ 2020-09-10 12:55 ` Rafael J. Wysocki 0 siblings, 0 replies; 7+ messages in thread From: Rafael J. Wysocki @ 2020-09-10 12:55 UTC (permalink / raw) To: Claude. Yen Cc: wsd_upstream, Rafael J. Wysocki, Frederic Weisbecker, Linux PM, Daniel Lezcano, Douglas Anderson, Bjorn Andersson, moderated list:ARM/Mediatek SoC..., Linux ARM, Greg Kroah-Hartman, Matthias Brugger, Thomas Gleixner, Stephen Boyd, Ingo Molnar, Linux Kernel Mailing List On Thursday, September 3, 2020 4:14:07 AM CEST Claude. Yen wrote: > On Tue, 2020-09-01 at 13:57 +0200, Rafael J. Wysocki wrote: > > On Tue, Sep 1, 2020 at 9:05 AM Claude Yen <claude.yen@mediatek.com> wrote: > > > > > > This series based on 5.9-rc1 > > > This patch makes s2idle call existing syscore callbacks. Currently, > > > when s2idle is selected as system suspend method, callbacks hooked > > > by register_syscore_ops() will not be triggered. This may induce > > > unexpected results. > > > > They are not executed by design. > > > > > For example, sched_clock_suspend() was added to s2idle flow in > > > commit 3f2552f7e9c5 ("timers/sched_clock: Prevent generic sched_clock > > > wrap caused by tick_freeze()") to fix clock wrap problem. However, > > > sched_clock_suspend() is originally registered in syscore callback. > > > > I'm not sure why this matters here. > > If functions in syscore callbacks are needed in s2idle, explicit > migration is needed like commit 3f2552f7e9c5 ("timers/sched_clock: > Prevent generic sched_clock wrap caused by tick_freeze()"). > Thus, I am wondering if such effort could be saved. Yes, it could. You can define platform ops for s2idle and invoke what's needed from there. > > > With this patch, if another syscore callback is needed in s2idle, > > > additional migration effort could be saved. > > > > s2idle cannot execute syscore callbacks, because it doesn' take > > non-boot CPUs offline and it won't do that. > > > > Thanks! > > Yes, the current design of syscore callback needs non-boot CPUs offline. > Considering the following case: in s2idle flow, there is a status that > only one CPU is alive and other CPUs have enter deepest idle state. > This situation is similar to getting non-boot CPUs offline, though all > CPUs are online from kernel's perspective. It is only similar AFAICS. You don't migrate interrupts during s2idle, for example. > Reply from Stephen mentioned that if an operation is needed in both > S2R and s2idle, CPU_PM notifier can be utilized. > In my opinion, CPU_PM notifier is particularly for CPU entering idle > state. In contrast, syscore callback is for system going low power > state. There exists semantic difference between these two callbacks. Fair enough. > Could the current design of syscore callback be re-designed as > system-wide suspend callback? No, it couldn't. > Proposed suspend flow in this patch: > > Freeze tasks > | > V > Device suspend callbacks > | > |-------------s2idle---------- > | | > V | > Disable nonboot CPUs Is this CPU last core to enter idle? > | | > V |------------- > syscore callbacks | | > | No Yes > V | | > platform suspend V V > enter idle syscore callback > | > V > enter idle > The primary problem with this is that on some architectures (x86 at least) the syscore things cannot be run during the s2idle flow. Also there is a way to invoke them through the platform ops as I said. Thanks! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-09-10 12:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-01 7:04 [PATCH] PM: s2idle: Introduce syscore callbacks in s2idle flow Claude Yen 2020-09-01 7:04 ` [PATCH 1/1] " Claude Yen 2020-09-01 7:40 ` Stephen Boyd 2020-09-03 3:43 ` Claude.Yen 2020-09-01 11:57 ` [PATCH] " Rafael J. Wysocki 2020-09-03 2:14 ` Claude.Yen 2020-09-10 12:55 ` Rafael J. Wysocki
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).