Date: Wed, 23 Jun 2004 12:40:40 -0700 (PDT) From: Nate Lawson <nate@root.org> To: "M. Warner Losh" <imp@bsdimp.com> Cc: acpi@freebsd.org Subject: Re: apm problem Message-ID: <20040623123827.O86825@root.org> In-Reply-To: <20040616.135044.85075412.imp@bsdimp.com> References: <20040616171408.0f88c928.liamfoy@sepulcrum.org> <20040616.135044.85075412.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 16 Jun 2004, M. Warner Losh wrote:
> 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.
I agree with this. I'd like to use ~0 instead of (u_int)-1. Up to you
though. Please commit.
> 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);
Please commit this patch after deciding whether you want to go with ~0 or
(u_int)-1.
-Nate
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040623123827.O86825>