[PATCH 1/2] ACPI: processor: idle: Return an error if both P_LVL{2,3} idle states are invalid

[PATCH 1/2] ACPI: processor: idle: Return an error if both P_LVL{2,3} idle states are invalid

[Giovanni Gherdovich]

Prior to commit 496121c02127e9c460b436244c38260b044cc45a ("ACPI: processor:
idle: Allow probing on platforms with one ACPI C-state"), the acpi_idle
driver wouldn't load on systems without a valid C-State at least as deep
as C2. The behavior was desirable for guests on hypervisors such as VMWare
ESXi, which by default don't have the _CST ACPI method, and set the C2 and
C3 latencies to 101 and 1001 microseconds respectively via the FADT, to
signify they're unsupported.

Since the above change though, these virtualized deployments end up loading
acpi_idle, and thus entering the default C1 C-State set by
acpi_processor_get_power_info_default(); this is undesirable for a system
that's communicating to the OS it doesn't want C-States (missing _CST, and
invalid C2/C3 in FADT).

Make acpi_processor_get_power_info_fadt() return ENODEV in that case, so
that acpi_processor_get_cstate_info() exits early and doesn't set
pr->flags.power = 1.

Fixes: 496121c02127 ("ACPI: processor: idle: Allow probing on platforms with one ACPI C-state")
Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
---
 drivers/acpi/processor_idle.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 586cc7d1d8aa..b181f7fc2090 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -268,6 +268,10 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
 			 ACPI_CX_DESC_LEN, "ACPI P_LVL3 IOPORT 0x%x",
 			 pr->power.states[ACPI_STATE_C3].address);
 
+	if (!pr->power.states[ACPI_STATE_C2].address &&
+	    !pr->power.states[ACPI_STATE_C3].address)
+		return -ENODEV;
+
 	return 0;
 }
 
-- 
2.43.0

[PATCH 2/2] ACPI: processor: idle: Remove obsolete comment

[Giovanni Gherdovich]

