lkml.org 
[lkml]   [2012]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] drivers/acpi/processor_driver.c: add missing kfree


    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
    >


    \
     
     \ /
      Last update: 2012-03-14 13:35    [W:0.029 / U:0.256 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site