Re: [PATCH v1 6/9] ACPI: processor: idle: Do not change power states if get power info failed

["lihuisong (C)" <lihuisong@huawei.com>]

在 2025/10/26 20:34, Rafael J. Wysocki 写道:
> On Fri, Oct 24, 2025 at 11:10 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>>
>> 在 2025/10/22 3:49, Rafael J. Wysocki 写道:
>>> On Mon, Sep 29, 2025 at 11:38 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>> Driver will update power states when processor power states have been
>>>> changed. To prevent any other abnormal issues, here add the verification
>>>> for the result of getting power information, don't change power states
>>>> and one error log when get power information failed.
>>> But the old states may not be usable any more in that case.
>> Yes
>>> If you want to check the acpi_processor_get_power_info(), it should
>>> disable ACPi idle entirely on failures.
>>   From the modification of this patch, this cpuidle device will be
>> disabled if the acpi_processor_get_power_info(）fails to get on this device.
>> And the cpuidle of the device will be disabled according to the
>> definition of cpuidle_not_available().
>> We should not call disable_cpuidle() to disable cpuidle of all CPUs.
> Since the same idle state data is used for all CPUs, I'd say cpuidle
Yes.
 From the current implementation perspective, the idle state is 
initialized by the first available CPU.
If there is one CPU get power management information failed later, the 
ACPI idle driver doesn't disable cpuidle of all CPUs and
just doesn't register cpudile_device and enable cpuidle_device.
> should be disabled for all of them in that case.
I can understand this. I think it is reasonable.
What do you think how to disable cpuidle of all CPUs here?
How about call disable_cpuidle() and disable all cpuidle devices?
>
> Alternatively, check if it works for any of them and apply the data
> from the CPU where it works to all of them.  If it doesn't work for
> any of them, there's nothing to apply.

How should we check if the idle states can work to all of CPUs?

>
>> So the modification in this patch is enough, right?
>>>> 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 | 15 ++++++++++-----
>>>>    1 file changed, 10 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>>>> index b0d6b51ee363..92b231f5d514 100644
>>>> --- a/drivers/acpi/processor_idle.c
>>>> +++ b/drivers/acpi/processor_idle.c
>>>> @@ -1315,6 +1315,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;
>>>> @@ -1344,16 +1345,20 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>>>>                   }
>>>>
>>>>                   /* Populate Updated C-state information */
>>>> -               acpi_processor_get_power_info(pr);
>>>> -               acpi_processor_setup_cpuidle_states(pr);
>>>> +               ret = acpi_processor_get_power_info(pr);
>>>> +               if (ret)
>>>> +                       pr_err("Get processor-%u power information failed.\n",
>>>> +                              pr->id);
>>>> +               else
>>>> +                       acpi_processor_setup_cpuidle_states(pr);
>>>>
>>>>                   /* Enable all cpuidle devices */
>>>>                   for_each_online_cpu(cpu) {
>>>>                           _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);
>>>> @@ -1363,7 +1368,7 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>>>>                   cpus_read_unlock();
>>>>           }
>>>>
>>>> -       return 0;
>>>> +       return ret;
>>>>    }
>>>>
>>>>    void acpi_processor_register_idle_driver(void)
>>>> --
>>>> 2.33.0
>>>>