KDB: Fix usability issues relating to the 'enter' key.

PATCH In review Andrei Warkentin 4 participants 14 messages Feb 17, 2012 – Feb 28, 2012

Subsystems (2)

TTY layer and serial drivers KGDB / KDB /debug_core

lore

Share & tools

lore

Andrei Warkentin

author

2012-02-17 23:52 UTC #0 lore raw

PATCH 1/1 KDB: Fix usability issues relating to the 'enter' key.

This fixes the following problems:
1) Typematic-repeat of 'enter' gives warning message.
2) Use of 'keypad enter' gives warning message.
3) Lag on the order of seconds between "break" and "make" when expecting the enter "break" code. Seen under virtualized environments such as VMware ESX.

Explanations:
1) Holding down 'enter' will not set a repeating sequence of 0x1c(make)-0x9c(make), but a repeating sequence of make codes, followed by one break code when the key is released. Thus, it's wrong to expect the break code after seeing the 'enter' make code.
2) Keypad enter generates different make/break, namely 0xe0 0x1c and 0xe0 0x9c. The 'generic' logic handles the 0xe0 escape already, but the special 'enter' logic always expects '0x9c' and not '0xe0 0x9c', so you get a warning message, again.
3) When expecting the 'enter' break code, the code polls the status register in a tight loop, like so -

Quoted 1 line

while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0);

However, it really should do something like -

Quoted 2 lines

while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0)
    cpu_relax(); /* pause */

Basically, it's a common optimization to have a fast path for accessing often accessed and slow changing I/O in a virtualized environment. The tight spinning in KDB seems to run against the logic by ESX keyboard virtualization code to detect when the fast path or the slow path should be used to satisfy the keyboard status read, leading to multi-second timeouts before the 'real' status comes through. Without knowing ESX internals, it's hard to say if this is an ESX bug or not, but letting the VM be explicitely descheduled seems to resolve the problem. I've seen something similar with shared MMIO buffers with VMs on Hyper-V.

Anyway, given (3), (2) and (1), we might as well blow away the entire special casing for 'enter'. The break codes will already be handled correctly, and we get rid of the bugs with repeat enters and keypad enter key. And of course, there is no need to AND with 0x7f when checking for 'enter', because we'll never ever get to this code with a break code (checked for much earlier).

I tried to figure out the history behind the 'enter' key special casing code, and it seems to have come from whatever the original KDB patch was. Perhaps someone can chime in.

Tested on ESX 5.0 and QEMU.

Signed-off-by Andrei Warkentin <[email protected]>

---
 kernel/debug/kdb/kdb_keyboard.c |   28 +---------------------------
 1 files changed, 1 insertions(+), 27 deletions(-)

kernel/debug/kdb/kdb_keyboard.c +1 ~~-27~~

index 4bca634..ed4a2f9 100644
@@ -178,34 +178,8 @@ int kdb_get_kbd_char(void)
 		return -1;	/* ignore unprintables */
 	}
 
-	if ((scancode & 0x7f) == 0x1c) {
-		/*
-		 * enter key.  All done.  Absorb the release scancode.
-		 */
-		while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0)
-			;
-
-		/*
-		 * Fetch the scancode
-		 */
-		scancode = inb(KBD_DATA_REG);
-		scanstatus = inb(KBD_STATUS_REG);
-
-		while (scanstatus & KBD_STAT_MOUSE_OBF) {
-			scancode = inb(KBD_DATA_REG);
-			scanstatus = inb(KBD_STATUS_REG);
-		}
-
-		if (scancode != 0x9c) {
-			/*
-			 * Wasn't an enter-release,  why not?
-			 */
-			kdb_printf("kdb: expected enter got 0x%x status 0x%x\n",
-			       scancode, scanstatus);
-		}
-
+	if (scancode == 0x1c)
 		return 13;
-	}
 
 	return keychar & 0xff;
 }

Signature · 1 line

1.7.4.1

Andrei Warkentin

2012-02-25 03:20 UTC #1 lore raw

Hi,

----- Original Message -----

Quoted 6 lines

From: "Andrei Warkentin" <[email protected]>
To: [email protected]
Cc: [email protected], "jason wessel" <[email protected]>, [email protected]
Sent: Friday, February 17, 2012 6:52:53 PM
Subject: [PATCH] KDB: Fix usability issues relating to the 'enter' key.

Quoted 7 lines

This fixes the following problems:
1) Typematic-repeat of 'enter' gives warning message.
2) Use of 'keypad enter' gives warning message.
3) Lag on the order of seconds between "break" and "make" when expecting the enter "break" code. Seen under virtualized environments such as VMware ESX.

Does this seem reasonable :-)?

Jason Wessel

maintainer

2012-02-26 13:11 UTC #2 lore raw

In reply to Andrei Warkentin

Quoted 46 lines · Andrei Warkentin

