On 2/27/24 8:13 AM, Janne Grunau wrote:
Hej,
On Mon, Feb 26, 2024, at 21:47, Mark Kettenis wrote:
Date: Sun, 25 Feb 2024 22:57:23 +0100 From: Marek Vasut marex@denx.de
On 2/25/24 5:07 PM, Janne Grunau wrote:
Hej,
On Wed, Feb 21, 2024, at 13:41, Marek Vasut wrote:
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Hector Martin marcan@marcan.st
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.
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 4cbc9acb73..774d3555d9 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;}- }
I vaguely recall a discussion about previous version of this, I think the suggestion was to make the list of ignored devices configurable via environment variable, so users can add to that list from U-Boot shell. Would it be possible to make it work this way ?
oh, I completely forgot that this patch was already submitted. I briefly looked through asahi tree for related patches and did not check whether this was previously submitted. I've added environment based blocking as separate patch with blocking either complete vendor IDs or vendor, product ID combinations. A separate patch to simplify authorship tracking and the implementation doesn't share any code.
It would be better to have only one patch which does not hard-code any USB IDs, and then add those blocked IDs via U-Boot default environment for this specific machine. We cannot predict what yubico will do in the future, whether they might make a device that shouldn't be blocked for example. If they do, the user should be able to unblock their device by running e.g. '=> setenv usb_blocklist' instead of updating their bootloader.
I think a simple list of blocked VID:PID pairs, maybe with wildcards, would be nice, i.e. something like 'usb_blocklist=1234:5678,1050:*' to block device 0x1234:0x5678 and all devices with VID 0x1050 . That should be easy to parse with strtok()/strtol() or some such and the code should not be too complex.
I do like the idea of having a configurable list of usb devices to ignore. The U-Boot USB stack is still not perfect and there are still USB devices that will prevent us from booting when connected. The list will provide a nice workaround for that issue.
That sounds like we should ignore USB devices in usb_scan_device() and not in the keyboard driver.
Maybe that is even better and more generic solution ?
But the yubikeys will cause the same problem on other boards as well. So I think it makes sense to put those in a default list.
We could move the list to a CONFIG symbol which has Yubikey's vendor ID as default value now that we do string parsing anyway.
There is include/env_default.h to define default env variable values too.