[PATCH 0/2] USB fixes: Add missing timeout, ignore YubiKeys

This mini series fixes one bug, but in the process makes YubiKeys work, which then regresses people who have one *and* a USB keyboard, since we only support a single keyboard device.
Therefore patch #1 makes U-Boot ignore YubiKeys, so #2 does not regress things.
Signed-off-by: Hector Martin marcan@marcan.st --- Hector Martin (2): usb: kbd: Ignore Yubikeys usb: hub: Add missing reset recovery delay
common/usb_hub.c | 7 +++++++ common/usb_kbd.c | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+) --- base-commit: 8ad1c9c26f7740806a162818b790d4a72f515b7e change-id: 20231029-usb-fixes-2-976486d1603c
Best regards,

We currently only support one USB keyboard device, but some devices emulate keyboards for other purposes. Most commonly, people run into this with Yubikeys, so let's ignore those.
Even if we end up supporting multiple keyboards in the future, it's safer to ignore known non-keyboard devices.
This is particularly important to avoid regressing some users, since YubiKeys often *don't* work due to other bugs in the USB stack, but will start to work once they are fixed.
Signed-off-by: Hector Martin marcan@marcan.st --- common/usb_kbd.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 352d86fb2ece..e8c102c567e4 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -120,6 +120,15 @@ struct usb_kbd_pdata {
extern int __maybe_unused net_busy_flag;
+/* + * Since we only support one usbkbd device in the iomux, + * ignore common keyboard-emulating devices that aren't + * real keyboards. + */ +const uint16_t vid_blocklist[] = { + 0x1050, /* Yubico */ +}; + /* The period of time between two calls of usb_kbd_testc(). */ static unsigned long kbd_testc_tms;
@@ -465,6 +474,7 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) struct usb_endpoint_descriptor *ep; struct usb_kbd_pdata *data; int epNum; + int i;
if (dev->descriptor.bNumConfigurations != 1) return 0; @@ -480,6 +490,15 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) if (iface->desc.bInterfaceProtocol != USB_PROT_HID_KEYBOARD) return 0;
+ for (i = 0; i < ARRAY_SIZE(vid_blocklist); i++) { + if (dev->descriptor.idVendor == vid_blocklist[i]) { + printf("Ignoring keyboard device 0x%x:0x%x\n", + dev->descriptor.idVendor, + dev->descriptor.idProduct); + return 0; + } + } + for (epNum = 0; epNum < iface->desc.bNumEndpoints; epNum++) { ep = &iface->ep_desc[epNum];

On 10/29/23 08:09, Hector Martin wrote:
We currently only support one USB keyboard device, but some devices emulate keyboards for other purposes. Most commonly, people run into this with Yubikeys, so let's ignore those.
Even if we end up supporting multiple keyboards in the future, it's safer to ignore known non-keyboard devices.
This is particularly important to avoid regressing some users, since YubiKeys often *don't* work due to other bugs in the USB stack, but will start to work once they are fixed.
Signed-off-by: Hector Martin marcan@marcan.st
common/usb_kbd.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 352d86fb2ece..e8c102c567e4 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -120,6 +120,15 @@ struct usb_kbd_pdata {
extern int __maybe_unused net_busy_flag;
+/*
- Since we only support one usbkbd device in the iomux,
- ignore common keyboard-emulating devices that aren't
- real keyboards.
- */
+const uint16_t vid_blocklist[] = {
- 0x1050, /* Yubico */
I wonder if it would be better to have this default, but make the list configurable via environment variable too. This way, if users run into more weird devices, they could just:
=> setenv vid_blocklist "0x1234,0x5678" ; saveenv ; usb reset

Some devices like YubiKeys need more time before SET_ADDRESS. The spec says we need to wait 10ms.
Signed-off-by: Hector Martin marcan@marcan.st --- common/usb_hub.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/common/usb_hub.c b/common/usb_hub.c index ba11a188ca64..858ada0f73be 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -391,6 +391,13 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port) break; }
+ /* + * USB 2.0 7.1.7.5: devices must be able to accept a SetAddress() + * request (refer to Section 11.24.2 and Section 9.4 respectively) + * after the reset recovery time 10 ms + */ + mdelay(10); + #if CONFIG_IS_ENABLED(DM_USB) struct udevice *child;

On 10/29/23 08:09, Hector Martin wrote:
Some devices like YubiKeys need more time before SET_ADDRESS. The spec says we need to wait 10ms.
Signed-off-by: Hector Martin marcan@marcan.st
Reviewed-by: Marek Vasut marex@denx.de

On Sun, Oct 29, 2023 at 3:09 AM Hector Martin marcan@marcan.st wrote:
This mini series fixes one bug, but in the process makes YubiKeys work, which then regresses people who have one *and* a USB keyboard, since we only support a single keyboard device.
Therefore patch #1 makes U-Boot ignore YubiKeys, so #2 does not regress things.
Signed-off-by: Hector Martin marcan@marcan.st
Hector Martin (2): usb: kbd: Ignore Yubikeys usb: hub: Add missing reset recovery delay
common/usb_hub.c | 7 +++++++ common/usb_kbd.c | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+)
base-commit: 8ad1c9c26f7740806a162818b790d4a72f515b7e change-id: 20231029-usb-fixes-2-976486d1603c
Series LGTM.
Reviewed-by: Neal Gompa neal@gompa.dev
participants (3)
-
Hector Martin
-
Marek Vasut
-
Neal Gompa