This fixes the following problems:
1) Typematic-repeat of 'enter' gives warning message.
2) Use of 'keypad enter' gives warning message.
3) Lag on the order of seconds between "break" and "make" when expecting the enter "break" code. Seen under virtualized environments such as VMware ESX.
Explanations:
1) Holding down 'enter' will not set a repeating sequence of 0x1c(make)-0x9c(make), but a repeating sequence of make codes, followed by one break code when the key is released. Thus, it's wrong to expect the break code after seeing the 'enter' make code.
2) Keypad enter generates different make/break, namely 0xe0 0x1c and 0xe0 0x9c. The 'generic' logic handles the 0xe0 escape already, but the special 'enter' logic always expects '0x9c' and not '0xe0 0x9c', so you get a warning message, again.
3) When expecting the 'enter' break code, the code polls the status register in a tight loop, like so -
Quoted 1 line
while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0);
However, it really should do something like -
Quoted 2 lines
while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0)
    cpu_relax(); /* pause */
Basically, it's a common optimization to have a fast path for accessing often accessed and slow changing I/O in a virtualized environment. The tight spinning in KDB seems to run against the logic by ESX keyboard virtualization code to detect when the fast path or the slow path should be used to satisfy the keyboard status read, leading to multi-second timeouts before the 'real' status comes through. Without knowing ESX internals, it's hard to say if this is an ESX bug or not, but letting the VM be explicitely descheduled seems to resolve the problem. I've seen something similar with shared MMIO buffers with VMs on Hyper-V.
Anyway, given (3), (2) and (1), we might as well blow away the entire special casing for 'enter'. The break codes will already be handled correctly, and we get rid of the bugs with repeat enters and keypad enter key. And of course, there is no need to AND with 0x7f when checking for 'enter', because we'll never ever get to this code with a break code (checked for much earlier).

Quoted 4 lines

I tried to figure out the history behind the 'enter' key special casing code, and it seems to have come from whatever the original KDB patch was. Perhaps someone can chime in.

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.

I do have a question about part of the section you deleted.

Quoted 12 lines

Tested on ESX 5.0 and QEMU.
Signed-off-by Andrei Warkentin <[email protected]>
---
 kernel/debug/kdb/kdb_keyboard.c |   28 +---------------------------
 1 files changed, 1 insertions(+), 27 deletions(-)
kernel/debug/kdb/kdb_keyboard.c +0 -0
index 4bca634..ed4a2f9 100644

kernel/debug/kdb/kdb_keyboard.c int kdb_get_kbd_char(void) · line 186

Quoted 9 lines

kernel/debug/kdb/kdb_keyboard.c +0 -6

@@ -178,34 +178,8 @@ int kdb_get_kbd_char(void)
 		return -1;	/* ignore unprintables */
 	}
 
-	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.

kernel/debug/kdb/kdb_keyboard.c int kdb_get_kbd_char(void) · line 194

Quoted 8 lines

kernel/debug/kdb/kdb_keyboard.c +0 -8

-
-		/*
-		 * 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.

kernel/debug/kdb/kdb_keyboard.c int kdb_get_kbd_char(void) · line 198

Quoted 4 lines

kernel/debug/kdb/kdb_keyboard.c +0 -4

-			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.

Thanks,
Jason.

Jason Wessel

maintainer

2012-02-26 13:58 UTC #3 lore raw

Quoted 47 lines · Jason Wessel

Quoted 46 lines · Andrei Warkentin
This fixes the following problems:
1) Typematic-repeat of 'enter' gives warning message.
2) Use of 'keypad enter' gives warning message.
3) Lag on the order of seconds between "break" and "make" when expecting the enter "break" code. Seen under virtualized environments such as VMware ESX.
Explanations:
1) Holding down 'enter' will not set a repeating sequence of 0x1c(make)-0x9c(make), but a repeating sequence of make codes, followed by one break code when the key is released. Thus, it's wrong to expect the break code after seeing the 'enter' make code.
2) Keypad enter generates different make/break, namely 0xe0 0x1c and 0xe0 0x9c. The 'generic' logic handles the 0xe0 escape already, but the special 'enter' logic always expects '0x9c' and not '0xe0 0x9c', so you get a warning message, again.
3) When expecting the 'enter' break code, the code polls the status register in a tight loop, like so -
Quoted 1 line
while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0);
However, it really should do something like -
Quoted 2 lines
while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0)
    cpu_relax(); /* pause */
Basically, it's a common optimization to have a fast path for accessing often accessed and slow changing I/O in a virtualized environment. The tight spinning in KDB seems to run against the logic by ESX keyboard virtualization code to detect when the fast path or the slow path should be used to satisfy the keyboard status read, leading to multi-second timeouts before the 'real' status comes through. Without knowing ESX internals, it's hard to say if this is an ESX bug or not, but letting the VM be explicitely descheduled seems to resolve the problem. I've seen something similar with shared MMIO buffers with VMs on Hyper-V.
Anyway, given (3), (2) and (1), we might as well blow away the entire special casing for 'enter'. The break codes will already be handled correctly, and we get rid of the bugs with repeat enters and keypad enter key. And of course, there is no need to AND with 0x7f when checking for 'enter', because we'll never ever get to this code with a break code (checked for much earlier).

Quoted 5 lines

