Bug 253249

Summary: USB keyboard: repeated and out of order characters
Product: Base System Reporter: Jan Martin Mikkelsen <janm>
Component: usbAssignee: freebsd-usb (Nobody) <usb>
Status: New ---    
Severity: Affects Some People CC: freebsdbugs, hselasky, wulf
Priority: ---    
Version: 12.2-RELEASE   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
usbdump output when the problem is seen.
none
console output with hw.usb.ukbd.debug=2
none
Patch to filter out USB keyboard packets with only "key roll over" errors
none
ukbd_rollover.patch
none
ukbd_rollover.patch none

Description Jan Martin Mikkelsen 2021-02-04 15:13:38 UTC
Created attachment 222162 [details]
usbdump output when the problem is seen.

When typing quickly on a USB keyboard, some characters are repeated and some appear out of order.

Attached is the output of usbdump and the console messages with hw.usb.ukbd.debug=2 when typing "m_count" quickly. The characters presented are "m_counntu".

Three different keyboards tested: Two from Unicomp and a Dell keyboard. The Dell keyboard does slightly better but the problem still exists. I do not see the problem when using the same keyboards on a Macintosh.

This appears to be similar to the problem here: https://forums.freebsd.org/threads/mac-keyboard-weirdness-freebsd-12-2.77728/
Comment 1 Jan Martin Mikkelsen 2021-02-04 15:15:17 UTC
Created attachment 222163 [details]
console output with hw.usb.ukbd.debug=2
Comment 2 Hans Petter Selasky freebsd_committer 2021-02-04 15:28:27 UTC
Try setting:
hw.usb.ukbd.pollrate=1000

In /boot/loader.conf

and reboot. Some keyboards simply poll for input too slow and then this problem may happen.

