A notification event 0x81 from an ACPI battery device requires us to
re-read the battery information structure. Do so, and if the battery's
reporting units have changed (as is the case on some Thinkpads) destroy
and recreate the battery in order to populate the fields correctly.
Signed-off-by: Matthew Garrett <[email protected]>
---
drivers/acpi/battery.c | 22 +++++++++++++++++-----
1 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index dc58402..69638c4 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -562,9 +562,10 @@ static void acpi_battery_quirks(struct acpi_battery *battery)
}
}
-static int acpi_battery_update(struct acpi_battery *battery)
+static int acpi_battery_update(struct acpi_battery *battery, bool get_info)
{
int result, old_present = acpi_battery_present(battery);
+ int old_power_unit = battery->power_unit;
result = acpi_battery_get_status(battery);
if (result)
return result;
@@ -587,6 +588,16 @@ static int acpi_battery_update(struct acpi_battery *battery)
if (!battery->bat.dev)
sysfs_add_battery(battery);
#endif
+ if (get_info) {
+ acpi_battery_get_info(battery);
+#ifdef CONFIG_ACPI_SYSFS_POWER
+ if (old_power_unit != battery->power_unit) {
+ /* The battery has changed its reporting units */
+ sysfs_remove_battery(battery);
+ sysfs_add_battery(battery);
+ }
+#endif
+ }
return acpi_battery_get_state(battery);
}
@@ -762,7 +773,7 @@ static print_func acpi_print_funcs[ACPI_BATTERY_NUMFILES] = {
static int acpi_battery_read(int fid, struct seq_file *seq)
{
struct acpi_battery *battery = seq->private;
- int result = acpi_battery_update(battery);
+ int result = acpi_battery_update(battery, false);
return acpi_print_funcs[fid](seq, result);
}
@@ -877,7 +888,8 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event)
#ifdef CONFIG_ACPI_SYSFS_POWER
old = battery->bat.dev;
#endif
- acpi_battery_update(battery);
+ acpi_battery_update(battery, (event == ACPI_BATTERY_NOTIFY_INFO ? true
+ : false));
acpi_bus_generate_proc_event(device, event,
acpi_battery_present(battery));
acpi_bus_generate_netlink_event(device->pnp.device_class,
@@ -908,7 +920,7 @@ static int acpi_battery_add(struct acpi_device *device)
if (ACPI_SUCCESS(acpi_get_handle(battery->device->handle,
"_BIX", &handle)))
set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
- acpi_battery_update(battery);
+ acpi_battery_update(battery, false);
#ifdef CONFIG_ACPI_PROCFS_POWER
result = acpi_battery_add_fs(device);
#endif
@@ -951,7 +963,7 @@ static int acpi_battery_resume(struct acpi_device *device)
return -EINVAL;
battery = acpi_driver_data(device);
battery->update_time = 0;
- acpi_battery_update(battery);
+ acpi_battery_update(battery, true);
return 0;
}
--
1.7.2.1
On 16 August 2010 21:32, Matthew Garrett <[email protected]> wrote:
> A notification event 0x81 from an ACPI battery device requires us to
> re-read the battery information structure. Do so, and if the battery's
> reporting units have changed (as is the case on some Thinkpads) destroy
> and recreate the battery in order to populate the fields correctly.
I've had three reports in upower of this bug (the units changing) so
users are really hitting this in real world usage. As soon as this
kernel fix gets released in a new kernel version I'll remove the
upower workaround. I can't reproduce this myself, so I've not tested
the patch, although the logic is solid in my opinion.
Richard.
Len, any chance you could pick this up?
--
Matthew Garrett | [email protected]
A notification event 0x81 from an ACPI battery device requires us to
re-read the battery information structure. Do so, and if the battery's
reporting units have changed (as is the case on some Thinkpads) destroy
and recreate the battery in order to populate the fields correctly.
Signed-off-by: Matthew Garrett <[email protected]>
---
Respun against Linus master
drivers/acpi/battery.c | 24 +++++++++++++++++-------
1 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 9fb9d5a..8da9c88 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -130,7 +130,7 @@ struct acpi_battery {
unsigned long flags;
};
-static int acpi_battery_update(struct acpi_battery *battery);
+static int acpi_battery_update(struct acpi_battery *battery, bool get_info);
#define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);
@@ -186,7 +186,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
int ret = 0;
struct acpi_battery *battery = to_acpi_battery(psy);
- if (acpi_battery_update(battery))
+ if (acpi_battery_update(battery, false))
return -ENODEV;
if (acpi_battery_present(battery)) {
@@ -610,9 +610,11 @@ static void acpi_battery_quirks2(struct acpi_battery *battery)
}
}
-static int acpi_battery_update(struct acpi_battery *battery)
+static int acpi_battery_update(struct acpi_battery *battery, bool get_info)
{
int result, old_present = acpi_battery_present(battery);
+ int old_power_unit = battery->power_unit;
+
result = acpi_battery_get_status(battery);
if (result)
return result;
@@ -631,6 +633,14 @@ static int acpi_battery_update(struct acpi_battery *battery)
}
if (!battery->bat.dev)
sysfs_add_battery(battery);
+ if (get_info) {
+ acpi_battery_get_info(battery);
+ if (old_power_unit != battery->power_unit) {
+ /* The battery has changed its reporting units */
+ sysfs_remove_battery(battery);
+ sysfs_add_battery(battery);
+ }
+ }
result = acpi_battery_get_state(battery);
acpi_battery_quirks2(battery);
return result;
@@ -808,7 +818,7 @@ static print_func acpi_print_funcs[ACPI_BATTERY_NUMFILES] = {
static int acpi_battery_read(int fid, struct seq_file *seq)
{
struct acpi_battery *battery = seq->private;
- int result = acpi_battery_update(battery);
+ int result = acpi_battery_update(battery, false);
return acpi_print_funcs[fid](seq, result);
}
@@ -919,7 +929,7 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event)
if (!battery)
return;
old = battery->bat.dev;
- acpi_battery_update(battery);
+ acpi_battery_update(battery, (event == ACPI_BATTERY_NOTIFY_INFO));
acpi_bus_generate_proc_event(device, event,
acpi_battery_present(battery));
acpi_bus_generate_netlink_event(device->pnp.device_class,
@@ -948,7 +958,7 @@ static int acpi_battery_add(struct acpi_device *device)
if (ACPI_SUCCESS(acpi_get_handle(battery->device->handle,
"_BIX", &handle)))
set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
- acpi_battery_update(battery);
+ acpi_battery_update(battery, false);
#ifdef CONFIG_ACPI_PROCFS_POWER
result = acpi_battery_add_fs(device);
#endif
@@ -989,7 +999,7 @@ static int acpi_battery_resume(struct acpi_device *device)
return -EINVAL;
battery = acpi_driver_data(device);
battery->update_time = 0;
- acpi_battery_update(battery);
+ acpi_battery_update(battery, true);
return 0;
}
--
Matthew Garrett | [email protected]
On Thursday, December 23, 2010, Matthew Garrett wrote:
> A notification event 0x81 from an ACPI battery device requires us to
> re-read the battery information structure. Do so, and if the battery's
> reporting units have changed (as is the case on some Thinkpads) destroy
> and recreate the battery in order to populate the fields correctly.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> ---
>
> Respun against Linus master
As I've already said on IRC, I'd simply add a new function like:
void acpi_battery_refresh(struct acpi_battery *battery)
{
if (!battery->bat.dev)
return;
acpi_battery_get_info(battery);
/* The battery may have changed its reporting units */
sysfs_remove_battery(battery);
sysfs_add_battery(battery);
}
and call it before acpi_battery_update()
(a) in acpi_battery_resume()
(b) in acpi_battery_notify() if event == ACPI_BATTERY_NOTIFY_INFO
That would be more straightforward IMHO.
Thanks,
Rafael
> drivers/acpi/battery.c | 24 +++++++++++++++++-------
> 1 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 9fb9d5a..8da9c88 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -130,7 +130,7 @@ struct acpi_battery {
> unsigned long flags;
> };
>
> -static int acpi_battery_update(struct acpi_battery *battery);
> +static int acpi_battery_update(struct acpi_battery *battery, bool get_info);
>
> #define to_acpi_battery(x) container_of(x, struct acpi_battery, bat);
>
> @@ -186,7 +186,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
> int ret = 0;
> struct acpi_battery *battery = to_acpi_battery(psy);
>
> - if (acpi_battery_update(battery))
> + if (acpi_battery_update(battery, false))
> return -ENODEV;
>
> if (acpi_battery_present(battery)) {
> @@ -610,9 +610,11 @@ static void acpi_battery_quirks2(struct acpi_battery *battery)
> }
> }
>
> -static int acpi_battery_update(struct acpi_battery *battery)
> +static int acpi_battery_update(struct acpi_battery *battery, bool get_info)
> {
> int result, old_present = acpi_battery_present(battery);
> + int old_power_unit = battery->power_unit;
> +
> result = acpi_battery_get_status(battery);
> if (result)
> return result;
> @@ -631,6 +633,14 @@ static int acpi_battery_update(struct acpi_battery *battery)
> }
> if (!battery->bat.dev)
> sysfs_add_battery(battery);
> + if (get_info) {
> + acpi_battery_get_info(battery);
> + if (old_power_unit != battery->power_unit) {
> + /* The battery has changed its reporting units */
> + sysfs_remove_battery(battery);
> + sysfs_add_battery(battery);
> + }
> + }
> result = acpi_battery_get_state(battery);
> acpi_battery_quirks2(battery);
> return result;
> @@ -808,7 +818,7 @@ static print_func acpi_print_funcs[ACPI_BATTERY_NUMFILES] = {
> static int acpi_battery_read(int fid, struct seq_file *seq)
> {
> struct acpi_battery *battery = seq->private;
> - int result = acpi_battery_update(battery);
> + int result = acpi_battery_update(battery, false);
> return acpi_print_funcs[fid](seq, result);
> }
>
> @@ -919,7 +929,7 @@ static void acpi_battery_notify(struct acpi_device *device, u32 event)
> if (!battery)
> return;
> old = battery->bat.dev;
> - acpi_battery_update(battery);
> + acpi_battery_update(battery, (event == ACPI_BATTERY_NOTIFY_INFO));
> acpi_bus_generate_proc_event(device, event,
> acpi_battery_present(battery));
> acpi_bus_generate_netlink_event(device->pnp.device_class,
> @@ -948,7 +958,7 @@ static int acpi_battery_add(struct acpi_device *device)
> if (ACPI_SUCCESS(acpi_get_handle(battery->device->handle,
> "_BIX", &handle)))
> set_bit(ACPI_BATTERY_XINFO_PRESENT, &battery->flags);
> - acpi_battery_update(battery);
> + acpi_battery_update(battery, false);
> #ifdef CONFIG_ACPI_PROCFS_POWER
> result = acpi_battery_add_fs(device);
> #endif
> @@ -989,7 +999,7 @@ static int acpi_battery_resume(struct acpi_device *device)
> return -EINVAL;
> battery = acpi_driver_data(device);
> battery->update_time = 0;
> - acpi_battery_update(battery);
> + acpi_battery_update(battery, true);
> return 0;
> }
>
>