Date: Wed, 16 Jun 2004 13:50:44 -0600 (MDT) From: "M. Warner Losh" <imp@bsdimp.com> To: liamfoy@sepulcrum.org Cc: acpi@freebsd.org Subject: Re: apm problem Message-ID: <20040616.135044.85075412.imp@bsdimp.com> In-Reply-To: <20040616171408.0f88c928.liamfoy@sepulcrum.org> References: <20040616171408.0f88c928.liamfoy@sepulcrum.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <20040616171408.0f88c928.liamfoy@sepulcrum.org>
Liam Foy <liamfoy@sepulcrum.org> writes:
: >---
: >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.
: >---
: >
: >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.
: >
: >-Nate
:
: Hey guys,
:
: I have been looking into this problem, and removing possibilites. I am
: although stuck at one point. I think I have narrowed this problem down to what
: bioscall() is returning to apm_get_pwstatus() in /usr/src/sys/i386/bios/apm.c.
: If -1 is returned as it should then apm_get_pwstatus() and apm_get_info() will
: work fine, thus filling the struct correctly. I although cannot understand
: what is possibly being returned in order for apm_get_pwstatus() to fill the
: stucture with a -1 when infact it should be 255. Either am looking at the
: wrong thing or this bug doesnt like us. Anyone been looking into this bug much
: themselves, or have an ideas I can look into ?
Short answer: I think Nate is confused. I posted an extensive review
on cvs-all@ recently.
Longer answer: There are two fields that the apm driver is setting to
-1. One of them is ap_batt_time. This one is completely legitimate.
It is an int. The kernel translates the value it gets from the APM
BIOS into seconds (it is either seconds or minutes depending on what
bits are set). acpi's emulation of apm is completely correct here.
The second field is ai_batteries. We set this to -1 to mean unknown.
This is converted to MAX_UINT because it is assigned to an u_int.
Arguably, this is a minor bug in the APM code. It is the interface,
however, and could easily be preserved by changing it to 0xffffffff.
There have been some recent changes to apm(8) that bogusly change the
check from -1 to 255 for this field. These changes are wrong, and the
original code is correct. However, gcc appears to hate the construct
(u_int) -1, so the correct fix is to change that to MAX_UINT. acpi's
emulation sets this to 0 incorrectly.
There's a third field that causes some confusion as well.
ai_capabilities is set to 0xff00 when apm cannot determine things.
This is outside of the APM BIOS specification (the upper bits are
something I made up to signal to apm(8) to not display the
cababilities because they were unknown). acpi's emulation gets this
one correct.
As it relates to acpi, however, there is one bug. First in
acpi_capm_get_info(), if we can't get the battery info, we do:
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 {
instead, this should be:
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 = -1; /* Unknown */ [*]
} else {
[*] or 0xffffffff instead of -1. 0 is clearly wrong, since it means
no batteries, not the number of batteries cannot be determined.
Finally, apm(8) needs the following patch, imho. If drivers are
producing results that produce bad things, they should be fixed, not
apm(8). I don't think that anything does actually produce them.
Index: apm.c
===================================================================
RCS file: /cache/ncvs/src/usr.sbin/apm/apm.c,v
retrieving revision 1.32
diff -u -r1.32 apm.c
--- apm.c 27 May 2004 19:23:27 -0000 1.32
+++ apm.c 16 Jun 2004 19:45:52 -0000
@@ -34,6 +34,8 @@
#define APMDEV "/dev/apm"
+#define APM_UNKNOWN 0xff /* Unknown in APM BIOS spec */
+
#define xh(a) (((a) & 0xff00) >> 8)
#define xl(a) ((a) & 0xff)
#define APMERR(a) xh(a)
@@ -156,7 +158,7 @@
printf("APM version: %d.%d\n", aip->ai_major, aip->ai_minor);
printf("APM Management: %s\n", aip->ai_status ? "Enabled" : "Disabled");
printf("AC Line status: ");
- if (aip->ai_acline >= 255)
+ if (aip->ai_acline == APM_UNKNOWN)
printf("unknown");
else if (aip->ai_acline > 1)
printf("invalid value (0x%x)", aip->ai_acline);
@@ -164,7 +166,7 @@
printf("%s", line_msg[aip->ai_acline]);
printf("\n");
printf("Battery status: ");
- if (aip->ai_batt_stat >= 255)
+ if (aip->ai_batt_stat == APM_UNKNOWN)
printf("unknown");
else if (aip->ai_batt_stat > 3)
printf("invalid value (0x%x)", aip->ai_batt_stat);
@@ -172,7 +174,7 @@
printf("%s", batt_msg[aip->ai_batt_stat]);
printf("\n");
printf("Remaining battery life: ");
- if (aip->ai_batt_life >= 255)
+ if (aip->ai_batt_life == APM_UNKNOWN)
printf("unknown\n");
else if (aip->ai_batt_life <= 100)
printf("%d%%\n", aip->ai_batt_life);
@@ -194,7 +196,7 @@
}
if (aip->ai_infoversion >= 1) {
printf("Number of batteries: ");
- if (aip->ai_batteries >= 255)
+ if (aip->ai_batteries == (u_int) -1)
printf("unknown\n");
else {
u_int i;
@@ -208,12 +210,12 @@
continue;
printf("Battery %d:\n", i);
printf("\tBattery status: ");
- if (aps.ap_batt_flag <= 255 &&
+ if (aps.ap_batt_flag != APM_UNKNOWN &&
(aps.ap_batt_flag & APM_BATT_NOT_PRESENT)) {
printf("not present\n");
continue;
}
- if (aps.ap_batt_stat >= 255)
+ if (aps.ap_batt_stat != APM_UNKNOWN)
printf("unknown\n");
else if (aps.ap_batt_stat > 3)
printf("invalid value (0x%x)\n",
@@ -222,7 +224,7 @@
printf("%s\n",
batt_msg[aps.ap_batt_stat]);
printf("\tRemaining battery life: ");
- if (aps.ap_batt_life >= 255)
+ if (aps.ap_batt_life == (u_int)-1)
printf("unknown\n");
else if (aps.ap_batt_life <= 100)
printf("%d%%\n", aps.ap_batt_life);
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040616.135044.85075412.imp>