* [PATCH 1/3] sched/idle: Remove stale comments
@ 2023-03-16 7:47 Kazuki H
2023-03-16 7:47 ` [PATCH 2/3] cpuidle: Don't pass any values to cpuidle_not_available Kazuki H
2023-03-16 7:47 ` [PATCH 3/3] PM: s2idle: Fully block the system from entering s2idle when cpuidle isn't supported Kazuki H
0 siblings, 2 replies; 6+ messages in thread
From: Kazuki H @ 2023-03-16 7:47 UTC (permalink / raw)
To: linux-pm
Cc: Hector Martin, Sven Peter, Kazuki H, Rafael J. Wysocki,
Daniel Lezcano, Pavel Machek, Len Brown, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann,
Steven Rostedt, Ben Segall, Mel Gorman,
Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
rcu_idle_enter/exit() got removed in commit 1098582a0f6c
("sched,idle,rcu: Push rcu_idle deeper into the idle path"), so this
comment is outdated. Remove it.
Signed-off-by: Kazuki H <kazukih0205@gmail.com>
---
kernel/sched/idle.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index f26ab2675f7d..dbfc2eb5ccbd 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -179,12 +179,6 @@ static void cpuidle_idle_call(void)
return;
}
- /*
- * The RCU framework needs to be told that we are entering an idle
- * section, so no more rcu read side critical sections and one more
- * step to the grace period
- */
-
if (cpuidle_not_available(drv, dev)) {
tick_nohz_idle_stop_tick();
--
2.40.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/3] cpuidle: Don't pass any values to cpuidle_not_available
2023-03-16 7:47 [PATCH 1/3] sched/idle: Remove stale comments Kazuki H
@ 2023-03-16 7:47 ` Kazuki H
2023-03-16 7:47 ` [PATCH 3/3] PM: s2idle: Fully block the system from entering s2idle when cpuidle isn't supported Kazuki H
1 sibling, 0 replies; 6+ messages in thread
From: Kazuki H @ 2023-03-16 7:47 UTC (permalink / raw)
To: linux-pm
Cc: Hector Martin, Sven Peter, Kazuki H, Rafael J. Wysocki,
Daniel Lezcano, Len Brown, Pavel Machek, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann,
Steven Rostedt, Ben Segall, Mel Gorman,
Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
There's no reason to pass any values to cpuidle_not_available() as the
function works standalone. Since we're planning to use the function in
other places, make it so to avoid code duplication.
Signed-off-by: Kazuki H <kazukih0205@gmail.com>
---
drivers/cpuidle/cpuidle.c | 6 ++++--
include/linux/cpuidle.h | 6 ++----
kernel/sched/idle.c | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 6eceb1988243..cc05acf4d2a8 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -48,9 +48,11 @@ void disable_cpuidle(void)
off = 1;
}
-bool cpuidle_not_available(struct cpuidle_driver *drv,
- struct cpuidle_device *dev)
+bool cpuidle_not_available(void)
{
+ struct cpuidle_device *dev = cpuidle_get_device();
+ struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
+
return off || !initialized || !drv || !dev || !dev->enabled;
}
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index fce476275e16..11de17924910 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -139,8 +139,7 @@ struct cpuidle_driver {
#ifdef CONFIG_CPU_IDLE
extern void disable_cpuidle(void);
-extern bool cpuidle_not_available(struct cpuidle_driver *drv,
- struct cpuidle_device *dev);
+extern bool cpuidle_not_available(void);
extern int cpuidle_select(struct cpuidle_driver *drv,
struct cpuidle_device *dev,
@@ -174,8 +173,7 @@ static inline struct cpuidle_device *cpuidle_get_device(void)
{return __this_cpu_read(cpuidle_devices); }
#else
static inline void disable_cpuidle(void) { }
-static inline bool cpuidle_not_available(struct cpuidle_driver *drv,
- struct cpuidle_device *dev)
+static inline bool cpuidle_not_available(void)
{return true; }
static inline int cpuidle_select(struct cpuidle_driver *drv,
struct cpuidle_device *dev, bool *stop_tick)
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index dbfc2eb5ccbd..558a5c987597 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -179,7 +179,7 @@ static void cpuidle_idle_call(void)
return;
}
- if (cpuidle_not_available(drv, dev)) {
+ if (cpuidle_not_available()) {
tick_nohz_idle_stop_tick();
default_idle_call();
--
2.40.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 3/3] PM: s2idle: Fully block the system from entering s2idle when cpuidle isn't supported
2023-03-16 7:47 [PATCH 1/3] sched/idle: Remove stale comments Kazuki H
2023-03-16 7:47 ` [PATCH 2/3] cpuidle: Don't pass any values to cpuidle_not_available Kazuki H
@ 2023-03-16 7:47 ` Kazuki H
2023-03-27 17:43 ` Rafael J. Wysocki
1 sibling, 1 reply; 6+ messages in thread
From: Kazuki H @ 2023-03-16 7:47 UTC (permalink / raw)
To: linux-pm
Cc: Hector Martin, Sven Peter, Kazuki H, Rafael J. Wysocki,
Daniel Lezcano, Len Brown, Pavel Machek, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann,
Steven Rostedt, Ben Segall, Mel Gorman,
Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
s2idle isn't supported on platforms that don't support cpuidle as of
31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze()
too").
There is a check in the cpuidle subsystem which would prevent the
system from entering s2idle. However, there is nothing in the suspend
framework which prevents this, which can cause the suspend subsystem to
think that the machine is entering s2idle while the cpuidle subsystem is
not, which can completely break the system.
Block the machine from entering s2idle when cpuidle isn't supported in
the suspend subsystem as well.
Link: https://lore.kernel.org/all/20230204152747.drte4uitljzngdt6@kazuki-mac
Fixes: 31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze() too")
Signed-off-by: Kazuki H <kazukih0205@gmail.com>
---
kernel/power/main.c | 12 +++++++++---
kernel/power/suspend.c | 5 +++++
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 31ec4a9b9d70..b14765447989 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -133,6 +133,8 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
if (i >= PM_SUSPEND_MEM && cxl_mem_active())
continue;
+ if (i == PM_SUSPEND_TO_IDLE && cpuidle_not_available())
+ continue;
if (mem_sleep_states[i]) {
const char *label = mem_sleep_states[i];
@@ -185,11 +187,15 @@ static ssize_t mem_sleep_store(struct kobject *kobj, struct kobj_attribute *attr
}
state = decode_suspend_state(buf, n);
- if (state < PM_SUSPEND_MAX && state > PM_SUSPEND_ON)
+ if (state == PM_SUSPEND_TO_IDLE && cpuidle_not_available())
+ goto einval;
+ if (state < PM_SUSPEND_MAX && state > PM_SUSPEND_ON) {
mem_sleep_current = state;
- else
- error = -EINVAL;
+ goto out;
+ }
+ einval:
+ error = -EINVAL;
out:
pm_autosleep_unlock();
return error ? error : n;
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 3f436282547c..55ddf525aaaf 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -556,6 +556,11 @@ static int enter_state(suspend_state_t state)
trace_suspend_resume(TPS("suspend_enter"), state, true);
if (state == PM_SUSPEND_TO_IDLE) {
+ if (cpuidle_not_available()) {
+ pr_warn("s2idle is unsupported when cpuidle is unavailable");
+ return -EINVAL;
+ }
+
#ifdef CONFIG_PM_DEBUG
if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
pr_warn("Unsupported test mode for suspend to idle, please choose none/freezer/devices/platform.\n");
--
2.40.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 3/3] PM: s2idle: Fully block the system from entering s2idle when cpuidle isn't supported
2023-03-16 7:47 ` [PATCH 3/3] PM: s2idle: Fully block the system from entering s2idle when cpuidle isn't supported Kazuki H
@ 2023-03-27 17:43 ` Rafael J. Wysocki
2023-04-17 19:09 ` Kazuki H
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2023-03-27 17:43 UTC (permalink / raw)
To: Kazuki H
Cc: linux-pm, Hector Martin, Sven Peter, Rafael J. Wysocki,
Daniel Lezcano, Len Brown, Pavel Machek, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann,
Steven Rostedt, Ben Segall, Mel Gorman,
Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
On Thu, Mar 16, 2023 at 8:49 AM Kazuki H <kazukih0205@gmail.com> wrote:
>
> s2idle isn't supported on platforms that don't support cpuidle as of
> 31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze()
> too").
This simply isn't correct.
As of the above commit, s2idle was still supported without cpuidle as
long as arch_cpu_idle() worked.
> There is a check in the cpuidle subsystem which would prevent the
> system from entering s2idle. However, there is nothing in the suspend
> framework which prevents this, which can cause the suspend subsystem to
> think that the machine is entering s2idle while the cpuidle subsystem is
> not, which can completely break the system.
>
> Block the machine from entering s2idle when cpuidle isn't supported in
> the suspend subsystem as well.
First, please explain why the cpuidle_not_available() check in
cpuidle_idle_call() is not sufficient to avoid the breakage.
> Link: https://lore.kernel.org/all/20230204152747.drte4uitljzngdt6@kazuki-mac
> Fixes: 31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze() too")
> Signed-off-by: Kazuki H <kazukih0205@gmail.com>
> ---
> kernel/power/main.c | 12 +++++++++---
> kernel/power/suspend.c | 5 +++++
> 2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 31ec4a9b9d70..b14765447989 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -133,6 +133,8 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
> for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
> if (i >= PM_SUSPEND_MEM && cxl_mem_active())
> continue;
> + if (i == PM_SUSPEND_TO_IDLE && cpuidle_not_available())
> + continue;
Not really.
> if (mem_sleep_states[i]) {
> const char *label = mem_sleep_states[i];
>
> @@ -185,11 +187,15 @@ static ssize_t mem_sleep_store(struct kobject *kobj, struct kobj_attribute *attr
> }
>
> state = decode_suspend_state(buf, n);
> - if (state < PM_SUSPEND_MAX && state > PM_SUSPEND_ON)
> + if (state == PM_SUSPEND_TO_IDLE && cpuidle_not_available())
And same here.
> + goto einval;
> + if (state < PM_SUSPEND_MAX && state > PM_SUSPEND_ON) {
> mem_sleep_current = state;
> - else
> - error = -EINVAL;
> + goto out;
> + }
>
> + einval:
> + error = -EINVAL;
> out:
> pm_autosleep_unlock();
> return error ? error : n;
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 3f436282547c..55ddf525aaaf 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -556,6 +556,11 @@ static int enter_state(suspend_state_t state)
>
> trace_suspend_resume(TPS("suspend_enter"), state, true);
> if (state == PM_SUSPEND_TO_IDLE) {
> + if (cpuidle_not_available()) {
> + pr_warn("s2idle is unsupported when cpuidle is unavailable");
> + return -EINVAL;
> + }
> +
> #ifdef CONFIG_PM_DEBUG
> if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
> pr_warn("Unsupported test mode for suspend to idle, please choose none/freezer/devices/platform.\n");
> --
Again, I need to understand what the problem is in the first place.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 3/3] PM: s2idle: Fully block the system from entering s2idle when cpuidle isn't supported
2023-03-27 17:43 ` Rafael J. Wysocki
@ 2023-04-17 19:09 ` Kazuki H
0 siblings, 0 replies; 6+ messages in thread
From: Kazuki H @ 2023-04-17 19:09 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-pm, Hector Martin, Sven Peter, Rafael J. Wysocki,
Daniel Lezcano, Len Brown, Pavel Machek, Ingo Molnar,
Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann,
Steven Rostedt, Ben Segall, Mel Gorman,
Daniel Bristot de Oliveira, Valentin Schneider, linux-kernel
Hi, apologies for the late response.
On Mon, Mar 27, 2023 at 07:43:02PM +0200, Rafael J. Wysocki wrote:
> On Thu, Mar 16, 2023 at 8:49 AM Kazuki H <kazukih0205@gmail.com> wrote:
> >
> > s2idle isn't supported on platforms that don't support cpuidle as of
> > 31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze()
> > too").
>
> This simply isn't correct.
>
> As of the above commit, s2idle was still supported without cpuidle as
> long as arch_cpu_idle() worked.
Sorry I linked the wrong commit, ef2b22ac540c (cpuidle / sleep: Use
broadcast timer for states that stop local timer) is the correct one.
This adds this block of code
if (cpuidle_not_available(drv, dev))
goto use_default;
which prevents this code, critical for s2idle, from executing
if (idle_should_freeze()) {
entered_state = cpuidle_enter_freeze(drv, dev);
if (entered_state >= 0) {
local_irq_enable();
goto exit_idle;
}
reflect = false;
next_state = cpuidle_find_deepest_state(drv, dev);
} else {
reflect = true;
/*
* Ask the cpuidle framework to choose a convenient idle state.
*/
next_state = cpuidle_select(drv, dev);
}
> > There is a check in the cpuidle subsystem which would prevent the
> > system from entering s2idle. However, there is nothing in the suspend
> > framework which prevents this, which can cause the suspend subsystem to
> > think that the machine is entering s2idle while the cpuidle subsystem is
> > not, which can completely break the system.
> >
> > Block the machine from entering s2idle when cpuidle isn't supported in
> > the suspend subsystem as well.
>
> First, please explain why the cpuidle_not_available() check in
> cpuidle_idle_call() is not sufficient to avoid the breakage.
cpuidle_idle_call() doesn't notify the PM subsystem that the host
doesn't support s2idle. We need a seperate check here, otherwise the PM
subsystem will happily continue entering s2idle without the other
subsystem's knowledge.
>
> > Link: https://lore.kernel.org/all/20230204152747.drte4uitljzngdt6@kazuki-mac
> > Fixes: 31a3409065d1 ("cpuidle / sleep: Do sanity checks in cpuidle_enter_freeze() too")
> > Signed-off-by: Kazuki H <kazukih0205@gmail.com>
> > ---
> > kernel/power/main.c | 12 +++++++++---
> > kernel/power/suspend.c | 5 +++++
> > 2 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/power/main.c b/kernel/power/main.c
> > index 31ec4a9b9d70..b14765447989 100644
> > --- a/kernel/power/main.c
> > +++ b/kernel/power/main.c
> > @@ -133,6 +133,8 @@ static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
> > for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
> > if (i >= PM_SUSPEND_MEM && cxl_mem_active())
> > continue;
> > + if (i == PM_SUSPEND_TO_IDLE && cpuidle_not_available())
> > + continue;
>
> Not really.
>
> > if (mem_sleep_states[i]) {
> > const char *label = mem_sleep_states[i];
> >
> > @@ -185,11 +187,15 @@ static ssize_t mem_sleep_store(struct kobject *kobj, struct kobj_attribute *attr
> > }
> >
> > state = decode_suspend_state(buf, n);
> > - if (state < PM_SUSPEND_MAX && state > PM_SUSPEND_ON)
> > + if (state == PM_SUSPEND_TO_IDLE && cpuidle_not_available())
>
> And same here.
>
> > + goto einval;
> > + if (state < PM_SUSPEND_MAX && state > PM_SUSPEND_ON) {
> > mem_sleep_current = state;
> > - else
> > - error = -EINVAL;
> > + goto out;
> > + }
> >
> > + einval:
> > + error = -EINVAL;
> > out:
> > pm_autosleep_unlock();
> > return error ? error : n;
> > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> > index 3f436282547c..55ddf525aaaf 100644
> > --- a/kernel/power/suspend.c
> > +++ b/kernel/power/suspend.c
> > @@ -556,6 +556,11 @@ static int enter_state(suspend_state_t state)
> >
> > trace_suspend_resume(TPS("suspend_enter"), state, true);
> > if (state == PM_SUSPEND_TO_IDLE) {
> > + if (cpuidle_not_available()) {
> > + pr_warn("s2idle is unsupported when cpuidle is unavailable");
> > + return -EINVAL;
> > + }
> > +
> > #ifdef CONFIG_PM_DEBUG
> > if (pm_test_level != TEST_NONE && pm_test_level <= TEST_CPUS) {
> > pr_warn("Unsupported test mode for suspend to idle, please choose none/freezer/devices/platform.\n");
> > --
>
> Again, I need to understand what the problem is in the first place.
Thanks,
Kazuki
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] sched/idle: Remove stale comments
@ 2023-03-16 7:45 Kazuki H
0 siblings, 0 replies; 6+ messages in thread
From: Kazuki H @ 2023-03-16 7:45 UTC (permalink / raw)
To: linux-pm
Cc: Kazuki H, Kazuki Hashimoto, Rafael J. Wysocki, Daniel Lezcano,
Len Brown, Pavel Machek, Ingo Molnar, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider,
linux-kernel
rcu_idle_enter/exit() got removed in commit 1098582a0f6c
("sched,idle,rcu: Push rcu_idle deeper into the idle path"), so this
comment is outdated. Remove it.
Signed-off-by: Kazuki Hashimoto <kazuki0205@gmail.com>
---
kernel/sched/idle.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index f26ab2675f7d..dbfc2eb5ccbd 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -179,12 +179,6 @@ static void cpuidle_idle_call(void)
return;
}
- /*
- * The RCU framework needs to be told that we are entering an idle
- * section, so no more rcu read side critical sections and one more
- * step to the grace period
- */
-
if (cpuidle_not_available(drv, dev)) {
tick_nohz_idle_stop_tick();
--
2.40.0
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-17 19:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-16 7:47 [PATCH 1/3] sched/idle: Remove stale comments Kazuki H
2023-03-16 7:47 ` [PATCH 2/3] cpuidle: Don't pass any values to cpuidle_not_available Kazuki H
2023-03-16 7:47 ` [PATCH 3/3] PM: s2idle: Fully block the system from entering s2idle when cpuidle isn't supported Kazuki H
2023-03-27 17:43 ` Rafael J. Wysocki
2023-04-17 19:09 ` Kazuki H
-- strict thread matches above, loose matches on Subject: below --
2023-03-16 7:45 [PATCH 1/3] sched/idle: Remove stale comments Kazuki H
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®