| Message ID | 20250328143040.9348-2-ggherdovich@suse.cz (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [1/2] ACPI: processor: idle: Return an error if both P_LVL{2,3} idle states are invalid | expand |
> -----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; }
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; > } > --
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; > > } > > --
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.
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;
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(-)