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

[Bruce Evans]

On Fri, 14 Mar 2014, Rui Paulo wrote:

> On 14 Mar 2014, at 02:32, Hans Petter Selasky <hps at bitfrost.no> wrote:
>
>> On 03/14/14 03:15, Rui Paulo wrote:
>>> On 10 Mar 2014, at 01:52, Hans Petter Selasky <hselasky at freebsd.org> wrote:

[>* excessive quoting deleted]

>>>> Modified: head/sys/dev/usb/input/ukbd.c
>>>> ==============================================================================
>>>> --- head/sys/dev/usb/input/ukbd.c	Mon Mar 10 06:41:48 2014	(r262971)
>>>> +++ head/sys/dev/usb/input/ukbd.c	Mon Mar 10 08:52:30 2014	(r262972)
>>>> @@ -1909,6 +1909,12 @@ ukbd_ioctl(keyboard_t *kbd, u_long cmd,
>>>> 	int result;
>>>>
>>>> 	/*
>>>> +	 * XXX Check of someone is calling us from a critical section:
>>>> +	 */
>>>> +	if (curthread->td_critnest != 0)
>>>> +		return (EDEADLK);
>>>
>>> Shouldn't this panic?
>>
>> This happens on shutdown, in some special case. Not sure if panic at shutdown is appropriate?
>
> I thought this was a programming error.  Do we know the special cases and why it happens?

Doesn't it already break setting of LEDs and perhaps other things in ddb
(when ddb traps a in a critical section)?  Panicing would give even better
breakage.  It is a programming error (or perhaps a hardware defect) to not
allow setting LEDs in ddb.  Some hardware makes this very difficult.
VGA and AT keyboard hardware is simpler than USB hardware, but syscons
gets this wrong, mostly in the opposite direction by allowing races.
Serial hardware is simpler still, but some serial drivers still get this
wrong.

Working console drivers support single-stepping through their own
(re)initialization (except parts of this hidden by the virtualization
needed to make it work).  Most reinitialization occurs in ioctls, where
it is probably simpler than in resume.  It includes things like LED
handling:

- someone hits caps lock.  The keyboard interrupt handler calls code to set
   the LED for caps lock (if this is done in software)
- someone is debugging keyboard code, perhaps even LED handling code, and
   execution stops in or before the LED handling code
- the debugging includes keystrokes that should toggle LEDs.  These may
   occur at any instruction in the LED handling code, where the
   reinitialization of the LEDs is in in an indeterminate state.  The
   intermediate state should be locked, but debugger activity must not
   block on this.  The keystrokes should have their normal action of
   toggling the LED, except possibly when it was in a half-way state --
   then toggling it could go either way)
- when the LED handling code finishes, or soon after, the LED state must
   not be indeterminate.  It might be the opposite of what it would
   have been without any debugger activity, but only if this is
   consistent with the software state.  Normal software state for things
   like caps lock has precedence over what the LED is showing.  Any
   debugger activity that toggles caps lock should toggle the software
   state first and set the LED to match.  Very occasionally, the states
   will get out of sync when an instruction modifies the hardware state.
   This should be fixed up as soon as possible.  In syscons, related
   fixups occur automatically for VGA things since everything is
   refreshed from the software state often.

LED toggling has to work in critical sections too.  Then the LED code
itself is not being reentered (unless parts of it are locked by
critical sections), but there are other complications.  Of course,
the LED code must not use any interrupts or do any context switches
in debugger mode.
   (This reminds me that the AT keyboard hardware and syscons software
   for it are horrible.  It takes a complicated protocol and several
   milliseconds to toggle a LED.  My first microcomputer in 1982 was
   much better.  It took a single memory access and several microseconds
   to set a LED to an 8-bit color.  Syscons has always used polled i/o
   to set LEDs.  This blocks its interrupt handler (and with spltty()
   or Giant, too many other interrupt handlers) for too long.  But it
   allows LED setting in debugger mode to work without additional
   complications or bugs.)

Bruce