On Tue, Apr 1, 2025 at 2:25 AM Zhang, Rui <rui.zhang@xxxxxxxxx> 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@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.
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.
