Date: Mon, 14 Jun 2004 12:46:13 -0700 (PDT) From: Nate Lawson <nate@root.org> To: "M. Warner Losh" <imp@bsdimp.com> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/usr.sbin/apm apm.c Message-ID: <20040614123620.E21496@root.org> In-Reply-To: <20040614.121924.35506950.imp@bsdimp.com> References: <20040614165326.BF0CF16A4F1@hub.freebsd.org> <20040614103813.I20782@root.org> <20040614.121924.35506950.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 14 Jun 2004, M. Warner Losh wrote:
> In message: <20040614103813.I20782@root.org>
> Nate Lawson <nate@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@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040614123620.E21496>