apm problem

[Liam J. Foy]

On Wed, 23 Jun 2004 12:40:40 -0700 (PDT)
Nate Lawson <nate at root.org> wrote:

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

We decided to go with -1. The apm.c patch currently will not apply due to
the re-structure of apm. The following patch will:

http://liamfoy.kerneled.org/apm.diff

After more digging, apm -l should return 255(stated in man page and acpi spec).
The following patch will make it work:

--- /usr/src/sys/dev/acpica/acpi_cmbat.c	Tue Jun 22 16:40:35 2004
+++ /hd2/acpi_cmbat.c	Tue Jun 22 17:02:18 2004
@@ -449,7 +449,7 @@
     static int	bat_units = 0;
     static struct acpi_cmbat_softc **bat = NULL;
 
-    cap = min = -1;
+    cap = min = 255;
     batt_stat = ACPI_BATT_STAT_NOT_PRESENT;
     error = 0;
 
@@ -545,7 +545,7 @@
 
     /* Battery life */
     if (valid_units == 0) {
-	cap = -1;
+	cap = 255;
 	batt_stat = ACPI_BATT_STAT_NOT_PRESENT;
     } else {
 	cap = total_cap / valid_units;
@@ -649,7 +649,7 @@
     }
 
     if (!sc->present) {
-	battinfo->cap = -1;
+	battinfo->cap = 255;
 	battinfo->min = -1;
 	battinfo->state = ACPI_BATT_STAT_NOT_PRESENT;
     } else {

I think all the patches need commiting.

> 
> -Nate
> _______________________________________________
> freebsd-acpi at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
> To unsubscribe, send any mail to "freebsd-acpi-unsubscribe at freebsd.org"


-- 
-Liam J. Foy
http://liamfoy.kerneled.org
"Now I wish it would rain down on me"