Re: [PATCH] KDB: Fix usability issues relating to the 'enter' key.

[Andrei Warkentin <awarkentin@vmware.com>]

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