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

[Nate Lawson]

On Mon, 14 Jun 2004, M. Warner Losh wrote:
> 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':

This is from APM v1.2, section 4.6.11:

If function successful:
Carry	=	0
BH	=	AC line status
		00H  Off-line
		01H  On-line
		02H  On backup power
		FFH = Unknown
                ^^^^^^^^^^^^^
		All other values reserved
BL	=	Battery status
		00H  High
		01H  Low
		02H  Critical
		03H  Charging
		FFH = Unknown
                ^^^^^^^^^^^^^
		All other values reserved

These are most certainly byte values since BH and BL are 8 bit registers.
Note that the spec defines them in terms of unsigned value (0xff).  Also,
note that other values are reserved.  This makes our sign extension of
0xffffffff incorrect since that is a reserved value.

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

The check is not optimal, which is why I was asking mux@ (if he wishes) to
change the kernel drivers to specify 0xff instead of -1 for unknown
values.  Just to be clear, I am _not_ asking him to change the binary
interface from u_int to uint8_t since that would break ABI and all 3rd
party battery stat apps.

> ap_batt_time already is an int, and is correct before and after the
> changes.

I left that one but it should also be set to 0xff.

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

Both apm and acpi kernel drivers do the wrong thing by storing -1 in a
u_int.

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

I don't understand what you mean.  In 4.6.17, these are defined as
follows:

Returns
	If function successful:
	Carry	=	0	APM is supported by BIOS
	BL	=	Number of battery units this machine supports. A
	value of 0 indicates this machine does not have any system batteries.
	Most systems will return 0, 1, or 2.
	CX	=	Capability flags
			Bit 0 = 1  System can enter global standby state.
			Indicates BIOS will post standby and standby-resume
			events.
			Bit 1 = 1  System can enter global suspend state.
			Indicates BIOS will post suspend and suspend-resume
			events.
			Bit 2 = 1  Resume timer will wake up from standby.
			Bit 3 = 1  Resume timer will wake up from suspend.
			Bit 4 = 1  Resume on ring indicator (internal COM
			or modem) will wake up from standby.
			Bit 5 = 1  Resume on ring indicator (internal COM
			or modem) will wake up from suspend.
			Bit 6 = 1  PCMCIA Ring indicator will wake up from
			standby.
			Bit 7 = 1  PCMCIA Ring indicator will wake up from
			suspend.

So setting batteries = 0 seems correct but capabilities should only be an
8 bit value (0xff max).  What do you mean by 0xff00?

-Nate