Quoted 4 lines
I tried to figure out the history behind the 'enter' key special casing code, and it seems to have come from whatever the original KDB patch was. Perhaps someone can chime in.

Andrei, if you agree with the attached patch, I'll put it in the merge queue. If you find problems we can go another iteration. :-)

Cheers,
Jason.

Quoted 1 line

From 797a228abdd5d560dddf217572d984b1dce97154 Mon Sep 17 00:00:00 2001

From: Jason Wessel <[email protected]>
Date: Sun, 26 Feb 2012 07:49:16 -0600
Subject: [PATCH 1/1] From: Andrei Warkentin <[email protected]> Subject:
[PATCH] KDB: Fix usability issues relating to the
'enter' key. Date: Sun Feb 26 07:51:01 CST 2012

Quoted 1 line

while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0);

However, it really should do something like -

Quoted 2 lines

while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0)
    cpu_relax(); /* pause */

Signed-off-by Andrei Warkentin <[email protected]>

[[email protected]: use cpu_relax() and remove expected msg]

Signed-off-by Jason Wessel <[email protected]>

---
 kernel/debug/kdb/kdb_keyboard.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

kernel/debug/kdb/kdb_keyboard.c +2 -9

index 4bca634..0351d0c 100644
@@ -183,7 +183,7 @@ int kdb_get_kbd_char(void)
 		 * enter key.  All done.  Absorb the release scancode.
 		 */
 		while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0)
-			;
+			cpu_relax();
 
 		/*
 		 * Fetch the scancode

@@ -192,18 +192,11 @@ int kdb_get_kbd_char(void)
 		scanstatus = inb(KBD_STATUS_REG);
 
 		while (scanstatus & KBD_STAT_MOUSE_OBF) {
+			cpu_relax();
 			scancode = inb(KBD_DATA_REG);
 			scanstatus = inb(KBD_STATUS_REG);
 		}
 
-		if (scancode != 0x9c) {
-			/*
-			 * Wasn't an enter-release,  why not?
-			 */
-			kdb_printf("kdb: expected enter got 0x%x status 0x%x\n",
-			       scancode, scanstatus);
-		}
-
 		return 13;
 	}

Signature · 1 line

1.7.5.4

Andrei Warkentin

2012-02-27 01:04 UTC #4 lore raw

Hi,

----- Original Message -----

Quoted 10 lines

From: "Jason Wessel" <[email protected]>
To: "Andrei Warkentin" <[email protected]>
Cc: [email protected], [email protected]
Sent: Sunday, February 26, 2012 8:58:37 AM
Subject: Re: [PATCH] KDB: Fix usability issues relating to the 'enter' key.
Andrei, if you agree with the attached patch, I'll put it in the merge queue. If you find problems we can go another iteration. :-)

Nak. As I've mentioned in my original email, I went down this route as well, only to ask the question of why the special ENTER code was there, since all it did was introduce problems elsewhere (like repeat ENTER handling, KP ENTER key...). This is exactly why my solution was to get rid of the special ENTER code altogether.

The code in kgdboc already will do input device reset on kdb exit, so there is no need to explicitely empty out the keyboard FIFO on every ENTER.

Thanks,
A

Jason Wessel

maintainer

2012-02-27 22:51 UTC #5 lore raw

Quoted 14 lines · Andrei Warkentin

Hi,
----- Original Message -----
Quoted 10 lines
From: "Jason Wessel" <[email protected]>
To: "Andrei Warkentin" <[email protected]>
Cc: [email protected], [email protected]
Sent: Sunday, February 26, 2012 8:58:37 AM
Subject: Re: [PATCH] KDB: Fix usability issues relating to the 'enter' key.
Andrei, if you agree with the attached patch, I'll put it in the merge queue. If you find problems we can go another iteration. :-)

Quoted 8 lines

Nak. As I've mentioned in my original email, I went down this route as well, only to ask the question of why the special ENTER code was there, since all it did was introduce problems elsewhere (like repeat ENTER handling, KP ENTER key...). This is exactly why my solution was to get rid of the special ENTER code altogether.
The code in kgdboc already will do input device reset on kdb exit, so there is no need to explicitely empty out the keyboard FIFO on every ENTER.

Did you try the patch I sent? It might not address the key repeat problem, but this is some thing I had not yet duplicated. Outside of the warning message (which was killed off in the patch), was there garbage characters or some notable behavior? Was it something you could see in qemu or only in ESX?

Having authored the keyboard/input handler with some suggestions from Dmitry, I was fairly certain it had no way to prevent the leak of the up keystroke unless you have the KDB specific capture where it waits to act on the "key up" event for an enter keystroke. The cleanup handler only cleans up state events where keys were down at the time the debug core became active. It will not prevent the leak of key strokes or state changes while the kernel was stopped. The goofy enter handler was supposed to take care of that.

I conclusively proved there is event leakage from using the original patch you provided. Here is the test case, which you should be able to execute with qemu or kvm (since you also mentioned that was what you were previously using).

