lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJZ5v0juH2kYx-fyyfoFLBTjg30y59Dwj1wBYXxuHvU2c7X31w@mail.gmail.com>
Date: Tue, 1 Apr 2025 14:13:54 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: "Zhang, Rui" <rui.zhang@...el.com>
Cc: "rafael@...nel.org" <rafael@...nel.org>, "ggherdovich@...e.cz" <ggherdovich@...e.cz>, 
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>, "lenb@...nel.org" <lenb@...nel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, 
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH 2/2] ACPI: processor: idle: Remove obsolete comment

On Tue, Apr 1, 2025 at 2:25 AM Zhang, Rui <rui.zhang@...el.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@...el.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Giovanni Gherdovich <ggherdovich@...e.cz>
> > > > Sent: Friday, March 28, 2025 10:31 PM
> > > > To: Rafael J . Wysocki <rafael@...nel.org>; Zhang, Rui
> > > > <rui.zhang@...el.com>
> > > > Cc: Len Brown <lenb@...nel.org>; Giovanni Gherdovich
> > > > <ggherdovich@...e.cz>; linux-acpi@...r.kernel.org; linux-
> > > > kernel@...r.kernel.org; linux-pm@...r.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@...e.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@...el.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@...el.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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