Re: [PATCH v3 2/2] ACPI: processor: idle: Optimize acpi idle driver registration

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

在 2025/8/24 0:13, Rafael J. Wysocki 写道:
> On Mon, Jul 28, 2025 at 9:06 AM Huisong Li <lihuisong@huawei.com> wrote:
>> Currently, the acpi idle driver is registered from within a CPU
>> hotplug callback. Although this didn't cause any functional issues,
>> this is questionable and confusing. And it is better to register
>> the cpuidle driver when all of the CPUs have been brought up.
>>
>> So add a new function to initialize acpi_idle_driver based on the
>> power management information of an available CPU and register cpuidle
>> driver in acpi_processor_driver_init().
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   drivers/acpi/processor_driver.c |  3 ++
>>   drivers/acpi/processor_idle.c   | 65 +++++++++++++++++++++------------
>>   include/acpi/processor.h        |  8 ++++
>>   3 files changed, 53 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 65e779be64ff..bc9f58a02c1d 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -263,6 +263,8 @@ static int __init acpi_processor_driver_init(void)
>>          if (result < 0)
>>                  return result;
>>
>> +       acpi_processor_register_idle_driver();
>> +
>>          result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>>                                     "acpi/cpu-drv:online",
>>                                     acpi_soft_cpu_online, NULL);
>> @@ -301,6 +303,7 @@ static void __exit acpi_processor_driver_exit(void)
>>
>>          cpuhp_remove_state_nocalls(hp_online);
>>          cpuhp_remove_state_nocalls(CPUHP_ACPI_CPUDRV_DEAD);
>> +       acpi_processor_unregister_idle_driver();
>>          driver_unregister(&acpi_processor_driver);
>>   }
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index 031738390f2d..c71802d42e8a 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -1360,7 +1360,48 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>>          return 0;
>>   }
>>
>> -static int acpi_processor_registered;
>> +void acpi_processor_register_idle_driver(void)
>> +{
>> +       struct acpi_processor *pr;
>> +       int ret = -ENODEV;
>> +       int cpu;
>> +
>> +       /*
>> +        * Acpi idle driver is used by all possible CPUs.
>> +        * Install the idle handler by the processor power info of one in them.
>> +        * Note that we use previously set idle handler will be used on
>> +        * platforms that only support C1.
>> +        */
>> +       for_each_cpu(cpu, (struct cpumask *)cpu_possible_mask) {
>> +               pr = per_cpu(processors, cpu);
>> +               if (!pr)
>> +                       continue;
>> +
>> +               ret = acpi_processor_get_power_info(pr);
>> +               if (!ret) {
>> +                       pr->flags.power_setup_done = 1;
>> +                       acpi_processor_setup_cpuidle_states(pr);
>> +                       break;
>> +               }
>> +       }
>> +
>> +       if (ret) {
>> +               pr_debug("No ACPI power information from any CPUs.\n");
>> +               return;
>> +       }
>> +
>> +       ret = cpuidle_register_driver(&acpi_idle_driver);
>> +       if (ret) {
>> +               pr_debug("register %s failed.\n", acpi_idle_driver.name);
>> +               return;
>> +       }
>> +       pr_debug("%s registered with cpuidle.\n", acpi_idle_driver.name);
>> +}
>> +
>> +void acpi_processor_unregister_idle_driver(void)
>> +{
>> +       cpuidle_unregister_driver(&acpi_idle_driver);
>> +}
>>
>>   int acpi_processor_power_init(struct acpi_processor *pr)
>>   {
>> @@ -1375,22 +1416,7 @@ int acpi_processor_power_init(struct acpi_processor *pr)
>>          if (!acpi_processor_get_power_info(pr))
>>                  pr->flags.power_setup_done = 1;
>>
>> -       /*
>> -        * Install the idle handler if processor power management is supported.
>> -        * Note that we use previously set idle handler will be used on
>> -        * platforms that only support C1.
>> -        */
>>          if (pr->flags.power) {
>> -               /* Register acpi_idle_driver if not already registered */
>> -               if (!acpi_processor_registered) {
>> -                       acpi_processor_setup_cpuidle_states(pr);
>> -                       retval = cpuidle_register_driver(&acpi_idle_driver);
>> -                       if (retval)
>> -                               return retval;
>> -                       pr_debug("%s registered with cpuidle\n",
>> -                                acpi_idle_driver.name);
>> -               }
>> -
>>                  dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>                  if (!dev)
>>                          return -ENOMEM;
>> @@ -1403,13 +1429,10 @@ int acpi_processor_power_init(struct acpi_processor *pr)
>>                   */
>>                  retval = cpuidle_register_device(dev);
>>                  if (retval) {
>> -                       if (acpi_processor_registered == 0)
>> -                               cpuidle_unregister_driver(&acpi_idle_driver);
>>                          kfree(dev);
>>                          per_cpu(acpi_cpuidle_device, pr->id) = NULL;
>>                          return retval;
>>                  }
>> -               acpi_processor_registered++;
>>          }
>>          return 0;
>>   }
>> @@ -1423,10 +1446,6 @@ int acpi_processor_power_exit(struct acpi_processor *pr)
>>
>>          if (pr->flags.power) {
>>                  cpuidle_unregister_device(dev);
>> -               acpi_processor_registered--;
>> -               if (acpi_processor_registered == 0)
>> -                       cpuidle_unregister_driver(&acpi_idle_driver);
>> -
>>                  kfree(dev);
>>          }
>>
>> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
>> index d0eccbd920e5..1249f5e81d92 100644
>> --- a/include/acpi/processor.h
>> +++ b/include/acpi/processor.h
>> @@ -423,6 +423,8 @@ int acpi_processor_power_init(struct acpi_processor *pr);
>>   int acpi_processor_power_exit(struct acpi_processor *pr);
>>   int acpi_processor_power_state_has_changed(struct acpi_processor *pr);
>>   int acpi_processor_hotplug(struct acpi_processor *pr);
>> +void acpi_processor_register_idle_driver(void);
>> +void acpi_processor_unregister_idle_driver(void);
>>   #else
>>   static inline int acpi_processor_power_init(struct acpi_processor *pr)
>>   {
>> @@ -443,6 +445,12 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
>>   {
>>          return -ENODEV;
>>   }
>> +static void acpi_processor_register_idle_driver(void)
>> +{
>> +}
>> +static void acpi_processor_unregister_idle_driver(void)
>> +{
>> +}
>>   #endif /* CONFIG_ACPI_PROCESSOR_IDLE */
>>
>>   /* in processor_thermal.c */
>> --
> Applied as 6.18 material, thanks!
>
> While at it, in the future, please always spell ACPI in capitals in
> patch subjects, changelogs and code comments.
Get it. Thanks.

>