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?
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;
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
