Returning ACPI_BATTERY_VALUE_UNKNOWN to userspace

Returning ACPI_BATTERY_VALUE_UNKNOWN to userspace

[Sitsofe Wheeler]

Hi,

I have an EeePC 900 with a battery/BIOS that does not report the rate at
which it charges/discharges. When I look at
/proc/acpi/battery/BAT0/state this is what is reported:

present:                 yes
capacity state:          ok
charging state:          charging
present rate:            unknown
remaining capacity:      3120 mAh
present voltage:         7889 mV

However looking at /sys/class/power_supply/BAT0/current_now reports:

-1000

Why -1000? I think it's because it's -1 * 1000 == -1000! In
drivers/acpi/battery.c, ACPI_BATTERY_VALUE_UNKNOWN is defined as being
0xFFFFFFFF. As rate_now is a signed int variable when it is assigned
ACPI_BATTERY_VALUE_UNKNOWN its value is -1. However, before the value is
returned via sysfs it is multiplied by 1000:
http://lxr.linux.no/linux+v2.6.35.7/drivers/acpi/battery.c#L222
(http://lxr.linux.no/linux+v2.6.35.7/drivers/acpi/battery.c#L524 shows
that acpi_battery_get_property will be called via sysfs).

If the above is a correct interpretation, this behaviour was introduced
when sysfs battery support was added in commit
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d7380965752505951668e85de59c128d1d6fd21f
so it has effectively been always been this way.

However, looking at the code for the userspace power reporting tool
upower shows that it is not expecting to test against -1000 - it is
trying to test against 0xffff:
http://cgit.freedesktop.org/DeviceKit/upower/tree/src/linux/up-device-supply.c?id=UPOWER_0_9_6#n583
. Unfortunately, it's not clear that testing 0xffff is ever the right
thing to do. I wrote the following test program:

#include <stdio.h>
#include <math.h>
int main(void) {
	double minusone = -1;
	double sysfs = -1000;
	double hex_kernel = (int) 0xffffffff;
	double hex_tested = 0xffff;
	double energy_rate = fabs(sysfs / 1000000.0);
	double energy_rate_minusone = fabs(minusone / 1000000.0);
	printf("%f %f %f %f %f %f\n", minusone, sysfs, hex_kernel, hex_tested, energy_rate, energy_rate_minusone);
	return 0;
}

Which output the following:

-1.000000 -1000.000000 -1.000000 65535.000000 0.001000 0.000001

Given that at least upower (which is already deployed) will need to be
changed, I'm unsure as to where fixes for this should go. Was it really
the intent for userspace to test for -1000 instead of -1 to determine
an unknown rate?

-- 
Sitsofe | http://sucs.org/~sits/

[PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property() (was: Re: Returning ACPI_BATTERY ...)

[Rafael J. Wysocki]

On Saturday, October 16, 2010, Sitsofe Wheeler wrote:
> Hi,

Hi,

> I have an EeePC 900 with a battery/BIOS that does not report the rate at
> which it charges/discharges. When I look at
> /proc/acpi/battery/BAT0/state this is what is reported:
> 
> present:                 yes
> capacity state:          ok
> charging state:          charging
> present rate:            unknown
> remaining capacity:      3120 mAh
> present voltage:         7889 mV
> 
> However looking at /sys/class/power_supply/BAT0/current_now reports:
> 
> -1000
> 
> Why -1000? I think it's because it's -1 * 1000 == -1000! In
> drivers/acpi/battery.c, ACPI_BATTERY_VALUE_UNKNOWN is defined as being
> 0xFFFFFFFF. As rate_now is a signed int variable when it is assigned
> ACPI_BATTERY_VALUE_UNKNOWN its value is -1.

That's because val->intval is used to return the value rather than because
rate_now is int.

I think this is a bug in the battery driver, that should return -1 in that case.

Matthew, what do you think?

> However, before the value is
> returned via sysfs it is multiplied by 1000:
> http://lxr.linux.no/linux+v2.6.35.7/drivers/acpi/battery.c#L222
> (http://lxr.linux.no/linux+v2.6.35.7/drivers/acpi/battery.c#L524 shows
> that acpi_battery_get_property will be called via sysfs).
> 
> If the above is a correct interpretation, this behaviour was introduced
> when sysfs battery support was added in commit
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=d7380965752505951668e85de59c128d1d6fd21f
> so it has effectively been always been this way.
> 
> However, looking at the code for the userspace power reporting tool
> upower shows that it is not expecting to test against -1000 - it is
> trying to test against 0xffff:

In fact, it should be written to test against -1 or an unsigned representation
of it (which is all ones in whatever unsigned data type used by it).

> http://cgit.freedesktop.org/DeviceKit/upower/tree/src/linux/up-device-supply.c?id=UPOWER_0_9_6#n583
> . Unfortunately, it's not clear that testing 0xffff is ever the right
> thing to do. I wrote the following test program:
> 
> #include <stdio.h>
> #include <math.h>
> int main(void) {
> 	double minusone = -1;
> 	double sysfs = -1000;
> 	double hex_kernel = (int) 0xffffffff;
> 	double hex_tested = 0xffff;
> 	double energy_rate = fabs(sysfs / 1000000.0);
> 	double energy_rate_minusone = fabs(minusone / 1000000.0);
> 	printf("%f %f %f %f %f %f\n", minusone, sysfs, hex_kernel, hex_tested, energy_rate, energy_rate_minusone);
> 	return 0;
> }
> 
> Which output the following:
> 
> -1.000000 -1000.000000 -1.000000 65535.000000 0.001000 0.000001
> 
> Given that at least upower (which is already deployed) will need to be
> changed, I'm unsure as to where fixes for this should go. Was it really
> the intent for userspace to test for -1000 instead of -1 to determine
> an unknown rate?

No, it isn't.

In fact, the driver is supposed to return -ENODATA in that case, which will
result in the read from /sys/class/power_supply/BAT0/current_now fail (I guess
upower should be able to cope with that).

So, I'd suggest applying the appended patch.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: ACPI / Battery: Return -ENODATA for unknown values in get_property()

The function acpi_battery_get_property() is called by the
power supply framework's function power_supply_show_property()
implementing the sysfs interface for power supply devices as the
ACPI battery driver's ->get_property() callback.  Thus it is supposed
to return -ENODATA if the value of the given property is unknown.
Unfortunately, however, it returns 0 in those cases and puts a
wrong (negative) value into the intval field of the
union power_supply_propval object provided by
power_supply_show_property().  In consequence, wron negative
values are read by user space from the battery's sysfs files.
Fix this by making acpi_battery_get_property() behave as
expected.

Reported-by: Sitsofe Wheeler <sitsofe@yahoo.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/battery.c |   35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/acpi/battery.c
===================================================================
--- linux-2.6.orig/drivers/acpi/battery.c
+++ linux-2.6/drivers/acpi/battery.c
@@ -186,6 +186,7 @@ static int acpi_battery_get_property(str
 				     enum power_supply_property psp,
 				     union power_supply_propval *val)
 {
+	int ret = 0;
 	struct acpi_battery *battery = to_acpi_battery(psy);
 
 	if (acpi_battery_present(battery)) {
@@ -214,26 +215,44 @@ static int acpi_battery_get_property(str
 		val->intval = battery->cycle_count;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
-		val->intval = battery->design_voltage * 1000;
+		if (battery->design_voltage == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODATA;
+		else
+			val->intval = battery->design_voltage * 1000;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = battery->voltage_now * 1000;
+		if (battery->voltage_now == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODATA;
+		else
+			val->intval = battery->voltage_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 	case POWER_SUPPLY_PROP_POWER_NOW:
-		val->intval = battery->rate_now * 1000;
+		if (battery->rate_now == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODATA;
+		else
+			val->intval = battery->rate_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 	case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
-		val->intval = battery->design_capacity * 1000;
+		if (battery->design_capacity == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODATA;
+		else
+			val->intval = battery->design_capacity * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
 	case POWER_SUPPLY_PROP_ENERGY_FULL:
-		val->intval = battery->full_charge_capacity * 1000;
+		if (battery->full_charge_capacity == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODATA;
+		else
+			val->intval = battery->full_charge_capacity * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_NOW:
 	case POWER_SUPPLY_PROP_ENERGY_NOW:
-		val->intval = battery->capacity_now * 1000;
+		if (battery->capacity_now == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODATA;
+		else
+			val->intval = battery->capacity_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = battery->model_number;
@@ -245,9 +264,9 @@ static int acpi_battery_get_property(str
 		val->strval = battery->serial_number;
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
-	return 0;
+	return ret;
 }
 
 static enum power_supply_property charge_battery_props[] = {

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property() (was: Re: Returning ACPI_BATTERY ...)

[Henrique de Moraes Holschuh]

On Sun, 17 Oct 2010, Rafael J. Wysocki wrote:
> In fact, the driver is supposed to return -ENODATA in that case, which will
> result in the read from /sys/class/power_supply/BAT0/current_now fail (I guess
> upower should be able to cope with that).

ENODATA?  Shouldn't it be ENXIO?  There is no non-blocking data stream
involved in a sysfs attribute.

Of course, the RIGHT thing would be to not expose in sysfs attributes
that are unsupported by the firmware/hardware in the first place, but
that's easier said than done.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Sitsofe Wheeler]

On Sun, Oct 17, 2010 at 03:19:53AM -0200, Henrique de Moraes Holschuh wrote:
> On Sun, 17 Oct 2010, Rafael J. Wysocki wrote:
> > In fact, the driver is supposed to return -ENODATA in that case, which will
> > result in the read from /sys/class/power_supply/BAT0/current_now
> > fail (I guess upower should be able to cope with that).
>
> ENODATA?  Shouldn't it be ENXIO?  There is no non-blocking data stream
> involved in a sysfs attribute.

Using ENODATA and ENXIO appears to solve the problem (upower reports a
rate of 0.0). However when plugging the battery in after previously only
being on AC power none of the /sys/class/power_supply/BAT0/uevent:*
files are created so upower never realises a battery has been plugged
in. A further issue with ENXIO is is the following repeatedly appears in
dmesg:

power_supply BAT0: driver failed to report `current_now' property

> Of course, the RIGHT thing would be to not expose in sysfs attributes
> that are unsupported by the firmware/hardware in the first place, but
> that's easier said than done.

My understanding is that this very hard to do because you can't tell if
the problem was transient (battery settling) or permanent (feature not
supported).

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Henrique de Moraes Holschuh]

On Sun, 17 Oct 2010, Sitsofe Wheeler wrote:
> On Sun, Oct 17, 2010 at 03:19:53AM -0200, Henrique de Moraes Holschuh wrote:
> > On Sun, 17 Oct 2010, Rafael J. Wysocki wrote:
> > > In fact, the driver is supposed to return -ENODATA in that case, which will
> > > result in the read from /sys/class/power_supply/BAT0/current_now
> > > fail (I guess upower should be able to cope with that).
> >
> > ENODATA?  Shouldn't it be ENXIO?  There is no non-blocking data stream
> > involved in a sysfs attribute.
> 
> Using ENODATA and ENXIO appears to solve the problem (upower reports a
> rate of 0.0). However when plugging the battery in after previously only
> being on AC power none of the /sys/class/power_supply/BAT0/uevent:*
> files are created so upower never realises a battery has been plugged
> in. A further issue with ENXIO is is the following repeatedly appears in
> dmesg:

You might have broken firmware that does not issue a notify when a battery
is plugged.  But it has been a long time, I don't recall if the battery
driver handles hotplugging without the help of the dock/bay driver (it
should, AFAIK).

> power_supply BAT0: driver failed to report `current_now' property

And it keeps silent if it gets ENODATA?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Sitsofe Wheeler]

On Sun, Oct 17, 2010 at 11:10:16AM -0200, Henrique de Moraes Holschuh wrote:
> On Sun, 17 Oct 2010, Sitsofe Wheeler wrote:
> > 
> > Using ENODATA and ENXIO appears to solve the problem (upower reports a
> > rate of 0.0). However when plugging the battery in after previously only
> > being on AC power none of the /sys/class/power_supply/BAT0/uevent:*
> > files are created so upower never realises a battery has been plugged
> 
> You might have broken firmware that does not issue a notify when a battery
> is plugged.  But it has been a long time, I don't recall if the battery
> driver handles hotplugging without the help of the dock/bay driver (it
> should, AFAIK).

Battery hotplug works fine without these patches. I should have said -
the uevent devices are there with a vanilla kernel no matter how many
times the battery is plugged in or unplugged (that's how I knew they
were missing with the patches added :) I am guessing some part of the
kernel/udev cannot handle being told ENODATA or ENXIO and bails out
before those nodes would be made.

> > in. A further issue with ENXIO is is the following repeatedly appears in
> > dmesg:
> > power_supply BAT0: driver failed to report `current_now' property
> 
> And it keeps silent if it gets ENODATA?

Yes.

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Rafael J. Wysocki]

On Sunday, October 17, 2010, Sitsofe Wheeler wrote:
> On Sun, Oct 17, 2010 at 11:10:16AM -0200, Henrique de Moraes Holschuh wrote:
> > On Sun, 17 Oct 2010, Sitsofe Wheeler wrote:
> > > 
> > > Using ENODATA and ENXIO appears to solve the problem (upower reports a
> > > rate of 0.0). However when plugging the battery in after previously only
> > > being on AC power none of the /sys/class/power_supply/BAT0/uevent:*
> > > files are created so upower never realises a battery has been plugged
> > 
> > You might have broken firmware that does not issue a notify when a battery
> > is plugged.  But it has been a long time, I don't recall if the battery
> > driver handles hotplugging without the help of the dock/bay driver (it
> > should, AFAIK).
> 
> Battery hotplug works fine without these patches. I should have said -
> the uevent devices are there with a vanilla kernel no matter how many
> times the battery is plugged in or unplugged (that's how I knew they
> were missing with the patches added :) I am guessing some part of the
> kernel/udev cannot handle being told ENODATA or ENXIO and bails out
> before those nodes would be made.
> 
> > > in. A further issue with ENXIO is is the following repeatedly appears in
> > > dmesg:
> > > power_supply BAT0: driver failed to report `current_now' property
> > 
> > And it keeps silent if it gets ENODATA?
> 
> Yes.

That's why I suggested to use -ENODATA. :-)

Still, if user space has problems with failing reads from the sysfs attributes,
it may be better to simply put -1 in there.  Patch is appended, please test.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: ACPI / Battery: Return -ENODATA for unknown values in get_property()

The function acpi_battery_get_property() is called by the
power supply framework's function power_supply_show_property()
implementing the sysfs interface for power supply devices as the
ACPI battery driver's ->get_property() callback.  Thus it should
return error code if the value of the given property is unknown.
However, it returns 0 in those cases and puts a wrong (negative)
value into the intval field of the union power_supply_propval object
provided by power_supply_show_property().  In consequence, wrong
negative values (typically -1000) are read by user space from the
battery's sysfs files.

Unfortunately, this problem cannot be fixed by simply making the
driver return error code for unknown property values, because that
breaks the existing user space.  Thus make the battery driver put
all ones (-1) into val->intval if the value of the property being
read is unknown.

Reported-by: Sitsofe Wheeler <sitsofe@yahoo.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/battery.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/acpi/battery.c
===================================================================
--- linux-2.6.orig/drivers/acpi/battery.c
+++ linux-2.6/drivers/acpi/battery.c
@@ -193,6 +193,8 @@ static int acpi_battery_get_property(str
 		acpi_battery_get_state(battery);
 	} else if (psp != POWER_SUPPLY_PROP_PRESENT)
 		return -ENODEV;
+	if (psp < POWER_SUPPLY_PROP_MODEL_NAME)
+		val->intval = -1;
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
 		if (battery->state & 0x01)
@@ -214,26 +216,32 @@ static int acpi_battery_get_property(str
 		val->intval = battery->cycle_count;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
-		val->intval = battery->design_voltage * 1000;
+		if (battery->design_voltage != ACPI_BATTERY_VALUE_UNKNOWN)
+			val->intval = battery->design_voltage * 1000;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = battery->voltage_now * 1000;
+		if (battery->voltage_now != ACPI_BATTERY_VALUE_UNKNOWN)
+			val->intval = battery->voltage_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 	case POWER_SUPPLY_PROP_POWER_NOW:
-		val->intval = battery->rate_now * 1000;
+		if (battery->rate_now != ACPI_BATTERY_VALUE_UNKNOWN)
+			val->intval = battery->rate_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 	case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
-		val->intval = battery->design_capacity * 1000;
+		if (battery->design_capacity != ACPI_BATTERY_VALUE_UNKNOWN)
+			val->intval = battery->design_capacity * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
 	case POWER_SUPPLY_PROP_ENERGY_FULL:
-		val->intval = battery->full_charge_capacity * 1000;
+		if (battery->full_charge_capacity != ACPI_BATTERY_VALUE_UNKNOWN)
+			val->intval = battery->full_charge_capacity * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_NOW:
 	case POWER_SUPPLY_PROP_ENERGY_NOW:
-		val->intval = battery->capacity_now * 1000;
+		if (battery->capacity_now != ACPI_BATTERY_VALUE_UNKNOWN)
+			val->intval = battery->capacity_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = battery->model_number;

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Sitsofe Wheeler]

On Sun, Oct 17, 2010 at 08:32:42PM +0200, Rafael J. Wysocki wrote:
> 
> Still, if user space has problems with failing reads from the sysfs
> attributes, it may be better to simply put -1 in there.  Patch is
> appended, please test.

This patch does what it says on the tin (returns -1 in sysfs on my EeePC
900). So:

Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>

It's a shame the previous changes didn't work as they stopped a buggy
upower using the -1 value (and producing a nonsense rate like 8.4e-06)
but it's not clear which part of the stack can't handle -ENODATA
perhaps it is another part of the kernel?

Richard, any chance of upower being changed to test for -1 before doing
doing anything with current_now (
http://cgit.freedesktop.org/DeviceKit/upower/tree/src/linux/up-device-supply.c?id=5387183d53c16a987a0737c1bdec1b62edf3daa6#n561)?
I guess there are a whole bunch of other attributes that could
theoretically be -1 and shouldn't be used if they return it...

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Rafael J. Wysocki]

On Thursday, October 21, 2010, Sitsofe Wheeler wrote:
> On Sun, Oct 17, 2010 at 08:32:42PM +0200, Rafael J. Wysocki wrote:
> > 
> > Still, if user space has problems with failing reads from the sysfs
> > attributes, it may be better to simply put -1 in there.  Patch is
> > appended, please test.
> 
> This patch does what it says on the tin (returns -1 in sysfs on my EeePC
> 900). So:
> 
> Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>
> 
> It's a shame the previous changes didn't work as they stopped a buggy
> upower using the -1 value (and producing a nonsense rate like 8.4e-06)

Hmm.  So upower _doesn't_ handle -1?  What does it do with -1000, then?

> but it's not clear which part of the stack can't handle -ENODATA
> perhaps it is another part of the kernel?

I don't really think it's a part of the kernel.

> Richard, any chance of upower being changed to test for -1 before doing
> doing anything with current_now (
> http://cgit.freedesktop.org/DeviceKit/upower/tree/src/linux/up-device-supply.c?id=5387183d53c16a987a0737c1bdec1b62edf3daa6#n561)?
> I guess there are a whole bunch of other attributes that could
> theoretically be -1 and shouldn't be used if they return it...

If user space doesn't handle -1 correctly too, I think the right approach for
us should be to use the previous version of the patch and return error code
for unknown values.

Thanks,
Rafael

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Sitsofe Wheeler]

On Thu, Oct 21, 2010 at 09:57:47PM +0200, Rafael J. Wysocki wrote:
> On Thursday, October 21, 2010, Sitsofe Wheeler wrote:
> > 
> > It's a shame the previous changes didn't work as they stopped a buggy
> > upower using the -1 value (and producing a nonsense rate like 8.4e-06)
> 
> Hmm.  So upower _doesn't_ handle -1?  What does it do with -1000, then?

It can't handle that either and outputs a nonsense rate like 0.0084.
Looking at the code, it would take a very strange value for it to realise
it is handling a special value as it does arithmetic on the sysfs value
before doing its check:

	/* get rate; it seems odd as it's either in uVh or uWh */
	energy_rate = fabs (sysfs_get_double (native_path, "current_now") / 1000000.0);

	/* convert charge to energy */
	if (energy == 0) {
		energy = sysfs_get_double (native_path, "charge_now") / 1000000.0;
		if (energy == 0)
			energy = sysfs_get_double (native_path, "charge_avg") / 1000000.0;
		energy *= voltage_design;
		energy_rate *= voltage_design;
	}

	/* some batteries don't update last_full attribute */
	if (energy > energy_full) {
		egg_warning ("energy %f bigger than full %f", energy, energy_full);
		energy_full = energy;
	}

	/* present voltage */
	voltage = sysfs_get_double (native_path, "voltage_now") / 1000000.0;
	if (voltage == 0)
		voltage = sysfs_get_double (native_path, "voltage_avg") / 1000000.0;

	/* ACPI gives out the special 'Ones' value for rate when it's unable
	 * to calculate the true rate. We should set the rate zero, and wait
	 * for the BIOS to stabilise. */
	if (energy_rate == 0xffff)
		energy_rate = 0;

By the time the comparison against energy_rate is done the original
sysfs value has at _least_ divided by 1000000.0 and made positive. Hence
the test program in my first mail where I mention that 0xfffff produced
65535.000000, fabs(-1000 / 1000000.0) produced 0.001000 and fabs(-1 /
1000000.0) produces 0.000001. That's also assuming it doesn't wind up
multiplying the previous value by voltage_design...

> > but it's not clear which part of the stack can't handle -ENODATA
> > perhaps it is another part of the kernel?
> 
> I don't really think it's a part of the kernel.

How do I find out which part is not producing those sysfs nodes?

> > Richard, any chance of upower being changed to test for -1 before doing
> > doing anything with current_now (
> > http://cgit.freedesktop.org/DeviceKit/upower/tree/src/linux/up-device-supply.c?id=5387183d53c16a987a0737c1bdec1b62edf3daa6#n561)?
> > I guess there are a whole bunch of other attributes that could
> > theoretically be -1 and shouldn't be used if they return it...
> 
> If user space doesn't handle -1 correctly too, I think the right approach for
> us should be to use the previous version of the patch and return error code
> for unknown values.

So long as sysfs can be made to work properly I am in agreement.

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Richard Hughes]

On 21 October 2010 17:54, Sitsofe Wheeler <sitsofe@yahoo.com> wrote:
> I guess there are a whole bunch of other attributes that could
> theoretically be -1 and shouldn't be used if they return it...

I think checking for <0 is probably a good idea, and I'm a little
surprised we don't do this already. Patch welcome, if this is what you
decide to do.

Richard.

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Rafael J. Wysocki]

On Thursday, October 21, 2010, Sitsofe Wheeler wrote:
> On Thu, Oct 21, 2010 at 09:57:47PM +0200, Rafael J. Wysocki wrote:
> > On Thursday, October 21, 2010, Sitsofe Wheeler wrote:
> > > 
> > > It's a shame the previous changes didn't work as they stopped a buggy
> > > upower using the -1 value (and producing a nonsense rate like 8.4e-06)
> > 
> > Hmm.  So upower _doesn't_ handle -1?  What does it do with -1000, then?
> 
> It can't handle that either and outputs a nonsense rate like 0.0084.
> Looking at the code, it would take a very strange value for it to realise
> it is handling a special value as it does arithmetic on the sysfs value
> before doing its check:
> 
> 	/* get rate; it seems odd as it's either in uVh or uWh */
> 	energy_rate = fabs (sysfs_get_double (native_path, "current_now") / 1000000.0);
> 
> 	/* convert charge to energy */
> 	if (energy == 0) {
> 		energy = sysfs_get_double (native_path, "charge_now") / 1000000.0;
> 		if (energy == 0)
> 			energy = sysfs_get_double (native_path, "charge_avg") / 1000000.0;
> 		energy *= voltage_design;
> 		energy_rate *= voltage_design;
> 	}
> 
> 	/* some batteries don't update last_full attribute */
> 	if (energy > energy_full) {
> 		egg_warning ("energy %f bigger than full %f", energy, energy_full);
> 		energy_full = energy;
> 	}
> 
> 	/* present voltage */
> 	voltage = sysfs_get_double (native_path, "voltage_now") / 1000000.0;
> 	if (voltage == 0)
> 		voltage = sysfs_get_double (native_path, "voltage_avg") / 1000000.0;
> 
> 	/* ACPI gives out the special 'Ones' value for rate when it's unable
> 	 * to calculate the true rate. We should set the rate zero, and wait
> 	 * for the BIOS to stabilise. */
> 	if (energy_rate == 0xffff)
> 		energy_rate = 0;
> 
> By the time the comparison against energy_rate is done the original
> sysfs value has at _least_ divided by 1000000.0 and made positive. Hence
> the test program in my first mail where I mention that 0xfffff produced
> 65535.000000, fabs(-1000 / 1000000.0) produced 0.001000 and fabs(-1 /
> 1000000.0) produces 0.000001. That's also assuming it doesn't wind up
> multiplying the previous value by voltage_design...
> 
> > > but it's not clear which part of the stack can't handle -ENODATA
> > > perhaps it is another part of the kernel?
> > 
> > I don't really think it's a part of the kernel.
> 
> How do I find out which part is not producing those sysfs nodes?
> 
> > > Richard, any chance of upower being changed to test for -1 before doing
> > > doing anything with current_now (
> > > http://cgit.freedesktop.org/DeviceKit/upower/tree/src/linux/up-device-supply.c?id=5387183d53c16a987a0737c1bdec1b62edf3daa6#n561)?
> > > I guess there are a whole bunch of other attributes that could
> > > theoretically be -1 and shouldn't be used if they return it...
> > 
> > If user space doesn't handle -1 correctly too, I think the right approach for
> > us should be to use the previous version of the patch and return error code
> > for unknown values.
> 
> So long as sysfs can be made to work properly I am in agreement.

OK, so can you test the patch below, please?

Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: ACPI / Battery: Return -ENODEV for unknown values in get_property()

The function acpi_battery_get_property() is called by the
power supply framework's function power_supply_show_property()
implementing the sysfs interface for power supply devices as the
ACPI battery driver's ->get_property() callback.  Thus it is supposed
to return error code if the value of the given property is unknown.
Unfortunately, however, it returns 0 in those cases and puts a
wrong (negative) value into the intval field of the
union power_supply_propval object provided by
power_supply_show_property().  In consequence, wron negative
values are read by user space from the battery's sysfs files.

Fix this by making acpi_battery_get_property() return -ENODEV
for properties with unknown values (-ENODEV is returned, because
power_supply_uevent() returns with error for any other error code
returned by power_supply_show_property()).

Reported-by: Sitsofe Wheeler <sitsofe@yahoo.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/battery.c |   35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/acpi/battery.c
===================================================================
--- linux-2.6.orig/drivers/acpi/battery.c
+++ linux-2.6/drivers/acpi/battery.c
@@ -186,6 +186,7 @@ static int acpi_battery_get_property(str
 				     enum power_supply_property psp,
 				     union power_supply_propval *val)
 {
+	int ret = 0;
 	struct acpi_battery *battery = to_acpi_battery(psy);
 
 	if (acpi_battery_present(battery)) {
@@ -214,26 +215,44 @@ static int acpi_battery_get_property(str
 		val->intval = battery->cycle_count;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
-		val->intval = battery->design_voltage * 1000;
+		if (battery->design_voltage == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODEV;
+		else
+			val->intval = battery->design_voltage * 1000;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		val->intval = battery->voltage_now * 1000;
+		if (battery->voltage_now == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODEV;
+		else
+			val->intval = battery->voltage_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 	case POWER_SUPPLY_PROP_POWER_NOW:
-		val->intval = battery->rate_now * 1000;
+		if (battery->rate_now == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODEV;
+		else
+			val->intval = battery->rate_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 	case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
-		val->intval = battery->design_capacity * 1000;
+		if (battery->design_capacity == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODEV;
+		else
+			val->intval = battery->design_capacity * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
 	case POWER_SUPPLY_PROP_ENERGY_FULL:
-		val->intval = battery->full_charge_capacity * 1000;
+		if (battery->full_charge_capacity == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODEV;
+		else
+			val->intval = battery->full_charge_capacity * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_NOW:
 	case POWER_SUPPLY_PROP_ENERGY_NOW:
-		val->intval = battery->capacity_now * 1000;
+		if (battery->capacity_now == ACPI_BATTERY_VALUE_UNKNOWN)
+			ret = -ENODEV;
+		else
+			val->intval = battery->capacity_now * 1000;
 		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = battery->model_number;
@@ -245,9 +264,9 @@ static int acpi_battery_get_property(str
 		val->strval = battery->serial_number;
 		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
-	return 0;
+	return ret;
 }
 
 static enum power_supply_property charge_battery_props[] = {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Sitsofe Wheeler]

On Sat, Oct 23, 2010 at 12:19:13AM +0200, Rafael J. Wysocki wrote:
> 
> OK, so can you test the patch below, please?

The latest patch seems to fix/workaround the problem. upower now reports
0 as the energy rate, there are no warnings in dmesg and battery hotplug
works. Looks good and there's the option for a future upower to
interpret the missing sysfs as meaning "unknown".

Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>

> The function acpi_battery_get_property() is called by the
> power supply framework's function power_supply_show_property()
> implementing the sysfs interface for power supply devices as the
> ACPI battery driver's ->get_property() callback.  Thus it is supposed
> to return error code if the value of the given property is unknown.
> Unfortunately, however, it returns 0 in those cases and puts a
> wrong (negative) value into the intval field of the
> union power_supply_propval object provided by
> power_supply_show_property().  In consequence, wron negative

wron -> wrong?

> Fix this by making acpi_battery_get_property() return -ENODEV
> for properties with unknown values (-ENODEV is returned, because
> power_supply_uevent() returns with error for any other error code
> returned by power_supply_show_property()).

OK that's sneaky and clever - technically power_supply_uevent should be
more robust but presumably things are already prepared to handle -ENODEV
so overloading the meaning leads to the smallest change.

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Sitsofe Wheeler]

On Fri, Oct 22, 2010 at 01:31:28PM +0100, Richard Hughes wrote:
> On 21 October 2010 17:54, Sitsofe Wheeler <sitsofe@yahoo.com> wrote:
> > I guess there are a whole bunch of other attributes that could
> > theoretically be -1 and shouldn't be used if they return it...
> 
> I think checking for <0 is probably a good idea, and I'm a little
> surprised we don't do this already. Patch welcome, if this is what you
> decide to do.

I can only guess that at some point in upower's past negative values for
current_rate were found to be valid so upower took the route of making
them absolute to work around that behaviour. If so, it would be good to
know whether there are still devices in this category running a stock
kernel.

If the latest patch to return -ENODEV goes in, then there's the
possibility for upower to detect the unknown state and report unknown
back to its users. Would the existing interfaces support outputting
unknown instead of a number? If not (and there are no plans to) I
suspect the best thing to do is to remove the test for 0xffff and
continue to return 0.

-- 
Sitsofe | http://sucs.org/~sits/

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Rafael J. Wysocki]

On Saturday, October 23, 2010, Sitsofe Wheeler wrote:
> On Sat, Oct 23, 2010 at 12:19:13AM +0200, Rafael J. Wysocki wrote:
> > 
> > OK, so can you test the patch below, please?
> 
> The latest patch seems to fix/workaround the problem. upower now reports
> 0 as the energy rate, there are no warnings in dmesg and battery hotplug
> works. Looks good and there's the option for a future upower to
> interpret the missing sysfs as meaning "unknown".
> 
> Tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>

Thanks for testing!

> > The function acpi_battery_get_property() is called by the
> > power supply framework's function power_supply_show_property()
> > implementing the sysfs interface for power supply devices as the
> > ACPI battery driver's ->get_property() callback.  Thus it is supposed
> > to return error code if the value of the given property is unknown.
> > Unfortunately, however, it returns 0 in those cases and puts a
> > wrong (negative) value into the intval field of the
> > union power_supply_propval object provided by
> > power_supply_show_property().  In consequence, wron negative
> 
> wron -> wrong?

Sure, thanks.

> > Fix this by making acpi_battery_get_property() return -ENODEV
> > for properties with unknown values (-ENODEV is returned, because
> > power_supply_uevent() returns with error for any other error code
> > returned by power_supply_show_property()).
> 
> OK that's sneaky and clever - technically power_supply_uevent should be
> more robust but presumably things are already prepared to handle -ENODEV

Yes, they are.  That's why I decided to use it. :-)

> so overloading the meaning leads to the smallest change.

That's correct.

I'll repost the patch shortly with fixed changelog and your tested-by.

Thanks,
Rafael

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Pavel Machek]

Hi!

> That's why I suggested to use -ENODATA. :-)
> 
> Still, if user space has problems with failing reads from the sysfs attributes,
> it may be better to simply put -1 in there.  Patch is appended, please test.

I'd say that reporting -1 for unknown is ugly. You can have -1A
current easily (charging at 1A), and I've seen machines reporting <0
current -- when charging. Logical and well-defined.

								Pavel


>  	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> -		val->intval = battery->design_voltage * 1000;
> +		if (battery->design_voltage != ACPI_BATTERY_VALUE_UNKNOWN)
> +			val->intval = battery->design_voltage * 1000;
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> -		val->intval = battery->voltage_now * 1000;
> +		if (battery->voltage_now != ACPI_BATTERY_VALUE_UNKNOWN)
> +			val->intval = battery->voltage_now * 1000;
>  		break;
>  	case POWER_SUPPLY_PROP_CURRENT_NOW:
>  	case POWER_SUPPLY_PROP_POWER_NOW:
> -		val->intval = battery->rate_now * 1000;
> +		if (battery->rate_now != ACPI_BATTERY_VALUE_UNKNOWN)
> +			val->intval = battery->rate_now * 1000;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>  	case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> -		val->intval = battery->design_capacity * 1000;
> +		if (battery->design_capacity != ACPI_BATTERY_VALUE_UNKNOWN)
> +			val->intval = battery->design_capacity * 1000;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_FULL:
>  	case POWER_SUPPLY_PROP_ENERGY_FULL:
> -		val->intval = battery->full_charge_capacity * 1000;
> +		if (battery->full_charge_capacity != ACPI_BATTERY_VALUE_UNKNOWN)
> +			val->intval = battery->full_charge_capacity * 1000;
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_NOW:
>  	case POWER_SUPPLY_PROP_ENERGY_NOW:
> -		val->intval = battery->capacity_now * 1000;
> +		if (battery->capacity_now != ACPI_BATTERY_VALUE_UNKNOWN)
> +			val->intval = battery->capacity_now * 1000;
>  		break;
>  	case POWER_SUPPLY_PROP_MODEL_NAME:
>  		val->strval = battery->model_number;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] ACPI / Battery: Return -ENODATA for unknown values in get_property()

[Rafael J. Wysocki]

On Monday, October 25, 2010, Pavel Machek wrote:
> Hi!
> 
> > That's why I suggested to use -ENODATA. :-)
> > 
> > Still, if user space has problems with failing reads from the sysfs attributes,
> > it may be better to simply put -1 in there.  Patch is appended, please test.
> 
> I'd say that reporting -1 for unknown is ugly. You can have -1A
> current easily (charging at 1A), and I've seen machines reporting <0
> current -- when charging. Logical and well-defined.

I posted a patch fixing that in a different way (makng the battery driver return
-ENODEV instead of -ENODATA) in the meantime, which has been confirmed to work.

Thanks,
Rafael