* [PATCH v2] ACPI: processor: idle: Fix resource leak and potential concurrent in acpi_processor_power_init()
@ 2025-07-23 12:10 Huisong Li
2025-07-23 13:35 ` Rafael J. Wysocki
2025-07-24 1:38 ` kernel test robot
0 siblings, 2 replies; 5+ messages in thread
From: Huisong Li @ 2025-07-23 12:10 UTC (permalink / raw)
To: rafael, lenb
Cc: linux-acpi, linux-kernel, linuxarm, jonathan.cameron, zhanjie9,
zhenglifeng1, yubowen8, liuyonglong, lihuisong
There are three kind of issues:
1> There are two resource leak issues in acpi_processor_power_init:
a> Don't unregister acpi_idle_driver when do kzalloc failed.
b> Don't free cpuidle device memory when register cpuidle device failed.
2> There isn't lock to prevent the global acpi_processor_registered, which
may lead to concurrent register cpuidle driver.
3> The cpuidle driver should be registered in advance when all of the CPUs
have been brought up instead of being in a CPU hotplug callback.
To solve these issues, 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>
---
v2: register cpuidle driver in advance when all of the CPUs have been
brought up.
v1 link: https://patchwork.kernel.org/project/linux-acpi/patch/20250619061327.1674384-1-lihuisong@huawei.com/
---
drivers/acpi/processor_driver.c | 5 +++
drivers/acpi/processor_idle.c | 71 ++++++++++++++++++++++-----------
include/acpi/processor.h | 9 +++++
3 files changed, 62 insertions(+), 23 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 65e779be64ff..ff944c93b6ff 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -263,6 +263,10 @@ static int __init acpi_processor_driver_init(void)
if (result < 0)
return result;
+ result = acpi_processor_register_idle_driver();
+ if (result)
+ pr_info("register idle driver failed, ret = %d.\n", result);
+
result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
"acpi/cpu-drv:online",
acpi_soft_cpu_online, NULL);
@@ -301,6 +305,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 2c2dc559e0f8..2408f1076631 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -1360,7 +1360,52 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
return 0;
}
-static int acpi_processor_registered;
+int acpi_processor_register_idle_driver(void)
+{
+ struct acpi_processor *pr;
+ int cpu;
+ int ret;
+
+ /*
+ * 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 == NULL)
+ continue;
+
+ ret = acpi_processor_get_power_info(pr);
+ if (!ret) {
+ pr->flags.power_setup_done = 1;
+ break;
+ }
+ }
+
+ if (unlikely(!pr))
+ return -ENODEV;
+
+ if (ret) {
+ pr_err("%s get power information failed.\n",
+ acpi_idle_driver.name);
+ return ret;
+ }
+
+ acpi_processor_setup_cpuidle_states(pr);
+ ret = cpuidle_register_driver(&acpi_idle_driver);
+ if (ret)
+ return ret;
+
+ pr_debug("%s registered with cpuidle\n", acpi_idle_driver.name);
+ return 0;
+}
+
+void acpi_processor_unregister_idle_driver(void)
+{
+ cpuidle_unregister_driver(&acpi_idle_driver);
+}
int acpi_processor_power_init(struct acpi_processor *pr)
{
@@ -1375,22 +1420,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,11 +1433,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);
+ per_cpu(acpi_cpuidle_device, pr->id) = NULL;
+ kfree(dev);
return retval;
}
- acpi_processor_registered++;
}
return 0;
}
@@ -1421,10 +1450,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..3cb41a3f2d9a 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);
+int 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,13 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
{
return -ENODEV;
}
+static int acpi_processor_register_idle_driver(void)
+{
+ return -ENODEV;
+}
+static void acpi_processor_unregister_idle_driver(void)
+{
+}
#endif /* CONFIG_ACPI_PROCESSOR_IDLE */
/* in processor_thermal.c */
--
2.33.0
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2] ACPI: processor: idle: Fix resource leak and potential concurrent in acpi_processor_power_init()
2025-07-23 12:10 [PATCH v2] ACPI: processor: idle: Fix resource leak and potential concurrent in acpi_processor_power_init() Huisong Li
@ 2025-07-23 13:35 ` Rafael J. Wysocki
2025-07-24 13:15 ` lihuisong (C)
2025-07-24 1:38 ` kernel test robot
1 sibling, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2025-07-23 13:35 UTC (permalink / raw)
To: Huisong Li
Cc: rafael, lenb, linux-acpi, linux-kernel, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong
On Wed, Jul 23, 2025 at 2:10 PM Huisong Li <lihuisong@huawei.com> wrote:
>
> There are three kind of issues:
> 1> There are two resource leak issues in acpi_processor_power_init:
> a> Don't unregister acpi_idle_driver when do kzalloc failed.
This is not a resource leak. As I said before, the driver need not be
unregistered on a memory allocation failure.
> b> Don't free cpuidle device memory when register cpuidle device failed.
This is a separate minor issue that needs to be addressed by a separate patch.
> 2> There isn't lock to prevent the global acpi_processor_registered, which
> may lead to concurrent register cpuidle driver.
That's not obvious because in principle the code in question is only
run during initialization which is serialized.
In theory, it could run in parallel CPU online, but that at least is
not default behavior AFAICS.
In any case, if you claim something like this, it is advisable to
mention a specific scenario in which the race in question can happen.
> 3> The cpuidle driver should be registered in advance when all of the CPUs
> have been brought up instead of being in a CPU hotplug callback.
The "in advance" piece above is rather confusing and it can be dropped
without changing the meaning of the rest of the sentence.
> To solve these issues, 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().
I think that the main problem here is that the cpuidle driver is
registered from within a CPU hotplug callback, which is questionable
and confusing. Usually, however, this doesn't lead to any functional
issues AFAICS.
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> v2: register cpuidle driver in advance when all of the CPUs have been
> brought up.
> v1 link: https://patchwork.kernel.org/project/linux-acpi/patch/20250619061327.1674384-1-lihuisong@huawei.com/
> ---
> drivers/acpi/processor_driver.c | 5 +++
> drivers/acpi/processor_idle.c | 71 ++++++++++++++++++++++-----------
> include/acpi/processor.h | 9 +++++
> 3 files changed, 62 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 65e779be64ff..ff944c93b6ff 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -263,6 +263,10 @@ static int __init acpi_processor_driver_init(void)
> if (result < 0)
> return result;
>
> + result = acpi_processor_register_idle_driver();
> + if (result)
> + pr_info("register idle driver failed, ret = %d.\n", result);
This registers the cpuidle driver before registering cpuidle devices
for all CPUs.
It would be better to make acpi_processor_register_idle_driver() print
the diagnostic message on failures and then it won't need to return a
value.
Note that it may fail if intel_idle is already registered, for
example, so the message should rather be a debug-level one.
> +
> result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> "acpi/cpu-drv:online",
> acpi_soft_cpu_online, NULL);
> @@ -301,6 +305,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 2c2dc559e0f8..2408f1076631 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -1360,7 +1360,52 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
> return 0;
> }
>
> -static int acpi_processor_registered;
> +int acpi_processor_register_idle_driver(void)
> +{
> + struct acpi_processor *pr;
> + int cpu;
> + int ret;
The ret variable needs to be initialized here or tools will complain,
and so it may be initialized to -ENODEV:
int ret = -ENODEV;
> +
> + /*
> + * 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 == NULL)
"if (!pr)" please.
> + continue;
> +
> + ret = acpi_processor_get_power_info(pr);
if (ret)
continue;
> + if (!ret) {
> + pr->flags.power_setup_done = 1;
I think this is set here to prevent the subsequent
acpi_processor_setup_cpuidle_states() call from bailing out, but is
this not too early to set it?
> + break;
> + }
> + }
> +
> + if (unlikely(!pr))
> + return -ENODEV;
This is unnecessary if ret is initialized to -ENODEV;
> +
> + if (ret) {
> + pr_err("%s get power information failed.\n",
> + acpi_idle_driver.name);
This message is confusing at best. It should be something like "No
ACPI power information for any CPUs" and the driver name in it has no
purpose.
> + return ret;
> + }
> +
> + acpi_processor_setup_cpuidle_states(pr);
I'd call this in the loop right before breaking out of it, so the
scope of pr is clear.
> + ret = cpuidle_register_driver(&acpi_idle_driver);
> + if (ret)
Print a diagnostic message here and do not return a value (ie. make
the function void).
> + return ret;
> +
> + pr_debug("%s registered with cpuidle\n", acpi_idle_driver.name);
> + return 0;
> +}
> +
> +void acpi_processor_unregister_idle_driver(void)
> +{
> + cpuidle_unregister_driver(&acpi_idle_driver);
> +}
>
> int acpi_processor_power_init(struct acpi_processor *pr)
> {
> @@ -1375,22 +1420,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,11 +1433,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);
> + per_cpu(acpi_cpuidle_device, pr->id) = NULL;
> + kfree(dev);
These two lines should be added in a separate patch.
> return retval;
> }
> - acpi_processor_registered++;
> }
> return 0;
> }
> @@ -1421,10 +1450,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..3cb41a3f2d9a 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);
> +int 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,13 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
> {
> return -ENODEV;
> }
> +static int acpi_processor_register_idle_driver(void)
> +{
> + return -ENODEV;
> +}
> +static void acpi_processor_unregister_idle_driver(void)
> +{
> +}
> #endif /* CONFIG_ACPI_PROCESSOR_IDLE */
>
> /* in processor_thermal.c */
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] ACPI: processor: idle: Fix resource leak and potential concurrent in acpi_processor_power_init()
2025-07-23 13:35 ` Rafael J. Wysocki
@ 2025-07-24 13:15 ` lihuisong (C)
0 siblings, 0 replies; 5+ messages in thread
From: lihuisong (C) @ 2025-07-24 13:15 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: lenb, linux-acpi, linux-kernel, linuxarm, jonathan.cameron,
zhanjie9, zhenglifeng1, yubowen8, liuyonglong, lihuisong
在 2025/7/23 21:35, Rafael J. Wysocki 写道:
> On Wed, Jul 23, 2025 at 2:10 PM Huisong Li <lihuisong@huawei.com> wrote:
>> There are three kind of issues:
>> 1> There are two resource leak issues in acpi_processor_power_init:
>> a> Don't unregister acpi_idle_driver when do kzalloc failed.
> This is not a resource leak. As I said before, the driver need not be
> unregistered on a memory allocation failure.
Ok, will remove it.
>
>> b> Don't free cpuidle device memory when register cpuidle device failed.
> This is a separate minor issue that needs to be addressed by a separate patch.
Got it. will add a separate patch in next version.
>
>> 2> There isn't lock to prevent the global acpi_processor_registered, which
>> may lead to concurrent register cpuidle driver.
> That's not obvious because in principle the code in question is only
> run during initialization which is serialized.
>
> In theory, it could run in parallel CPU online, but that at least is
> not default behavior AFAICS.
Ack.
>
> In any case, if you claim something like this, it is advisable to
> mention a specific scenario in which the race in question can happen.
Yeah.
Anyway, it is ok if the initialization is serialized as you mentioned.
>
>> 3> The cpuidle driver should be registered in advance when all of the CPUs
>> have been brought up instead of being in a CPU hotplug callback.
> The "in advance" piece above is rather confusing and it can be dropped
> without changing the meaning of the rest of the sentence.
Ack
>
>> To solve these issues, 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().
> I think that the main problem here is that the cpuidle driver is
> registered from within a CPU hotplug callback, which is questionable
> and confusing. Usually, however, this doesn't lead to any functional
> issues AFAICS.
Ok, this patch will use this tone in the commit log.
>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> v2: register cpuidle driver in advance when all of the CPUs have been
>> brought up.
>> v1 link: https://patchwork.kernel.org/project/linux-acpi/patch/20250619061327.1674384-1-lihuisong@huawei.com/
>> ---
>> drivers/acpi/processor_driver.c | 5 +++
>> drivers/acpi/processor_idle.c | 71 ++++++++++++++++++++++-----------
>> include/acpi/processor.h | 9 +++++
>> 3 files changed, 62 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 65e779be64ff..ff944c93b6ff 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -263,6 +263,10 @@ static int __init acpi_processor_driver_init(void)
>> if (result < 0)
>> return result;
>>
>> + result = acpi_processor_register_idle_driver();
>> + if (result)
>> + pr_info("register idle driver failed, ret = %d.\n", result);
> This registers the cpuidle driver before registering cpuidle devices
> for all CPUs.
>
> It would be better to make acpi_processor_register_idle_driver() print
> the diagnostic message on failures and then it won't need to return a
> value.
Ack
>
> Note that it may fail if intel_idle is already registered, for
> example, so the message should rather be a debug-level one.
All right.
>> +
>> result = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>> "acpi/cpu-drv:online",
>> acpi_soft_cpu_online, NULL);
>> @@ -301,6 +305,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 2c2dc559e0f8..2408f1076631 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -1360,7 +1360,52 @@ int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>> return 0;
>> }
>>
>> -static int acpi_processor_registered;
>> +int acpi_processor_register_idle_driver(void)
>> +{
>> + struct acpi_processor *pr;
>> + int cpu;
>> + int ret;
> The ret variable needs to be initialized here or tools will complain,
> and so it may be initialized to -ENODEV:
>
> int ret = -ENODEV;
Ack
>
>> +
>> + /*
>> + * 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 == NULL)
> "if (!pr)" please.
Ack
>
>> + continue;
>> +
>> + ret = acpi_processor_get_power_info(pr);
> if (ret)
> continue;
Please see the following reply.
>
>> + if (!ret) {
>> + pr->flags.power_setup_done = 1;
> I think this is set here to prevent the subsequent
> acpi_processor_setup_cpuidle_states() call from bailing out, but is
> this not too early to set it?
Setting to 1 is necessary here,
otherwise acpi_processor_setup_cpuidle_states would not initialize
acpi_idle_driver because this flag is false.
>
>> + break;
>> + }
>> + }
>> +
>> + if (unlikely(!pr))
>> + return -ENODEV;
> This is unnecessary if ret is initialized to -ENODEV;
ok
>
>> +
>> + if (ret) {
>> + pr_err("%s get power information failed.\n",
>> + acpi_idle_driver.name);
> This message is confusing at best. It should be something like "No
> ACPI power information for any CPUs" and the driver name in it has no
> purpose.
Ack
>
>> + return ret;
>> + }
>> +
>> + acpi_processor_setup_cpuidle_states(pr);
> I'd call this in the loop right before breaking out of it, so the
> scope of pr is clear.
will fix it as you said.
>
>> + ret = cpuidle_register_driver(&acpi_idle_driver);
>> + if (ret)
> Print a diagnostic message here and do not return a value (ie. make
> the function void).
All right.
>
>> + return ret;
>> +
>> + pr_debug("%s registered with cpuidle\n", acpi_idle_driver.name);
>> + return 0;
>> +}
>> +
>> +void acpi_processor_unregister_idle_driver(void)
>> +{
>> + cpuidle_unregister_driver(&acpi_idle_driver);
>> +}
>>
>> int acpi_processor_power_init(struct acpi_processor *pr)
>> {
>> @@ -1375,22 +1420,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,11 +1433,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);
>> + per_cpu(acpi_cpuidle_device, pr->id) = NULL;
>> + kfree(dev);
> These two lines should be added in a separate patch.
Ack.
>
>
>> return retval;
>> }
>> - acpi_processor_registered++;
>> }
>> return 0;
>> }
>> @@ -1421,10 +1450,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..3cb41a3f2d9a 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);
>> +int 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,13 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
>> {
>> return -ENODEV;
>> }
>> +static int acpi_processor_register_idle_driver(void)
>> +{
>> + return -ENODEV;
>> +}
>> +static void acpi_processor_unregister_idle_driver(void)
>> +{
>> +}
>> #endif /* CONFIG_ACPI_PROCESSOR_IDLE */
>>
>> /* in processor_thermal.c */
>> --
>> 2.33.0
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] ACPI: processor: idle: Fix resource leak and potential concurrent in acpi_processor_power_init()
2025-07-23 12:10 [PATCH v2] ACPI: processor: idle: Fix resource leak and potential concurrent in acpi_processor_power_init() Huisong Li
2025-07-23 13:35 ` Rafael J. Wysocki
@ 2025-07-24 1:38 ` kernel test robot
1 sibling, 0 replies; 5+ messages in thread
From: kernel test robot @ 2025-07-24 1:38 UTC (permalink / raw)
To: Huisong Li, rafael, lenb
Cc: oe-kbuild-all, linux-acpi, linux-kernel, linuxarm,
jonathan.cameron, zhanjie9, zhenglifeng1, yubowen8, liuyonglong,
lihuisong
Hi Huisong,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge linus/master v6.16-rc7 next-20250723]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Huisong-Li/ACPI-processor-idle-Fix-resource-leak-and-potential-concurrent-in-acpi_processor_power_init/20250723-201246
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20250723121034.3685996-1-lihuisong%40huawei.com
patch subject: [PATCH v2] ACPI: processor: idle: Fix resource leak and potential concurrent in acpi_processor_power_init()
config: i386-buildonly-randconfig-001-20250724 (https://download.01.org/0day-ci/archive/20250724/202507240807.1ild86sv-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250724/202507240807.1ild86sv-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507240807.1ild86sv-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/acpi/processor_core.c:15:
>> include/acpi/processor.h:452:13: warning: 'acpi_processor_unregister_idle_driver' defined but not used [-Wunused-function]
452 | static void acpi_processor_unregister_idle_driver(void)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> include/acpi/processor.h:448:12: warning: 'acpi_processor_register_idle_driver' defined but not used [-Wunused-function]
448 | static int acpi_processor_register_idle_driver(void)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/acpi_processor_unregister_idle_driver +452 include/acpi/processor.h
443
444 static inline int acpi_processor_hotplug(struct acpi_processor *pr)
445 {
446 return -ENODEV;
447 }
> 448 static int acpi_processor_register_idle_driver(void)
449 {
450 return -ENODEV;
451 }
> 452 static void acpi_processor_unregister_idle_driver(void)
453 {
454 }
455 #endif /* CONFIG_ACPI_PROCESSOR_IDLE */
456
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] ACPI: processor: idle: Fix resource leak and potential concurrent in acpi_processor_power_init()
@ 2025-07-27 0:26 kernel test robot
0 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2025-07-27 0:26 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Dan Carpenter
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250723121034.3685996-1-lihuisong@huawei.com>
References: <20250723121034.3685996-1-lihuisong@huawei.com>
TO: Huisong Li <lihuisong@huawei.com>
TO: rafael@kernel.org
TO: lenb@kernel.org
CC: linux-acpi@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linuxarm@huawei.com
CC: jonathan.cameron@huawei.com
CC: zhanjie9@hisilicon.com
CC: zhenglifeng1@huawei.com
CC: yubowen8@huawei.com
CC: liuyonglong@huawei.com
CC: lihuisong@huawei.com
Hi Huisong,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge linus/master v6.16-rc7 next-20250725]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Huisong-Li/ACPI-processor-idle-Fix-resource-leak-and-potential-concurrent-in-acpi_processor_power_init/20250723-201246
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20250723121034.3685996-1-lihuisong%40huawei.com
patch subject: [PATCH v2] ACPI: processor: idle: Fix resource leak and potential concurrent in acpi_processor_power_init()
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: x86_64-randconfig-161-20250725 (https://download.01.org/0day-ci/archive/20250727/202507271001.EPy5YhFz-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202507271001.EPy5YhFz-lkp@intel.com/
smatch warnings:
drivers/acpi/processor_idle.c:1387 acpi_processor_register_idle_driver() error: uninitialized symbol 'pr'.
drivers/acpi/processor_idle.c:1390 acpi_processor_register_idle_driver() error: uninitialized symbol 'ret'.
vim +/pr +1387 drivers/acpi/processor_idle.c
46bcfad7a819bd Deepthi Dharwar 2011-10-28 1362
75621e77853b5b Huisong Li 2025-07-23 1363 int acpi_processor_register_idle_driver(void)
75621e77853b5b Huisong Li 2025-07-23 1364 {
75621e77853b5b Huisong Li 2025-07-23 1365 struct acpi_processor *pr;
75621e77853b5b Huisong Li 2025-07-23 1366 int cpu;
75621e77853b5b Huisong Li 2025-07-23 1367 int ret;
75621e77853b5b Huisong Li 2025-07-23 1368
75621e77853b5b Huisong Li 2025-07-23 1369 /*
75621e77853b5b Huisong Li 2025-07-23 1370 * Acpi idle driver is used by all possible CPUs.
75621e77853b5b Huisong Li 2025-07-23 1371 * Install the idle handler by the processor power info of one in them.
75621e77853b5b Huisong Li 2025-07-23 1372 * Note that we use previously set idle handler will be used on
75621e77853b5b Huisong Li 2025-07-23 1373 * platforms that only support C1.
75621e77853b5b Huisong Li 2025-07-23 1374 */
75621e77853b5b Huisong Li 2025-07-23 1375 for_each_cpu(cpu, (struct cpumask *)cpu_possible_mask) {
75621e77853b5b Huisong Li 2025-07-23 1376 pr = per_cpu(processors, cpu);
75621e77853b5b Huisong Li 2025-07-23 1377 if (pr == NULL)
75621e77853b5b Huisong Li 2025-07-23 1378 continue;
75621e77853b5b Huisong Li 2025-07-23 1379
75621e77853b5b Huisong Li 2025-07-23 1380 ret = acpi_processor_get_power_info(pr);
75621e77853b5b Huisong Li 2025-07-23 1381 if (!ret) {
75621e77853b5b Huisong Li 2025-07-23 1382 pr->flags.power_setup_done = 1;
75621e77853b5b Huisong Li 2025-07-23 1383 break;
75621e77853b5b Huisong Li 2025-07-23 1384 }
75621e77853b5b Huisong Li 2025-07-23 1385 }
75621e77853b5b Huisong Li 2025-07-23 1386
75621e77853b5b Huisong Li 2025-07-23 @1387 if (unlikely(!pr))
75621e77853b5b Huisong Li 2025-07-23 1388 return -ENODEV;
75621e77853b5b Huisong Li 2025-07-23 1389
75621e77853b5b Huisong Li 2025-07-23 @1390 if (ret) {
75621e77853b5b Huisong Li 2025-07-23 1391 pr_err("%s get power information failed.\n",
75621e77853b5b Huisong Li 2025-07-23 1392 acpi_idle_driver.name);
75621e77853b5b Huisong Li 2025-07-23 1393 return ret;
75621e77853b5b Huisong Li 2025-07-23 1394 }
75621e77853b5b Huisong Li 2025-07-23 1395
75621e77853b5b Huisong Li 2025-07-23 1396 acpi_processor_setup_cpuidle_states(pr);
75621e77853b5b Huisong Li 2025-07-23 1397 ret = cpuidle_register_driver(&acpi_idle_driver);
75621e77853b5b Huisong Li 2025-07-23 1398 if (ret)
75621e77853b5b Huisong Li 2025-07-23 1399 return ret;
75621e77853b5b Huisong Li 2025-07-23 1400
75621e77853b5b Huisong Li 2025-07-23 1401 pr_debug("%s registered with cpuidle\n", acpi_idle_driver.name);
75621e77853b5b Huisong Li 2025-07-23 1402 return 0;
75621e77853b5b Huisong Li 2025-07-23 1403 }
75621e77853b5b Huisong Li 2025-07-23 1404
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-07-27 0:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-23 12:10 [PATCH v2] ACPI: processor: idle: Fix resource leak and potential concurrent in acpi_processor_power_init() Huisong Li
2025-07-23 13:35 ` Rafael J. Wysocki
2025-07-24 13:15 ` lihuisong (C)
2025-07-24 1:38 ` kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2025-07-27 0:26 kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.