cvs commit: src/usr.sbin/apm apm.c

[M. Warner Losh]

In message: <20040614103813.I20782 at root.org>
            Nate Lawson <nate at root.org> writes:
: On Mon, 14 Jun 2004, Maxime Henrion wrote:
: > mux         2004-06-14 16:53:20 UTC
: >
: >   FreeBSD src repository
: >
: >   Modified files:
: >     usr.sbin/apm         apm.c
: >   Log:
: >   Factor out some duplicated code and fix some style(9) issues.
: >
: >   Submitted by:   Liam J. Foy <liamfoy at sepulcrum.org>
: >
: >   Revision  Changes    Path
: >   1.33      +61 -68    src/usr.sbin/apm/apm.c
: 
: This looks fine.  If you get a chance, it might be nice if you'd look into
: this task:
: 
: ---
: Fix drivers and the apm compat interface -- Currently, the apm compat
: interface expects byte values but the ABI used is a set of u_ints and an
: int. Either the apm or acpi battery drivers (or both) are setting the
: value to -1, which results in 0xffffffff being passed back as the current
: state. Really, only 255 should be returned in this case. The apm userland
: utility marks values >= 255 as "unknown" to work around this. But really
: the underlying drivers should be fixed.

This seems bogus based on what I know of the real APM interface.  The
APM interface doesn't have byte values.  The underlying drivers are
historically correct.

: ---
: Note that we can't change the ABI but fixing the sign-extension issue when
: the kernel drivers fill out the structures would be helpful.

apm has traditionally set the values to -1 for 'don't care':

...
	if (sc->bios.r.edx == 0xffff)	/* Time is unknown */
		app->ap_batt_time = -1;
...
	if (apm_bioscall()) {
		aip->ai_batteries = -1;	/* Unknown */
		aip->ai_capabilities = 0xff00; /* Unknown, with no bits set */
	} else {
...

Since apm in the kernel defines the apm interface, people emulating it
should conform to that interface.  apm(8) was bogusly broken in 1.32
to change the comparison against -1 to >= 255.  Prior to that, it did
the right thing because it checked ai_batteries against a cast -1.
ap_batt_time already is an int, and is correct before and after the
changes.

We can change this to be 0xffffffff, of course, but apm(1) has to cope
(and at one time did cope correctly) with this interface.  If the apci
emulation code isn't doing the right thing, it should be change to
return a compatible value.

	if (acpi_battery_get_battinfo(-1, &batt)) {
		aip->ai_batt_stat = 0xff;	/* unknown */
		aip->ai_batt_life = 0xff;	/* unknown */
		aip->ai_batt_time = -1;		/* unknown */
-		aip->ai_batteries = 0;
	} else {

should really be:

+		aip->ai_batteries = -1;
+		aip->ai_capabilities = 0xff00;

to properly emulate the APM interface.

Warner