From owner-cvs-all@FreeBSD.ORG Mon Jun 14 20:50:13 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 42CC016A4CE; Mon, 14 Jun 2004 20:50:13 +0000 (GMT) Received: from harmony.village.org (rover.village.org [168.103.84.182]) by mx1.FreeBSD.org (Postfix) with ESMTP id 6CD3043D48; Mon, 14 Jun 2004 20:50:12 +0000 (GMT) (envelope-from imp@bsdimp.com) Received: from localhost (warner@rover2.village.org [10.0.0.1]) by harmony.village.org (8.12.11/8.12.11) with ESMTP id i5EKkwg5055969; Mon, 14 Jun 2004 14:46:58 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Mon, 14 Jun 2004 14:47:08 -0600 (MDT) Message-Id: <20040614.144708.43002800.imp@bsdimp.com> To: nate@root.org From: "M. Warner Losh" In-Reply-To: <20040614123620.E21496@root.org> References: <20040614103813.I20782@root.org> <20040614.121924.35506950.imp@bsdimp.com> <20040614123620.E21496@root.org> X-Mailer: Mew version 3.3 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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 20:50:13 -0000 In message: <20040614123620.E21496@root.org> Nate Lawson writes: : 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: I understand that this is the APM BIOS specification. However, the APM BIOS specification is not exactly the same thing as the ioctl interface that FreeBSD provides. : 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 This is for the ap_batt_stat field, which is already handled properly. : BL = Battery status : 00H High : 01H Low : 02H Critical : 03H Charging : FFH = Unknown : ^^^^^^^^^^^^^ : All other values reserved This is for the ap_acline field, which is already handeld properly. : 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. We're talking about the ap_batt_time and and the ai_batteries fields only. In the case of ap_batt_time, it is an int. >= 0 is the number of seconds that is remaining. < 0 is unknown. In the ai_batteries case, 0xffffffff is correct because that's what the FreeBSD api defines it to be, rightly or wrongly. It is not the case that there is an error here. That's the part of the definition we cannot change. Therefore, the 1.32 change to apm.c that tried to assume that ai_batteries was a byte was incorrect. That is simply not the interface that FreeBSD provides. I understand you think we should return 0xff in this case, but I contend that is a change to the ABI and API which gives us no real benefit. That's not to say that maybe there could be a better interface, but small changes like the ones you propose aren't likely worth the problem. : > 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. Actually, the check is wrong. casted -1 is correct. Alternatively, checking against 0xffffffff. The two are the same thing for the systems we're talking about, and the conversion is a perfectly legal and well defined operation in all versions of C. We can't change the kernel drivers to use 0xff because that breaks the ABI. Third party consumers of this interface are already checking against -1, so to change it to 0xff would break them for very little gain. I do understand what you are proposing, I'm telling you that it too is an ABI change. : > 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. No it should not. It is an 'int' which can store -1 perfectly well. Also, 0xff is a valid amount of time left on the batteries. This field is defined to be an integer number of seconds left in the battery life. The APM BIOS defined it differently (if the top bit is set, then it is in minutes, not seconds) and the kernel converts it to the right number of seconds. 0xffff would also be bad, because you can have batteries that last longer that 65534 seconds in the APM standard (it allows for lives up to 1966020 seconds). : > 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. This is only minorly bogus. Changing it to 0xffffffff would be 100% ABI compatible, so this is a very minor nit. : > 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: I'm saying that the range of values for ai_capabilities is larger than those specified in the standard, and that 0xff in the high 8 bits means "This field is not relevant" either because the APM version was too small to report this information, or because the thing that's emulating it isn't there at all. ... : So setting batteries = 0 seems correct but capabilities should only be an : 8 bit value (0xff max). What do you mean by 0xff00? 0xff00 means 'I don't know' in the FreeBSD APM interface. It is a special value, outside of the APM spec, designed to signal "I couldn't even ask this question." apm(8) once upon a time used this to not even display the fields in question. The other changes in 1.32 are fine, but the one that changed the check on ai_batteries to >= 255 for ai_batteries is incorrect and should be reverted. Warner