| Message ID | 20241125132029.7241-9-patryk.wlazlyn@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | SRF: Fix offline CPU preventing pc6 entry | expand |
On Mon, Nov 25, 2024 at 2:21 PM Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> wrote: > > Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> > Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > --- > drivers/acpi/processor_idle.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 586cc7d1d8aa..4b4ac8d55b55 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -803,7 +803,11 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr) > > state->flags = 0; > > - state->enter_dead = acpi_idle_play_dead; > + /* AMD doesn't want to use mwait for play dead. */ > + bool amd_or_hygon = boot_cpu_data.x86_vendor == X86_VENDOR_AMD || > + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON; > + if (!(cx->entry_method == ACPI_CSTATE_FFH && amd_or_hygon)) > + state->enter_dead = acpi_idle_play_dead; > > if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2) > drv->safe_state_index = count; > -- I don't think this is needed. There is a vendor check in mwait_play_dead() already, no need to duplicate it in the otherwise arch-agnostic ACPI code.
On Mon, Nov 25, 2024 at 02:20:28PM +0100, Patryk Wlazlyn wrote: > Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> > Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > --- > drivers/acpi/processor_idle.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 586cc7d1d8aa..4b4ac8d55b55 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -803,7 +803,11 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr) > > state->flags = 0; > > - state->enter_dead = acpi_idle_play_dead; > + /* AMD doesn't want to use mwait for play dead. */ > + bool amd_or_hygon = boot_cpu_data.x86_vendor == X86_VENDOR_AMD || > + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON; > + if (!(cx->entry_method == ACPI_CSTATE_FFH && amd_or_hygon)) > + state->enter_dead = acpi_idle_play_dead; So I don't like this. Less exceptions is better. This *SHOULD* never trigger on AMD anyway, because they recommend IO port C[23]. But if their partner BIOS engineer does a wobbly and they end up in MWAIT anyway, it *should* all work regardless.
> So I don't like this. Less exceptions is better. > > This *SHOULD* never trigger on AMD anyway, because they recommend IO > port C[23]. But if their partner BIOS engineer does a wobbly and they > end up in MWAIT anyway, it *should* all work regardless. Agreed. I thought relaying on BIOS to not put FFH states there was a concern. I believe Gautham confirmed that AMD would be fine executing that, it's just that they prefer ioidle (or hlt?).
On Mon, Nov 25, 2024 at 02:20:28PM +0100, Patryk Wlazlyn wrote: > Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> > Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com> You can drop this patch. It is very rare to find a platform that supports FFH based C1 and doesn't have a IOPORT based C2. -- Thanks and Regards gautham. > --- > drivers/acpi/processor_idle.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 586cc7d1d8aa..4b4ac8d55b55 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -803,7 +803,11 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr) > > state->flags = 0; > > - state->enter_dead = acpi_idle_play_dead; > + /* AMD doesn't want to use mwait for play dead. */ > + bool amd_or_hygon = boot_cpu_data.x86_vendor == X86_VENDOR_AMD || > + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON; > + if (!(cx->entry_method == ACPI_CSTATE_FFH && amd_or_hygon)) > + state->enter_dead = acpi_idle_play_dead; > > if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2) > drv->safe_state_index = count; > -- > 2.47.0 >
On Mon, Nov 25, 2024 at 03:56:25PM +0100, Patryk Wlazlyn wrote: > > So I don't like this. Less exceptions is better. > > > > This *SHOULD* never trigger on AMD anyway, because they recommend IO > > port C[23]. But if their partner BIOS engineer does a wobbly and they > > end up in MWAIT anyway, it *should* all work regardless. > Agreed. > I thought relaying on BIOS to not put FFH states there was a concern. > I believe Gautham confirmed that AMD would be fine executing that, > it's just that they prefer ioidle (or hlt?). Yes, HLT or IOPORT based idle states for CPU Offline are preferrable. But FFH based idle states work just fine. -- Thanks and Regards gautham.
On 11/25/24 4:15 PM, Gautham R. Shenoy wrote: > On Mon, Nov 25, 2024 at 02:20:28PM +0100, Patryk Wlazlyn wrote: >> Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> >> Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com> > > You can drop this patch. It is very rare to find a platform that > supports FFH based C1 and doesn't have a IOPORT based C2. > > -- > Thanks and Regards > gautham. > > >> --- >> drivers/acpi/processor_idle.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c >> index 586cc7d1d8aa..4b4ac8d55b55 100644 >> --- a/drivers/acpi/processor_idle.c >> +++ b/drivers/acpi/processor_idle.c >> @@ -803,7 +803,11 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr) >> >> state->flags = 0; >> >> - state->enter_dead = acpi_idle_play_dead; >> + /* AMD doesn't want to use mwait for play dead. */ >> + bool amd_or_hygon = boot_cpu_data.x86_vendor == X86_VENDOR_AMD || >> + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON; >> + if (!(cx->entry_method == ACPI_CSTATE_FFH && amd_or_hygon)) >> + state->enter_dead = acpi_idle_play_dead; >> >> if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2) >> drv->safe_state_index = count; >> -- >> 2.47.0 >> ACK, thanks!
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 586cc7d1d8aa..4b4ac8d55b55 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -803,7 +803,11 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr) state->flags = 0; - state->enter_dead = acpi_idle_play_dead; + /* AMD doesn't want to use mwait for play dead. */ + bool amd_or_hygon = boot_cpu_data.x86_vendor == X86_VENDOR_AMD || + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON; + if (!(cx->entry_method == ACPI_CSTATE_FFH && amd_or_hygon)) + state->enter_dead = acpi_idle_play_dead; if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2) drv->safe_state_index = count;
Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> Suggested-by: Gautham R. Shenoy <gautham.shenoy@amd.com> --- drivers/acpi/processor_idle.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)