Messages in this thread Patch in this message | | | From | Kazuki H <> | Date | Tue, 18 Apr 2023 04:09:03 +0900 | Subject | Re: [PATCH 3/3] PM: s2idle: Fully block the system from entering s2idle when cpuidle isn't supported |
| |
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
| |