Since commit 496121c02127e9c460b436244c38260b044cc45a ("ACPI: processor:
idle: Allow probing on platforms with one ACPI C-state"), the comment
doesn't reflect the code anymore; remove it.

Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
---
 drivers/acpi/processor_idle.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index b181f7fc2090..2a076c7a825a 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -482,10 +482,6 @@ static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
 
 	pr->power.count = acpi_processor_power_verify(pr);
 
-	/*
-	 * if one state of type C2 or C3 is available, mark this
-	 * CPU as being "idle manageable"
-	 */
 	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
 		if (pr->power.states[i].valid) {
 			pr->power.count = i;
-- 
2.43.0

Re: [PATCH 1/2] ACPI: processor: idle: Return an error if both P_LVL{2,3} idle states are invalid

[Zhang, Rui]

On Fri, 2025-03-28 at 15:30 +0100, Giovanni Gherdovich wrote:
> Prior to commit 496121c02127e9c460b436244c38260b044cc45a ("ACPI:
> processor:
> idle: Allow probing on platforms with one ACPI C-state"), the
> acpi_idle
> driver wouldn't load on systems without a valid C-State at least as
> deep
> as C2. The behavior was desirable for guests on hypervisors such as
> VMWare
> ESXi, which by default don't have the _CST ACPI method, and set the
> C2 and
> C3 latencies to 101 and 1001 microseconds respectively via the FADT,
> to
> signify they're unsupported.
> 
> Since the above change though, these virtualized deployments end up
> loading
> acpi_idle, and thus entering the default C1 C-State set by
> acpi_processor_get_power_info_default(); this is undesirable for a
> system
> that's communicating to the OS it doesn't want C-States (missing
> _CST, and
> invalid C2/C3 in FADT).
> 
> Make acpi_processor_get_power_info_fadt() return ENODEV in that case,
> so
> that acpi_processor_get_cstate_info() exits early and doesn't set
> pr->flags.power = 1.
> 
> Fixes: 496121c02127 ("ACPI: processor: idle: Allow probing on
> platforms with one ACPI C-state")
> Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>

LGTM.

Reviewed-by: Zhang Rui <rui.zhang@intel.com>

-rui
> ---
>  drivers/acpi/processor_idle.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/acpi/processor_idle.c
> b/drivers/acpi/processor_idle.c
> index 586cc7d1d8aa..b181f7fc2090 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -268,6 +268,10 @@ static int
> acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
>  			 ACPI_CX_DESC_LEN, "ACPI P_LVL3 IOPORT
> 0x%x",
>  			 pr->power.states[ACPI_STATE_C3].address);
>  
> +	if (!pr->power.states[ACPI_STATE_C2].address &&
> +	    !pr->power.states[ACPI_STATE_C3].address)
> +		return -ENODEV;
> +
>  	return 0;
>  }
>  

RE: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment

[Zhang, Rui]

> -----Original Message-----
> From: Giovanni Gherdovich <ggherdovich@suse.cz>
> Sent: Friday, March 28, 2025 10:31 PM
> To: Rafael J . Wysocki <rafael@kernel.org>; Zhang, Rui <rui.zhang@intel.com>
> Cc: Len Brown <lenb@kernel.org>; Giovanni Gherdovich
> <ggherdovich@suse.cz>; linux-acpi@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-pm@vger.kernel.org
> Subject: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment
> Importance: High
> 
> Since commit 496121c02127e9c460b436244c38260b044cc45a ("ACPI:
> processor:
> idle: Allow probing on platforms with one ACPI C-state"), the comment
> doesn't reflect the code anymore; remove it.
> 
> Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
> ---
>  drivers/acpi/processor_idle.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index b181f7fc2090..2a076c7a825a 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -482,10 +482,6 @@ static int acpi_processor_get_cstate_info(struct
> acpi_processor *pr)
> 
>  	pr->power.count = acpi_processor_power_verify(pr);
> 
> -	/*
> -	 * if one state of type C2 or C3 is available, mark this
> -	 * CPU as being "idle manageable"
> -	 */
>  	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
>  		if (pr->power.states[i].valid) {
>  			pr->power.count = i;
> --
> 2.43.0

I think we can clean up a bit more. How about the patch below?

From 115d3a07febff32eed49f9343ef111e7e1452f9d Mon Sep 17 00:00:00 2001
From: "Zhang, Rui" <rui.zhang@intel.com>
Date: Mon, 31 Mar 2025 07:29:57 +0000
Subject: [PATCH] ACPI: processor: idle: Simplify
 acpi_processor_get_cstate_info() logic

Since commit 496121c02127 ("ACPI: processor: idle: Allow probing on
platforms with one ACPI C-state"), acpi_idle driver can be probed with
C1 only.

Optimize the logic for setting pr->power.count and pr->flags.power by
1. unconditionally set pr->flags.power leveraging the fact that C1 is
   always valid after acpi_processor_get_power_info_default().
2. update acpi_processor_power_verify() to return the highest valid
   C-state directly.

No functional change intended.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/processor_idle.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 698897b29de2..7ce8c3802937 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -442,7 +442,7 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
 
 		lapic_timer_check_state(i, pr, cx);
 		tsc_check_state(cx->type);
-		working++;
+		working = i;
 	}
 
 	if (buggy_latency) {
@@ -457,7 +457,6 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
 
 static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
 {
-	unsigned int i;
 	int result;
 
 
@@ -477,17 +476,7 @@ static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
 	acpi_processor_get_power_info_default(pr);
 
 	pr->power.count = acpi_processor_power_verify(pr);
-
-	/*
-	 * if one state of type C2 or C3 is available, mark this
-	 * CPU as being "idle manageable"
-	 */
-	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
-		if (pr->power.states[i].valid) {
-			pr->power.count = i;
-			pr->flags.power = 1;
-		}
-	}
+	pr->flags.power = 1;
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH 1/2] ACPI: processor: idle: Return an error if both P_LVL{2,3} idle states are invalid

[Rafael J. Wysocki]

On Mon, Mar 31, 2025 at 3:09 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Fri, 2025-03-28 at 15:30 +0100, Giovanni Gherdovich wrote:
> > Prior to commit 496121c02127e9c460b436244c38260b044cc45a ("ACPI:
> > processor:
> > idle: Allow probing on platforms with one ACPI C-state"), the
> > acpi_idle
> > driver wouldn't load on systems without a valid C-State at least as
> > deep
> > as C2. The behavior was desirable for guests on hypervisors such as
> > VMWare
> > ESXi, which by default don't have the _CST ACPI method, and set the
> > C2 and
> > C3 latencies to 101 and 1001 microseconds respectively via the FADT,
> > to
> > signify they're unsupported.
> >
> > Since the above change though, these virtualized deployments end up
> > loading
> > acpi_idle, and thus entering the default C1 C-State set by
> > acpi_processor_get_power_info_default(); this is undesirable for a
> > system
> > that's communicating to the OS it doesn't want C-States (missing
> > _CST, and
> > invalid C2/C3 in FADT).
> >
> > Make acpi_processor_get_power_info_fadt() return ENODEV in that case,
> > so
> > that acpi_processor_get_cstate_info() exits early and doesn't set
> > pr->flags.power = 1.
> >
> > Fixes: 496121c02127 ("ACPI: processor: idle: Allow probing on
> > platforms with one ACPI C-state")
> > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
>
> LGTM.
>
> Reviewed-by: Zhang Rui <rui.zhang@intel.com>

Applied as 6.15-rc material, thanks!

> > ---
> >  drivers/acpi/processor_idle.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/acpi/processor_idle.c
> > b/drivers/acpi/processor_idle.c
> > index 586cc7d1d8aa..b181f7fc2090 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -268,6 +268,10 @@ static int
> > acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
> >                        ACPI_CX_DESC_LEN, "ACPI P_LVL3 IOPORT
> > 0x%x",
> >                        pr->power.states[ACPI_STATE_C3].address);
> >
> > +     if (!pr->power.states[ACPI_STATE_C2].address &&
> > +         !pr->power.states[ACPI_STATE_C3].address)
> > +             return -ENODEV;
> > +
> >       return 0;
> >  }
> >
>

Re: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment

[Rafael J. Wysocki]

On Mon, Mar 31, 2025 at 9:38 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> > -----Original Message-----
> > From: Giovanni Gherdovich <ggherdovich@suse.cz>
> > Sent: Friday, March 28, 2025 10:31 PM
> > To: Rafael J . Wysocki <rafael@kernel.org>; Zhang, Rui <rui.zhang@intel.com>
> > Cc: Len Brown <lenb@kernel.org>; Giovanni Gherdovich
> > <ggherdovich@suse.cz>; linux-acpi@vger.kernel.org; linux-
> > kernel@vger.kernel.org; linux-pm@vger.kernel.org
> > Subject: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment
> > Importance: High
> >
> > Since commit 496121c02127e9c460b436244c38260b044cc45a ("ACPI:
> > processor:
> > idle: Allow probing on platforms with one ACPI C-state"), the comment
> > doesn't reflect the code anymore; remove it.
> >
> > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
> > ---
> >  drivers/acpi/processor_idle.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> > index b181f7fc2090..2a076c7a825a 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -482,10 +482,6 @@ static int acpi_processor_get_cstate_info(struct
> > acpi_processor *pr)
> >
> >       pr->power.count = acpi_processor_power_verify(pr);
> >
> > -     /*
> > -      * if one state of type C2 or C3 is available, mark this
> > -      * CPU as being "idle manageable"
> > -      */
> >       for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
> >               if (pr->power.states[i].valid) {
> >                       pr->power.count = i;
> > --
> > 2.43.0
>
> I think we can clean up a bit more. How about the patch below?
>
> From 115d3a07febff32eed49f9343ef111e7e1452f9d Mon Sep 17 00:00:00 2001
> From: "Zhang, Rui" <rui.zhang@intel.com>
> Date: Mon, 31 Mar 2025 07:29:57 +0000
> Subject: [PATCH] ACPI: processor: idle: Simplify
>  acpi_processor_get_cstate_info() logic
>
> Since commit 496121c02127 ("ACPI: processor: idle: Allow probing on
> platforms with one ACPI C-state"), acpi_idle driver can be probed with
> C1 only.
>
> Optimize the logic for setting pr->power.count and pr->flags.power by
> 1. unconditionally set pr->flags.power leveraging the fact that C1 is
>    always valid after acpi_processor_get_power_info_default().
> 2. update acpi_processor_power_verify() to return the highest valid
>    C-state directly.
>
> No functional change intended.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/processor_idle.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 698897b29de2..7ce8c3802937 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -442,7 +442,7 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
>
>                 lapic_timer_check_state(i, pr, cx);
>                 tsc_check_state(cx->type);
> -               working++;
> +               working = i;

What if some states are skipped because they are invalid?  'working'
can be less than 'i' then AFAICS.

>         }
>
>         if (buggy_latency) {
> @@ -457,7 +457,6 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
>
>  static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
>  {
> -       unsigned int i;
>         int result;
>
>
> @@ -477,17 +476,7 @@ static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
>         acpi_processor_get_power_info_default(pr);
>
>         pr->power.count = acpi_processor_power_verify(pr);
> -
> -       /*
> -        * if one state of type C2 or C3 is available, mark this
> -        * CPU as being "idle manageable"
> -        */
> -       for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
> -               if (pr->power.states[i].valid) {
> -                       pr->power.count = i;
> -                       pr->flags.power = 1;
> -               }
> -       }
> +       pr->flags.power = 1;
>
>         return 0;
>  }
> --

Re: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment

[Zhang, Rui]

On Mon, 2025-03-31 at 14:07 +0200, Rafael J. Wysocki wrote:
> On Mon, Mar 31, 2025 at 9:38 AM Zhang, Rui <rui.zhang@intel.com> wrote:
> > 
> > > -----Original Message-----
> > > From: Giovanni Gherdovich <ggherdovich@suse.cz>
> > > Sent: Friday, March 28, 2025 10:31 PM
> > > To: Rafael J . Wysocki <rafael@kernel.org>; Zhang, Rui
> > > <rui.zhang@intel.com>
> > > Cc: Len Brown <lenb@kernel.org>; Giovanni Gherdovich
> > > <ggherdovich@suse.cz>; linux-acpi@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; linux-pm@vger.kernel.org
> > > Subject: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment
> > > Importance: High
> > > 
> > > Since commit 496121c02127e9c460b436244c38260b044cc45a ("ACPI:
> > > processor:
> > > idle: Allow probing on platforms with one ACPI C-state"), the
> > > comment
> > > doesn't reflect the code anymore; remove it.
> > > 
> > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
> > > ---
> > >  drivers/acpi/processor_idle.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/processor_idle.c
> > > b/drivers/acpi/processor_idle.c
> > > index b181f7fc2090..2a076c7a825a 100644
> > > --- a/drivers/acpi/processor_idle.c
> > > +++ b/drivers/acpi/processor_idle.c
> > > @@ -482,10 +482,6 @@ static int
> > > acpi_processor_get_cstate_info(struct
> > > acpi_processor *pr)
> > > 
> > >       pr->power.count = acpi_processor_power_verify(pr);
> > > 
> > > -     /*
> > > -      * if one state of type C2 or C3 is available, mark this
> > > -      * CPU as being "idle manageable"
> > > -      */
> > >       for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
> > >               if (pr->power.states[i].valid) {
> > >                       pr->power.count = i;
> > > --
> > > 2.43.0
> > 
> > I think we can clean up a bit more. How about the patch below?
> > 
> > From 115d3a07febff32eed49f9343ef111e7e1452f9d Mon Sep 17 00:00:00
> > 2001
> > From: "Zhang, Rui" <rui.zhang@intel.com>
> > Date: Mon, 31 Mar 2025 07:29:57 +0000
> > Subject: [PATCH] ACPI: processor: idle: Simplify
> >  acpi_processor_get_cstate_info() logic
> > 
> > Since commit 496121c02127 ("ACPI: processor: idle: Allow probing on
> > platforms with one ACPI C-state"), acpi_idle driver can be probed
> > with
> > C1 only.
> > 
> > Optimize the logic for setting pr->power.count and pr->flags.power by
> > 1. unconditionally set pr->flags.power leveraging the fact that C1 is
> >    always valid after acpi_processor_get_power_info_default().
> > 2. update acpi_processor_power_verify() to return the highest valid
> >    C-state directly.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/processor_idle.c | 15 ++-------------
> >  1 file changed, 2 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/acpi/processor_idle.c
> > b/drivers/acpi/processor_idle.c
> > index 698897b29de2..7ce8c3802937 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -442,7 +442,7 @@ static int acpi_processor_power_verify(struct
> > acpi_processor *pr)
> > 
> >                 lapic_timer_check_state(i, pr, cx);
> >                 tsc_check_state(cx->type);
> > -               working++;
> > +               working = i;
> 
> What if some states are skipped because they are invalid?  'working'
> can be less than 'i' then AFAICS.

yes, but please refer to my comments here and below,

1. 'working' is used as return value only in
acpi_processor_power_verify().


> 
> >         }
> > 
> >         if (buggy_latency) {
> > @@ -457,7 +457,6 @@ static int acpi_processor_power_verify(struct
> > acpi_processor *pr)
> > 
> >  static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
> >  {
> > -       unsigned int i;
> >         int result;
> > 
> > 
> > @@ -477,17 +476,7 @@ static int acpi_processor_get_cstate_info(struct
> > acpi_processor *pr)
> >         acpi_processor_get_power_info_default(pr);
> > 
> >         pr->power.count = acpi_processor_power_verify(pr);

2. acpi_processor_get_cstate_info(), which is the only caller of
acpi_processor_power_verify(), use this return value to set
pr->power.count.

> > -
> > -       /*
> > -        * if one state of type C2 or C3 is available, mark this
> > -        * CPU as being "idle manageable"
> > -        */
> > -       for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
> > -               if (pr->power.states[i].valid) {
> > -                       pr->power.count = i;

3. use a loop to override pr->power.count with the index of the highest
valid state

So I'm proposing to return the index of the highest valid state directly
in acpi_processor_power_verify() and then we don't need this loop any
more.

thanks,
rui

> > -                       pr->flags.power = 1;
> > -               }
> > -       }
> > +       pr->flags.power = 1;
> > 
> >         return 0;
> >  }
> > --

Re: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment

[Rafael J. Wysocki]

On Tue, Apr 1, 2025 at 2:25 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Mon, 2025-03-31 at 14:07 +0200, Rafael J. Wysocki wrote:
> > On Mon, Mar 31, 2025 at 9:38 AM Zhang, Rui <rui.zhang@intel.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Giovanni Gherdovich <ggherdovich@suse.cz>
> > > > Sent: Friday, March 28, 2025 10:31 PM
> > > > To: Rafael J . Wysocki <rafael@kernel.org>; Zhang, Rui
> > > > <rui.zhang@intel.com>
> > > > Cc: Len Brown <lenb@kernel.org>; Giovanni Gherdovich
> > > > <ggherdovich@suse.cz>; linux-acpi@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; linux-pm@vger.kernel.org
> > > > Subject: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment
> > > > Importance: High
> > > >
> > > > Since commit 496121c02127e9c460b436244c38260b044cc45a ("ACPI:
> > > > processor:
> > > > idle: Allow probing on platforms with one ACPI C-state"), the
> > > > comment
> > > > doesn't reflect the code anymore; remove it.
> > > >
> > > > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
> > > > ---
> > > >  drivers/acpi/processor_idle.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/processor_idle.c
> > > > b/drivers/acpi/processor_idle.c
> > > > index b181f7fc2090..2a076c7a825a 100644
> > > > --- a/drivers/acpi/processor_idle.c
> > > > +++ b/drivers/acpi/processor_idle.c
> > > > @@ -482,10 +482,6 @@ static int
> > > > acpi_processor_get_cstate_info(struct
> > > > acpi_processor *pr)
> > > >
> > > >       pr->power.count = acpi_processor_power_verify(pr);
> > > >
> > > > -     /*
> > > > -      * if one state of type C2 or C3 is available, mark this
> > > > -      * CPU as being "idle manageable"
> > > > -      */
> > > >       for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
> > > >               if (pr->power.states[i].valid) {
> > > >                       pr->power.count = i;
> > > > --
> > > > 2.43.0
> > >
> > > I think we can clean up a bit more. How about the patch below?
> > >
> > > From 115d3a07febff32eed49f9343ef111e7e1452f9d Mon Sep 17 00:00:00
> > > 2001
> > > From: "Zhang, Rui" <rui.zhang@intel.com>
> > > Date: Mon, 31 Mar 2025 07:29:57 +0000
> > > Subject: [PATCH] ACPI: processor: idle: Simplify
> > >  acpi_processor_get_cstate_info() logic
> > >
> > > Since commit 496121c02127 ("ACPI: processor: idle: Allow probing on
> > > platforms with one ACPI C-state"), acpi_idle driver can be probed
> > > with
> > > C1 only.
> > >
> > > Optimize the logic for setting pr->power.count and pr->flags.power by
> > > 1. unconditionally set pr->flags.power leveraging the fact that C1 is
> > >    always valid after acpi_processor_get_power_info_default().
> > > 2. update acpi_processor_power_verify() to return the highest valid
> > >    C-state directly.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/acpi/processor_idle.c | 15 ++-------------
> > >  1 file changed, 2 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/acpi/processor_idle.c
> > > b/drivers/acpi/processor_idle.c
> > > index 698897b29de2..7ce8c3802937 100644
> > > --- a/drivers/acpi/processor_idle.c
> > > +++ b/drivers/acpi/processor_idle.c
> > > @@ -442,7 +442,7 @@ static int acpi_processor_power_verify(struct
> > > acpi_processor *pr)
> > >
> > >                 lapic_timer_check_state(i, pr, cx);
> > >                 tsc_check_state(cx->type);
> > > -               working++;
> > > +               working = i;
> >
> > What if some states are skipped because they are invalid?  'working'
> > can be less than 'i' then AFAICS.
>
> yes, but please refer to my comments here and below,
>
> 1. 'working' is used as return value only in acpi_processor_power_verify().
>
> >
> > >         }
> > >
> > >         if (buggy_latency) {
> > > @@ -457,7 +457,6 @@ static int acpi_processor_power_verify(struct
> > > acpi_processor *pr)
> > >
> > >  static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
> > >  {
> > > -       unsigned int i;
> > >         int result;
> > >
> > >
> > > @@ -477,17 +476,7 @@ static int acpi_processor_get_cstate_info(struct
> > > acpi_processor *pr)
> > >         acpi_processor_get_power_info_default(pr);
> > >
> > >         pr->power.count = acpi_processor_power_verify(pr);
>
> 2. acpi_processor_get_cstate_info(), which is the only caller of
> acpi_processor_power_verify(), use this return value to set
> pr->power.count.

So far so good.

> > > -
> > > -       /*
> > > -        * if one state of type C2 or C3 is available, mark this
> > > -        * CPU as being "idle manageable"
> > > -        */
> > > -       for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
> > > -               if (pr->power.states[i].valid) {
> > > -                       pr->power.count = i;
>
> 3. use a loop to override pr->power.count with the index of the highest
> valid state

I see.

> So I'm proposing to return the index of the highest valid state directly
> in acpi_processor_power_verify() and then we don't need this loop any
> more.

OK, so I'd prefer to first rename power.count to power.max_index
(which it really is) and then make the changes you have proposed.

Re: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment

[Zhang, Rui]

On Tue, 2025-04-01 at 14:13 +0200, Rafael J. Wysocki wrote:
> 
> > So I'm proposing to return the index of the highest valid state
> > directly
> > in acpi_processor_power_verify() and then we don't need this loop any
> > more.
> 
> OK, so I'd prefer to first rename power.count to power.max_index
> (which it really is) and then make the changes you have proposed.

well, in other cases, like in acpi_processor_evaluate_cst() and in the
_LPI case, power.count is still set and used as the total number of
cstates.

in this acpi_processor_get_cstate_info() case, maybe we should drop this
change
-               working++;
+               working = i;
So power.count is still consistent in all these cases.

For the current for loop that overrides power.count, I think we can just
drop it, because no one checks power.count after it, which means no one
actually uses power.count as max_index.

-rui

Re: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment

[Rafael J. Wysocki]

On Thu, Apr 3, 2025 at 4:54 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Tue, 2025-04-01 at 14:13 +0200, Rafael J. Wysocki wrote:
> >
> > > So I'm proposing to return the index of the highest valid state
> > > directly
> > > in acpi_processor_power_verify() and then we don't need this loop any
> > > more.
> >
> > OK, so I'd prefer to first rename power.count to power.max_index
> > (which it really is) and then make the changes you have proposed.
>
> well, in other cases, like in acpi_processor_evaluate_cst() and in the
> _LPI case, power.count is still set and used as the total number of
> cstates.
>
> in this acpi_processor_get_cstate_info() case, maybe we should drop this
> change
> -               working++;
> +               working = i;
> So power.count is still consistent in all these cases.

OK

> For the current for loop that overrides power.count, I think we can just
> drop it, because no one checks power.count after it, which means no one
> actually uses power.count as max_index.

Sounds good to me.

Re: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment

[Zhang, Rui]

On Fri, 2025-03-28 at 15:30 +0100, Giovanni Gherdovich wrote:
> Since commit 496121c02127e9c460b436244c38260b044cc45a ("ACPI:
> processor:
> idle: Allow probing on platforms with one ACPI C-state"), the comment
> doesn't reflect the code anymore; remove it.
> 
> Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>

This is a standalone cleanup, and further cleanups are posted in a
separate patch set on top of this one, so

Acked-by: Zhang Rui <rui.zhang@intel.com>

> ---
>  drivers/acpi/processor_idle.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c
> b/drivers/acpi/processor_idle.c
> index b181f7fc2090..2a076c7a825a 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -482,10 +482,6 @@ static int acpi_processor_get_cstate_info(struct
> acpi_processor *pr)
>  
>  	pr->power.count = acpi_processor_power_verify(pr);
>  
> -	/*
> -	 * if one state of type C2 or C3 is available, mark this
> -	 * CPU as being "idle manageable"
> -	 */
>  	for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
>  		if (pr->power.states[i].valid) {
>  			pr->power.count = i;

Re: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment

[Rafael J. Wysocki]

On Wed, Apr 9, 2025 at 2:54 AM Zhang, Rui <rui.zhang@intel.com> wrote:
>
> On Fri, 2025-03-28 at 15:30 +0100, Giovanni Gherdovich wrote:
> > Since commit 496121c02127e9c460b436244c38260b044cc45a ("ACPI:
> > processor:
> > idle: Allow probing on platforms with one ACPI C-state"), the comment
> > doesn't reflect the code anymore; remove it.
> >
> > Signed-off-by: Giovanni Gherdovich <ggherdovich@suse.cz>
>
> This is a standalone cleanup, and further cleanups are posted in a
> separate patch set on top of this one, so
>
> Acked-by: Zhang Rui <rui.zhang@intel.com>

Applied as 6.16 material, thanks!

> > ---
> >  drivers/acpi/processor_idle.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_idle.c
> > b/drivers/acpi/processor_idle.c
> > index b181f7fc2090..2a076c7a825a 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -482,10 +482,6 @@ static int acpi_processor_get_cstate_info(struct
> > acpi_processor *pr)
> >
> >       pr->power.count = acpi_processor_power_verify(pr);
> >
> > -     /*
> > -      * if one state of type C2 or C3 is available, mark this
> > -      * CPU as being "idle manageable"
> > -      */
> >       for (i = 1; i < ACPI_PROCESSOR_MAX_POWER; i++) {
> >               if (pr->power.states[i].valid) {
> >                       pr->power.count = i;
>