cvs commit: src/usr.sbin/apm apm.c

[M. Warner Losh]

In message: <20040614123620.E21496 at root.org>
            Nate Lawson <nate at root.org> writes:
: On Mon, 14 Jun 2004, M. Warner Losh wrote:
: > In message: <20040614103813.I20782 at root.org>
: >             Nate Lawson <nate at root.org> 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 <liamfoy at sepulcrum.org>
: > : >
: > : >   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