[PATCH] acpi: Update battery information on notification 0x81

[PATCH] acpi: Update battery information on notification 0x81

[Matthew Garrett]

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 <mjg@redhat.com>
---
 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

Re: [PATCH] acpi: Update battery information on notification 0x81

[Richard Hughes]

On 16 August 2010 21:32, Matthew Garrett <mjg@redhat.com> 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.

Re: [PATCH] acpi: Update battery information on notification 0x81

[Matthew Garrett]

Len, any chance you could pick this up?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH] acpi: Update battery information on notification 0x81

[Matthew Garrett]

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 <mjg@redhat.com>
---

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 | mjg59@srcf.ucam.org

Re: [PATCH] acpi: Update battery information on notification 0x81

[Rafael J. Wysocki]

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 <mjg@redhat.com>
> ---
> 
> 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;
>  }
> 
> 

Re: [PATCH] acpi: Update battery information on notification 0x81

[Matthew Garrett]

On Thu, Dec 23, 2010 at 11:23:42PM +0100, Rafael J. Wysocki wrote:

> 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.

That might be more reasonable. Want to send that, or shall I?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] acpi: Update battery information on notification 0x81

[Rafael J. Wysocki]

On Monday, January 03, 2011, Matthew Garrett wrote:
> On Thu, Dec 23, 2010 at 11:23:42PM +0100, Rafael J. Wysocki wrote:
> 
> > 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.
> 
> That might be more reasonable. Want to send that, or shall I?

I can do that.

Thanks,
Rafael