Returning ACPI_BATTERY_VALUE_UNKNOWN to userspace

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?

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

Quoted 3 lines · Rafael J. Wysocki

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.

Quoted 7 lines · Henrique de Moraes Holschuh

Quoted 3 lines · Rafael J. Wysocki

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

Quoted 5 lines · Sitsofe Wheeler

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

Quoted 3 lines

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?

Quoted 11 lines · Henrique de Moraes Holschuh

Quoted 5 lines · Sitsofe Wheeler

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.

Quoted 5 lines

Quoted 3 lines

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.

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.

Quoted 4 lines · Rafael J. Wysocki

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 <[email protected]>

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

Quoted 3 lines · Sitsofe Wheeler

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?

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 6 lines · Rafael J. Wysocki

Quoted 3 lines · Sitsofe Wheeler

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

Quoted 4 lines

Quoted 2 lines

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?

Quoted 6 lines

Quoted 5 lines

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

Quoted 3 lines

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?

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

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

Quoted 2 lines · Rafael J. Wysocki

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 <[email protected]>

Quoted 9 lines

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?

Quoted 4 lines

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.

I guess there are a whole bunch of other attributes that could theoretically be -1 and shouldn't be used if they return it...

Quoted 2 lines · Sitsofe Wheeler

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.

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.

drivers/acpi/battery.c +12 -7

 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi!

Quoted 4 lines

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.