1) boot the qemu
2) Use kgdboc=kbd
3) break into the debugger to the kdb prompt
4) type "g" but to not press return
5) Connect to the qemu back end debug connection with gdb and set a breakpoint at line 402 of atkbd.c which should be the call to:
input_event(dev, EV_MSC, MSC_RAW, code);
6) continue the gdb connection
7) In the qemu monitor enter the command:
sendkey ret 4000

After 4 seconds when the key is released you will catch the leaked event in the atkbd.c, and if you had X running it propagates all the way up the input chain.

Jason.

Andrei Warkentin

2012-02-27 23:18 UTC #6 lore raw

Hi,

----- Original Message -----

Quoted 11 lines

From: "Jason Wessel" <[email protected]>
To: "Andrei Warkentin" <[email protected]>
Cc: [email protected], [email protected], "Andrei Warkentin" <[email protected]>
Sent: Monday, February 27, 2012 5:50:59 PM
Subject: Re: [PATCH] KDB: Fix usability issues relating to the 'enter' key.
Did you try the patch I sent? It might not address the key repeat problem, but this is some thing I had not yet duplicated. Outside of the warning message (which was killed off in the patch), was there garbage characters or some notable behavior? Was it something you could see in qemu or only in ESX?

Let me elaborate. The cpu_relax() you added solves the hanging.

Quoted 22 lines

Having authored the keyboard/input handler with some suggestions from Dmitry, I was fairly certain it had no way to prevent the leak of the up keystroke unless you have the KDB specific capture where it waits to act on the "key up" event for an enter keystroke. The cleanup handler only cleans up state events where keys were down at the time the debug core became active. It will not prevent the leak of key strokes or state changes while the kernel was stopped. The goofy enter handler was supposed to take care of that.
I conclusively proved there is event leakage from using the original patch you provided. Here is the test case, which you should be able to
execute with qemu or kvm (since you also mentioned that was what you were previously using).
1) boot the qemu
2) Use kgdboc=kbd
3) break into the debugger to the kdb prompt
4) type "g" but to not press return
5) Connect to the qemu back end debug connection with gdb and set a breakpoint at line 402 of atkbd.c which should be the call to:

Quoted 8 lines

   	input_event(dev, EV_MSC, MSC_RAW, code);
6) continue the gdb connection
7) In the qemu monitor enter the command:
sendkey ret 4000
After 4 seconds when the key is released you will catch the leaked event in the atkbd.c, and if you had X running it propagates all the way up the input chain.

I know I should have read deeper into the input code. I assumed it performed a reset on the input device. Assumptions are bad. Sorry.

So here is the thing. With the patch you sent, you would still leak the break keypress for the keypad enter. This is because the break code for KP ENTER is 0xe0 0x9c, and the inb(KBD_DATA_REG) done in the goofy handler will read just the 0xe0 part, leaving 0x9c behind, which will be leaked out.

So here is my question - do you want me to fix the existing goofy handler to properly handle KP ENTER and ENTER repeats? Or do you think it's appropriate to empty out the FIFO during KDB exit as part of post_exp kgdb_io op? Or reset the input device?

Thanks for investigating this.

Jason Wessel

maintainer

2012-02-27 23:28 UTC #7 lore raw

Quoted 48 lines · Andrei Warkentin

Hi,
----- Original Message -----
Quoted 11 lines
From: "Jason Wessel" <[email protected]>
To: "Andrei Warkentin" <[email protected]>
Cc: [email protected], [email protected], "Andrei Warkentin" <[email protected]>
Sent: Monday, February 27, 2012 5:50:59 PM
Subject: Re: [PATCH] KDB: Fix usability issues relating to the 'enter' key.
Did you try the patch I sent? It might not address the key repeat problem, but this is some thing I had not yet duplicated. Outside of the warning message (which was killed off in the patch), was there garbage characters or some notable behavior? Was it something you could see in qemu or only in ESX?
Let me elaborate. The cpu_relax() you added solves the hanging.
Quoted 21 lines
Having authored the keyboard/input handler with some suggestions from Dmitry, I was fairly certain it had no way to prevent the leak of the up keystroke unless you have the KDB specific capture where it waits to act on the "key up" event for an enter keystroke. The cleanup handler only cleans up state events where keys were down at the time the debug core became active. It will not prevent the leak of key strokes or state changes while the kernel was stopped. The goofy enter handler was supposed to take care of that.
I conclusively proved there is event leakage from using the original patch you provided. Here is the test case, which you should be able to
execute with qemu or kvm (since you also mentioned that was what you were previously using).
1) boot the qemu
2) Use kgdboc=kbd
3) break into the debugger to the kdb prompt
4) type "g" but to not press return
5) Connect to the qemu back end debug connection with gdb and set a breakpoint at line 402 of atkbd.c which should be the call to:
Quoted 8 lines
   	input_event(dev, EV_MSC, MSC_RAW, code);
6) continue the gdb connection
7) In the qemu monitor enter the command:
sendkey ret 4000
After 4 seconds when the key is released you will catch the leaked event in the atkbd.c, and if you had X running it propagates all the way up the input chain.
I know I should have read deeper into the input code. I assumed it performed a reset on the input device. Assumptions are bad. Sorry.

