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

[lihuisong (C)]

Hi Rafael,

Can you take a look at my reply?
If it is ok to you, I will send it later.

/Huisong
在 2025/7/14 9:33, lihuisong (C) 写道:
> 在 2025/7/3 19:09, Rafael J. Wysocki 写道:
> > On Thu, Jul 3, 2025 at 8:23 AM lihuisong (C) <lihuisong@xxxxxxxxxx> 
> > 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@xxxxxxxxxx> 
> > > > 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@xxxxxxxxxx>
> > > > > ---
> > > > >    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