Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init

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

在 2025/7/3 19:09, Rafael J. Wysocki 写道:
> On Thu, Jul 3, 2025 at 8:23 AM lihuisong (C) <lihuisong@huawei.com> wrote:
>> Hi,
>>
>> Thanks for your review.
>>
>>
>> 在 2025/7/3 1:42, Rafael J. Wysocki 写道:
>>> On Thu, Jun 19, 2025 at 8:13 AM Huisong Li <lihuisong@huawei.com> wrote:
>>>> There are two resource rollback issues in acpi_processor_power_init:
>>>> 1> Do not unregister acpi_idle_driver when do kzalloc failed.
>>>> 2> Do not free cpuidle device memory when register cpuidle device failed.
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>>> ---
>>>>    drivers/acpi/processor_idle.c | 24 +++++++++++++++++-------
>>>>    1 file changed, 17 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>>>> index 2c2dc559e0f8..3548ab9dac9e 100644
>>>> --- a/drivers/acpi/processor_idle.c
>>>> +++ b/drivers/acpi/processor_idle.c
>>>> @@ -1392,8 +1392,10 @@ int acpi_processor_power_init(struct acpi_processor *pr)
>>>>                   }
>>>>
>>>>                   dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>>>> -               if (!dev)
>>>> -                       return -ENOMEM;
>>>> +               if (!dev) {
>>>> +                       retval = -ENOMEM;
>>>> +                       goto unregister_driver;
>>> No, unregistering the driver here is pointless.
>> I don't quite understand why here is pointless. Can you explain it?
> When this function is run for another CPU, it will attempt to register
> the driver again if it is unregistered here.
Yeah, got it.
So registering cpuidle also has a potential race issue here.
>
> Quite frankly, the driver should be registered before running this
> function because it is a CPU hotplug callback and registering a
> cpuidle driver from within it is quite questionable.
>
> Alternatively, it can be registered when all of the CPUs have been brought up.
Agree with you.
The reason why is that the initialization of acpi_idle_driver depands on 
the power management information of CPU.
But the power management information of CPU is obtained in this callback.
I have an idea.
Because acpi_idle_driver is applied to all possiable CPUs. And use the 
power information of the first onlined CPU to initialize and register 
acpi_idle_driver, currently.
So I think we can use this logic and dependency to extract a function to 
initialize and register acpi_idle_driver. And put this function to 
acpi_processor_driver_init().
I tested it is ok.
What do you think about this?

/Huisong
>
>>>> +               }
>>>>                   per_cpu(acpi_cpuidle_device, pr->id) = dev;
>>>>
>>>>                   acpi_processor_setup_cpuidle_dev(pr, dev);
>>>> @@ -1402,14 +1404,22 @@ int acpi_processor_power_init(struct acpi_processor *pr)
>>>>                    * must already be registered before registering device
>>>>                    */
>>>>                   retval = cpuidle_register_device(dev);
>>>> -               if (retval) {
>>>> -                       if (acpi_processor_registered == 0)
>>>> -                               cpuidle_unregister_driver(&acpi_idle_driver);
>>> Pretty much the same as here.
>>>
>>> It would be good to clean up dev here though.
>> It is ok if above is pointless.
>>>> -                       return retval;
>>>> -               }
>>>> +               if (retval)
>>>> +                       goto free_cpuidle_device;
>>>> +
>>>>                   acpi_processor_registered++;
>>>>           }
>>>>           return 0;
>>>> +
>>>> +free_cpuidle_device:
>>>> +       per_cpu(acpi_cpuidle_device, pr->id) = NULL;
>>>> +       kfree(dev);
>>>> +
>>>> +unregister_driver:
>>>> +       if (acpi_processor_registered == 0)
>>>> +               cpuidle_unregister_driver(&acpi_idle_driver);
>>>> +
>>>> +       return retval;
>>>>    }
>>>>
>>>>    int acpi_processor_power_exit(struct acpi_processor *pr)
>>>> --
>>>> 2.33.0
>>>>