[opensuse-kernel] [PATCH] LKDB: Fix usability issues with 'enter'.
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 -
while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0);
However, it really should do something like -
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). Also, fix all tight inb loops to do cpu_relax(). A similar patch is being discussed with Jason Wessel for the mainline KDB. He brought up the point of stray make/break codes getting introduced (from the enter) on KDB exit, which is a problem that doesn't exist in mainline KDB (input device is reset). For LKDB, I could see that this can happen, although I haven't seen any strange behavior per se. I wanted to get some feedback before I modified kdba_local_arch_cleanup to flush the RX FIFO. Tested on ESX 5.0 and QEMU. Signed-off-by: Andrei Warkentin <andreiw@vmware.com> --- arch/x86/kdb/kdba_io.c | 67 ++++++++++++++++++++---------------------------- 1 files changed, 28 insertions(+), 39 deletions(-) diff --git a/arch/x86/kdb/kdba_io.c b/arch/x86/kdb/kdba_io.c index 8fdf59c..39cd7ce 100644 --- a/arch/x86/kdb/kdba_io.c +++ b/arch/x86/kdb/kdba_io.c @@ -46,10 +46,12 @@ static void kdb_kbdsend(unsigned char byte) { int timeout; - for (timeout = 200 * 1000; timeout && (inb(KBD_STATUS_REG) & KBD_STAT_IBF); timeout--); + for (timeout = 200 * 1000; timeout && (inb(KBD_STATUS_REG) & KBD_STAT_IBF); timeout--) + cpu_relax(); outb(byte, KBD_DATA_REG); udelay(40); - for (timeout = 200 * 1000; timeout && (~inb(KBD_STATUS_REG) & KBD_STAT_OBF); timeout--); + for (timeout = 200 * 1000; timeout && (~inb(KBD_STATUS_REG) & KBD_STAT_OBF); timeout--) + cpu_relax(); inb(KBD_DATA_REG); udelay(40); } @@ -281,31 +283,6 @@ static int get_kbd_char(void) } 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? - */ - lkdb_printf("kdb: expected enter got 0x%x status 0x%x\n", - scancode, scanstatus); - } - lkdb_printf("\n"); return 13; } @@ -376,21 +353,27 @@ void kdba_local_arch_setup(void) if (!kdb_check_kbd_exists()) return; - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_command(KBD_CCMD_READ_MODE); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); for (timeout = 200 * 1000; timeout && - (!(kbd_read_status() & KBD_STAT_OBF)); timeout--); + (!(kbd_read_status() & KBD_STAT_OBF)); timeout--) + cpu_relax(); c = kbd_read_input(); c &= ~KBD_MODE_KBD_INT; - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_command(KBD_CCMD_WRITE_MODE); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_output(c); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); mdelay(1); #endif /* CONFIG_VT_CONSOLE */ } @@ -404,21 +387,27 @@ void kdba_local_arch_cleanup(void) if (!kdb_check_kbd_exists()) return; - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_command(KBD_CCMD_READ_MODE); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); for (timeout = 200 * 1000; timeout && - (!(kbd_read_status() & KBD_STAT_OBF)); timeout--); + (!(kbd_read_status() & KBD_STAT_OBF)); timeout--) + cpu_relax(); c = kbd_read_input(); c |= KBD_MODE_KBD_INT; - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_command(KBD_CCMD_WRITE_MODE); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_output(c); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); mdelay(1); #endif /* CONFIG_VT_CONSOLE */ } -- 1.7.8.3 -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-kernel+owner@opensuse.org
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/27/2012 01:02 AM, Andrei Warkentin wrote:
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.
None of the openSUSE kernels contain kdba_io. Was this intended for SLES? If so, please go through Bugzilla. The openSUSE list isn't for SLES discussion. - -Jeff
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 -
while ((inb(KBD_STATUS_REG) & KBD_STAT_OBF) == 0);
However, it really should do something like -
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). Also, fix all tight inb loops to do cpu_relax().
A similar patch is being discussed with Jason Wessel for the mainline KDB.
He brought up the point of stray make/break codes getting introduced (from the enter) on KDB exit, which is a problem that doesn't exist in mainline KDB (input device is reset). For LKDB, I could see that this can happen, although I haven't seen any strange behavior per se. I wanted to get some feedback before I modified kdba_local_arch_cleanup to flush the RX FIFO.
Tested on ESX 5.0 and QEMU.
Signed-off-by: Andrei Warkentin <andreiw@vmware.com> --- arch/x86/kdb/kdba_io.c | 67 ++++++++++++++++++++---------------------------- 1 files changed, 28 insertions(+), 39 deletions(-)
diff --git a/arch/x86/kdb/kdba_io.c b/arch/x86/kdb/kdba_io.c index 8fdf59c..39cd7ce 100644 --- a/arch/x86/kdb/kdba_io.c +++ b/arch/x86/kdb/kdba_io.c @@ -46,10 +46,12 @@ static void kdb_kbdsend(unsigned char byte) { int timeout; - for (timeout = 200 * 1000; timeout && (inb(KBD_STATUS_REG) & KBD_STAT_IBF); timeout--); + for (timeout = 200 * 1000; timeout && (inb(KBD_STATUS_REG) & KBD_STAT_IBF); timeout--) + cpu_relax(); outb(byte, KBD_DATA_REG); udelay(40); - for (timeout = 200 * 1000; timeout && (~inb(KBD_STATUS_REG) & KBD_STAT_OBF); timeout--); + for (timeout = 200 * 1000; timeout && (~inb(KBD_STATUS_REG) & KBD_STAT_OBF); timeout--) + cpu_relax(); inb(KBD_DATA_REG); udelay(40); } @@ -281,31 +283,6 @@ static int get_kbd_char(void) }
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? - */ - lkdb_printf("kdb: expected enter got 0x%x status 0x%x\n", - scancode, scanstatus); - } - lkdb_printf("\n"); return 13; } @@ -376,21 +353,27 @@ void kdba_local_arch_setup(void) if (!kdb_check_kbd_exists()) return;
- while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_command(KBD_CCMD_READ_MODE); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); for (timeout = 200 * 1000; timeout && - (!(kbd_read_status() & KBD_STAT_OBF)); timeout--); + (!(kbd_read_status() & KBD_STAT_OBF)); timeout--) + cpu_relax(); c = kbd_read_input(); c &= ~KBD_MODE_KBD_INT; - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_command(KBD_CCMD_WRITE_MODE); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_output(c); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); mdelay(1); #endif /* CONFIG_VT_CONSOLE */ } @@ -404,21 +387,27 @@ void kdba_local_arch_cleanup(void) if (!kdb_check_kbd_exists()) return;
- while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_command(KBD_CCMD_READ_MODE); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); for (timeout = 200 * 1000; timeout && - (!(kbd_read_status() & KBD_STAT_OBF)); timeout--); + (!(kbd_read_status() & KBD_STAT_OBF)); timeout--) + cpu_relax(); c = kbd_read_input(); c |= KBD_MODE_KBD_INT; - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_command(KBD_CCMD_WRITE_MODE); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); kbd_write_output(c); mdelay(1); - while (kbd_read_status() & KBD_STAT_IBF); + while (kbd_read_status() & KBD_STAT_IBF) + cpu_relax(); mdelay(1); #endif /* CONFIG_VT_CONSOLE */ }
- -- Jeff Mahoney SUSE Labs -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQIcBAEBAgAGBQJPpEyzAAoJEB57S2MheeWynysP/3RQefnrew9SXhVTNMq9BbII xUnzg8qpdbXC4WeKYilQgSKdkfSKi/Kcaw6yjiv4MYZQXTrSdBCFmTgZCQ0YhRJw +rX3LY73aIpJha5WsxCb/nklsmi1Kti4fvT2S6f0AWR7pZ4pXExH5U9/w2mkv7ad rWdB+LGf1DsuC3HnkRqhlSlBz0JFePvXibDTLrnBN3fw56nVcegEM8IHMI77dD96 9JvoCiglvsPb3wSmnvjU2RMiMFQsPDK7TlJf1avLULtOwPe4ZN2qgLDLTbWrTgb6 BfEJA9yKU3e2pPvBadGEZqkCbZybvVLhQmZ33NCGSOflZ7vVDJE1OkPfk5tWReZx OzYhTa+4GjXFfM/monQ6Cks3F99QSBqv/sMIFmxtG2WnzXJMval13mk5lV1zfnDv Kw+yTu7GKAPosnoJ/7hJfKQ+g9BTr3Q4CPj91TraHByqtndDitO38xJvKnuOaWsp bASKULczPh73WW1HVaIPUArdgY+baK5f6VJeYZmcshiEC1cETTGeH8MTrBq6Cbbn cuxw7YO98tpmAjAmv5Q1eB7ePNKahq7qZc3U+hv2KoN35tt0SZC2Ensc0VdX5obl UMNyslGcqsR//JODgv/4hIf1BWKDy4fhCyzzKJIFXm7OLYDIPTIL88T5AnUoCeAh KfE+voCo4XGOcKP+W+Iy =HJIE -----END PGP SIGNATURE----- -- To unsubscribe, e-mail: opensuse-kernel+unsubscribe@opensuse.org To contact the owner, e-mail: opensuse-kernel+owner@opensuse.org
participants (2)
-
Andrei Warkentin -
Jeff Mahoney