From owner-svn-src-head@FreeBSD.ORG Wed Jul 20 15:32:33 2011 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 55FDD1065670; Wed, 20 Jul 2011 15:32:33 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id 7E1C28FC14; Wed, 20 Jul 2011 15:32:32 +0000 (UTC) Received: from c122-106-165-191.carlnfd1.nsw.optusnet.com.au (c122-106-165-191.carlnfd1.nsw.optusnet.com.au [122.106.165.191]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p6KFWKAL030693 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 21 Jul 2011 01:32:21 +1000 Date: Thu, 21 Jul 2011 01:32:20 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Hans Petter Selasky In-Reply-To: <201107201249.39550.hselasky@c2i.net> Message-ID: <20110720221325.E1436@besplex.bde.org> References: <201107181610.49443.hselasky@c2i.net> <4E26AFF8.8080107@FreeBSD.org> <201107201249.39550.hselasky@c2i.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: "svn-src-head@FreeBSD.org" , "svn-src-all@FreeBSD.org" , "src-committers@FreeBSD.org" , Andriy Gapon Subject: Re: svn commit: r223989 - head/sys/dev/usb/input X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Jul 2011 15:32:33 -0000 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