kern/63171: USB keyboard rollover error [ukbd.c patch]

[Brian Candler]

>Number:         63171
>Category:       kern
>Synopsis:       USB keyboard rollover error [ukbd.c patch]
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Sat Feb 21 06:40:14 PST 2004
>Closed-Date:
>Last-Modified:
>Originator:     Brian Candler
>Release:        FreeBSD 4.8-RELEASE i386
>Organization:
>Environment:
System: 4.8-RELEASE FreeBSD 4.8-RELEASE #5: Sat Feb 21 13:49:32 GMT 2004 root at vaio.linnet.org:/usr/src/sys/compile/VAIO i386
Sony Vaio PCG-C1F
USB Keyboard (Packard Bell PB-KB400)

	
>Description:
N-key rollover fails to work properly with a USB keyboard, giving duplicate
characters.

Thisi is a prorbleme when youo a rer typing veryr quiuckcly!

A second, unrelated issue is that there is a wrong pointer type being passed
to usbd_set_polling, which manifests itself as a compiler warning but could
cause memory corruption. The attached patch corrects both problems.

>How-To-Repeat:
	
Plug in a USB keyboard. Activate it using

#!/bin/sh
kbdcontrol -k /dev/kbd1 </dev/console
kbdcontrol -l /usr/share/syscons/keymaps/uk.cp850.kbd # optional
kbdcontrol -r slow    # makes it easier to demonstrate the problem

Perform the following operations:

Press "a", press "s", release "a", press "d"

The sequence "asds" (instead of "asd") is printed. If you keep "s" held and
keep releasing and pressing "d", you will get "ds" printed each time you
press "d".

If you enable USB debugging in the kernel, and set
# sysctl -w hw.usb.ukbd.debug=1
then you get

Feb 20 15:21:21 vaio /kernel: 0x428 (1064) released
Feb 20 15:21:21 vaio /kernel: 
Feb 20 15:21:26 vaio /kernel: 0x4 (4) pressed                 <<< a
Feb 20 15:21:26 vaio /kernel: 4 
Feb 20 15:21:26 vaio /kernel: 0x16 (22) pressed               <<< s
Feb 20 15:21:26 vaio /kernel: 4 22 
Feb 20 15:21:26 vaio /kernel: 0x404 (1028) released
Feb 20 15:21:26 vaio /kernel: 0x416 (1046) released
Feb 20 15:21:26 vaio /kernel: 22 
Feb 20 15:21:26 vaio /kernel: 0x7 (7) pressed                 <<< d
Feb 20 15:21:26 vaio /kernel: 0x16 (22) pressed               <<< s
Feb 20 15:21:26 vaio /kernel: 7 22 
Feb 20 15:21:26 vaio /kernel: 0x416 (1046) released
Feb 20 15:21:26 vaio /kernel: 7 
Feb 20 15:21:27 vaio /kernel: 0x407 (1031) released
Feb 20 15:21:27 vaio /kernel: 

This is even after updating my ukbd.c to the latest in CVS (rev 1.46).

>Fix:

The keyboard driver keeps a list of keys held (ks_ndata and ks_odata) and
looks for differences between them to identify key presses and releases.
However it makes the erroneous assumption that the first zero entry seen
indicates the end of the list. In fact, when you press the key sequence
above, you get

(1) _ _ _ _ _ _		Initial empty list
(2) a _ _ _ _ _		"a" pressed
(3) a s _ _ _ _		"s" pressed
(4) _ s _ _ _ _		"a" released
(5) d s _ _ _ _		"d" pressed

At least this is true with my USB keyboard; I haven't checked the HID spec,
but given that my keyboard works correctly under Windows98 with no rollover
problems I think it is entitled to behave in this way. The fix is very
straightforward, changing 'break' to 'continue' in a few places.

The pointer error looks like an oversight, and the last part of the patch
makes what seems to be the obvious correction, although I am not intimate
with the USB data structures so this ought to be checked by someone who is.
(usbd_set_polling requires a usbd_interface_handle, not a
usbd_device_handle, as its first parameter).

Regards,

Brian Candler.

--- sys/dev/usb/ukbd.c-1.46	Sat Feb 21 12:17:38 2004
+++ sys/dev/usb/ukbd.c	Sat Feb 21 13:47:56 2004
@@ -43,7 +43,7 @@
 __FBSDID("$FreeBSD: /repoman/r/ncvs/src/sys/dev/usb/ukbd.c,v 1.46 2004/01/03 15:01:04 sanpei Exp $");
 
 /*
- * HID spec: http://www.usb.org/developers/data/devclass/hid1_1.pdf
+ * HID spec: http://www.usb.org/developers/devclass_docs/HID1_11.pdf
  */
 
 #include "opt_kbd.h"
@@ -746,10 +746,10 @@
 	for (i = 0; i < NKEYCODE; i++) {
 		key = state->ks_odata.keycode[i];
 		if (key == 0)
-			break;
+			continue;
 		for (j = 0; j < NKEYCODE; j++) {
 			if (ud->keycode[j] == 0)
-				break;
+				continue;
 			if (key == ud->keycode[j])
 				goto rfound;
 		}
@@ -762,11 +762,11 @@
 	for (i = 0; i < NKEYCODE; i++) {
 		key = ud->keycode[i];
 		if (key == 0)
-			break;
+			continue;
 		state->ks_ntime[i] = now + kbd->kb_delay1;
 		for (j = 0; j < NKEYCODE; j++) {
 			if (state->ks_odata.keycode[j] == 0)
-				break;
+				continue;
 			if (key == state->ks_odata.keycode[j]) {
 				state->ks_ntime[i] = state->ks_otime[j];
 				if (state->ks_otime[j] > now)
@@ -1327,21 +1327,19 @@
 ukbd_poll(keyboard_t *kbd, int on)
 {
 	ukbd_state_t *state;
-	usbd_device_handle dev;
 	int s;
 
 	state = (ukbd_state_t *)kbd->kb_data;
-	usbd_interface2device_handle(state->ks_iface, &dev);
 
 	s = splusb();
 	if (on) {
 		if (state->ks_polling == 0)
-			usbd_set_polling(dev, on);
+			usbd_set_polling(state->ks_iface, on);
 		++state->ks_polling;
 	} else {
 		--state->ks_polling;
 		if (state->ks_polling == 0)
-			usbd_set_polling(dev, on);
+			usbd_set_polling(state->ks_iface, on);
 	}
 	splx(s);
 	return 0;
>Release-Note:
>Audit-Trail:
>Unformatted: