diff mbox series

[RFC,v4,3/8] ACPI: processor_idle: Use acpi_idle_play_dead() for all C-states

Message ID 20241125132029.7241-4-patryk.wlazlyn@linux.intel.com (mailing list archive)
State Changes Requested, archived
Headers show
Series SRF: Fix offline CPU preventing pc6 entry | expand

Commit Message

Patryk Wlazlyn Nov. 25, 2024, 1:20 p.m. UTC 
From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

Notice that acpi_processor_setup_cstates() can set state->enter_dead to acpi_idle_play_dead() for all C-states unconditionally and remove the
confusing C-state type check done before setting it.

No intentional functional impact.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Tested-by: Mario Limonciello <mario.limonciello@amd.com> # 6.12-rc7
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 drivers/acpi/processor_idle.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Rafael J. Wysocki Nov. 25, 2024, 1:24 p.m. UTC | #1 
On Mon, Nov 25, 2024 at 2:20 PM Patryk Wlazlyn
<patryk.wlazlyn@linux.intel.com> wrote:
>
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
>
> Notice that acpi_processor_setup_cstates() can set state->enter_dead to acpi_idle_play_dead() for all C-states unconditionally and remove the
> confusing C-state type check done before setting it.
>
> No intentional functional impact.
>
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Tested-by: Mario Limonciello <mario.limonciello@amd.com> # 6.12-rc7
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

This already is in the queue of patches to be merged during the 6.13
merge window.

I gather that it has been included here for completeness.

> ---
>  drivers/acpi/processor_idle.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index ce728cf7e301..698897b29de2 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -800,12 +800,12 @@ 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 ||
> -                   cx->type == ACPI_STATE_C3) {
> -                       state->enter_dead = acpi_idle_play_dead;
> -                       if (cx->type != ACPI_STATE_C3)
> -                               drv->safe_state_index = count;
> -               }
> +
> +               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
> --
> 2.47.0
>
>
Gautham R. Shenoy Nov. 25, 2024, 2:36 p.m. UTC | #2 
On Mon, Nov 25, 2024 at 02:20:23PM +0100, Patryk Wlazlyn wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> Notice that acpi_processor_setup_cstates() can set state->enter_dead to acpi_idle_play_dead() for all C-states unconditionally and remove the
> confusing C-state type check done before setting it.
> 
> No intentional functional impact.
> 
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Tested-by: Mario Limonciello <mario.limonciello@amd.com> # 6.12-rc7
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

I am fine with this change.
Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

> ---
>  drivers/acpi/processor_idle.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index ce728cf7e301..698897b29de2 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -800,12 +800,12 @@ 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 ||
> -		    cx->type == ACPI_STATE_C3) {
> -			state->enter_dead = acpi_idle_play_dead;
> -			if (cx->type != ACPI_STATE_C3)
> -				drv->safe_state_index = count;
> -		}
> +
> +		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
> -- 
> 2.47.0
> 
--
Thanks and Regards
gautham.
Patryk Wlazlyn Nov. 25, 2024, 2:39 p.m. UTC | #3 
> This already is in the queue of patches to be merged during the 6.13
> merge window.
>
> I gather that it has been included here for completeness.
As in the previous email.
Just an update to the series. Purely RFC. I'll rebase when we all agree.
diff mbox series

Patch

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index ce728cf7e301..698897b29de2 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -800,12 +800,12 @@  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 ||
-		    cx->type == ACPI_STATE_C3) {
-			state->enter_dead = acpi_idle_play_dead;
-			if (cx->type != ACPI_STATE_C3)
-				drv->safe_state_index = count;
-		}
+
+		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