Re: svn commit: r190857 - head/sys/dev/kbdmux

On Sun, Apr 12, 2009 at 8:03 AM, Fabian Keil
<freebsd-listen_at_fabiankeil.de> wrote:
> Maksim Yevmenkin <emax_at_FreeBSD.org> wrote:
>
>> Author: emax
>> Date: Wed Apr  8 20:52:30 2009
>> New Revision: 190857
>> URL: http://svn.freebsd.org/changeset/base/190857
>>
>> Log:
>>   Undo SVN rev 183283
>>
>>   Do not use Giant for kbdmux(4) locking. This is wrong and apparently
>>   causing more problems than it solves. This will re-open the issue
>>   where interrupt handlers may race with kbdmux(4) in polling mode.
>>   Typical symptoms include (but not limited to) duplicated and/or
>>   missing characters when low level console functions (such as gets)
>>   are used while interrupts are enabled (for example geli password
>>   prompt, mountroot prompt etc.)
>>
>>   MFC after:  3 days
>>
>> Modified:
>>   head/sys/dev/kbdmux/kbdmux.c

[...]

> How about turning this into a compile-time or sysctl option
> or at least mentioning the change in /usr/src/UPDATING?

i do not believe that compile-time nor sysctl option is the right
choice here as it is never allowed to use locks in low-level keyboard
drivers. entry in UPDATING is probably a good idea.

> With this commit, providing the geli key phrase on
> boot is impossible on my AMD64 dual core system.

right, i really hate to say this, but the amount of bug reports, lor's
etc.that were submitted after this change is far more (imo) than
amount of people that ran relatively uncommon setup with encrypted
root (which, btw, prompted the original commit). of course, there is
no question, it has to be fixed.

> Not even enabling the "visible characters" option helps
> because obviously backspace is broken too.

if you do not need kbdmix(4) you might just want to disable it on your
system. i think it should help with your particular problem.

> Before theses locks were introduces I worked around the problem
> with this gets() hack (which forced me to reduce the key entropy):
> http://www.fabiankeil.de/sourcecode/freebsd/gets-no-duplicates.diff
> and now I will simply revert your commit locally, but I assume I'm
> not the only geli user who prefers to be able to boot the system
> without local patches.

if your primary keyboard is atkbd(4), you might want to try the
following patch. it is completely untested (i did not even compile
it), so be warned ...

Index: atkbd.c
===================================================================
--- atkbd.c     (revision 190905)
+++ atkbd.c     (working copy)
_at__at_ -476,7 +476,7 _at__at_
 static int
 atkbd_intr(keyboard_t *kbd, void *arg)
 {
-       atkbd_state_t *state;
+       atkbd_state_t *state = (atkbd_state_t *)kbd->kb_data;
        int delay[2];
        int c;

_at__at_ -485,7 +485,6 _at__at_
                 * The keyboard was not detected before;
                 * it must have been reconnected!
                 */
-               state = (atkbd_state_t *)kbd->kb_data;
                init_keyboard(state->kbdc, &kbd->kb_type,
                              kbd->kb_config);
                KBD_FOUND_DEVICE(kbd);
_at__at_ -497,6 +496,9 _at__at_
        }

        if (KBD_IS_ACTIVE(kbd) && KBD_IS_BUSY(kbd)) {
+               if (state->ks_polling)
+                       return 0;
+
                /* let the callback function to process the input */
                (*kbd->kb_callback.kc_func)(kbd, KBDIO_KEYINPUT,
                                            kbd->kb_callback.kc_arg);

thanks,
max