lkml.org 
[lkml]   [2011]   [Aug]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] Battery: sysfs_remove_battery(): possible circular locking
    On (08/05/11 13:10), lan,Tianyu wrote:
    > I think changing 'the marker' to 'battery->bat.name' will introduce
    > problem.
    > In the sysfs_add_battery(), when the 'battery->bat.name' is assigned,
    > the power_supply_register() and device_create_file() have not been
    > invoked. In this time, maybe sysfs_remove_battery() will be invoked and
    > cause device_remove_file() and power_supply_unregister() invoked without
    > device file created and power supply registered.
    >
    > sysfs_remove_battery() will be invoked in the battery_notify(),
    > acpi_battery_refresh() and sysfs_remove_battery() which causes the
    > situation. This is also the cause of bug 35642.
    >

    Well, how about using separate (independent lock) for sysfs_remove_battery()
    case? Since we can't safely drop battery->lock in sysfs_remove_battery() before
    power_supply_unregister() call.

    Not sure if it should be within struct acpi_battery, perhaps we could
    have it as a 'global' battery lock. Anyway, here it is:

    ---

    drivers/acpi/battery.c | 10 +++++++---
    1 files changed, 7 insertions(+), 3 deletions(-)

    diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
    index 87c0a8d..7711d94 100644
    --- a/drivers/acpi/battery.c
    +++ b/drivers/acpi/battery.c
    @@ -99,6 +99,7 @@ enum {

    struct acpi_battery {
    struct mutex lock;
    + struct mutex sysfs_lock;
    struct power_supply bat;
    struct acpi_device *device;
    struct notifier_block pm_nb;
    @@ -573,16 +574,16 @@ static int sysfs_add_battery(struct acpi_battery *battery)

    static void sysfs_remove_battery(struct acpi_battery *battery)
    {
    - mutex_lock(&battery->lock);
    + mutex_lock(&battery->sysfs_lock);
    if (!battery->bat.dev) {
    - mutex_unlock(&battery->lock);
    + mutex_unlock(&battery->sysfs_lock);
    return;
    }

    device_remove_file(battery->bat.dev, &alarm_attr);
    power_supply_unregister(&battery->bat);
    battery->bat.dev = NULL;
    - mutex_unlock(&battery->lock);
    + mutex_unlock(&battery->sysfs_lock);
    }

    /*
    @@ -982,6 +983,7 @@ static int acpi_battery_add(struct acpi_device *device)
    strcpy(acpi_device_class(device), ACPI_BATTERY_CLASS);
    device->driver_data = battery;
    mutex_init(&battery->lock);
    + mutex_init(&battery->sysfs_lock);
    if (ACPI_SUCCESS(acpi_get_handle(battery->device->handle,
    "_BIX", &handle)))
    set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
    @@ -1010,6 +1012,7 @@ static int acpi_battery_add(struct acpi_device *device)
    fail:
    sysfs_remove_battery(battery);
    mutex_destroy(&battery->lock);
    + mutex_destroy(&battery->sysfs_lock);
    kfree(battery);
    return result;
    }
    @@ -1027,6 +1030,7 @@ static int acpi_battery_remove(struct acpi_device *device, int type)
    #endif
    sysfs_remove_battery(battery);
    mutex_destroy(&battery->lock);
    + mutex_destroy(&battery->sysfs_lock);
    kfree(battery);
    return 0;
    }


    \
     
     \ /
      Last update: 2011-08-05 18:43    [W:0.028 / U:212.116 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site