Quoted 8 lines

So here is the thing. With the patch you sent, you would still leak the break keypress for the keypad enter. This is because the break code for KP ENTER is 0xe0 0x9c, and the inb(KBD_DATA_REG) done in the goofy handler will read just the 0xe0 part, leaving 0x9c behind, which will be leaked out.
So here is my question - do you want me to fix the existing goofy handler to properly handle KP ENTER and ENTER repeats? Or do you think it's appropriate to empty out the FIFO during KDB exit as part of post_exp kgdb_io op? Or reset the input device?

Thanks for the explanation. I agree that we still have a leak with the patch I had sent you. The old kdb code simply was incomplete and likely assumed people didn't ever use anything except the main keyboard's enter key.

I think the right choice is to correctly handle any that generates enter/return and do not leave the kdb keyboard handler until the release scan code is processed. We have to do this because the physical release delay will always generate another scan code later on regardless of clearing the fifo or resetting the device. The computer is simply faster than the human behind the console in this case. :-)

If you have some idea how to correctly fix up the KP ENTER please send a patch, else I will have a look at it sometime soon. It is definitely worth fixing.

Cheers,
Jason.

Andrei Warkentin

2012-02-27 23:35 UTC #8 lore raw

Hi,

----- Original Message -----

Quoted 10 lines

From: "Jason Wessel" <[email protected]>
To: "Andrei Warkentin" <[email protected]>
Cc: [email protected], [email protected], "Andrei Warkentin" <[email protected]>
Sent: Monday, February 27, 2012 6:28:20 PM
Subject: Re: [PATCH] KDB: Fix usability issues relating to the 'enter' key.
If you have some idea how to correctly fix up the KP ENTER please send a patch, else I will have a look at it sometime soon. It is definitely worth fixing.

Will send an updated patch. Thanks again.

Andrei Warkentin

2012-02-28 02:28 UTC #9 lore raw

PATCH 1/1 KDB: Fix usability issues relating to the 'enter' key.

In reply to Jason Wessel

This fixes the following problems:
1) Typematic-repeat of 'enter' gives warning message and leaks make/break if KDB exits. Repeats
look something like 0x1c 0x1c .... 0x9c
2) Use of 'keypad enter' gives warning message and leaks the ENTER break/make code out if KDB exits. KP ENTER repeats look someting like 0xe0 0x1c 0xe0 0x1c ... 0xe0 0x9c.
3) Lag on the order of seconds between "break" and "make" when expecting the enter "break" code. Seen under virtualized environments such as VMware ESX.

The existing special enter handler tries to glob the enter break code, but this fails if the other (KP) enter was used, or if there was a key repeat. It also fails if you mashed some keys along with enter, and you ended up with a non-enter make or non-enter break code coming after the enter make code. So first, we modify the handler to handle these cases. But performing these actions on every enter is annoying since now you can't hold ENTER down to scroll <more>d messages in KDB. Since this special behaviour is only necessary to handle the exiting KDB ('g' + ENTER) without leaking scancodes to the OS, we perform the cleanup in kgdboc_post_exp_handler path.

Tested on QEMU. Set a bp on atkbd.c to verify no scan code was leaked.

Cc Andrei Warkentin <[email protected]>

Cc Jason Wessel <[email protected]>

Signed-off-by Andrei Warkentin <[email protected]>

---
 drivers/tty/serial/kgdboc.c     |   15 +++++++
 include/linux/kdb.h             |    1 +
 kernel/debug/kdb/kdb_keyboard.c |   87 +++++++++++++++++++++++++++++---------
 3 files changed, 82 insertions(+), 21 deletions(-)

drivers/tty/serial/kgdboc.c +15 -0

index 2b42a01..c74b47b 100644
@@ -284,6 +284,8 @@ static void kgdboc_pre_exp_handler(void)
 
 static void kgdboc_post_exp_handler(void)
 {
+	int i;
+
 	/* decrement the module count when the debugger detaches */
 	if (!kgdb_connected)
 		module_put(THIS_MODULE);

@@ -291,6 +293,19 @@ static void kgdboc_post_exp_handler(void)
 		dbg_restore_graphics = 0;
 		con_debug_leave();
 	}
+
+	/*
+	 * For i8042 keyboard, we still have the ENTER
+	 * break code in the buffer after KDB processed
+	 * the 'g' command and exited. Clean it up.
+	 */
+	for (i = 0; i < kdb_poll_idx; i++) {
+		if (kdb_poll_funcs[i] == kdb_get_kbd_char) {
+			kdb_kbd_cleanup_state();
+			break;
+		}
+	}
+
 	kgdboc_restore_input();
 }

include/linux/kdb.h +1 -0

index 0647258..9bf65cc 100644
@@ -124,6 +124,7 @@ extern void kdb_init(int level);
 typedef int (*get_char_func)(void);
 extern get_char_func kdb_poll_funcs[];
 extern int kdb_get_kbd_char(void);
+extern void kdb_kbd_cleanup_state(void);
 
 static inline
 int kdb_process_cpu(const struct task_struct *p)

