[PATCH] drivers/acpi/processor_driver.c: add missing kfree

[PATCH] drivers/acpi/processor_driver.c: add missing kfree

[Julia Lawall]

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;
 }
 

Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree

[Srivatsa S. Bhat]

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;
>  }
> 

Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree

[Deepthi Dharwar]

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
> 

[PATCH] drivers/acpi/processor_driver.c: add missing kfree

[Julia Lawall]

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;
 }
 

[PATCH] drivers/acpi/processor_driver.c: add missing kfree

[Julia Lawall]

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;
 }
 

[PATCH] drivers/acpi/processor_driver.c: add missing kfree

[Julia Lawall]

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
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;.  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;
 }
 

Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree

[Srivatsa S. Bhat]

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;
>  }
> 
> 

Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree

[Julia Lawall]

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
>

Re: [PATCH] drivers/acpi/processor_driver.c: add missing kfree

[Srivatsa S. Bhat]

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;
>>>  }
>>>
>>>
>>