From owner-cvs-all@FreeBSD.ORG Mon Jun 14 19:46:48 2004 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 94E4616A4CE for ; Mon, 14 Jun 2004 19:46:48 +0000 (GMT) Received: from root.org (root.org [67.118.192.226]) by mx1.FreeBSD.org (Postfix) with SMTP id 6879E43D31 for ; Mon, 14 Jun 2004 19:46:48 +0000 (GMT) (envelope-from nate@root.org) Received: (qmail 21694 invoked by uid 1000); 14 Jun 2004 19:46:13 -0000 Date: Mon, 14 Jun 2004 12:46:13 -0700 (PDT) From: Nate Lawson To: "M. Warner Losh" In-Reply-To: <20040614.121924.35506950.imp@bsdimp.com> Message-ID: <20040614123620.E21496@root.org> References: <20040614165326.BF0CF16A4F1@hub.freebsd.org> <20040614103813.I20782@root.org> <20040614.121924.35506950.imp@bsdimp.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: mux@FreeBSD.org cc: cvs-src@FreeBSD.org cc: src-committers@FreeBSD.org cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/usr.sbin/apm apm.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Jun 2004 19:46:48 -0000 On Mon, 14 Jun 2004, M. Warner Losh wrote: > In message: <20040614103813.I20782@root.org> > Nate Lawson 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 > : > > : > 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