svn commit: r223989 - head/sys/dev/usb/input

[Bruce Evans]

On Wed, 20 Jul 2011, Hans Petter Selasky wrote:

> On Wednesday 20 July 2011 12:37:44 Andriy Gapon wrote:
>>> This special code is a workaround. The problem is that when not polling
>>> the  key presses will be fed into syscons I think, and not returned via
>>> the polling function. That's why there is a timer there to distinguish
>>> when polling starts and polling stops. It is not enough just to
>>> enable/disable polling around each key-press like currently done.
>>
>> I must admit that I failed to understand this paragraph, so I think that I
>> should shut up until I know the relevant code better.
>
> Try and see for yourself, maybe you will understand it then? Remove the
> kbd_active == 0 branch in the ukbd.c. Then get into the mount-root prompt.
> Then start pressing keys. You will see that some keys are captured and some
> are not. This is because two clients are listening for keys.

The low-level console driver was fundamentally broken by the
multiple-console changes in 2001 (and was a little broken before then),
except that these changes didn't affect releases until much later
(RELENG_4 doesn't have the breakage).  Loss of keystrokes at the
mountroot prompt is exactly as expected, since there is often an
interrupt handler listening for them, and it is exactly as expected
that the interrupt handler wins the race to read them some of the time.
The proportion of wins depends on the driver, and for some reason the
syscons AT keyboard interrupt handler rarely wins so the bug is not
so noticeable with it.  It is especially noticeable for the sio driver
for a technical reason not related to the interrupt handler reason
(see below).

The low-level console driver should switch to polling mode to read the
keystrokes.  However, its API is broken as designed, so it can only
[tell the driver level to] do this around each poll for a character.
Polling involves polling each possible console in turn, so polling mode
must be left after unsuccessfully polling for a short time so as to
get to the next console.  This is now done even if there is only 1
console.  Before the multiple-console changes, polling mode was only
left after _successfully_ polling for as long as necessary to read
a character, so things mostly worked -- the race was only lost if
a character arrives in between polls, and you can't type fast enough
for the time between polls to be significant compared with the time
in polls.

The multiple-console changes left the driver interface function cn_getc()
unused.  cn_getc() switches into polling mode and then polls for as
long as necessary to read a character, and of course it doesn't switch
out of polling mode after each poll.  This doesn't work for multiple
consoles, at least if more than 1 console is active, since it doesn't
return until one instance of it succeeds.  So the polling loop was
moved out of the driver layer, even when only 1 console is active.
The existing interface cn_checkc() was used to poll each console in
turn.  This interface was always broken as designed, but it was used
only during panic() to check for a key to abort the dump and the
timeout, so the brokenness was little noticed (and if panic() weren't
broken too, it would stop all interrupt handlers and other listeners).

cn_getc() should have been used to keep the case of 1 active console
working until the API is fixed.

Next, starting in 2006 the API was changed to be even more broken than
as designed, by removing the working cn_getc() interface from it but
keeping the non-working cn_checkc() interface, and obfuscating this
change by renaming cn_checkc to cn_getc.  Driver functions were also
renamed, so now the only instances of the *checkc in drivers are in
dcons.  The higher-level functions cngetc() and cncheckc() still
have their old names, so they no longer match the lower-level function
names.

A non-broken API needs cn_open() and cn_close() functions which would
normally switch the driver in an out of polling mode.  Given these
interfaces easy to fix the per-character poll to work as well as before
the multiple console changes, including for multiple active consoles.
Just call cn_open() and cn_close() on every active console around the
whole polling loop.  A little more is required to prevent races between
characters, and to avoid the races inherent in the cn_checkc() API.
For multi-char input like that at the mountroot prompt, calling
cn_open() and cn_close() around the loop in gets(9) is adequate.  The
functions should be almost no-ops when called nested for things like
this.

     BTW, gets(9) is bogusly named.  It is not harmful like gets(3),
     since it takes a buffer size arg.  It is used approximately once,
     for mountroot input, so renaming it would be easy.  Perhaps it
     should be named cn_gets() and be implemented closer to the console
     driver, or be implemented closer to printf() (it is now in libkern).

For debugger entry and panics, the whole operation should be wrapped
by cn_open()/cn_close().  This covers most cases.  Some console drivers
now sort of work in debugger mode by abusing the kdb_active variable,
or because debugger entry stops interrupts and other CPUs.

I started fixing this in FreeBSD in 2000 (I got yokota to do most of the
work, especially in syscons).  There was a cn_dbctl() interface which
was intended to be used like ioctl() (it allows any number of
sub-functions) but was only used for operations like open() and close().
This was called on debugger entry and exit to inform console drivers
that the should switch to polling mode for things like keyboards and
do anything else necessary to run in debugger mode.  syscons, but no
other driver, used this, mainly to avoid hacking on the kdb_active
flag.

     There should be significant differences, but were only small ones
     in practice, between being in debugger mode and being in polling
     mode.  For example, entering console i/o mode for syscons should
     involve switching the video mode and perhaps the frame buffer to
     a special one, in case the current one is unusable for some reason
     (it might be controlled by X, or in the middle of an initialization,
     or you might just want to avoid scribbling on its frame buffer).
     Thus, entering console i/o mode might be an extemely heavyweight
     operation.  You don't want to do it on every entry to debugger mode.
     Even if the switch is very fast, it would make the screen flicker
     to switch the frame buffer on every entry to the debugger for things
     like tracing (but not displaying) every instruction when single
     stepping using 'n' in ddb.

I missed the need for the cn_open() and cn_close() functions when I
added cn_dbctl().  cn_dbctl() sounds debugger-specific and was intended
to be so.  I had forgotten that it was based on the console driver in
my x86 debugger which was written and used mainly between just before
FreeBSD existed.  The debugger has functions for open(), close(),
input(), output() and ioctl().  It supports multiple devices.  The
input() function is like cn_checkc() was and depends on the device
being open to avoid interference.  I/o on every device can be turned
on and off independently for each device and direction once the devices
are open.  Debugger entry opens all available devices and debugger
exit closes all open devices, but only if the entry is deep enough to
need to do i/o (that's if there is something to display or input needs
to be waited for).  Supported devices are PC VGA screens in simple
modes, PC AT keyboards, various UARTs and a memory output device.
Open/close of a VGA device involved switching its frame buffer.
Open/close of UARTs involved switching their complete h/w state
(possible since this is small and registers are almost read/write).

This seemed more than needed for FreeBSD, so the only thing I copied
from it into FreeBSD was the state switch for UARTs.  This turned into
a switch on every i/o call instead of a switch in open/close.  It is
difficult to do device state switches reentrantly, especially on every
i/o call, but they do work almost reentrantly for UARTs in sio (much
more so than debugger entry, using the stack for the state, etc.).  For
more heavyweight switches like those for frame buffers, it is obvious
that switching on every i/o call is no good, since at best the screen
will flicker on every poll, and you can't stack many frame buffers in
an 8K kernel stack...

The cn_dbctl() API was only used by syscons, and was removed in 2006,
leaving the parts of syscons that used it more broken than before it
existed :-(.  (Some of these parts hacked on [k]db_active, but checks
of that weren't restored.  The checks of the corresponding `debugger'
variable weren't even removed, but since `debugger' was only changed
from 0 to 1 in in removed code, the checks have no effect.)

sio used to work better at the mountroot prompt for another reason:
unlike for keyboards, it is common to have a dedicated serial console.
Then interrupts don't need to be used, so the interrupt handler is not
almost guaranteed to eat the input.  Originally, interrupts were only
enabled on sio serial consoles when the console was also used as an
open tty.  However, for serial consoles to detect changes that map to
debugger entries, it is necessary to always enable interrupts on
serial console devices.  It is then impossible for the interrupt handler
to not see input unrelated to debugger entry, and difficult for it to
buffer the input where the console driver can see it.  This change was
made in 2001 (only if a debugger and entries to it from sio are enabled).

The state switch on every in sio i/o causes additional problems with
the excessive polling for multiple consoles.  Although the state switch
is fairly reentrant and attempts to avoid null changes to the hardware,
some devices don't like it being done at a high rate.  Sometimes
(especially when it is actually needed and drivers that don't do it
would just hang; e.g., if the current state is 9600 bps but the console
state is 115200 bps); the state switch can't be null (and in the speed
change example, is certain to clobber any i/o in progress).  Then the
polling can't work, and excessive polling makes the problem worse.

My version of sio uses the following hack so that input at mountroot is
possible even with very destructive state switches:

% Index: sio.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/dev/sio/sio.c,v
% retrieving revision 1.442
% diff -u -2 -r1.442 sio.c
% --- sio.c	25 Jun 2004 10:51:33 -0000	1.442
% +++ sio.c	18 Oct 2010 09:58:33 -0000
% @@ -2881,6 +3370,12 @@
%  	 * them by clearing the MCR_IENABLE bit, since that might cause
%  	 * an interrupt by floating the IRQ line.
% +	 * We don't want loopback.
% +	 * XXX we clobber MCR_PRESCALE and MCR_DRS and 2 reserved bits.
%  	 */
%  	outb(iobase + com_mcr, (sp->mcr & MCR_IENABLE) | MCR_DTR | MCR_RTS);
% +	if (sp->cfcr != CFCR_8BITS || sp->dlbl != dlbl || sp->dlbh != dlbh
% +	    || (sp->mcr & ~(MCR_IENABLE | MCR_DRS | MCR_DTR | MCR_RTS)))
% +		for (i = 0; i < 100; i++)
% +			(void)inb(0x84);
%  }
%

This adds about 100 uS of delay to the state switch, but only if the
state switch is non-null.  This results in the device being in the
state that is good for debugger i/o much longer that it is in the
(different and thus bad) state at the time the call than it otherwise
would be, so there is a much larger chance of the i/o's getting through.

A delay in each poll can be added unconditionally even if the driver
doesn't dream of state switches, to reduce contention with the interrupt
handler.  sio's mode switch gives smaller but perhaps significant delays
in each poll accidentally by doing lots of i/o's to read the current
state to see if it needs to be switched, etc.  This might reduce the
races significantly.

The delay of 100 uS is a compromise, bised towards the lowest value
that mostly works.  I tried silly delays like 1 second.  It's painful
to type if the rate is limited by a several 1-second delays to a
fraction of 1 cps.  But if the delay is much shorter than 100 uS, then
it's painful to type because most of the input doesn't get through or
garbage or extras get through.

Bruce