--HPS
Comment 3 Jan Martin Mikkelsen 2021-02-04 15:36:46 UTC
(In reply to Hans Petter Selasky from comment #2)

Thanks. Unfortunately, no significant change.
Comment 4 Hans Petter Selasky freebsd_committer 2021-02-04 15:47:05 UTC
USB keybards can report multiple events at the same time.

If multiple events are detected between USB interrupt events, it is not possible to distinguish the order of the key events. This is undocumented behaviour!

Are these keyboards firmware upgradeable?

--HPS
Comment 5 Jan Martin Mikkelsen 2021-02-04 16:20:52 UTC
(In reply to Hans Petter Selasky from comment #4)

I do not believe they are firmware upgradeable.

A naïve reading of the USB trace by just looking at the USB HID key codes, gives me this sequence of events, from the 'c' keyboard down event.

'c' down:
 frame[0] READ 8 bytes
 0000  00 00 06 00 00 00 00 00  -- -- -- -- -- -- -- --  |........        |
15:50:33.960284 usbus0.4 SUBM-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=0,IVAL=10
 frame[0] READ 8 bytes

'c' down, 'o' down:
15:50:33.960285 usbus0.4 DONE-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=8,IVAL=10,ERR=0
 frame[0] READ 8 bytes
 0000  00 00 06 12 00 00 00 00  -- -- -- -- -- -- -- --  |........        |
15:50:34.000288 usbus0.4 SUBM-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=0,IVAL=10
 frame[0] READ 8 bytes

'c' up, 'o' down:
15:50:34.000289 usbus0.4 DONE-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=8,IVAL=10,ERR=0
 frame[0] READ 8 bytes
 0000  00 00 00 12 00 00 00 00  -- -- -- -- -- -- -- --  |........        |

'o' down, 'u' down:
15:50:34.032281 usbus0.4 SUBM-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=0,IVAL=10
 frame[0] READ 8 bytes
15:50:34.032283 usbus0.4 DONE-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=8,IVAL=10,ERR=0
 frame[0] READ 8 bytes
 0000  00 00 18 12 00 00 00 00  -- -- -- -- -- -- -- --  |........        |

'o' up, 'u' down:
15:50:34.064285 usbus0.4 SUBM-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=0,IVAL=10
 frame[0] READ 8 bytes
15:50:34.064286 usbus0.4 DONE-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=8,IVAL=10,ERR=0
 frame[0] READ 8 bytes
 0000  00 00 18 00 00 00 00 00  -- -- -- -- -- -- -- --  |........        |

'u' down, 'n' down:
15:50:34.096285 usbus0.4 SUBM-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=0,IVAL=10
 frame[0] READ 8 bytes
15:50:34.096286 usbus0.4 DONE-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=8,IVAL=10,ERR=0
 frame[0] READ 8 bytes
 0000  00 00 18 11 00 00 00 00  -- -- -- -- -- -- -- --  |........        |
15:50:34.136286 usbus0.4 SUBM-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=0,IVAL=10
 frame[0] READ 8 bytes

Not sure what this one is (I haven't looked at the protocol spec).
15:50:34.136287 usbus0.4 DONE-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=8,IVAL=10,ERR=0
 frame[0] READ 8 bytes
 0000  00 00 01 01 01 01 01 01  -- -- -- -- -- -- -- --  |........        |
15:50:34.149057 usbus0.4 SUBM-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=0,IVAL=10
 frame[0] READ 8 bytes

'u' down, 'n' down, 't' down:
15:50:34.149057 usbus0.4 DONE-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=8,IVAL=10,ERR=0
 frame[0] READ 8 bytes
 0000  00 00 18 11 17 00 00 00  -- -- -- -- -- -- -- --  |........        |
15:50:34.174101 usbus0.4 SUBM-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=0,IVAL=10
 frame[0] READ 8 bytes

'u' up, 'n' down, 't' down:
15:50:34.174101 usbus0.4 DONE-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=8,IVAL=10,ERR=0
 frame[0] READ 8 bytes
 0000  00 00 00 11 17 00 00 00  -- -- -- -- -- -- -- --  |........        |
15:50:34.264291 usbus0.4 SUBM-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=0,IVAL=10
 frame[0] READ 8 bytes

'n' up, 't' down:
15:50:34.264292 usbus0.4 DONE-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=8,IVAL=10,ERR=0
 frame[0] READ 8 bytes
 0000  00 00 00 00 17 00 00 00  -- -- -- -- -- -- -- --  |........        |
15:50:34.277144 usbus0.4 SUBM-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=0,IVAL=10
 frame[0] READ 8 bytes

Nothing down:
15:50:34.277145 usbus0.4 DONE-INTR-EP=00000081,SPD=LOW,NFR=1,SLEN=8,IVAL=10,ERR=0
 frame[0] READ 8 bytes
 0000  00 00 00 00 00 00 00 00  -- -- -- -- -- -- -- --  |........        |


That looks like a valid order to me, but I don't know the details of the protocol. I'm not sure how "counntu" could come from that sequence of events. 

With the 'u' at the end of the keystrokes that made it to userland, it almost looks like the packets were processed out of order. Is that possible?
Comment 6 Jan Martin Mikkelsen 2021-02-04 18:44:18 UTC
Having now looked at the spec and the code, I can see that the "00 00 01 01 01 01 01 01" packet is reporting a "roll over" error. Then ukbd.c marks all of the keys as released, and then on the next packet they go back down again.

I will change the ukbd.c to ignore a packet that looks like that and see how that goes.
Comment 7 Jan Martin Mikkelsen 2021-02-05 13:03:45 UTC
Created attachment 222178 [details]
Patch to filter out USB keyboard packets with only "key roll over" errors

The attached patch resolves the issue for me. There may well be a better (or more general) approach.
Comment 8 Hans Petter Selasky freebsd_committer 2021-02-05 13:13:07 UTC
wulf@ - can you have a look at this?
Comment 9 Vladimir Kondratyev freebsd_committer 2021-02-05 16:28:15 UTC
Created attachment 222185 [details]
ukbd_rollover.patch

Your patch assumes that keyboard uses boot protocol and all modifier keys are released (sc->sc_buffer[0] == 0).

I made a patch that does not depend on these assumptions. Could you test it?
It filters entire packet if any array item got the RollOver value. That should be ok as according to specs "The keyboard must report a phantom state indexing Usage(ErrorRollOver) in all array fields"
Comment 10 Jan Martin Mikkelsen 2021-02-05 16:54:26 UTC
(In reply to Vladimir Kondratyev from comment #9)


Thanks for the patch. Will do a build and let you know.
Comment 11 Vladimir Kondratyev freebsd_committer 2021-02-06 09:00:01 UTC
Created attachment 222202 [details]
ukbd_rollover.patch

Ouch. Previous patch have a bug. It is badly ported from hkbd(4) and do a return instead of goto tr_setup which stops keyboard input after RollOver condition.

Fixed version is reuploaded. I am sorry.
Comment 12 Jan Martin Mikkelsen 2021-02-06 15:30:08 UTC
(In reply to Vladimir Kondratyev from comment #11)

No problem. I ran out of time to test before I left the office on Friday, so will give this a try later.

Looking at the new patch, once you stop processing the packet, and just set up the next transfer instead, the "sc->sc_ndata = sc->sc_odata" assignment does not seem necessary. Am I missing something?
Comment 13 Vladimir Kondratyev freebsd_committer 2021-02-06 17:56:21 UTC
(In reply to Jan Martin Mikkelsen from comment #12)
> Am I missing something?
You forgot about repeat callout. It will fire and apply sc->sc_ndata after about a half a second from rollover packet.

Your patch does not suffer form callout issues as it goes to tr_setup before sc->sc_ndata is cleared.
Comment 14 Vladimir Kondratyev freebsd_committer 2021-02-06 18:25:55 UTC
(In reply to Vladimir Kondratyev from comment #13)
> from rollover packet.
That should be read as "from packet that precedes the rollover packet"
Comment 15 Jan Martin Mikkelsen 2021-02-12 10:02:18 UTC
(In reply to Vladimir Kondratyev from comment #11)

I just tested the new patch, it works for me. Thank you!

(Sorry about the delay, wasn't working from the office for most of this week.)
Comment 16 commit-hook freebsd_committer 2021-02-13 18:22:05 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=032d3153877ef1767c121bbdf8e00f4f93b30a5d

commit 032d3153877ef1767c121bbdf8e00f4f93b30a5d
Author:     Vladimir Kondratyev <wulf@FreeBSD.org>
AuthorDate: 2021-02-13 18:12:56 +0000
Commit:     Vladimir Kondratyev <wulf@FreeBSD.org>
CommitDate: 2021-02-13 18:12:56 +0000

    ukbd: Fix handling of keyboard ErrorRollOver reports

    Ignore fantom keyboard state reports entirelly rather than ignore
    RollOver states for each key separatelly.  Latter results in spurious
    release/push pairs of events on each fantom keyboard state report.

    Reported by:    Jan Martin Mikkelsen <janm_AT_transactionware_DOT_com>
    Submitted by:   Jan Martin Mikkelsen (initial version)
    PR:             253249
    MFC after:      1 week

 sys/dev/usb/input/ukbd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Comment 17 commit-hook freebsd_committer 2021-02-13 18:22:09 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=812c59ed614df94380e0b1f9ff4a3d15b78ce1bf

commit 812c59ed614df94380e0b1f9ff4a3d15b78ce1bf
Author:     Vladimir Kondratyev <wulf@FreeBSD.org>
AuthorDate: 2021-02-13 18:18:07 +0000
Commit:     Vladimir Kondratyev <wulf@FreeBSD.org>
CommitDate: 2021-02-13 18:18:07 +0000

    hkbd: Fix handling of keyboard ErrorRollOver reports

    Ignore fantom keyboard state reports entirelly rather than ignore
    RollOver states for each key separatelly.  Latter results in spurious
    release/push pairs of events on each fantom keyboard state report.

    Reported by:    Jan Martin Mikkelsen <janm_AT_transactionware_DOT_com>
    Submitted by:   Jan Martin Mikkelsen (initial version)
    PR:             253249
    MFC after:      1 week

 sys/dev/hid/hkbd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Comment 18 commit-hook freebsd_committer 2021-02-23 23:44:08 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=98b3658c4e8a3760a2e4f71a8a4a3dec2d55065a

commit 98b3658c4e8a3760a2e4f71a8a4a3dec2d55065a
Author:     Vladimir Kondratyev <wulf@FreeBSD.org>
AuthorDate: 2021-02-13 18:18:07 +0000
Commit:     Vladimir Kondratyev <wulf@FreeBSD.org>
CommitDate: 2021-02-23 23:41:49 +0000

    hkbd: Fix handling of keyboard ErrorRollOver reports

    Ignore fantom keyboard state reports entirelly rather than ignore
    RollOver states for each key separatelly.  Latter results in spurious
    release/push pairs of events on each fantom keyboard state report.

    Reported by:    Jan Martin Mikkelsen <janm_AT_transactionware_DOT_com>
    Submitted by:   Jan Martin Mikkelsen (initial version)
    PR:             253249
    MFC after:      1 week

    (cherry picked from commit 812c59ed614df94380e0b1f9ff4a3d15b78ce1bf)

 sys/dev/hid/hkbd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Comment 19 commit-hook freebsd_committer 2021-02-23 23:44:10 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=e7211ca03a85e3a980daf389af823fefb24b8869

commit e7211ca03a85e3a980daf389af823fefb24b8869
Author:     Vladimir Kondratyev <wulf@FreeBSD.org>
AuthorDate: 2021-02-13 18:12:56 +0000
Commit:     Vladimir Kondratyev <wulf@FreeBSD.org>
CommitDate: 2021-02-23 23:41:49 +0000

    ukbd: Fix handling of keyboard ErrorRollOver reports

    Ignore fantom keyboard state reports entirelly rather than ignore
    RollOver states for each key separatelly.  Latter results in spurious
    release/push pairs of events on each fantom keyboard state report.

    Reported by:    Jan Martin Mikkelsen <janm_AT_transactionware_DOT_com>
    Submitted by:   Jan Martin Mikkelsen (initial version)
    PR:             253249
    MFC after:      1 week

    (cherry picked from commit 032d3153877ef1767c121bbdf8e00f4f93b30a5d)

 sys/dev/usb/input/ukbd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Comment 20 commit-hook freebsd_committer 2021-02-23 23:45:11 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9bdb559d97268967e893b9f37dc556ae5dcd44fb

commit 9bdb559d97268967e893b9f37dc556ae5dcd44fb
Author:     Vladimir Kondratyev <wulf@FreeBSD.org>
AuthorDate: 2021-02-13 18:12:56 +0000
Commit:     Vladimir Kondratyev <wulf@FreeBSD.org>
CommitDate: 2021-02-23 23:43:28 +0000

    ukbd: Fix handling of keyboard ErrorRollOver reports

    Ignore fantom keyboard state reports entirelly rather than ignore
    RollOver states for each key separatelly.  Latter results in spurious
    release/push pairs of events on each fantom keyboard state report.

    Reported by:    Jan Martin Mikkelsen <janm_AT_transactionware_DOT_com>
    Submitted by:   Jan Martin Mikkelsen (initial version)
    PR:             253249
    MFC after:      1 week

    (cherry picked from commit 032d3153877ef1767c121bbdf8e00f4f93b30a5d)

 sys/dev/usb/input/ukbd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)