* [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init @ 2025-06-19 6:13 Huisong Li 2025-06-28 0:54 ` lihuisong (C) 2025-07-02 17:42 ` Rafael J. Wysocki 0 siblings, 2 replies; 9+ messages in thread From: Huisong Li @ 2025-06-19 6:13 UTC (permalink / raw) To: rafael, lenb Cc: linux-acpi, linux-kernel, linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong, lihuisong 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; + } 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); - 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init 2025-06-19 6:13 [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init Huisong Li @ 2025-06-28 0:54 ` lihuisong (C) 2025-07-02 17:42 ` Rafael J. Wysocki 1 sibling, 0 replies; 9+ messages in thread From: lihuisong (C) @ 2025-06-28 0:54 UTC (permalink / raw) To: rafael, lenb Cc: linux-acpi, linux-kernel, linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong Kindly ping for review. 在 2025/6/19 14:13, Huisong Li 写道: > 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; > + } > 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); > - 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init 2025-06-19 6:13 [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init Huisong Li 2025-06-28 0:54 ` lihuisong (C) @ 2025-07-02 17:42 ` Rafael J. Wysocki 2025-07-03 6:23 ` lihuisong (C) 1 sibling, 1 reply; 9+ messages in thread From: Rafael J. Wysocki @ 2025-07-02 17:42 UTC (permalink / raw) To: Huisong Li Cc: rafael, lenb, linux-acpi, linux-kernel, linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong 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. > + } > 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. > - 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init 2025-07-02 17:42 ` Rafael J. Wysocki @ 2025-07-03 6:23 ` lihuisong (C) 2025-07-03 11:09 ` Rafael J. Wysocki 0 siblings, 1 reply; 9+ messages in thread From: lihuisong (C) @ 2025-07-03 6:23 UTC (permalink / raw) To: Rafael J. Wysocki Cc: lenb, linux-acpi, linux-kernel, linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong 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? > >> + } >> 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 >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init 2025-07-03 6:23 ` lihuisong (C) @ 2025-07-03 11:09 ` Rafael J. Wysocki 2025-07-14 1:33 ` lihuisong (C) 0 siblings, 1 reply; 9+ messages in thread From: Rafael J. Wysocki @ 2025-07-03 11:09 UTC (permalink / raw) To: lihuisong (C) Cc: Rafael J. Wysocki, lenb, linux-acpi, linux-kernel, linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong 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. 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. > > > >> + } > >> 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 > >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init 2025-07-03 11:09 ` Rafael J. Wysocki @ 2025-07-14 1:33 ` lihuisong (C) 2025-07-22 11:05 ` lihuisong (C) 2025-07-22 11:10 ` Rafael J. Wysocki 0 siblings, 2 replies; 9+ messages in thread From: lihuisong (C) @ 2025-07-14 1:33 UTC (permalink / raw) To: Rafael J. Wysocki Cc: lenb, linux-acpi, linux-kernel, linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong, lihuisong 在 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 >>>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init 2025-07-14 1:33 ` lihuisong (C) @ 2025-07-22 11:05 ` lihuisong (C) 2025-07-22 11:10 ` Rafael J. Wysocki 1 sibling, 0 replies; 9+ messages in thread From: lihuisong (C) @ 2025-07-22 11:05 UTC (permalink / raw) To: Rafael J. Wysocki Cc: lenb, linux-acpi, linux-kernel, linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong, lihuisong 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@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 >>>>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init 2025-07-14 1:33 ` lihuisong (C) 2025-07-22 11:05 ` lihuisong (C) @ 2025-07-22 11:10 ` Rafael J. Wysocki 2025-07-22 11:28 ` lihuisong (C) 1 sibling, 1 reply; 9+ messages in thread From: Rafael J. Wysocki @ 2025-07-22 11:10 UTC (permalink / raw) To: lihuisong (C) Cc: Rafael J. Wysocki, lenb, linux-acpi, linux-kernel, linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong On Mon, Jul 14, 2025 at 3:34 AM lihuisong (C) <lihuisong@huawei.com> wrote: > > > 在 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? This is one of the options I mentioned above, isn't it? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init 2025-07-22 11:10 ` Rafael J. Wysocki @ 2025-07-22 11:28 ` lihuisong (C) 0 siblings, 0 replies; 9+ messages in thread From: lihuisong (C) @ 2025-07-22 11:28 UTC (permalink / raw) To: Rafael J. Wysocki Cc: lenb, linux-acpi, linux-kernel, linuxarm, jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong, lihuisong 在 2025/7/22 19:10, Rafael J. Wysocki 写道: > On Mon, Jul 14, 2025 at 3:34 AM lihuisong (C) <lihuisong@huawei.com> wrote: >> >> 在 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? > This is one of the options I mentioned above, isn't it? Yes, it is what you mentioned. I think this option is relatively easier to handle. I am not sure if I understand correctly. So I wanted to confirmed with you. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2025-07-22 11:28 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-06-19 6:13 [PATCH] ACPI: processor: idle: Fix resource rollback in acpi_processor_power_init Huisong Li 2025-06-28 0:54 ` lihuisong (C) 2025-07-02 17:42 ` Rafael J. Wysocki 2025-07-03 6:23 ` lihuisong (C) 2025-07-03 11:09 ` Rafael J. Wysocki 2025-07-14 1:33 ` lihuisong (C) 2025-07-22 11:05 ` lihuisong (C) 2025-07-22 11:10 ` Rafael J. Wysocki 2025-07-22 11:28 ` lihuisong (C)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®