* Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-10 17:05 [PATCH] drivers/acpi/processor_driver.c: add missing kfree Julia Lawall
@ 2012-03-14 11:46 ` Srivatsa S. Bhat
2012-03-14 12:32 ` Julia Lawall
2012-03-15 8:32 ` Julia Lawall
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Srivatsa S. Bhat @ 2012-03-14 11:46 UTC (permalink / raw)
To: Julia Lawall
Cc: Len Brown, kernel-janitors, linux-acpi, linux-kernel,
Julia Lawall, Deepthi Dharwar, Venkatesh Pallipadi, trenn,
bhelgaas
On 03/10/2012 10:35 PM, Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
>
> The function acpi_processor_add is stored in the ops.add field of a
> acpi_driver structure. This function is then called in
> acpi_bus_driver_init. On failure, this function clears the field
> device->driver_data, but does not free its contents. Thus the free has to
> be done by the add function. In acpi_processor_add, the corresponding
> value is pr. This value is currently freed on failure before storing it in
> device->driver_data, but not after. This free is added in the error
> handling code at the end of the function. The static global variable
"static global variable"?? never heard that one before ;-)
Maybe you meant "per_cpu variable processors"..
> processors is also cleared so that it does not refer to a dangling pointer.
> processor_device_array is cleared as well.
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
>
> ---
> This is only compile tested. In particular, I don't know if it is correct
> to add per_cpu(processor_device_array, pr->id) = NULL;.
No, you shouldn't set it to NULL. processor_device_array was added to check
for buggy BIOSes and return gracefully. Check commit cd8e2b48d (and also the
bugzilla link in that commit).
To have a robust check for buggy BIOSes, we must let it be as it is, even if
it is a stale pointer. That way we can still catch subsequent calls to this
function with the same acpi id (because of a buggy BIOS) and take appropriate
actions.
Other than that, the patch looks good to me.
Regards,
Srivatsa S. Bhat
IBM Linux Technology Center
> It has nothing to
> do with pr, but it looks like stale information in the case of a failure.
>
> drivers/acpi/processor_driver.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 2801b41..9bb0017 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> return -ENOMEM;
>
> if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
> - kfree(pr);
> - return -ENOMEM;
> + result = -ENOMEM;
> + goto err_free_pr;
> }
>
> pr->handle = device->handle;
> @@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> dev = get_cpu_device(pr->id);
> if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
> result = -EFAULT;
> - goto err_free_cpumask;
> + goto err_clear_processors;
> }
>
> /*
> @@ -595,9 +595,13 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>
> err_remove_sysfs:
> sysfs_remove_link(&device->dev.kobj, "sysdev");
> +err_clear_processors:
> + per_cpu(processors, pr->id) = NULL;
> + per_cpu(processor_device_array, pr->id) = NULL;
> err_free_cpumask:
> free_cpumask_var(pr->throttling.shared_cpu_map);
> -
> +err_free_pr:
> + kfree(pr);
> return result;
> }
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-14 11:46 ` Srivatsa S. Bhat
@ 2012-03-14 12:32 ` Julia Lawall
2012-03-14 13:02 ` Srivatsa S. Bhat
0 siblings, 1 reply; 9+ messages in thread
From: Julia Lawall @ 2012-03-14 12:32 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Julia Lawall, Len Brown, kernel-janitors, linux-acpi,
linux-kernel, Julia Lawall, Deepthi Dharwar, Venkatesh Pallipadi,
trenn, bhelgaas
On Wed, 14 Mar 2012, Srivatsa S. Bhat wrote:
> On 03/10/2012 10:35 PM, Julia Lawall wrote:
>
>> From: Julia Lawall <julia@diku.dk>
>>
>> The function acpi_processor_add is stored in the ops.add field of a
>> acpi_driver structure. This function is then called in
>> acpi_bus_driver_init. On failure, this function clears the field
>> device->driver_data, but does not free its contents. Thus the free has to
>> be done by the add function. In acpi_processor_add, the corresponding
>> value is pr. This value is currently freed on failure before storing it in
>> device->driver_data, but not after. This free is added in the error
>> handling code at the end of the function. The static global variable
>
>
> "static global variable"?? never heard that one before ;-)
> Maybe you meant "per_cpu variable processors"..
>
>> processors is also cleared so that it does not refer to a dangling pointer.
>> processor_device_array is cleared as well.
>>
>> Signed-off-by: Julia Lawall <julia@diku.dk>
>>
>> ---
>> This is only compile tested. In particular, I don't know if it is correct
>> to add per_cpu(processor_device_array, pr->id) = NULL;.
>
>
> No, you shouldn't set it to NULL. processor_device_array was added to check
> for buggy BIOSes and return gracefully. Check commit cd8e2b48d (and also the
> bugzilla link in that commit).
>
> To have a robust check for buggy BIOSes, we must let it be as it is, even if
> it is a stale pointer. That way we can still catch subsequent calls to this
> function with the same acpi id (because of a buggy BIOS) and take appropriate
> actions.
>
> Other than that, the patch looks good to me.
Thanks for the feedback. Just to be clear, I should keep
+err_clear_processors:
+ per_cpu(processors, pr->id) = NULL;
and just drop:
+ per_cpu(processor_device_array, pr->id) = NULL;
julia
>
> Regards,
> Srivatsa S. Bhat
> IBM Linux Technology Center
>
>> It has nothing to
>> do with pr, but it looks like stale information in the case of a failure.
>>
>> drivers/acpi/processor_driver.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 2801b41..9bb0017 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>> return -ENOMEM;
>>
>> if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
>> - kfree(pr);
>> - return -ENOMEM;
>> + result = -ENOMEM;
>> + goto err_free_pr;
>> }
>>
>> pr->handle = device->handle;
>> @@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>> dev = get_cpu_device(pr->id);
>> if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
>> result = -EFAULT;
>> - goto err_free_cpumask;
>> + goto err_clear_processors;
>> }
>>
>> /*
>> @@ -595,9 +595,13 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>>
>> err_remove_sysfs:
>> sysfs_remove_link(&device->dev.kobj, "sysdev");
>> +err_clear_processors:
>> + per_cpu(processors, pr->id) = NULL;
>> + per_cpu(processor_device_array, pr->id) = NULL;
>> err_free_cpumask:
>> free_cpumask_var(pr->throttling.shared_cpu_map);
>> -
>> +err_free_pr:
>> + kfree(pr);
>> return result;
>> }
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-14 12:32 ` Julia Lawall
@ 2012-03-14 13:02 ` Srivatsa S. Bhat
0 siblings, 0 replies; 9+ messages in thread
From: Srivatsa S. Bhat @ 2012-03-14 13:02 UTC (permalink / raw)
To: Julia Lawall
Cc: Len Brown, kernel-janitors, linux-acpi, linux-kernel,
Julia Lawall, Deepthi Dharwar, Venkatesh Pallipadi, trenn,
bhelgaas
On 03/14/2012 06:02 PM, Julia Lawall wrote:
>
>
> On Wed, 14 Mar 2012, Srivatsa S. Bhat wrote:
>
>> On 03/10/2012 10:35 PM, Julia Lawall wrote:
>>
>>> From: Julia Lawall <julia@diku.dk>
>>>
>>> The function acpi_processor_add is stored in the ops.add field of a
>>> acpi_driver structure. This function is then called in
>>> acpi_bus_driver_init. On failure, this function clears the field
>>> device->driver_data, but does not free its contents. Thus the free
>>> has to
>>> be done by the add function. In acpi_processor_add, the corresponding
>>> value is pr. This value is currently freed on failure before storing
>>> it in
>>> device->driver_data, but not after. This free is added in the error
>>> handling code at the end of the function. The static global variable
>>
>>
>> "static global variable"?? never heard that one before ;-)
>> Maybe you meant "per_cpu variable processors"..
>>
>>> processors is also cleared so that it does not refer to a dangling
>>> pointer.
>>> processor_device_array is cleared as well.
>>>
>>> Signed-off-by: Julia Lawall <julia@diku.dk>
>>>
>>> ---
>>> This is only compile tested. In particular, I don't know if it is
>>> correct
>>> to add per_cpu(processor_device_array, pr->id) = NULL;.
>>
>>
>> No, you shouldn't set it to NULL. processor_device_array was added to
>> check
>> for buggy BIOSes and return gracefully. Check commit cd8e2b48d (and
>> also the
>> bugzilla link in that commit).
>>
>> To have a robust check for buggy BIOSes, we must let it be as it is,
>> even if
>> it is a stale pointer. That way we can still catch subsequent calls to
>> this
>> function with the same acpi id (because of a buggy BIOS) and take
>> appropriate
>> actions.
>>
>> Other than that, the patch looks good to me.
>
> Thanks for the feedback. Just to be clear, I should keep
>
> +err_clear_processors:
> + per_cpu(processors, pr->id) = NULL;
>
> and just drop:
>
> + per_cpu(processor_device_array, pr->id) = NULL;
>
Yep, that's right. But in fact, you can even do better than that...
That is, drop the above line and put a comment that explains why we
shouldn't set per_cpu(processor_device_array..) to NULL. That way,
in future people will know that setting it to NULL was left out on
purpose.
And please adjust the commit message too, as I pointed in my previous
mail :-)
Regards,
Srivatsa S. Bhat
>>>
>>> drivers/acpi/processor_driver.c | 12 ++++++++----
>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/acpi/processor_driver.c
>>> b/drivers/acpi/processor_driver.c
>>> index 2801b41..9bb0017 100644
>>> --- a/drivers/acpi/processor_driver.c
>>> +++ b/drivers/acpi/processor_driver.c
>>> @@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct
>>> acpi_device *device)
>>> return -ENOMEM;
>>>
>>> if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map,
>>> GFP_KERNEL)) {
>>> - kfree(pr);
>>> - return -ENOMEM;
>>> + result = -ENOMEM;
>>> + goto err_free_pr;
>>> }
>>>
>>> pr->handle = device->handle;
>>> @@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct
>>> acpi_device *device)
>>> dev = get_cpu_device(pr->id);
>>> if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
>>> result = -EFAULT;
>>> - goto err_free_cpumask;
>>> + goto err_clear_processors;
>>> }
>>>
>>> /*
>>> @@ -595,9 +595,13 @@ static int __cpuinit acpi_processor_add(struct
>>> acpi_device *device)
>>>
>>> err_remove_sysfs:
>>> sysfs_remove_link(&device->dev.kobj, "sysdev");
>>> +err_clear_processors:
>>> + per_cpu(processors, pr->id) = NULL;
>>> + per_cpu(processor_device_array, pr->id) = NULL;
>>> err_free_cpumask:
>>> free_cpumask_var(pr->throttling.shared_cpu_map);
>>> -
>>> +err_free_pr:
>>> + kfree(pr);
>>> return result;
>>> }
>>>
>>>
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-10 17:05 [PATCH] drivers/acpi/processor_driver.c: add missing kfree Julia Lawall
2012-03-14 11:46 ` Srivatsa S. Bhat
@ 2012-03-15 8:32 ` Julia Lawall
2012-03-15 9:19 ` Srivatsa S. Bhat
2012-03-15 9:44 ` Deepthi Dharwar
2012-03-16 10:02 ` Julia Lawall
2012-03-17 15:38 ` Julia Lawall
3 siblings, 2 replies; 9+ messages in thread
From: Julia Lawall @ 2012-03-15 8:32 UTC (permalink / raw)
To: Len Brown, srivatsa.bhat, deepthi, venki, trenn, bhelgaas
Cc: kernel-janitors, linux-acpi, linux-kernel
From: Julia Lawall <Julia.Lawall@lip6.fr>
The function acpi_processor_add is stored in the ops.add field of a
acpi_driver structure. This function is then called in
acpi_bus_driver_init. On failure, this function clears the field
device->driver_data, but does not free its contents. Thus the free has to
be done by the add function. In acpi_processor_add, the corresponding
value is pr. This value is currently freed on failure before storing it in
device->driver_data, but not after. This free is added in the error
handling code at the end of the function. The per_cpu variable
processors is also cleared so that it does not refer to a dangling pointer.
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
drivers/acpi/processor_driver.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 2801b41..98c3648 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
return -ENOMEM;
if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
- kfree(pr);
- return -ENOMEM;
+ result = -ENOMEM;
+ goto err_free_pr;
}
pr->handle = device->handle;
@@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
dev = get_cpu_device(pr->id);
if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
result = -EFAULT;
- goto err_free_cpumask;
+ goto err_clear_processor;
}
/*
@@ -595,9 +595,14 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
err_remove_sysfs:
sysfs_remove_link(&device->dev.kobj, "sysdev");
+err_clear_processor:
+ /* processor_device_array is not cleared to allow checks for buggy
+ BIOSes */
+ per_cpu(processors, pr->id) = NULL;
err_free_cpumask:
free_cpumask_var(pr->throttling.shared_cpu_map);
-
+err_free_pr:
+ kfree(pr);
return result;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-15 8:32 ` Julia Lawall
@ 2012-03-15 9:19 ` Srivatsa S. Bhat
2012-03-15 9:44 ` Deepthi Dharwar
1 sibling, 0 replies; 9+ messages in thread
From: Srivatsa S. Bhat @ 2012-03-15 9:19 UTC (permalink / raw)
To: Julia Lawall
Cc: Len Brown, deepthi, venki, trenn, bhelgaas, kernel-janitors,
linux-acpi, linux-kernel
On 03/15/2012 02:02 PM, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> The function acpi_processor_add is stored in the ops.add field of a
> acpi_driver structure. This function is then called in
> acpi_bus_driver_init. On failure, this function clears the field
> device->driver_data, but does not free its contents. Thus the free has to
> be done by the add function. In acpi_processor_add, the corresponding
> value is pr. This value is currently freed on failure before storing it in
> device->driver_data, but not after. This free is added in the error
> handling code at the end of the function. The per_cpu variable
> processors is also cleared so that it does not refer to a dangling pointer.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
See a minor nitpick below.. Other than that, everything looks great!
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> drivers/acpi/processor_driver.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 2801b41..98c3648 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> return -ENOMEM;
>
> if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
> - kfree(pr);
> - return -ENOMEM;
> + result = -ENOMEM;
> + goto err_free_pr;
> }
>
> pr->handle = device->handle;
> @@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> dev = get_cpu_device(pr->id);
> if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
> result = -EFAULT;
> - goto err_free_cpumask;
> + goto err_clear_processor;
> }
>
> /*
> @@ -595,9 +595,14 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>
> err_remove_sysfs:
> sysfs_remove_link(&device->dev.kobj, "sysdev");
> +err_clear_processor:
> + /* processor_device_array is not cleared to allow checks for buggy
> + BIOSes */
Multi-line comments in the kernel are generally written like:
/*
* blah blah blah
* blah blah ....
*/
> + per_cpu(processors, pr->id) = NULL;
> err_free_cpumask:
> free_cpumask_var(pr->throttling.shared_cpu_map);
> -
> +err_free_pr:
> + kfree(pr);
> return result;
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-15 8:32 ` Julia Lawall
2012-03-15 9:19 ` Srivatsa S. Bhat
@ 2012-03-15 9:44 ` Deepthi Dharwar
1 sibling, 0 replies; 9+ messages in thread
From: Deepthi Dharwar @ 2012-03-15 9:44 UTC (permalink / raw)
To: Julia Lawall
Cc: Len Brown, srivatsa.bhat, venki, trenn, bhelgaas, kernel-janitors,
linux-acpi, linux-kernel
On 03/15/2012 02:02 PM, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> The function acpi_processor_add is stored in the ops.add field of a
> acpi_driver structure. This function is then called in
> acpi_bus_driver_init. On failure, this function clears the field
> device->driver_data, but does not free its contents. Thus the free has to
> be done by the add function. In acpi_processor_add, the corresponding
> value is pr. This value is currently freed on failure before storing it in
> device->driver_data, but not after. This free is added in the error
> handling code at the end of the function. The per_cpu variable
> processors is also cleared so that it does not refer to a dangling pointer.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
Acked-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> ---
> drivers/acpi/processor_driver.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 2801b41..98c3648 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> return -ENOMEM;
>
> if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
> - kfree(pr);
> - return -ENOMEM;
> + result = -ENOMEM;
> + goto err_free_pr;
> }
>
> pr->handle = device->handle;
> @@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
> dev = get_cpu_device(pr->id);
> if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
> result = -EFAULT;
> - goto err_free_cpumask;
> + goto err_clear_processor;
> }
>
> /*
> @@ -595,9 +595,14 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
>
> err_remove_sysfs:
> sysfs_remove_link(&device->dev.kobj, "sysdev");
> +err_clear_processor:
> + /* processor_device_array is not cleared to allow checks for buggy
> + BIOSes */
> + per_cpu(processors, pr->id) = NULL;
> err_free_cpumask:
> free_cpumask_var(pr->throttling.shared_cpu_map);
> -
> +err_free_pr:
> + kfree(pr);
> return result;
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-10 17:05 [PATCH] drivers/acpi/processor_driver.c: add missing kfree Julia Lawall
2012-03-14 11:46 ` Srivatsa S. Bhat
2012-03-15 8:32 ` Julia Lawall
@ 2012-03-16 10:02 ` Julia Lawall
2012-03-17 15:38 ` Julia Lawall
3 siblings, 0 replies; 9+ messages in thread
From: Julia Lawall @ 2012-03-16 10:02 UTC (permalink / raw)
To: Len Brown, srivatsa.bhat, deepthi, venki, trenn, bhelgaas
Cc: kernel-janitors, linux-acpi, linux-kernel
From: Julia Lawall <Julia.Lawall@lip6.fr>
The function acpi_processor_add is stored in the ops.add field of a
acpi_driver structure. This function is then called in
acpi_bus_driver_init. On failure, this function clears the field
device->driver_data, but does not free its contents. Thus the free has to
be done by the add function. In acpi_processor_add, the corresponding
value is pr. This value is currently freed on failure before storing it in
device->driver_data, but not after. This free is added in the error
handling code at the end of the function. The per_cpu variable
processors is also cleared so that it does not refer to a dangling pointer.
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
v2: Adjusted the comment format
drivers/acpi/processor_driver.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 2801b41..c5ce91c 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
return -ENOMEM;
if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
- kfree(pr);
- return -ENOMEM;
+ result = -ENOMEM;
+ goto err_free_pr;
}
pr->handle = device->handle;
@@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
dev = get_cpu_device(pr->id);
if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
result = -EFAULT;
- goto err_free_cpumask;
+ goto err_clear_processor;
}
/*
@@ -595,9 +595,16 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
err_remove_sysfs:
sysfs_remove_link(&device->dev.kobj, "sysdev");
+err_clear_processor:
+ /*
+ * processor_device_array is not cleared to allow checks for buggy
+ * BIOSes
+ */
+ per_cpu(processors, pr->id) = NULL;
err_free_cpumask:
free_cpumask_var(pr->throttling.shared_cpu_map);
-
+err_free_pr:
+ kfree(pr);
return result;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH] drivers/acpi/processor_driver.c: add missing kfree
2012-03-10 17:05 [PATCH] drivers/acpi/processor_driver.c: add missing kfree Julia Lawall
` (2 preceding siblings ...)
2012-03-16 10:02 ` Julia Lawall
@ 2012-03-17 15:38 ` Julia Lawall
3 siblings, 0 replies; 9+ messages in thread
From: Julia Lawall @ 2012-03-17 15:38 UTC (permalink / raw)
To: Len Brown, srivatsa.bhat, deepthi, venki, trenn, bhelgaas
Cc: kernel-janitors, linux-acpi, linux-kernel
From: Julia Lawall <Julia.Lawall@lip6.fr>
The function acpi_processor_add is stored in the ops.add field of a
acpi_driver structure. This function is then called in
acpi_bus_driver_init. On failure, this function clears the field
device->driver_data, but does not free its contents. Thus the free has to
be done by the add function. In acpi_processor_add, the corresponding
value is pr. This value is currently freed on failure before storing it in
device->driver_data, but not after. This free is added in the error
handling code at the end of the function. The per_cpu variable
processors is also cleared so that it does not refer to a dangling pointer.
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Acked-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
---
v3: Added reviews and acks
drivers/acpi/processor_driver.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 2801b41..c5ce91c 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -536,8 +536,8 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
return -ENOMEM;
if (!zalloc_cpumask_var(&pr->throttling.shared_cpu_map, GFP_KERNEL)) {
- kfree(pr);
- return -ENOMEM;
+ result = -ENOMEM;
+ goto err_free_pr;
}
pr->handle = device->handle;
@@ -577,7 +577,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
dev = get_cpu_device(pr->id);
if (sysfs_create_link(&device->dev.kobj, &dev->kobj, "sysdev")) {
result = -EFAULT;
- goto err_free_cpumask;
+ goto err_clear_processor;
}
/*
@@ -595,9 +595,16 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
err_remove_sysfs:
sysfs_remove_link(&device->dev.kobj, "sysdev");
+err_clear_processor:
+ /*
+ * processor_device_array is not cleared to allow checks for buggy
+ * BIOSes
+ */
+ per_cpu(processors, pr->id) = NULL;
err_free_cpumask:
free_cpumask_var(pr->throttling.shared_cpu_map);
-
+err_free_pr:
+ kfree(pr);
return result;
}
^ permalink raw reply related [flat|nested] 9+ messages in thread