kernel/debug/kdb/kdb_keyboard.c +66 ~~-21~~

101 lines not on this page · raw message

Signature · 1 line

1.7.8.3

Andrei Warkentin

2012-02-28 05:27 UTC #10 lore raw

PATCH 1/1 KDB: Fix usability issues relating to the 'enter' key.

In reply to Jason Wessel

Fixed previous regression where if kbd was not used to 'g' + ENTER, the cleanup code would hang.

Tested on QEMU. Set a bp on atkbd.c to verify no scan code was leaked.

Cc Andrei Warkentin <[email protected]>

Cc Jason Wessel <[email protected]>

Signed-off-by Andrei Warkentin <[email protected]>

---
 drivers/tty/serial/kgdboc.c     |   15 ++++++
 include/linux/kdb.h             |    1 +
 kernel/debug/kdb/kdb_keyboard.c |  104 ++++++++++++++++++++++++++++++---------
 3 files changed, 98 insertions(+), 22 deletions(-)

drivers/tty/serial/kgdboc.c +15 -0

index 2b42a01..c74b47b 100644
@@ -284,6 +284,8 @@ static void kgdboc_pre_exp_handler(void)
 
 static void kgdboc_post_exp_handler(void)
 {
+	int i;
+
 	/* decrement the module count when the debugger detaches */
 	if (!kgdb_connected)
 		module_put(THIS_MODULE);

@@ -291,6 +293,19 @@ static void kgdboc_post_exp_handler(void)
 		dbg_restore_graphics = 0;
 		con_debug_leave();
 	}
+
+	/*
+	 * For i8042 keyboard, we still have the ENTER
+	 * break code in the buffer after KDB processed
+	 * the 'g' command and exited. Clean it up.
+	 */
+	for (i = 0; i < kdb_poll_idx; i++) {
+		if (kdb_poll_funcs[i] == kdb_get_kbd_char) {
+			kdb_kbd_cleanup_state();
+			break;
+		}
+	}
+
 	kgdboc_restore_input();
 }

include/linux/kdb.h +1 -0

index 0647258..9bf65cc 100644
@@ -124,6 +124,7 @@ extern void kdb_init(int level);
 typedef int (*get_char_func)(void);
 extern get_char_func kdb_poll_funcs[];
 extern int kdb_get_kbd_char(void);
+extern void kdb_kbd_cleanup_state(void);
 
 static inline
 int kdb_process_cpu(const struct task_struct *p)

kernel/debug/kdb/kdb_keyboard.c +82 ~~-22~~

131 lines not on this page · raw message

Signature · 1 line

1.7.9.2

Jason Wessel

maintainer

2012-02-28 13:53 UTC #11 lore raw

I think we are a lot closer this time. I attached a new patch based on your prior version.

Quoted 12 lines · Andrei Warkentin

This fixes the following problems:
1) Typematic-repeat of 'enter' gives warning message and leaks make/break if KDB exits. Repeats
look something like 0x1c 0x1c .... 0x9c
2) Use of 'keypad enter' gives warning message and leaks the ENTER break/make code out if KDB exits.
KP ENTER repeats look someting like 0xe0 0x1c
0xe0 0x1c ... 0xe0 0x9c.
3) Lag on the order of seconds between "break" and "make" when expecting the enter "break" code. Seen under virtualized environments such as VMware ESX.

Quoted 12 lines

The existing special enter handler tries to glob the enter break code, but this fails if the other (KP) enter was used, or if there was a key repeat. It also fails if you mashed some keys along with enter, and you ended up with a non-enter make or non-enter break code coming after the enter make code. So first, we modify the handler to handle these cases. But performing these actions on every enter is annoying since now you can't hold ENTER down to scroll <more>d messages in KDB. Since this special behaviour is only necessary to handle the exiting KDB ('g' + ENTER) without leaking scancodes to the OS, we perform the cleanup in kgdboc_post_exp_handler path.

This is the wrong place to do this. The cleanup needs to get executed any time you are going to leave kdb_main(), because there are a few conditions like cpu switch, and transition to kgdb where you would leak the enter code on to the buffer handler without ever returning to the OS. Also if you did not set CONFIG_KDB_KEYBOARD your patch did not compile properly. We could have fixed this by moving the code to the input cleanup in kgdboc, but this is not needed at all if you take a look at the revised version.

Quoted 4 lines

Fixed previous regression where if kbd was not used to 'g' + ENTER, the cleanup code would hang.

Was this a regression in the out of tree code or something in the mainline kdb?

Quoted 4 lines

index 4bca634..34a8722 100644
kernel/debug/kdb/kdb_keyboard.c +1 -0
+void kdb_kbd_cleanup_state(void)

kernel/debug/kdb/kdb_keyboard.c

Quoted 10 lines

kernel/debug/kdb/kdb_keyboard.c +10 -0

