1. Patchew
  2. linux
  3. [RFC PATCH v4 0/8] SRF: Fix offline CPU preventing pc6 entry
  4. View patch

[RFC PATCH v4 8/8] acpi_idle: Disallow play_dead with FFH cstate on AMD platforms

Patryk Wlazlyn posted 8 patches 1 year, 2 months ago
There is a newer version of this series
[RFC PATCH v4 8/8] acpi_idle: Disallow play_dead with FFH cstate on AMD platforms
Posted by Patryk Wlazlyn 1 year, 2 months ago 
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;
-- 
2.47.0
Re: [RFC PATCH v4 8/8] acpi_idle: Disallow play_dead with FFH cstate on AMD platforms
Posted by Gautham R. Shenoy 1 year, 2 months ago 
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
>
Re: [RFC PATCH v4 8/8] acpi_idle: Disallow play_dead with FFH cstate on AMD platforms
Posted by Patryk Wlazlyn 1 year, 2 months ago 
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!
Re: [RFC PATCH v4 8/8] acpi_idle: Disallow play_dead with FFH cstate on AMD platforms
Posted by Peter Zijlstra 1 year, 2 months ago 
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.
Re: [RFC PATCH v4 8/8] acpi_idle: Disallow play_dead with FFH cstate on AMD platforms
Posted by Patryk Wlazlyn 1 year, 2 months ago 
> 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?).
Re: [RFC PATCH v4 8/8] acpi_idle: Disallow play_dead with FFH cstate on AMD platforms
Posted by Gautham R. Shenoy 1 year, 2 months ago 
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.
Re: [RFC PATCH v4 8/8] acpi_idle: Disallow play_dead with FFH cstate on AMD platforms
Posted by Rafael J. Wysocki 1 year, 2 months ago 
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.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.