| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dfpay3ihwqtxri4izfstvk6eyty5r67w4m4xda6gcjax2y5iii@ujuko5cnv3vd>
Date: Tue, 18 Apr 2023 04:09:03 +0900
From: Kazuki H <kazukih0205@...il.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: linux-pm@...r.kernel.org, Hector Martin <marcan@...can.st>,
Sven Peter <sven@...npeter.dev>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Juri Lelli <juri.lelli@...hat.com>,
Vincent Guittot <vincent.guittot@...aro.org>,
Dietmar Eggemann <dietmar.eggemann@....com>,
Steven Rostedt <rostedt@...dmis.org>,
Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
Daniel Bristot de Oliveira <bristot@...hat.com>,
Valentin Schneider <vschneid@...hat.com>,
linux-kernel@...r.kernel.org
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@...il.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@...il.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
Powered by blists - more mailing lists