Hej, On Mon, Feb 26, 2024, at 21:47, Mark Kettenis wrote: >> Date: Sun, 25 Feb 2024 22:57:23 +0100 >> From: Marek Vasut <ma...@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 <mar...@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 <mar...@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. > 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. Janne