+{
+ int scancode, scanstatus;
+
+ /*
+ * Nothing to clean up, since either
+ * ENTER was never pressed, or has already
+ * gotten cleaned up.
+ */
+ if (!kbd_last_ret)
+ return;

I added kbd_last_ret = 0; right here so this cannot get triggered a second time if the kdb main loop is entered and exited without going to the shell (which can happen on a soft single step operation on some archs).

Jason.

PS We can iterate other patches you sent after we settle on this one. Thanks!

From: Andrei Warkentin <[email protected]>
Date: Tue, 28 Feb 2012 06:55:05 -0600
Subject: [PATCH] KDB: Fix usability issues relating to the 'enter' key.

The existing special enter handler tries to glob the enter break code, but this fails if the other (KP) enter was used, or if there was a key repeat. It also fails if you mashed some keys along with enter, and you ended up with a non-enter make or non-enter break code coming after the enter make code. So first, we modify the handler to handle these cases. But performing these actions on every enter is annoying since now you can't hold ENTER down to scroll <more>d messages in KDB. Since this special behaviour is only necessary to handle the exiting KDB ('g' + ENTER) without leaking scancodes to the OS. This cleanup needs to get executed anytime the kdb_main loop exits.

Tested on QEMU. Set a bp on atkbd.c to verify no scan code was leaked.

Cc Andrei Warkentin <[email protected]>

[[email protected]: move cleanup calls to kdb_main.c]

Signed-off-by Andrei Warkentin <[email protected]>

Signed-off-by Jason Wessel <[email protected]>

---
 kernel/debug/kdb/kdb_keyboard.c |   95 ++++++++++++++++++++++++++++++---------
 kernel/debug/kdb/kdb_main.c     |    3 +
 kernel/debug/kdb/kdb_private.h  |    7 +++
 3 files changed, 83 insertions(+), 22 deletions(-)

kernel/debug/kdb/kdb_keyboard.c +73 ~~-22~~

index 4bca634..118527a 100644
@@ -25,6 +25,7 @@
 #define KBD_STAT_MOUSE_OBF	0x20	/* Mouse output buffer full */
 
 static int kbd_exists;
+static int kbd_last_ret;
 
 /*
  * Check if the keyboard controller has a keypress for us.

@@ -90,8 +91,11 @@ int kdb_get_kbd_char(void)
 		return -1;
 	}
 
-	if ((scancode & 0x80) != 0)
+	if ((scancode & 0x80) != 0) {
+		if (scancode == 0x9c)
+			kbd_last_ret = 0;
 		return -1;
+	}
 
 	scancode &= 0x7f;

@@ -178,35 +182,82 @@ int kdb_get_kbd_char(void)
 		return -1;	/* ignore unprintables */
 	}
 
-	if ((scancode & 0x7f) == 0x1c) {
-		/*
-		 * enter key.  All done.  Absorb the release scancode.
-		 */
+	if (scancode == 0x1c) {
+		kbd_last_ret = 1;
+		return 13;
+	}
+
+	return keychar & 0xff;
+}
+EXPORT_SYMBOL_GPL(kdb_get_kbd_char);
+
+/*
+ * Best effort cleanup of ENTER break codes on leaving KDB. Called on
+ * exiting KDB, when we know we processed an ENTER or KP ENTER scan
+ * code.
+ */
+void kdb_kbd_cleanup_state(void)
+{
+	int scancode, scanstatus;
+
+	/*
+	 * Nothing to clean up, since either
+	 * ENTER was never pressed, or has already
+	 * gotten cleaned up.
+	 */
+	if (!kbd_last_ret)
+		return;
+
+	kbd_last_ret = 0;
+	/*
+	 * Enter key. Need to absorb the break code here, lest it gets
+	 * leaked out if we exit KDB as the result of processing 'g'.
+	 *
+	 * This has several interesting implications:
+	 * + Need to handle KP ENTER, which has break code 0xe0 0x9c.
+	 * + Need to handle repeat ENTER and repeat KP ENTER. Repeats
+	 *   only get a break code at the end of the repeated
+	 *   sequence. This means we can't propagate the repeated key
+	 *   press, and must swallow it away.
+	 * + Need to handle possible PS/2 mouse input.
+	 * + Need to handle mashed keys.
+	 */
+
+	while (1) {
 		while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0)
-			;
+			cpu_relax();
 
 		/*
-		 * Fetch the scancode
+		 * Fetch the scancode.
 		 */
 		scancode = inb(KBD_DATA_REG);
 		scanstatus = inb(KBD_STATUS_REG);
 
-		while (scanstatus & KBD_STAT_MOUSE_OBF) {
-			scancode = inb(KBD_DATA_REG);
-			scanstatus = inb(KBD_STATUS_REG);
-		}
+		/*
+		 * Skip mouse input.
+		 */
+		if (scanstatus & KBD_STAT_MOUSE_OBF)
+			continue;
 
-		if (scancode != 0x9c) {
-			/*
-			 * Wasn't an enter-release,  why not?
-			 */
-			kdb_printf("kdb: expected enter got 0x%x status 0x%x\n",
-			       scancode, scanstatus);
-		}
+		/*
+		 * If we see 0xe0, this is either a break code for KP
+		 * ENTER, or a repeat make for KP ENTER. Either way,
+		 * since the second byte is equivalent to an ENTER,
+		 * skip the 0xe0 and try again.
+		 *
+		 * If we see 0x1c, this must be a repeat ENTER or KP
+		 * ENTER (and we swallowed 0xe0 before). Try again.
+		 *
+		 * We can also see make and break codes for other keys
+		 * mashed before or after pressing ENTER. Thus, if we
+		 * see anything other than 0x9c, we have to try again.
+		 *
+		 * Note, if you held some key as ENTER was depressed,
+		 * that break code would get leaked out.
+		 */
+		if (scancode != 0x9c)
+			continue;
 
-		return 13;
+		return;
 	}
