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.0On 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)
> --
在 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)
>> --
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.
在 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. >
© 2016 - 2025 Red Hat, Inc.