Re: [PATCHv1] ACPI: processor idle: Allow playing dead in C3 state

From: "Lazar, Lijo" <lijo.lazar@amd.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>,
	"Gong, Richard" <richard.gong@amd.com>
Cc: Len Brown <lenb@kernel.org>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Mario Limonciello <mario.limonciello@amd.com>
Subject: Re: [PATCHv1] ACPI: processor idle: Allow playing dead in C3 state
Date: Tue, 28 Sep 2021 20:38:15 +0530	[thread overview]
Message-ID: <7b8a7c2a-027f-16c9-0258-d91917c39bac@amd.com> (raw)
In-Reply-To: <CAJZ5v0g+VcGz65s=iu4fzWmkHS7Z0Dx9tMpDgFZ46f3U4jRkoQ@mail.gmail.com>

On 9/28/2021 7:18 PM, Rafael J. Wysocki wrote:
> On Tue, Sep 28, 2021 at 3:00 PM Gong, Richard <richard.gong@amd.com> wrote:
>>
>> Hi Rafael,
>>
>> On 9/24/2021 11:31 AM, Rafael J. Wysocki wrote:
>>> On Wed, Sep 22, 2021 at 3:31 PM Richard Gong <richard.gong@amd.com> wrote:
>>>> When some cores are disabled on AMD platforms, the system will no longer
>>>> be able to enter suspend-to-idle s0ix.
>>>>
>>>> Update to allow playing dead in C3 state so that the CPUs can enter the
>>>> deepest state on AMD platforms.
>>>>
>>>> BugLink: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1708&amp;data=04%7C01%7Clijo.lazar%40amd.com%7C44c85e9ae1dd4bec087e08d98286aa72%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637684337168161919%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Qoh%2BwD%2B1JzPYBc0rrGfNCdQWY1pKo8WBhLPmsmQ6G%2Bo%3D&amp;reserved=0
>>>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> Signed-off-by: Richard Gong <richard.gong@amd.com>
>>>> ---
>>>>    drivers/acpi/processor_idle.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>>>> index f37fba9e5ba0..61d5a72d218e 100644
>>>> --- a/drivers/acpi/processor_idle.c
>>>> +++ b/drivers/acpi/processor_idle.c
>>>> @@ -789,7 +789,8 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
>>>>                   state->enter = acpi_idle_enter;
>>>>
>>>>                   state->flags = 0;
>>>> -               if (cx->type == ACPI_STATE_C1 || cx->type == ACPI_STATE_C2) {
>>>> +               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;
>>>>                   }
>>>> --
>>> Tentatively applied as 5.16 material, but have you done any research
>>> on why this restriction has been there in the first place?
>>
>> Yes, we need this change to align with the updated firmware on AMD
>> platforms.
> 
> Well, it wouldn't be necessary to change the code otherwise.
> 
> Still, without the patch it is not allowed to play dead in deep idle
> states (C3 type) and do you know why?
> 

Found the original patch thread, unfortunately there is no discussion on 
why C3 was skipped or not allowed - 
https://www.spinics.net/lists/linux-pm/msg27003.html

Maybe, Intel CPU offlining worked through MWAITs and AMD bothered only 
till C2.

Thanks,
Lijo

> Or IOW how likely is this change to break anything on legacy platforms?
> 