* [PATCH v2 1/7] ACPI: processor: idle: Mark the state as invalid if its entry method is illegal
2025-11-03 8:42 [PATCH v2 0/7] ACPI: processor: idle: enhance and cleancode for cpuidle state Huisong Li
@ 2025-11-03 8:42 ` Huisong Li
2025-11-03 17:49 ` Rafael J. Wysocki
2025-11-03 8:42 ` [PATCH v2 2/7] ACPI: processor: idle: Mark the state as invalid when get lpi_state->arch_flags failed Huisong Li
` (5 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Huisong Li @ 2025-11-03 8:42 UTC (permalink / raw)
To: rafael, lenb
Cc: linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, lihuisong
According to ACPI spec, entry method in LPI sub-package must be buffer
or integer. And the entry method is very key in cpuidle. So mark the state
as invalid.
Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/acpi/processor_idle.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 341825e8ac63..9f1040eac051 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -956,6 +956,9 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
lpi_state->entry_method = ACPI_CSTATE_INTEGER;
lpi_state->address = obj->integer.value;
} else {
+ pr_debug("Entry method of state-%d is illegal, disable it.\n",
+ state_idx);
+ lpi_state->flags = 0;
continue;
}
--
2.33.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/7] ACPI: processor: idle: Mark the state as invalid if its entry method is illegal
2025-11-03 8:42 ` [PATCH v2 1/7] ACPI: processor: idle: Mark the state as invalid if its entry method is illegal Huisong Li
@ 2025-11-03 17:49 ` Rafael J. Wysocki
2025-11-04 9:30 ` lihuisong (C)
0 siblings, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2025-11-03 17:49 UTC (permalink / raw)
To: Huisong Li
Cc: rafael, lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> According to ACPI spec, entry method in LPI sub-package must be buffer
> or integer. And the entry method is very key in cpuidle. So mark the state
> as invalid.
>
> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> drivers/acpi/processor_idle.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 341825e8ac63..9f1040eac051 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -956,6 +956,9 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
> lpi_state->entry_method = ACPI_CSTATE_INTEGER;
> lpi_state->address = obj->integer.value;
> } else {
> + pr_debug("Entry method of state-%d is illegal, disable it.\n",
> + state_idx);
> + lpi_state->flags = 0;
Why does lpi_state->flags need to be cleared, isn't it 0 already?
> continue;
> }
>
> --
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/7] ACPI: processor: idle: Mark the state as invalid if its entry method is illegal
2025-11-03 17:49 ` Rafael J. Wysocki
@ 2025-11-04 9:30 ` lihuisong (C)
2025-11-04 15:00 ` Rafael J. Wysocki
0 siblings, 1 reply; 22+ messages in thread
From: lihuisong (C) @ 2025-11-04 9:30 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
在 2025/11/4 1:49, Rafael J. Wysocki 写道:
> On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>> According to ACPI spec, entry method in LPI sub-package must be buffer
>> or integer. And the entry method is very key in cpuidle. So mark the state
>> as invalid.
>>
>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> drivers/acpi/processor_idle.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 341825e8ac63..9f1040eac051 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -956,6 +956,9 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>> lpi_state->entry_method = ACPI_CSTATE_INTEGER;
>> lpi_state->address = obj->integer.value;
>> } else {
>> + pr_debug("Entry method of state-%d is illegal, disable it.\n",
>> + state_idx);
>> + lpi_state->flags = 0;
> Why does lpi_state->flags need to be cleared, isn't it 0 already?
Good point.
Do we need to add debug log?
>
>> continue;
>> }
>>
>> --
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/7] ACPI: processor: idle: Mark the state as invalid if its entry method is illegal
2025-11-04 9:30 ` lihuisong (C)
@ 2025-11-04 15:00 ` Rafael J. Wysocki
2025-11-05 8:53 ` lihuisong (C)
0 siblings, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2025-11-04 15:00 UTC (permalink / raw)
To: lihuisong (C)
Cc: Rafael J. Wysocki, lenb, linux-acpi, linux-kernel, Sudeep.Holla,
linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
On Tue, Nov 4, 2025 at 10:30 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/11/4 1:49, Rafael J. Wysocki 写道:
> > On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
> >> According to ACPI spec, entry method in LPI sub-package must be buffer
> >> or integer. And the entry method is very key in cpuidle. So mark the state
> >> as invalid.
> >>
> >> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> >> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >> ---
> >> drivers/acpi/processor_idle.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >> index 341825e8ac63..9f1040eac051 100644
> >> --- a/drivers/acpi/processor_idle.c
> >> +++ b/drivers/acpi/processor_idle.c
> >> @@ -956,6 +956,9 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
> >> lpi_state->entry_method = ACPI_CSTATE_INTEGER;
> >> lpi_state->address = obj->integer.value;
> >> } else {
> >> + pr_debug("Entry method of state-%d is illegal, disable it.\n",
> >> + state_idx);
> >> + lpi_state->flags = 0;
> > Why does lpi_state->flags need to be cleared, isn't it 0 already?
> Good point.
> Do we need to add debug log?
Well, is it helpful?
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/7] ACPI: processor: idle: Mark the state as invalid if its entry method is illegal
2025-11-04 15:00 ` Rafael J. Wysocki
@ 2025-11-05 8:53 ` lihuisong (C)
0 siblings, 0 replies; 22+ messages in thread
From: lihuisong (C) @ 2025-11-05 8:53 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
在 2025/11/4 23:00, Rafael J. Wysocki 写道:
> On Tue, Nov 4, 2025 at 10:30 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2025/11/4 1:49, Rafael J. Wysocki 写道:
>>> On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>> According to ACPI spec, entry method in LPI sub-package must be buffer
>>>> or integer. And the entry method is very key in cpuidle. So mark the state
>>>> as invalid.
>>>>
>>>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> ---
>>>> drivers/acpi/processor_idle.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>>>> index 341825e8ac63..9f1040eac051 100644
>>>> --- a/drivers/acpi/processor_idle.c
>>>> +++ b/drivers/acpi/processor_idle.c
>>>> @@ -956,6 +956,9 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
>>>> lpi_state->entry_method = ACPI_CSTATE_INTEGER;
>>>> lpi_state->address = obj->integer.value;
>>>> } else {
>>>> + pr_debug("Entry method of state-%d is illegal, disable it.\n",
>>>> + state_idx);
>>>> + lpi_state->flags = 0;
>>> Why does lpi_state->flags need to be cleared, isn't it 0 already?
>> Good point.
>> Do we need to add debug log?
> Well, is it helpful?
I think it is useful for developers.
They have a way to know why a state is disabled.
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 2/7] ACPI: processor: idle: Mark the state as invalid when get lpi_state->arch_flags failed
2025-11-03 8:42 [PATCH v2 0/7] ACPI: processor: idle: enhance and cleancode for cpuidle state Huisong Li
2025-11-03 8:42 ` [PATCH v2 1/7] ACPI: processor: idle: Mark the state as invalid if its entry method is illegal Huisong Li
@ 2025-11-03 8:42 ` Huisong Li
2025-11-03 17:53 ` Rafael J. Wysocki
2025-11-03 8:42 ` [PATCH v2 3/7] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe Huisong Li
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Huisong Li @ 2025-11-03 8:42 UTC (permalink / raw)
To: rafael, lenb
Cc: linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, lihuisong
The architecture specific context loss flags is important for ARM.
And this flag is used to control the execution of different code
flows in acpi_processor_ffh_lpi_enter().
So mark the state as invalid when get lpi_state->arch_flags of this
state failed.
Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/acpi/processor_idle.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 9f1040eac051..5213a545fa78 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -983,8 +983,12 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
if (obj_get_integer(pkg_elem + 2, &lpi_state->flags))
lpi_state->flags = 0;
- if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags))
- lpi_state->arch_flags = 0;
+ if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags)) {
+ pr_err("Get state-%d architecture specific context loss flags failed, disable it.\n",
+ state_idx);
+ lpi_state->flags = 0;
+ continue;
+ }
if (obj_get_integer(pkg_elem + 4, &lpi_state->res_cnt_freq))
lpi_state->res_cnt_freq = 1;
--
2.33.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 2/7] ACPI: processor: idle: Mark the state as invalid when get lpi_state->arch_flags failed
2025-11-03 8:42 ` [PATCH v2 2/7] ACPI: processor: idle: Mark the state as invalid when get lpi_state->arch_flags failed Huisong Li
@ 2025-11-03 17:53 ` Rafael J. Wysocki
0 siblings, 0 replies; 22+ messages in thread
From: Rafael J. Wysocki @ 2025-11-03 17:53 UTC (permalink / raw)
To: Huisong Li
Cc: rafael, lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> The architecture specific context loss flags is important for ARM.
> And this flag is used to control the execution of different code
> flows in acpi_processor_ffh_lpi_enter().
>
> So mark the state as invalid when get lpi_state->arch_flags of this
> state failed.
>
> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> drivers/acpi/processor_idle.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 9f1040eac051..5213a545fa78 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -983,8 +983,12 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
> if (obj_get_integer(pkg_elem + 2, &lpi_state->flags))
> lpi_state->flags = 0;
>
> - if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags))
> - lpi_state->arch_flags = 0;
> + if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags)) {
> + pr_err("Get state-%d architecture specific context loss flags failed, disable it.\n",
> + state_idx);
> + lpi_state->flags = 0;
> + continue;
This is ARM-specific behavior you want to put into generic code.
I'm not going to apply this.
> + }
>
> if (obj_get_integer(pkg_elem + 4, &lpi_state->res_cnt_freq))
> lpi_state->res_cnt_freq = 1;
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 3/7] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe
2025-11-03 8:42 [PATCH v2 0/7] ACPI: processor: idle: enhance and cleancode for cpuidle state Huisong Li
2025-11-03 8:42 ` [PATCH v2 1/7] ACPI: processor: idle: Mark the state as invalid if its entry method is illegal Huisong Li
2025-11-03 8:42 ` [PATCH v2 2/7] ACPI: processor: idle: Mark the state as invalid when get lpi_state->arch_flags failed Huisong Li
@ 2025-11-03 8:42 ` Huisong Li
2025-11-03 17:56 ` Rafael J. Wysocki
2025-11-03 8:42 ` [PATCH v2 4/7] ACPI: processor: idle: Disable ACPI idle if get power information failed in power notify Huisong Li
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Huisong Li @ 2025-11-03 8:42 UTC (permalink / raw)
To: rafael, lenb
Cc: linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, lihuisong
The platform used LPI need check if the LPI support and the entry
method is valid by the acpi_processor_ffh_lpi_probe(). But the return
of acpi_processor_ffh_lpi_probe() in acpi_processor_setup_cpuidle_dev()
isn't verified by any caller.
What's more, acpi_processor_get_power_info() is a more logical place for
verifying the validity of FFH LPI than acpi_processor_setup_cpuidle_dev().
So move acpi_processor_ffh_lpi_probe() from the latter to the former and
verify its return.
Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/acpi/processor_idle.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 5213a545fa78..c73df5933691 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1266,7 +1266,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
dev->cpu = pr->id;
if (pr->flags.has_lpi)
- return acpi_processor_ffh_lpi_probe(pr->id);
+ return 0;
return acpi_processor_setup_cpuidle_cx(pr, dev);
}
@@ -1277,7 +1277,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
ret = acpi_processor_get_lpi_info(pr);
if (ret)
- ret = acpi_processor_get_cstate_info(pr);
+ return acpi_processor_get_cstate_info(pr);
+
+ if (pr->flags.has_lpi) {
+ ret = acpi_processor_ffh_lpi_probe(pr->id);
+ if (ret)
+ pr_err("Processor FFH LPI state is invalid.\n");
+ }
return ret;
}
--
2.33.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 3/7] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe
2025-11-03 8:42 ` [PATCH v2 3/7] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe Huisong Li
@ 2025-11-03 17:56 ` Rafael J. Wysocki
2025-11-04 9:37 ` lihuisong (C)
0 siblings, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2025-11-03 17:56 UTC (permalink / raw)
To: Huisong Li
Cc: rafael, lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> The platform used LPI need check if the LPI support and the entry
> method is valid by the acpi_processor_ffh_lpi_probe(). But the return
> of acpi_processor_ffh_lpi_probe() in acpi_processor_setup_cpuidle_dev()
> isn't verified by any caller.
>
> What's more, acpi_processor_get_power_info() is a more logical place for
> verifying the validity of FFH LPI than acpi_processor_setup_cpuidle_dev().
> So move acpi_processor_ffh_lpi_probe() from the latter to the former and
> verify its return.
>
> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
If you want to add this Fixes: tag, please add some information on
what systems it breaks on and what the symptoms are to the changelog.
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> drivers/acpi/processor_idle.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 5213a545fa78..c73df5933691 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1266,7 +1266,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
>
> dev->cpu = pr->id;
> if (pr->flags.has_lpi)
> - return acpi_processor_ffh_lpi_probe(pr->id);
> + return 0;
>
> return acpi_processor_setup_cpuidle_cx(pr, dev);
> }
> @@ -1277,7 +1277,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
>
> ret = acpi_processor_get_lpi_info(pr);
> if (ret)
> - ret = acpi_processor_get_cstate_info(pr);
> + return acpi_processor_get_cstate_info(pr);
> +
> + if (pr->flags.has_lpi) {
> + ret = acpi_processor_ffh_lpi_probe(pr->id);
> + if (ret)
> + pr_err("Processor FFH LPI state is invalid.\n");
> + }
>
> return ret;
> }
> --
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 3/7] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe
2025-11-03 17:56 ` Rafael J. Wysocki
@ 2025-11-04 9:37 ` lihuisong (C)
0 siblings, 0 replies; 22+ messages in thread
From: lihuisong (C) @ 2025-11-04 9:37 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
在 2025/11/4 1:56, Rafael J. Wysocki 写道:
> On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>> The platform used LPI need check if the LPI support and the entry
>> method is valid by the acpi_processor_ffh_lpi_probe(). But the return
>> of acpi_processor_ffh_lpi_probe() in acpi_processor_setup_cpuidle_dev()
>> isn't verified by any caller.
>>
>> What's more, acpi_processor_get_power_info() is a more logical place for
>> verifying the validity of FFH LPI than acpi_processor_setup_cpuidle_dev().
>> So move acpi_processor_ffh_lpi_probe() from the latter to the former and
>> verify its return.
>>
>> Fixes: a36a7fecfe60 ("ACPI / processor_idle: Add support for Low Power Idle(LPI) states")
> If you want to add this Fixes: tag, please add some information on
> what systems it breaks on and what the symptoms are to the changelog.
Sorry, I didn't specifically construct it.
This patch is just an optimization patch. All right, I will drop this
Fixes tag.
Thanks for your reminder.
>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> drivers/acpi/processor_idle.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 5213a545fa78..c73df5933691 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -1266,7 +1266,7 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
>>
>> dev->cpu = pr->id;
>> if (pr->flags.has_lpi)
>> - return acpi_processor_ffh_lpi_probe(pr->id);
>> + return 0;
>>
>> return acpi_processor_setup_cpuidle_cx(pr, dev);
>> }
>> @@ -1277,7 +1277,13 @@ static int acpi_processor_get_power_info(struct acpi_processor *pr)
>>
>> ret = acpi_processor_get_lpi_info(pr);
>> if (ret)
>> - ret = acpi_processor_get_cstate_info(pr);
>> + return acpi_processor_get_cstate_info(pr);
>> +
>> + if (pr->flags.has_lpi) {
>> + ret = acpi_processor_ffh_lpi_probe(pr->id);
>> + if (ret)
>> + pr_err("Processor FFH LPI state is invalid.\n");
>> + }
>>
>> return ret;
>> }
>> --
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 4/7] ACPI: processor: idle: Disable ACPI idle if get power information failed in power notify
2025-11-03 8:42 [PATCH v2 0/7] ACPI: processor: idle: enhance and cleancode for cpuidle state Huisong Li
` (2 preceding siblings ...)
2025-11-03 8:42 ` [PATCH v2 3/7] ACPI: processor: idle: Relocate and verify acpi_processor_ffh_lpi_probe Huisong Li
@ 2025-11-03 8:42 ` Huisong Li
2025-11-03 18:09 ` Rafael J. Wysocki
2025-11-03 8:42 ` [PATCH v2 5/7] ACPI: processor: idle: Remove useless codes about the verification of cstate count Huisong Li
` (2 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Huisong Li @ 2025-11-03 8:42 UTC (permalink / raw)
To: rafael, lenb
Cc: linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, lihuisong
The old states may not be usable any more if get power information
failed in power notify. The ACPI idle should be disabled entirely.
Fixes: f427e5f1cf75 ("ACPI / processor: Get power info before updating the C-states")
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/acpi/processor_idle.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index c73df5933691..4627b00257e6 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1317,6 +1317,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
int cpu;
struct acpi_processor *_pr;
struct cpuidle_device *dev;
+ int ret = 0;
if (disabled_by_idle_boot_param())
return 0;
@@ -1345,8 +1346,18 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
cpuidle_disable_device(dev);
}
- /* Populate Updated C-state information */
- acpi_processor_get_power_info(pr);
+ /*
+ * Populate Updated C-state information
+ * The same idle state is used for all CPUs, cpuidle of all CPUs
+ * should be disabled.
+ */
+ ret = acpi_processor_get_power_info(pr);
+ if (ret) {
+ pr_err("Get processor-%u power information failed, disable cpuidle of all CPUs\n",
+ pr->id);
+ goto release_lock;
+ }
+
acpi_processor_setup_cpuidle_states(pr);
/* Enable all cpuidle devices */
@@ -1354,18 +1365,19 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
_pr = per_cpu(processors, cpu);
if (!_pr || !_pr->flags.power_setup_done)
continue;
- acpi_processor_get_power_info(_pr);
- if (_pr->flags.power) {
+ ret = acpi_processor_get_power_info(_pr);
+ if (!ret && _pr->flags.power) {
dev = per_cpu(acpi_cpuidle_device, cpu);
acpi_processor_setup_cpuidle_dev(_pr, dev);
cpuidle_enable_device(dev);
}
}
+release_lock:
cpuidle_resume_and_unlock();
cpus_read_unlock();
}
- return 0;
+ return ret;
}
void acpi_processor_register_idle_driver(void)
--
2.33.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/7] ACPI: processor: idle: Disable ACPI idle if get power information failed in power notify
2025-11-03 8:42 ` [PATCH v2 4/7] ACPI: processor: idle: Disable ACPI idle if get power information failed in power notify Huisong Li
@ 2025-11-03 18:09 ` Rafael J. Wysocki
2025-11-04 9:54 ` lihuisong (C)
0 siblings, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2025-11-03 18:09 UTC (permalink / raw)
To: Huisong Li
Cc: rafael, lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> The old states may not be usable any more if get power information
> failed in power notify. The ACPI idle should be disabled entirely.
How does it actually disable anything? It only changes the
acpi_processor_power_state_has_changed() return value AFAICS, but that
return value isn't checked.
> Fixes: f427e5f1cf75 ("ACPI / processor: Get power info before updating the C-states")
So how does it fix anything?
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> drivers/acpi/processor_idle.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index c73df5933691..4627b00257e6 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1317,6 +1317,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
> int cpu;
> struct acpi_processor *_pr;
> struct cpuidle_device *dev;
> + int ret = 0;
>
> if (disabled_by_idle_boot_param())
> return 0;
> @@ -1345,8 +1346,18 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
> cpuidle_disable_device(dev);
> }
>
> - /* Populate Updated C-state information */
> - acpi_processor_get_power_info(pr);
> + /*
> + * Populate Updated C-state information
> + * The same idle state is used for all CPUs, cpuidle of all CPUs
> + * should be disabled.
> + */
> + ret = acpi_processor_get_power_info(pr);
> + if (ret) {
> + pr_err("Get processor-%u power information failed, disable cpuidle of all CPUs\n",
> + pr->id);
pr_info() at most, preferably pr_debug() or maybe pr_info_once().
> + goto release_lock;
"unlock" would be a better name.
> + }
> +
> acpi_processor_setup_cpuidle_states(pr);
>
> /* Enable all cpuidle devices */
> @@ -1354,18 +1365,19 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
> _pr = per_cpu(processors, cpu);
> if (!_pr || !_pr->flags.power_setup_done)
> continue;
> - acpi_processor_get_power_info(_pr);
> - if (_pr->flags.power) {
> + ret = acpi_processor_get_power_info(_pr);
This does not need to be called if _pr->flags.power is unset. Why are
you changing this?
> + if (!ret && _pr->flags.power) {
> dev = per_cpu(acpi_cpuidle_device, cpu);
> acpi_processor_setup_cpuidle_dev(_pr, dev);
> cpuidle_enable_device(dev);
> }
If it succeeds for the next CPU, the return value will be still 0, won't it?
> }
> +release_lock:
> cpuidle_resume_and_unlock();
> cpus_read_unlock();
> }
>
> - return 0;
> + return ret;
> }
>
> void acpi_processor_register_idle_driver(void)
> --
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/7] ACPI: processor: idle: Disable ACPI idle if get power information failed in power notify
2025-11-03 18:09 ` Rafael J. Wysocki
@ 2025-11-04 9:54 ` lihuisong (C)
2025-11-04 16:19 ` Rafael J. Wysocki
0 siblings, 1 reply; 22+ messages in thread
From: lihuisong (C) @ 2025-11-04 9:54 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
在 2025/11/4 2:09, Rafael J. Wysocki 写道:
> On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>> The old states may not be usable any more if get power information
>> failed in power notify. The ACPI idle should be disabled entirely.
> How does it actually disable anything? It only changes the
> acpi_processor_power_state_has_changed() return value AFAICS, but that
> return value isn't checked.
The acpi_processor_power_state_has_changed() will disable all cpuidle
device first.
AFAICS, the disabled cpuidle_device would not do cpuidle, please see
cpuidle_not_available() and cpuidle_idle_call().
It's enough for this?
>
>> Fixes: f427e5f1cf75 ("ACPI / processor: Get power info before updating the C-states")
> So how does it fix anything?
>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> drivers/acpi/processor_idle.c | 22 +++++++++++++++++-----
>> 1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index c73df5933691..4627b00257e6 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -1317,6 +1317,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>> int cpu;
>> struct acpi_processor *_pr;
>> struct cpuidle_device *dev;
>> + int ret = 0;
>>
>> if (disabled_by_idle_boot_param())
>> return 0;
>> @@ -1345,8 +1346,18 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>> cpuidle_disable_device(dev);
>> }
>>
>> - /* Populate Updated C-state information */
>> - acpi_processor_get_power_info(pr);
>> + /*
>> + * Populate Updated C-state information
>> + * The same idle state is used for all CPUs, cpuidle of all CPUs
>> + * should be disabled.
>> + */
>> + ret = acpi_processor_get_power_info(pr);
>> + if (ret) {
>> + pr_err("Get processor-%u power information failed, disable cpuidle of all CPUs\n",
>> + pr->id);
> pr_info() at most, preferably pr_debug() or maybe pr_info_once().
Ack, pr_info_once is good to me.
>
>> + goto release_lock;
> "unlock" would be a better name.
Ack
>
>> + }
>> +
>> acpi_processor_setup_cpuidle_states(pr);
>>
>> /* Enable all cpuidle devices */
>> @@ -1354,18 +1365,19 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>> _pr = per_cpu(processors, cpu);
>> if (!_pr || !_pr->flags.power_setup_done)
>> continue;
>> - acpi_processor_get_power_info(_pr);
>> - if (_pr->flags.power) {
>> + ret = acpi_processor_get_power_info(_pr);
> This does not need to be called if _pr->flags.power is unset. Why are
> you changing this?
_pr->flags.power is set in acpi_processor_get_power_info().
Ok, I know what you mean.
_pr->flags.power is unset if acpi_processor_get_power_info fail to execute.
But it may be the old value. So here should be necessary.
>
>> + if (!ret && _pr->flags.power) {
>> dev = per_cpu(acpi_cpuidle_device, cpu);
>> acpi_processor_setup_cpuidle_dev(_pr, dev);
>> cpuidle_enable_device(dev);
>> }
> If it succeeds for the next CPU, the return value will be still 0, won't it?
I think it is 0.
Do we need to do something for it, like, adding debug log?
>
>> }
>> +release_lock:
>> cpuidle_resume_and_unlock();
>> cpus_read_unlock();
>> }
>>
>> - return 0;
>> + return ret;
>> }
>>
>> void acpi_processor_register_idle_driver(void)
>> --
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 4/7] ACPI: processor: idle: Disable ACPI idle if get power information failed in power notify
2025-11-04 9:54 ` lihuisong (C)
@ 2025-11-04 16:19 ` Rafael J. Wysocki
2025-11-05 9:06 ` lihuisong (C)
0 siblings, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2025-11-04 16:19 UTC (permalink / raw)
To: lihuisong (C)
Cc: Rafael J. Wysocki, lenb, linux-acpi, linux-kernel, Sudeep.Holla,
linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
On Tue, Nov 4, 2025 at 10:54 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/11/4 2:09, Rafael J. Wysocki 写道:
> > On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
> >> The old states may not be usable any more if get power information
> >> failed in power notify. The ACPI idle should be disabled entirely.
> > How does it actually disable anything? It only changes the
> > acpi_processor_power_state_has_changed() return value AFAICS, but that
> > return value isn't checked.
> The acpi_processor_power_state_has_changed() will disable all cpuidle
> device first.
> AFAICS, the disabled cpuidle_device would not do cpuidle, please see
> cpuidle_not_available() and cpuidle_idle_call().
> It's enough for this?
Well, not really.
acpi_processor_register_idle_driver() has been changed to call
acpi_processor_get_power_info() for each CPU before registering the
idle driver and if that is successful, it will set
flags.power_setup_done for the given processor and call
acpi_processor_setup_cpuidle_states(). That processor need not be
CPU0.
However, the code updated by the $subject patch calls
acpi_processor_get_power_info() for CPU0 and the patch would make it
skip re-enabling cpuidle for all CPUs if it failed.
It essentially needs to do what is done in
acpi_processor_register_idle_driver(): find a CPU for which
acpi_processor_get_power_info() does not fail, then call
acpi_processor_setup_cpuidle_states() and re-enable cpuidle for all
CPUs unless acpi_processor_get_power_info() fails for all of them.
But there is still a question of whether or not this addresses any
breakage seen in the field. If not, maybe it's better to leave this
code as is for the time being?
I don't see why it is part of this series to be honest. It is not a cleanup.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 4/7] ACPI: processor: idle: Disable ACPI idle if get power information failed in power notify
2025-11-04 16:19 ` Rafael J. Wysocki
@ 2025-11-05 9:06 ` lihuisong (C)
0 siblings, 0 replies; 22+ messages in thread
From: lihuisong (C) @ 2025-11-05 9:06 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
在 2025/11/5 0:19, Rafael J. Wysocki 写道:
> On Tue, Nov 4, 2025 at 10:54 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2025/11/4 2:09, Rafael J. Wysocki 写道:
>>> On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>> The old states may not be usable any more if get power information
>>>> failed in power notify. The ACPI idle should be disabled entirely.
>>> How does it actually disable anything? It only changes the
>>> acpi_processor_power_state_has_changed() return value AFAICS, but that
>>> return value isn't checked.
>> The acpi_processor_power_state_has_changed() will disable all cpuidle
>> device first.
>> AFAICS, the disabled cpuidle_device would not do cpuidle, please see
>> cpuidle_not_available() and cpuidle_idle_call().
>> It's enough for this?
> Well, not really.
>
> acpi_processor_register_idle_driver() has been changed to call
> acpi_processor_get_power_info() for each CPU before registering the
> idle driver and if that is successful, it will set
> flags.power_setup_done for the given processor and call
> acpi_processor_setup_cpuidle_states(). That processor need not be
> CPU0.
>
> However, the code updated by the $subject patch calls
> acpi_processor_get_power_info() for CPU0 and the patch would make it
> skip re-enabling cpuidle for all CPUs if it failed.
>
> It essentially needs to do what is done in
> acpi_processor_register_idle_driver(): find a CPU for which
> acpi_processor_get_power_info() does not fail, then call
> acpi_processor_setup_cpuidle_states() and re-enable cpuidle for all
> CPUs unless acpi_processor_get_power_info() fails for all of them.
From the initialization perspective, I also think this approach is
appropriate.
>
> But there is still a question of whether or not this addresses any
> breakage seen in the field. If not, maybe it's better to leave this
> code as is for the time being?
AFAICS, this power notify can be received on each CPU.
It may be appropriate to update cpuidle state of this CPU if ACPI idle
supports per-cpu idle state.
Now that all CPUs have the same idle state.
I think it is ok to keep here the same as the initialization logic as
you said above.
>
> I don't see why it is part of this series to be honest. It is not a cleanup.
Yes, now it's more like a bugfix.
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 5/7] ACPI: processor: idle: Remove useless codes about the verification of cstate count
2025-11-03 8:42 [PATCH v2 0/7] ACPI: processor: idle: enhance and cleancode for cpuidle state Huisong Li
` (3 preceding siblings ...)
2025-11-03 8:42 ` [PATCH v2 4/7] ACPI: processor: idle: Disable ACPI idle if get power information failed in power notify Huisong Li
@ 2025-11-03 8:42 ` Huisong Li
2025-11-03 18:10 ` Rafael J. Wysocki
2025-11-03 8:42 ` [PATCH v2 6/7] ACPI: processor: idle: Redefine setup idle functions to void Huisong Li
2025-11-03 8:42 ` [PATCH v2 7/7] ACPI: processor: idle: Redefine acpi_processor_setup_cpuidle_dev " Huisong Li
6 siblings, 1 reply; 22+ messages in thread
From: Huisong Li @ 2025-11-03 8:42 UTC (permalink / raw)
To: rafael, lenb
Cc: linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, lihuisong
The acpi_processor_setup_cstates and acpi_processor_setup_cpuidle_cx will
be called after successfully obtaining the power information. These setup
functions have their own main role, but also verify the validity of cstate
count.
Actually, the acpi_processor_get_power_info_cst will return failure if the
cstate count is zero and acpi_processor_get_power_info will return failure.
So the verification of cstate count in these functions are useless.
No intentional functional impact.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/acpi/processor_idle.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 4627b00257e6..1f332f02d273 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -732,8 +732,8 @@ static int __cpuidle acpi_idle_enter_s2idle(struct cpuidle_device *dev,
return 0;
}
-static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
- struct cpuidle_device *dev)
+static void acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
+ struct cpuidle_device *dev)
{
int i, count = ACPI_IDLE_STATE_START;
struct acpi_processor_cx *cx;
@@ -753,14 +753,9 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
if (count == CPUIDLE_STATE_MAX)
break;
}
-
- if (!count)
- return -EINVAL;
-
- return 0;
}
-static int acpi_processor_setup_cstates(struct acpi_processor *pr)
+static void acpi_processor_setup_cstates(struct acpi_processor *pr)
{
int i, count;
struct acpi_processor_cx *cx;
@@ -822,11 +817,6 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
}
drv->state_count = count;
-
- if (!count)
- return -EINVAL;
-
- return 0;
}
static inline void acpi_processor_cstate_first_run_checks(void)
@@ -1248,7 +1238,8 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
if (pr->flags.has_lpi)
return acpi_processor_setup_lpi_states(pr);
- return acpi_processor_setup_cstates(pr);
+ acpi_processor_setup_cstates(pr);
+ return 0;
}
/**
@@ -1268,7 +1259,8 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
if (pr->flags.has_lpi)
return 0;
- return acpi_processor_setup_cpuidle_cx(pr, dev);
+ acpi_processor_setup_cpuidle_cx(pr, dev);
+ return 0;
}
static int acpi_processor_get_power_info(struct acpi_processor *pr)
--
2.33.0
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/7] ACPI: processor: idle: Remove useless codes about the verification of cstate count
2025-11-03 8:42 ` [PATCH v2 5/7] ACPI: processor: idle: Remove useless codes about the verification of cstate count Huisong Li
@ 2025-11-03 18:10 ` Rafael J. Wysocki
2025-11-04 10:03 ` lihuisong (C)
0 siblings, 1 reply; 22+ messages in thread
From: Rafael J. Wysocki @ 2025-11-03 18:10 UTC (permalink / raw)
To: Huisong Li
Cc: rafael, lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>
> The acpi_processor_setup_cstates and acpi_processor_setup_cpuidle_cx will
> be called after successfully obtaining the power information. These setup
> functions have their own main role, but also verify the validity of cstate
> count.
>
> Actually, the acpi_processor_get_power_info_cst will return failure if the
> cstate count is zero and acpi_processor_get_power_info will return failure.
>
> So the verification of cstate count in these functions are useless.
>
> No intentional functional impact.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> drivers/acpi/processor_idle.c | 22 +++++++---------------
> 1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 4627b00257e6..1f332f02d273 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -732,8 +732,8 @@ static int __cpuidle acpi_idle_enter_s2idle(struct cpuidle_device *dev,
> return 0;
> }
>
> -static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
> - struct cpuidle_device *dev)
> +static void acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
> + struct cpuidle_device *dev)
> {
> int i, count = ACPI_IDLE_STATE_START;
> struct acpi_processor_cx *cx;
> @@ -753,14 +753,9 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
> if (count == CPUIDLE_STATE_MAX)
> break;
> }
> -
> - if (!count)
> - return -EINVAL;
> -
> - return 0;
> }
>
> -static int acpi_processor_setup_cstates(struct acpi_processor *pr)
> +static void acpi_processor_setup_cstates(struct acpi_processor *pr)
> {
> int i, count;
> struct acpi_processor_cx *cx;
> @@ -822,11 +817,6 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
> }
>
> drv->state_count = count;
> -
> - if (!count)
> - return -EINVAL;
> -
> - return 0;
> }
>
> static inline void acpi_processor_cstate_first_run_checks(void)
> @@ -1248,7 +1238,8 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
> if (pr->flags.has_lpi)
> return acpi_processor_setup_lpi_states(pr);
>
> - return acpi_processor_setup_cstates(pr);
> + acpi_processor_setup_cstates(pr);
> + return 0;
> }
>
> /**
> @@ -1268,7 +1259,8 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
> if (pr->flags.has_lpi)
> return 0;
>
> - return acpi_processor_setup_cpuidle_cx(pr, dev);
> + acpi_processor_setup_cpuidle_cx(pr, dev);
> + return 0;
> }
>
> static int acpi_processor_get_power_info(struct acpi_processor *pr)
> --
Does this patch depend on the previous patches in the series? If it
doesn't, why don't you send it independently?
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/7] ACPI: processor: idle: Remove useless codes about the verification of cstate count
2025-11-03 18:10 ` Rafael J. Wysocki
@ 2025-11-04 10:03 ` lihuisong (C)
2025-11-04 16:20 ` Rafael J. Wysocki
0 siblings, 1 reply; 22+ messages in thread
From: lihuisong (C) @ 2025-11-04 10:03 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: lenb, linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
在 2025/11/4 2:10, Rafael J. Wysocki 写道:
> On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
>> The acpi_processor_setup_cstates and acpi_processor_setup_cpuidle_cx will
>> be called after successfully obtaining the power information. These setup
>> functions have their own main role, but also verify the validity of cstate
>> count.
>>
>> Actually, the acpi_processor_get_power_info_cst will return failure if the
>> cstate count is zero and acpi_processor_get_power_info will return failure.
>>
>> So the verification of cstate count in these functions are useless.
>>
>> No intentional functional impact.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> drivers/acpi/processor_idle.c | 22 +++++++---------------
>> 1 file changed, 7 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 4627b00257e6..1f332f02d273 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -732,8 +732,8 @@ static int __cpuidle acpi_idle_enter_s2idle(struct cpuidle_device *dev,
>> return 0;
>> }
>>
>> -static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>> - struct cpuidle_device *dev)
>> +static void acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>> + struct cpuidle_device *dev)
>> {
>> int i, count = ACPI_IDLE_STATE_START;
>> struct acpi_processor_cx *cx;
>> @@ -753,14 +753,9 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>> if (count == CPUIDLE_STATE_MAX)
>> break;
>> }
>> -
>> - if (!count)
>> - return -EINVAL;
>> -
>> - return 0;
>> }
>>
>> -static int acpi_processor_setup_cstates(struct acpi_processor *pr)
>> +static void acpi_processor_setup_cstates(struct acpi_processor *pr)
>> {
>> int i, count;
>> struct acpi_processor_cx *cx;
>> @@ -822,11 +817,6 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
>> }
>>
>> drv->state_count = count;
>> -
>> - if (!count)
>> - return -EINVAL;
>> -
>> - return 0;
>> }
>>
>> static inline void acpi_processor_cstate_first_run_checks(void)
>> @@ -1248,7 +1238,8 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
>> if (pr->flags.has_lpi)
>> return acpi_processor_setup_lpi_states(pr);
>>
>> - return acpi_processor_setup_cstates(pr);
>> + acpi_processor_setup_cstates(pr);
>> + return 0;
>> }
>>
>> /**
>> @@ -1268,7 +1259,8 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
>> if (pr->flags.has_lpi)
>> return 0;
>>
>> - return acpi_processor_setup_cpuidle_cx(pr, dev);
>> + acpi_processor_setup_cpuidle_cx(pr, dev);
>> + return 0;
>> }
>>
>> static int acpi_processor_get_power_info(struct acpi_processor *pr)
>> --
> Does this patch depend on the previous patches in the series? If it
> doesn't, why don't you send it independently?
Good suggestion. Thanks, got it.
This patch doesn't depend on them.
But patch 6/7 and 7/7 depend on this patch and patch 3/7.
If they still need some times to discuss, I can send this patch first.
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/7] ACPI: processor: idle: Remove useless codes about the verification of cstate count
2025-11-04 10:03 ` lihuisong (C)
@ 2025-11-04 16:20 ` Rafael J. Wysocki
0 siblings, 0 replies; 22+ messages in thread
From: Rafael J. Wysocki @ 2025-11-04 16:20 UTC (permalink / raw)
To: lihuisong (C)
Cc: Rafael J. Wysocki, lenb, linux-acpi, linux-kernel, Sudeep.Holla,
linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8
On Tue, Nov 4, 2025 at 11:03 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>
>
> 在 2025/11/4 2:10, Rafael J. Wysocki 写道:
> > On Mon, Nov 3, 2025 at 9:42 AM Huisong Li <lihuisong@huawei.com> wrote:
> >> The acpi_processor_setup_cstates and acpi_processor_setup_cpuidle_cx will
> >> be called after successfully obtaining the power information. These setup
> >> functions have their own main role, but also verify the validity of cstate
> >> count.
> >>
> >> Actually, the acpi_processor_get_power_info_cst will return failure if the
> >> cstate count is zero and acpi_processor_get_power_info will return failure.
> >>
> >> So the verification of cstate count in these functions are useless.
> >>
> >> No intentional functional impact.
> >>
> >> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >> ---
> >> drivers/acpi/processor_idle.c | 22 +++++++---------------
> >> 1 file changed, 7 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >> index 4627b00257e6..1f332f02d273 100644
> >> --- a/drivers/acpi/processor_idle.c
> >> +++ b/drivers/acpi/processor_idle.c
> >> @@ -732,8 +732,8 @@ static int __cpuidle acpi_idle_enter_s2idle(struct cpuidle_device *dev,
> >> return 0;
> >> }
> >>
> >> -static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
> >> - struct cpuidle_device *dev)
> >> +static void acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
> >> + struct cpuidle_device *dev)
> >> {
> >> int i, count = ACPI_IDLE_STATE_START;
> >> struct acpi_processor_cx *cx;
> >> @@ -753,14 +753,9 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
> >> if (count == CPUIDLE_STATE_MAX)
> >> break;
> >> }
> >> -
> >> - if (!count)
> >> - return -EINVAL;
> >> -
> >> - return 0;
> >> }
> >>
> >> -static int acpi_processor_setup_cstates(struct acpi_processor *pr)
> >> +static void acpi_processor_setup_cstates(struct acpi_processor *pr)
> >> {
> >> int i, count;
> >> struct acpi_processor_cx *cx;
> >> @@ -822,11 +817,6 @@ static int acpi_processor_setup_cstates(struct acpi_processor *pr)
> >> }
> >>
> >> drv->state_count = count;
> >> -
> >> - if (!count)
> >> - return -EINVAL;
> >> -
> >> - return 0;
> >> }
> >>
> >> static inline void acpi_processor_cstate_first_run_checks(void)
> >> @@ -1248,7 +1238,8 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
> >> if (pr->flags.has_lpi)
> >> return acpi_processor_setup_lpi_states(pr);
> >>
> >> - return acpi_processor_setup_cstates(pr);
> >> + acpi_processor_setup_cstates(pr);
> >> + return 0;
> >> }
> >>
> >> /**
> >> @@ -1268,7 +1259,8 @@ static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
> >> if (pr->flags.has_lpi)
> >> return 0;
> >>
> >> - return acpi_processor_setup_cpuidle_cx(pr, dev);
> >> + acpi_processor_setup_cpuidle_cx(pr, dev);
> >> + return 0;
> >> }
> >>
> >> static int acpi_processor_get_power_info(struct acpi_processor *pr)
> >> --
> > Does this patch depend on the previous patches in the series? If it
> > doesn't, why don't you send it independently?
> Good suggestion. Thanks, got it.
> This patch doesn't depend on them.
> But patch 6/7 and 7/7 depend on this patch and patch 3/7.
> If they still need some times to discuss, I can send this patch first.
Yes, please send it separately as I said. The rest of the series can
be rebased on it.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 6/7] ACPI: processor: idle: Redefine setup idle functions to void
2025-11-03 8:42 [PATCH v2 0/7] ACPI: processor: idle: enhance and cleancode for cpuidle state Huisong Li
` (4 preceding siblings ...)
2025-11-03 8:42 ` [PATCH v2 5/7] ACPI: processor: idle: Remove useless codes about the verification of cstate count Huisong Li
@ 2025-11-03 8:42 ` Huisong Li
2025-11-03 8:42 ` [PATCH v2 7/7] ACPI: processor: idle: Redefine acpi_processor_setup_cpuidle_dev " Huisong Li
6 siblings, 0 replies; 22+ messages in thread
From: Huisong Li @ 2025-11-03 8:42 UTC (permalink / raw)
To: rafael, lenb
Cc: linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, lihuisong
Notice that the acpi_processor_setup_cpuidle_states() don't need to
return any value because their callers don't check them anyway.
In addition, acpi_processor_setup_lpi_states() wouldn't execute with
failure. So redefine setup idle functions to void.
No intentional functional impact.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/acpi/processor_idle.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 1f332f02d273..46614cf1ae8b 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1185,7 +1185,7 @@ static int acpi_idle_lpi_enter(struct cpuidle_device *dev,
return -EINVAL;
}
-static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
+static void acpi_processor_setup_lpi_states(struct acpi_processor *pr)
{
int i;
struct acpi_lpi_state *lpi;
@@ -1193,7 +1193,7 @@ static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
struct cpuidle_driver *drv = &acpi_idle_driver;
if (!pr->flags.has_lpi)
- return -EOPNOTSUPP;
+ return;
for (i = 0; i < pr->power.count && i < CPUIDLE_STATE_MAX; i++) {
lpi = &pr->power.lpi_states[i];
@@ -1211,8 +1211,6 @@ static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
}
drv->state_count = i;
-
- return 0;
}
/**
@@ -1221,13 +1219,13 @@ static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
*
* @pr: the ACPI processor
*/
-static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
+static void acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
{
int i;
struct cpuidle_driver *drv = &acpi_idle_driver;
if (!pr->flags.power_setup_done || !pr->flags.power)
- return -EINVAL;
+ return;
drv->safe_state_index = -1;
for (i = ACPI_IDLE_STATE_START; i < CPUIDLE_STATE_MAX; i++) {
@@ -1235,11 +1233,12 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
drv->states[i].desc[0] = '\0';
}
- if (pr->flags.has_lpi)
- return acpi_processor_setup_lpi_states(pr);
+ if (pr->flags.has_lpi) {
+ acpi_processor_setup_lpi_states(pr);
+ return;
+ }
acpi_processor_setup_cstates(pr);
- return 0;
}
/**
--
2.33.0
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH v2 7/7] ACPI: processor: idle: Redefine acpi_processor_setup_cpuidle_dev to void
2025-11-03 8:42 [PATCH v2 0/7] ACPI: processor: idle: enhance and cleancode for cpuidle state Huisong Li
` (5 preceding siblings ...)
2025-11-03 8:42 ` [PATCH v2 6/7] ACPI: processor: idle: Redefine setup idle functions to void Huisong Li
@ 2025-11-03 8:42 ` Huisong Li
6 siblings, 0 replies; 22+ messages in thread
From: Huisong Li @ 2025-11-03 8:42 UTC (permalink / raw)
To: rafael, lenb
Cc: linux-acpi, linux-kernel, Sudeep.Holla, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, lihuisong
Notice that the acpi_processor_setup_cpuidle_dev() don't need to
return any value because their callers don't check them anyway.
So redefine the function to void.
No intentional functional impact.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/acpi/processor_idle.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 46614cf1ae8b..2ae51c42f544 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1248,18 +1248,17 @@ static void acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
* @pr: the ACPI processor
* @dev : the cpuidle device
*/
-static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
- struct cpuidle_device *dev)
+static void acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
+ struct cpuidle_device *dev)
{
if (!pr->flags.power_setup_done || !pr->flags.power || !dev)
- return -EINVAL;
+ return;
dev->cpu = pr->id;
if (pr->flags.has_lpi)
- return 0;
+ return;
acpi_processor_setup_cpuidle_cx(pr, dev);
- return 0;
}
static int acpi_processor_get_power_info(struct acpi_processor *pr)
--
2.33.0
^ permalink raw reply [flat|nested] 22+ messages in thread