Date: Mon, 23 Dec 2013 20:55:56 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John-Mark Gurney <jmg@funkthat.com> Cc: "freebsd-arm@freebsd.org" <arm@FreeBSD.org>, freebsd-arch@FreeBSD.org Subject: Re: mountroot issues (was Re: 10.0-release proposed patch for Atmel) Message-ID: <20131223164823.B954@besplex.bde.org> In-Reply-To: <20131222192842.GI99167@funkthat.com> References: <B5B10EE3-955D-4A8C-A233-9DADF6898A54@bsdimp.com> <20131222192842.GI99167@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 22 Dec 2013, John-Mark Gurney wrote:
> Warner Losh wrote this message on Sat, Dec 21, 2013 at 23:44 -0700:
>> Right now, the mountroot prompt doesn't work on Atmel CPUs. Almost all the characters are eaten. I recently committed an elegant fix for this into head to mask the interrupt for new characters and only do polling.
>
> So, a similar issue plages i386/amd64 too. There the console mostly
> works, but it will drop characters on occasion... The problem is that
> mount root spins calling into the console code instead of asking the
> console code for a single character and having the console code wait
> for this character... and if you type a character while it's outside
> the console routines, that character will be dropped...
This was broken by the multiple console changes (and buggy drivers).
It last (mostly) worked in FreeBSD-4, since there were no multiple
console changes there. It worked just like you hinted: the mount root
code _doesn't_ spin calling into the console code, but calls into the
console code which spins. Non-buggy drivers of course do something
to prevent interrupt handlers doing anything while spinning.
syscons is the main FreeBSD driver that I know of that once upon a
time was non-buggy here. In sccngetch(), it simply disabled all tty
interrupts including the keyboard one using spltty(). This worked
between FreeBSD-1 and FreeBSD-4. This was broken by SMPng. spltty()
became null of course, and the keyboard interrupt became Giant-locked
(it still is). But the spltty()s for locking out the interrupt handler
in console code were not replaced by Giant locking, and Giant locking
(or almost any normal locking) is not permitted in console drivers
anyway. Apparently, AT keyboard interrupts have magic timing (delayed
interrupts?) allows console interrupt to mostly work without locking.
Multiplexed keyboards with some actually being USB increase the problem
significantly -- they probably have different interrupt timing, and
since no normal locking is permitted it is difficult to lock things
throughout the poll. The big SMPng change occurred just 6 weeks before
multiple consoles. Thus there was a small window in which the old API
would have worked if its locking had been updated. Closing of this
window gave the current brokenness less any fixes for this brokenness
in syscons's cngrab() -- the polling loop is at too high a level for
locking in the driver to help, and syscons didn't have any anyway. My
cndbctl() API was designed to fix this problem, except it was
misdesigned and only applied for ddb. Syscons was the only console
driver that supported it, but syscons only used it for console _output_,
and it was broken a few years after the multiple console changes.
cngrab() was mostly designed by me and mostly implemented by avg. It
was only used by syscons too. Strangely, it now does mostly keyboard
stuff where cndbctl() did only screen stuff. I don't see where it
does anything to stop the interrupt handler doing anything.
Syscons console output was also broken by the null spltty(). The
breakage was more serious and was work around by sprinkling buggy
locking. This sometimes gives deadlock if the locks are non-recursive,
and doesn't work if they are recursive. Syscons was never properly
locked. spltty() was like a recursive lock. It basically only worked
for locking out interrupt handlers, and if you used it more (say to
prevent the console driver entering when spltty() is raised), then
you make it like a non-recursive lock and get deadlock. The races
from not locking are the usual ones. You can have one thread in
console output updating critical pointers or in console input accessing
critical device registers. Then another thread may want to do console
i/o. With no locking, these threads just clobber each others' state.
With locking, the console output cannot be as synchronous as desired,
and may cause deadlock. Deadlock is only a serious problem for ddb
console i/o, but after avoiding it there it is trivial to avoid it
for threads. Normal locking is not permitted anywhere in ddb since
it may deadlock, and blowing away the locks is not a solution since
it gives the problem of clobbering state. With normal locking, deadlock
always occurs in cases like the following:
console driver aquires locks
console driver changes critical state --> debugger trap ...
debugger does console i/o
console driver blocks aquiring same locks (deadlock)
The debugger trap may be on the same or a different CPU. If it is on
the same CPU, then it is obviously difficult to get back to the
interrupted code. (Similarly if the critical code is interrupted by
a fast interrupt handler, an NMI, or some other trap. Except for
debugger traps, the problem can be reduced by forbidding console i/o
except in emergencies like panics.) If it is only a different CPU,
then the CPU handling the trap can more easily wait for the CPU doing
the i/o, but this is still hard and not done (kdb_enter() actually
starts by stopping all the other CPUs as soon as possible).
The com driver in 386BSD and 4.4BSD worked similarly to syscons. It
used splhigh() instead of spltty() around its polling loops. This
disables too many more interrupts. This even breaks clock interrupts
and thus even the system time in the 386BSD era when the system time
was just in timer interrupt ticks.
In sio, I used essentially the cngrab() method, but pushed the method
into the driver since I don't like churning APIs (the driver calls the
methid whenever it is entered, so as to not depend on higher layers
doing it). This was based on multiple console code in my debugger,
where the the enabled consoles were "opened" on each entry to the
driver, or by a debugger command to change the set of enable consoles.
There was also an "ioctl" method to change the state of enabled consoles.
The "open" method should do a complete initialization of the device state
if necessary. Similarly, the "close" method should try to restore the
orginal state.
This was of course broken by the multiple console changes. The "pollc"
method never really worked, since it cannot keep the device "open"
across polls unless a higher level does the "open", and switching the
device state in open/close makes it flap too much if the switch is non-
null. sio also had some smaller bugs here:
- when there are shared interrupts, devices attached to the interrupt
or even all sio devices are polled for activity. Since the polling
doesn't use the interrupt status, it detects activity on devices
that are not supposed to interrupt.
- there is some SMP modification (amplification?) of the previous bug.
I forget the details.
uart did nothing to disable device interrupts while polling, at least
for ns8250. uart's locking is convoluted and still seems buggy:
from a slightly old version:
% static void
% ns8250_putc(struct uart_bas *bas, int c)
% {
% int limit;
%
% limit = 250000;
% while ((uart_getreg(bas, REG_LSR) & LSR_THRE) == 0 && --limit)
% DELAY(4);
% uart_setreg(bas, REG_DATA, c);
% uart_barrier(bas);
% limit = 250000;
% while ((uart_getreg(bas, REG_LSR) & LSR_TEMT) == 0 && --limit)
% DELAY(4);
% }
This is wrapped by uart_putc(). uart_putc() acquires a mutex (di->hwmtx),
unless kdb_active it doesn't acquire the mutex. Not acquiring the mutex
avoids deadlock with ddb (if not kdb), but means that the mutex doesn't
actually work for ddb i/o, and ddb input is the most common case of input.
di->hwmtx doesn't do much except provide locking for console i/o routines.
With correct console grabbing, it should be impossible for the interrupt
handle to run while console i/o is in progress. Synchronization occurs
when grabbing acquires the mutex.
%
% static int
% ns8250_rxready(struct uart_bas *bas)
% {
%
% return ((uart_getreg(bas, REG_LSR) & LSR_RXRDY) != 0 ? 1 : 0);
% }
Similarly. Upper layers acquire the mutex.
%
% static int
% ns8250_getc(struct uart_bas *bas, struct mtx *hwmtx)
% {
% int c;
%
% uart_lock(hwmtx);
%
% while ((uart_getreg(bas, REG_LSR) & LSR_RXRDY) == 0) {
% uart_unlock(hwmtx);
% DELAY(4);
% uart_lock(hwmtx);
% }
%
% c = uart_getreg(bas, REG_DATA);
%
% uart_unlock(hwmtx);
%
% return (c);
% }
This function does its own locking. It releases the mutex after every
poll. Releasing the mutex doesn't do much except let the interrupt
handler run and eat your input. It is done because the function may
take aritrarily long to return, but I don't see why locking up all i/o
on the device should be a problem. Anyway, correct grabbing gives a
much longer-term "lock" on all the i/o (not the mutex, but whatever
grabbing does to stop interrupt activity and keep it stopped). It makes
the locking in the above have no effect.
This function uses a correct API, but is currently bogus since the API
is broken. uart still mostly uses the old console API internally, but
the current console API has been broken to not have cn_getc, leaving
this function unattached to it. In the console API, cngetc() still
exists at the top level, but the cn_getc method became unused because
the polling loop for it moved to the top level. It was changed to use
only cn_checkc for input. Then cn_checkc was broken by renaming it
to cn_getc and removing the real cn_getc. The CONS_DRIVER() obfuscation
hides some of the details of this obfuscation from drivers, and uart
still uses the old names internally.
With multiple active consoles, bounding of the time taken by console i/o
is actually a problem for output too. You might have a mixture of fast
and slow devices. cngetc() only has to wait for one device, but cnputc()
has to wait for all of them. It would be better if the output to the
fastest device went out first, but there are no outer loops for output,
so the output goes out in device order. The exact arrangement is:
- cngets(): grab all devices until input is read
- cngetc(): missing grabbing. Thus broken for external use. It is mainly
used by cngets() where it is grabbed globally, and by cnputc() where
it is grabbed around its call.
- cncheckc(): missing grabbing. Thus broken for external use. It is
mainly used when shutting down. Now a transient grab is correct.
Grabbing is still needed for device initialization in some cases.
Perhaps in shutdown a nearby printf() does (should do) sufficient
initialization.
- cnputs(): missing grabbing. It loops calling cnputc().
- cnputc(): missing grabbing
So grabbing is basically only implemented for input.
> The problem is, not many of us spend time at the mountroot prompt, and
> so even if we notice the issue, it's so minor that we just deal w/
> it...
Also, it is not noticeable for syscons. I rarely use serial consoles
and deal with the problem for sio by holding down keys until the the
right characters get through. Perhaps transiently disabling interrupts,
or more likely doing lots of initialization and finalization on every
console driver entry, makes the problem less noticeable for sio. The
initialization and finalization does lots of slow bus accesses. This
makes the duty cycle maybe 99% with interrupts disabled and 1% enabled.
I now remember experimenting with much longer intentional delays in
the initialization to work around the problem of losing state on
non-null mode switches (especially high frequency ones caused by polling
for input). The delays can be made as large as 10-20 milliseconds before
becoming perceptible.
> The method I came up with years ago was to add a routine/flag that would
> have the console wait for a character instead of simply returning when
> there was no character... Though if we implement cngrab properly where
> we don't flush buffers, turn off interupts, etc, then that would work
> too...
That's how it used to work. It just doesn't work for multiple consoles.
However, in the usual case of only 1 active console, this method would
work fine. The API churn also means that you can't simply go back to
the old method for 1 active console :-(. You could also try the old
method with a limit of a second or so for each device. After getting
input from one, give preference to that one so the delays for cycling
round them all are not too painful. The timeouts would also be useful
for output.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131223164823.B954>