lkml.org 
[lkml]   [2012]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] KDB: Fix usability issues relating to the 'enter' key.
Hi Jason,

Thank you for the review. Comments inline.

----- Original Message -----
> From: "Jason Wessel" <jason.wessel@windriver.com>
> To: "Andrei Warkentin" <andreiw@vmware.com>
> Cc: kgdb-bugreport@lists.sourceforge.net, linux-kernel@vger.kernel.org
> Sent: Sunday, February 26, 2012 8:10:56 AM
> Subject: Re: [PATCH] KDB: Fix usability issues relating to the 'enter' key.
>
> I did not write the original code, but I can explain why there was
> some special logic.
>
> When you restore the system back to the running state you do not want
> to send any other key scan codes back to the kernel. The idea being
> that you type "go" and press enter to resume kernel execution. At
> that point you do not want to send a random scan code back to the
> kernel, ideally you want to leave everything as it was. This also
> handled the case where there was a PS/2 style mouse attached.

The input device is already reset on kdb exit to a safe state, however, by the
code inside kgdboc_post_exp_handler, which calls kgdboc_restore_input().

The existing logic also already correctly will handle the case of a PS/2 mouse, i.e.:

/*
* Ignore mouse events.
*/
if (scanstatus & KBD_STAT_MOUSE_OBF)
return -1;

...so there is nothing that the non-ENTER-specific code already doesn't deal with.
That special ENTER code also doesn't correctly handle typematic repeat, nor does it handle
key-pad enter (as I mentioned before in the patch description, this has a different
make and break codes, prefixed with 0xe0).

>
> I do have a question about part of the section you deleted.
>
>
> > - if ((scancode & 0x7f) == 0x1c) {
> > - /*
> > - * enter key. All done. Absorb the release scancode.
> > - */
> > - while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0)
> > - ;
>
> Seems there is a bug here. There is a cpu_relax() missing.

Right, that's exactly what I said in the patch :-).

>
> > -
> > - /*
> > - * Fetch the scancode
> > - */
> > - scancode = inb(KBD_DATA_REG);
> > - scanstatus = inb(KBD_STATUS_REG);
> > -
> > - while (scanstatus & KBD_STAT_MOUSE_OBF) {
>
> There should also be a cpu_relax() right here.

Correct.

>
> > - scancode = inb(KBD_DATA_REG);
> > - scanstatus = inb(KBD_STATUS_REG);
> > - }
> > -
>
>
> If you put the two cpu_relax() pieces in do you still end up with a
> problem? If this does not work for you the possibility to exists to
> clear the keyboard/mouse state on the kdb exit.

You will solve the multi-second hangs on ESX. However, this still doesn't fix the
bogus messages during typematic repeat (holding down ENTER) or keypad enter key use.

Basically, if you nuke the entire ENTER-key special case block, the existing key will
correctly handle both typematic, break codes, and KP ENTER key use (as well as any
mouse events). Cleanup of mouse/keyboard state is done already on kdb exit by kgdboc, which
is the only consumer of the kdb_keyboard code.

A


\
 
 \ /
  Last update: 2012-02-27 02:03    [W:0.074 / U:5.872 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site