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@xxxxxxxxx> wrote:
> > 
> > > -----Original Message-----
> > > From: Giovanni Gherdovich <ggherdovich@xxxxxxx>
> > > Sent: Friday, March 28, 2025 10:31 PM
> > > To: Rafael J . Wysocki <rafael@xxxxxxxxxx>; Zhang, Rui
> > > <rui.zhang@xxxxxxxxx>
> > > Cc: Len Brown <lenb@xxxxxxxxxx>; Giovanni Gherdovich
> > > <ggherdovich@xxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; linux-
> > > kernel@xxxxxxxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx
> > > 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@xxxxxxx>
> > > ---
> > >  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@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > ---
> >  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;
> >  }
> > --