| Message ID | 20220420134417.24546-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/2] ACPI: processor: Do not use C3 w/o ARB_DIS=1 | expand |
On Wed, Apr 20, 2022 at 3:44 PM Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > commit d6b88ce2eb9d ("ACPI: processor idle: Allow playing dead in C3 state") > was supposedly just trying to enable C3 when the CPU is offlined, > but it also mistakenly enabled C3 usage without setting ARB_DIS=1 > in normal idle scenarios. > > This results in a machine that won't boot past the point when it first > enters C3. Restore the correct behaviour (either demote to C1/C2, or > use C3 but also set ARB_DIS=1). > > I hit this on a Fujitsu Siemens Lifebook S6010 (P3) machine. > > Cc: stable@vger.kernel.org > Cc: Woody Suwalski <wsuwalski@gmail.com> > Cc: Mario Limonciello <mario.limonciello@amd.com> > Cc: Richard Gong <richard.gong@amd.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Fixes: d6b88ce2eb9d ("ACPI: processor idle: Allow playing dead in C3 state") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/acpi/processor_idle.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 4556c86c3465..54f0a1915025 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -793,10 +793,10 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr) > > state->flags = 0; > if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2 || > - cx->type == ACPI_STATE_C3) { > + cx->type == ACPI_STATE_C3) > state->enter_dead = acpi_idle_play_dead; > + if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2) > drv->safe_state_index = count; > - } > /* > * Halt-induced C1 is not good for ->enter_s2idle, because it > * re-enables interrupts on exit. Moreover, C1 is generally not > -- Good catch, but I would prefer doing the below which should be equivalent (modulo GMail-induced white space breakage): --- drivers/acpi/processor_idle.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Index: linux-pm/drivers/acpi/processor_idle.c =================================================================== --- linux-pm.orig/drivers/acpi/processor_idle.c +++ linux-pm/drivers/acpi/processor_idle.c @@ -795,7 +795,8 @@ static int acpi_processor_setup_cstates( if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2 || cx->type == ACPI_STATE_C3) { state->enter_dead = acpi_idle_play_dead; - drv->safe_state_index = count; + if (cx->type != ACPI_STATE_C3) + drv->safe_state_index = count; } /* * Halt-induced C1 is not good for ->enter_s2idle, because it
Rafael J. Wysocki wrote: > On Wed, Apr 20, 2022 at 3:44 PM Ville Syrjala > <ville.syrjala@linux.intel.com> wrote: >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> commit d6b88ce2eb9d ("ACPI: processor idle: Allow playing dead in C3 state") >> was supposedly just trying to enable C3 when the CPU is offlined, >> but it also mistakenly enabled C3 usage without setting ARB_DIS=1 >> in normal idle scenarios. >> >> This results in a machine that won't boot past the point when it first >> enters C3. Restore the correct behaviour (either demote to C1/C2, or >> use C3 but also set ARB_DIS=1). >> >> I hit this on a Fujitsu Siemens Lifebook S6010 (P3) machine. >> >> Cc: stable@vger.kernel.org >> Cc: Woody Suwalski <wsuwalski@gmail.com> >> Cc: Mario Limonciello <mario.limonciello@amd.com> >> Cc: Richard Gong <richard.gong@amd.com> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Fixes: d6b88ce2eb9d ("ACPI: processor idle: Allow playing dead in C3 state") >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> --- >> drivers/acpi/processor_idle.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c >> index 4556c86c3465..54f0a1915025 100644 >> --- a/drivers/acpi/processor_idle.c >> +++ b/drivers/acpi/processor_idle.c >> @@ -793,10 +793,10 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr) >> >> state->flags = 0; >> if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2 || >> - cx->type == ACPI_STATE_C3) { >> + cx->type == ACPI_STATE_C3) >> state->enter_dead = acpi_idle_play_dead; >> + if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2) >> drv->safe_state_index = count; >> - } >> /* >> * Halt-induced C1 is not good for ->enter_s2idle, because it >> * re-enables interrupts on exit. Moreover, C1 is generally not >> -- > Good catch, but I would prefer doing the below which should be > equivalent (modulo GMail-induced white space breakage): > > --- > drivers/acpi/processor_idle.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/acpi/processor_idle.c > =================================================================== > --- linux-pm.orig/drivers/acpi/processor_idle.c > +++ linux-pm/drivers/acpi/processor_idle.c > @@ -795,7 +795,8 @@ static int acpi_processor_setup_cstates( > if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2 || > cx->type == ACPI_STATE_C3) { > state->enter_dead = acpi_idle_play_dead; > - drv->safe_state_index = count; > + if (cx->type != ACPI_STATE_C3) > + drv->safe_state_index = count; > } > /* > * Halt-induced C1 is not good for ->enter_s2idle, because it I have tested both solutions from Ville and Rafael, and both work OK on T40. Clearly, since Ville has duped the issue on a non-T40 machine, the processor_power_dmi_table[] solution is incorrect. Rafael's solution seems simpler - so I also like it more :-) Thanks, Woody
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 4556c86c3465..54f0a1915025 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -793,10 +793,10 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr) state->flags = 0; if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2 || - cx->type == ACPI_STATE_C3) { + cx->type == ACPI_STATE_C3) state->enter_dead = acpi_idle_play_dead; + if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2) drv->safe_state_index = count; - } /* * Halt-induced C1 is not good for ->enter_s2idle, because it * re-enables interrupts on exit. Moreover, C1 is generally not