-
-	return keychar & 0xff;
 }
-EXPORT_SYMBOL_GPL(kdb_get_kbd_char);

kernel/debug/kdb/kdb_main.c +3 -0

index e2ae734..67b847d 100644
@@ -1400,6 +1400,9 @@ int kdb_main_loop(kdb_reason_t reason, kdb_reason_t reason2, int error,
 	if (KDB_STATE(DOING_SS))
 		KDB_STATE_CLEAR(SSBPT);
 
+	/* Clean up any keyboard devices before leaving */
+	kdb_kbd_cleanup_state();
+
 	return result;
 }

kernel/debug/kdb/kdb_private.h +7 -0

index e381d10..47c4e56 100644
@@ -246,6 +246,13 @@ extern void debug_kusage(void);
 
 extern void kdb_set_current_task(struct task_struct *);
 extern struct task_struct *kdb_current_task;
+
+#ifdef CONFIG_KDB_KEYBOARD
+extern void kdb_kbd_cleanup_state(void);
+#else /* ! CONFIG_KDB_KEYBOARD */
+#define kdb_kbd_cleanup_state()
+#endif /* ! CONFIG_KDB_KEYBOARD */
+
 #ifdef CONFIG_MODULES
 extern struct list_head *kdb_modules;
 #endif /* CONFIG_MODULES */

Signature · 1 line

1.7.5.4

Andrei Warkentin

Acked-by

2012-02-28 17:24 UTC #12 lore raw

Hi Jason,

----- Original Message -----

Quoted 10 lines

From: "Jason Wessel" <[email protected]>
To: "Andrei Warkentin" <[email protected]>
Cc: [email protected], [email protected], "Andrei Warkentin" <[email protected]>
Sent: Tuesday, February 28, 2012 8:53:20 AM
Subject: Re: [PATCH] KDB: Fix usability issues relating to the 'enter' key.
I think we are a lot closer this time. I attached a new patch based on your prior version.

Quoted 11 lines

This is the wrong place to do this. The cleanup needs to get executed
any time you are going to leave kdb_main(), because there are a few conditions like cpu switch, and transition to kgdb where you would leak the enter code on to the buffer handler without ever returning to
the OS. Also if you did not set CONFIG_KDB_KEYBOARD your patch did not compile properly. We could have fixed this by moving the code to the input cleanup in kgdboc, but this is not needed at all if you take a look at the revised version.

Oh I see now. Okay. I'll keep this in mind.

Quoted 8 lines

Quoted 4 lines
Fixed previous regression where if kbd was not used to 'g' + ENTER, the cleanup code would hang.
Was this a regression in the out of tree code or something in the mainline kdb?

The first patch I sent out didn't have the kbd_last_ret bit. I found this while using netkgdb and kgdboc together, but I'm certain this would also apply if you ran kgdboc=ttyS0,kbd and broke in/resumed from serial without ever touching the i8042 keyboard.

Quoted 6 lines

I added kbd_last_ret = 0; right here so this cannot get triggered a second time if the kdb main loop is entered and exited without going to the shell (which can happen on a soft single step operation on some archs).

Looks good to me! I gave it a spin as well.

Acked-by Andrei Warkentin <[email protected]>

Andrei Warkentin

2012-02-27 01:01 UTC #13 lore raw

In reply to Jason Wessel

Hi Jason,

Thank you for the review. Comments inline.

----- Original Message -----

Quoted 9 lines

From: "Jason Wessel" <[email protected]>
To: "Andrei Warkentin" <[email protected]>
Cc: [email protected], [email protected]
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.

Quoted 6 lines

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).

Quoted 4 lines

I do have a question about part of the section you deleted.

Quoted 8 lines

kernel/debug/kdb/kdb_keyboard.c int kdb_get_kbd_char(void) · line 186
Quoted 6 lines
kernel/debug/kdb/kdb_keyboard.c +0 -6
-	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 :-).

Quoted 11 lines

kernel/debug/kdb/kdb_keyboard.c int kdb_get_kbd_char(void) · line 194
Quoted 9 lines
kernel/debug/kdb/kdb_keyboard.c +0 -8
-
-		/*
-		 * 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.

Quoted 10 lines

kernel/debug/kdb/kdb_keyboard.c int kdb_get_kbd_char(void) · line 198
Quoted 5 lines
kernel/debug/kdb/kdb_keyboard.c +0 -4
-			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.