| Message ID | 20110712172331.GA9557@oksana.dev.rtsoft.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 12, 2011 at 09:23:31PM +0400, Anton Vorontsov wrote: > On Tue, Jul 12, 2011 at 04:30:17PM +0100, Stefan Hajnoczi wrote: > [...] > > > I believe the whole ACPI battery logic is overcomplicated, and > > > needs a bit of rework. In the meantime, we could move 'psy->dev = > > > dev;' assignment into the end of the function, where _register > > > could not fail, i.e. something like this: > > > > Aha! I didn't do this is because I don't know the code and was afraid > > some other function somewhere would use psy->dev. If you think it is > > safer this way I'll resend the patch. > > Neither is a proper fix, unfortunately. :-( Without some external lock > you can't use psy->dev as a flag to check if the registration was > successful. There is really no point trying to force core functions > to keep psy->dev in a sane state after registration has failed. > > So, instead of patching power_supply core, I'd suggest the > following patch (on top of 2/3 and 3/3, so far only compile- > tested). How does it look? Comments below. Thanks for adding this lock. > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index 2ae2fca..475e17c 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -101,6 +101,7 @@ enum { > > struct acpi_battery { > struct mutex lock; > + struct mutex bat_lock; > struct power_supply bat; > struct acpi_device *device; > struct notifier_block pm_nb; > @@ -559,8 +560,10 @@ static int sysfs_add_battery(struct acpi_battery *battery) > battery->bat.get_property = acpi_battery_get_property; > > result = power_supply_register(&battery->device->dev, &battery->bat); > - if (result) > + if (result) { > + battery->bat.dev = NULL; > return result; > + } > return device_create_file(battery->bat.dev, &alarm_attr); > } > > @@ -613,31 +616,40 @@ static int acpi_battery_update(struct acpi_battery *battery) > result = acpi_battery_get_status(battery); > if (result) > return result; > + > + mutex_lock(&battery->bat_lock); > + > if (!acpi_battery_present(battery)) { > sysfs_remove_battery(battery); > battery->update_time = 0; > - return 0; > + result = 0; > + goto out_unlock; > } > if (!battery->update_time || > old_present != acpi_battery_present(battery)) { > result = acpi_battery_get_info(battery); > if (result) > - return result; > + goto out_unlock; > acpi_battery_quirks(battery); > acpi_battery_init_alarm(battery); > } > if (!battery->bat.dev) { > result = sysfs_add_battery(battery); > if (result) > - return result; > + goto out_unlock; > } > result = acpi_battery_get_state(battery); > acpi_battery_quirks2(battery); > + > +out_unlock: > + mutex_unlock(&battery->bat_lock); > return result; > } > > -static void acpi_battery_refresh(struct acpi_battery *battery) > +static void sysfs_readd_battery(struct acpi_battery *battery) s/readd/read/? > { > + mutex_lock(&battery->bat_lock); > + > if (!battery->bat.dev) > return; Remember to unlock before return: if (!battery->bat.dev) goto out; > > @@ -645,6 +657,13 @@ static void acpi_battery_refresh(struct acpi_battery *battery) > /* The battery may have changed its reporting units. */ > sysfs_remove_battery(battery); > sysfs_add_battery(battery); > + out: > + mutex_unlock(&battery->bat_lock); > +} Stefan -- 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
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index 2ae2fca..475e17c 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -101,6 +101,7 @@ enum { struct acpi_battery { struct mutex lock; + struct mutex bat_lock; struct power_supply bat; struct acpi_device *device; struct notifier_block pm_nb; @@ -559,8 +560,10 @@ static int sysfs_add_battery(struct acpi_battery *battery) battery->bat.get_property = acpi_battery_get_property; result = power_supply_register(&battery->device->dev, &battery->bat); - if (result) + if (result) { + battery->bat.dev = NULL; return result; + } return device_create_file(battery->bat.dev, &alarm_attr); } @@ -613,31 +616,40 @@ static int acpi_battery_update(struct acpi_battery *battery) result = acpi_battery_get_status(battery); if (result) return result; + + mutex_lock(&battery->bat_lock); + if (!acpi_battery_present(battery)) { sysfs_remove_battery(battery); battery->update_time = 0; - return 0; + result = 0; + goto out_unlock; } if (!battery->update_time || old_present != acpi_battery_present(battery)) { result = acpi_battery_get_info(battery); if (result) - return result; + goto out_unlock; acpi_battery_quirks(battery); acpi_battery_init_alarm(battery); } if (!battery->bat.dev) { result = sysfs_add_battery(battery); if (result) - return result; + goto out_unlock; } result = acpi_battery_get_state(battery); acpi_battery_quirks2(battery); + +out_unlock: + mutex_unlock(&battery->bat_lock); return result; } -static void acpi_battery_refresh(struct acpi_battery *battery) +static void sysfs_readd_battery(struct acpi_battery *battery) { + mutex_lock(&battery->bat_lock); + if (!battery->bat.dev) return; @@ -645,6 +657,13 @@ static void acpi_battery_refresh(struct acpi_battery *battery) /* The battery may have changed its reporting units. */ sysfs_remove_battery(battery); sysfs_add_battery(battery); + + mutex_unlock(&battery->bat_lock); +} + +static void acpi_battery_refresh(struct acpi_battery *battery) +{ + sysfs_readd_battery(battery); } /* -------------------------------------------------------------------------- @@ -941,8 +960,10 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event) dev_name(&device->dev), event, acpi_battery_present(battery)); /* acpi_battery_update could remove power_supply object */ + mutex_lock(&battery->bat_lock); if (old && battery->bat.dev) power_supply_changed(&battery->bat); + mutex_unlock(&battery->bat_lock); } static int battery_notify(struct notifier_block *nb, @@ -952,8 +973,7 @@ static int battery_notify(struct notifier_block *nb, pm_nb); switch (mode) { case PM_POST_SUSPEND: - sysfs_remove_battery(battery); - sysfs_add_battery(battery); + sysfs_readd_battery(battery); break; } @@ -975,6 +995,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->bat_lock); if (ACPI_SUCCESS(acpi_get_handle(battery->device->handle, "_BIX", &handle))) set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags); @@ -1019,6 +1040,7 @@ static int acpi_battery_remove(struct acpi_device *device, int type) acpi_battery_remove_fs(device); #endif sysfs_remove_battery(battery); + mutex_destroy(&battery->bat_lock); mutex_destroy(&battery->lock); kfree(battery); return 0;