[PATCH 0/6] USB keyboard improvements for asahi / desktop systems
Apple USB Keyboards from 2021 need quirks to be useable. The boot HID keyboard protocol is unfortunately not described in the first interface descriptor but the second. This needs several changes. The USB keyboard driver has to look at all (2) interface descriptors during probing. Since I didn't want to rebuild the USB driver probe code the Apple keyboards are bound to the keyboard driver via USB vendor and product IDs. To make the keyboards useable on Apple silicon devices the xhci driver needs to initializes rings for the endpoints of the first two interface descriptors. If this is causes concerns regarding regressions or memory use the USB_MAX_ACTIVE_INTERFACES define could be turned into a CONFIG option. Even after this changes the keyboards still do not probe successfully since they apparently do not behave HID standard compliant. They only generate reports on key events. This leads the final check whether the keyboard is operational to fail unless the user presses keys during the probe. Skip this check for known keyboards. Keychron seems to emulate Apple keyboards (some models even "re-use" Apple's USB vendor ID) so apply this quirk as well.
Some devices like Yubikeys emulate a keyboard. since u-boot only binds a single keyboard block this kind of devices from the USB keyboard driver.
Signed-off-by: Janne Grunau j@jannau.net --- Hector Martin (1): usb: kbd: Ignore Yubikeys
Janne Grunau (5): usb: xhci: refactor xhci_set_configuration usb: xhci: Set up endpoints for the first 2 interfaces usb: xhci: Abort transfers with unallocated rings usb: kbd: support Apple Magic Keyboards (2021) usb: kbd: Add probe quirk for Apple and Keychron keyboards
common/usb_kbd.c | 80 +++++++++++++++++++++++++-- drivers/usb/host/xhci-ring.c | 5 ++ drivers/usb/host/xhci.c | 126 +++++++++++++++++++++++++++---------------- include/usb.h | 6 +++ 4 files changed, 167 insertions(+), 50 deletions(-) --- base-commit: 37345abb97ef0dd9c50a03b2a72617612dcae585 change-id: 20240218-asahi-keyboards-f2ddaf0022b2
Best regards,
From: Janne Grunau j@jannau.net
In the next step endpoints for multiple interfaces are set up. Move most of the per endpoint initialization to separate function to avoid another identation level.
Signed-off-by: Janne Grunau j@jannau.net --- drivers/usb/host/xhci.c | 119 +++++++++++++++++++++++++++++------------------- 1 file changed, 73 insertions(+), 46 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index d13cbff9b3..534c4b973f 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -475,67 +475,34 @@ static int xhci_configure_endpoints(struct usb_device *udev, bool ctx_change) }
/** - * Configure the endpoint, programming the device contexts. + * Fill endpoint contexts for interface descriptor ifdesc. * - * @param udev pointer to the USB device structure - * Return: returns the status of the xhci_configure_endpoints + * @param udev pointer to the USB device structure + * @param ctrl pointer to the xhci pravte device structure + * @param virt_dev pointer to the xhci virtual device structure + * @param ifdesc pointer to the USB interface config descriptor + * Return: returns the status of xhci_init_ep_contexts_if */ -static int xhci_set_configuration(struct usb_device *udev) +static int xhci_init_ep_contexts_if(struct usb_device *udev, + struct xhci_ctrl *ctrl, + struct xhci_virt_device *virt_dev, + struct usb_interface *ifdesc + ) { - struct xhci_container_ctx *in_ctx; - struct xhci_container_ctx *out_ctx; - struct xhci_input_control_ctx *ctrl_ctx; - struct xhci_slot_ctx *slot_ctx; struct xhci_ep_ctx *ep_ctx[MAX_EP_CTX_NUM]; int cur_ep; - int max_ep_flag = 0; int ep_index; unsigned int dir; unsigned int ep_type; - struct xhci_ctrl *ctrl = xhci_get_ctrl(udev); - int num_of_ep; - int ep_flag = 0; u64 trb_64 = 0; - int slot_id = udev->slot_id; - struct xhci_virt_device *virt_dev = ctrl->devs[slot_id]; - struct usb_interface *ifdesc; u32 max_esit_payload; unsigned int interval; unsigned int mult; unsigned int max_burst; unsigned int avg_trb_len; unsigned int err_count = 0; + int num_of_ep = ifdesc->no_of_ep;
- out_ctx = virt_dev->out_ctx; - in_ctx = virt_dev->in_ctx; - - num_of_ep = udev->config.if_desc[0].no_of_ep; - ifdesc = &udev->config.if_desc[0]; - - ctrl_ctx = xhci_get_input_control_ctx(in_ctx); - /* Initialize the input context control */ - ctrl_ctx->add_flags = cpu_to_le32(SLOT_FLAG); - ctrl_ctx->drop_flags = 0; - - /* EP_FLAG gives values 1 & 4 for EP1OUT and EP2IN */ - for (cur_ep = 0; cur_ep < num_of_ep; cur_ep++) { - ep_flag = xhci_get_ep_index(&ifdesc->ep_desc[cur_ep]); - ctrl_ctx->add_flags |= cpu_to_le32(1 << (ep_flag + 1)); - if (max_ep_flag < ep_flag) - max_ep_flag = ep_flag; - } - - xhci_inval_cache((uintptr_t)out_ctx->bytes, out_ctx->size); - - /* slot context */ - xhci_slot_copy(ctrl, in_ctx, out_ctx); - slot_ctx = xhci_get_slot_ctx(ctrl, in_ctx); - slot_ctx->dev_info &= ~(cpu_to_le32(LAST_CTX_MASK)); - slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(max_ep_flag + 1) | 0); - - xhci_endpoint_copy(ctrl, in_ctx, out_ctx, 0); - - /* filling up ep contexts */ for (cur_ep = 0; cur_ep < num_of_ep; cur_ep++) { struct usb_endpoint_descriptor *endpt_desc = NULL; struct usb_ss_ep_comp_descriptor *ss_ep_comp_desc = NULL; @@ -561,7 +528,8 @@ static int xhci_set_configuration(struct usb_device *udev) avg_trb_len = max_esit_payload;
ep_index = xhci_get_ep_index(endpt_desc); - ep_ctx[ep_index] = xhci_get_ep_ctx(ctrl, in_ctx, ep_index); + ep_ctx[ep_index] = xhci_get_ep_ctx(ctrl, virt_dev->in_ctx, + ep_index);
/* Allocate the ep rings */ virt_dev->eps[ep_index].ring = xhci_ring_alloc(ctrl, 1, true); @@ -614,6 +582,65 @@ static int xhci_set_configuration(struct usb_device *udev) } }
+ return 0; +} + +/** + * Configure the endpoint, programming the device contexts. + * + * @param udev pointer to the USB device structure + * Return: returns the status of the xhci_configure_endpoints + */ +static int xhci_set_configuration(struct usb_device *udev) +{ + struct xhci_container_ctx *out_ctx; + struct xhci_container_ctx *in_ctx; + struct xhci_input_control_ctx *ctrl_ctx; + struct xhci_slot_ctx *slot_ctx; + int err; + int cur_ep; + int max_ep_flag = 0; + struct xhci_ctrl *ctrl = xhci_get_ctrl(udev); + int num_of_ep; + int ep_flag = 0; + int slot_id = udev->slot_id; + struct xhci_virt_device *virt_dev = ctrl->devs[slot_id]; + struct usb_interface *ifdesc; + + out_ctx = virt_dev->out_ctx; + in_ctx = virt_dev->in_ctx; + + num_of_ep = udev->config.if_desc[0].no_of_ep; + ifdesc = &udev->config.if_desc[0]; + + ctrl_ctx = xhci_get_input_control_ctx(in_ctx); + /* Initialize the input context control */ + ctrl_ctx->add_flags = cpu_to_le32(SLOT_FLAG); + ctrl_ctx->drop_flags = 0; + + /* EP_FLAG gives values 1 & 4 for EP1OUT and EP2IN */ + for (cur_ep = 0; cur_ep < num_of_ep; cur_ep++) { + ep_flag = xhci_get_ep_index(&ifdesc->ep_desc[cur_ep]); + ctrl_ctx->add_flags |= cpu_to_le32(1 << (ep_flag + 1)); + if (max_ep_flag < ep_flag) + max_ep_flag = ep_flag; + } + + xhci_inval_cache((uintptr_t)out_ctx->bytes, out_ctx->size); + + /* slot context */ + xhci_slot_copy(ctrl, in_ctx, out_ctx); + slot_ctx = xhci_get_slot_ctx(ctrl, in_ctx); + slot_ctx->dev_info &= ~(cpu_to_le32(LAST_CTX_MASK)); + slot_ctx->dev_info |= cpu_to_le32(LAST_CTX(max_ep_flag + 1) | 0); + + xhci_endpoint_copy(ctrl, in_ctx, out_ctx, 0); + + /* filling up ep contexts */ + err = xhci_init_ep_contexts_if(udev, ctrl, virt_dev, ifdesc); + if (err < 0) + return err; + return xhci_configure_endpoints(udev, false); }
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Janne Grunau j@jannau.net
In the next step endpoints for multiple interfaces are set up. Move most of the per endpoint initialization to separate function to avoid another identation level.
Signed-off-by: Janne Grunau j@jannau.net
Reviewed-by: Marek Vasut marex@denx.de
From: Janne Grunau j@jannau.net
Apple USB keyboards carry the HID keyboard boot protocol on the second interface. Using the second interface in the USB keyboard driver does not work since the xhci has not allocated a transfer ring. --- drivers/usb/host/xhci.c | 31 +++++++++++++++++++------------ include/usb.h | 6 ++++++ 2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 534c4b973f..741e186ee0 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -606,24 +606,28 @@ static int xhci_set_configuration(struct usb_device *udev) int slot_id = udev->slot_id; struct xhci_virt_device *virt_dev = ctrl->devs[slot_id]; struct usb_interface *ifdesc; + unsigned int ifnum; + unsigned int max_ifnum = min((unsigned int)USB_MAX_ACTIVE_INTERFACES, + (unsigned int)udev->config.no_of_if);
out_ctx = virt_dev->out_ctx; in_ctx = virt_dev->in_ctx;
- num_of_ep = udev->config.if_desc[0].no_of_ep; - ifdesc = &udev->config.if_desc[0]; - ctrl_ctx = xhci_get_input_control_ctx(in_ctx); /* Initialize the input context control */ ctrl_ctx->add_flags = cpu_to_le32(SLOT_FLAG); ctrl_ctx->drop_flags = 0;
- /* EP_FLAG gives values 1 & 4 for EP1OUT and EP2IN */ - for (cur_ep = 0; cur_ep < num_of_ep; cur_ep++) { - ep_flag = xhci_get_ep_index(&ifdesc->ep_desc[cur_ep]); - ctrl_ctx->add_flags |= cpu_to_le32(1 << (ep_flag + 1)); - if (max_ep_flag < ep_flag) - max_ep_flag = ep_flag; + for (ifnum = 0; ifnum < max_ifnum; ifnum++) { + ifdesc = &udev->config.if_desc[ifnum]; + num_of_ep = ifdesc->no_of_ep; + /* EP_FLAG gives values 1 & 4 for EP1OUT and EP2IN */ + for (cur_ep = 0; cur_ep < num_of_ep; cur_ep++) { + ep_flag = xhci_get_ep_index(&ifdesc->ep_desc[cur_ep]); + ctrl_ctx->add_flags |= cpu_to_le32(1 << (ep_flag + 1)); + if (max_ep_flag < ep_flag) + max_ep_flag = ep_flag; + } }
xhci_inval_cache((uintptr_t)out_ctx->bytes, out_ctx->size); @@ -637,9 +641,12 @@ static int xhci_set_configuration(struct usb_device *udev) xhci_endpoint_copy(ctrl, in_ctx, out_ctx, 0);
/* filling up ep contexts */ - err = xhci_init_ep_contexts_if(udev, ctrl, virt_dev, ifdesc); - if (err < 0) - return err; + for (ifnum = 0; ifnum < max_ifnum; ifnum++) { + ifdesc = &udev->config.if_desc[ifnum]; + err = xhci_init_ep_contexts_if(udev, ctrl, virt_dev, ifdesc); + if (err < 0) + return err; + }
return xhci_configure_endpoints(udev, false); } diff --git a/include/usb.h b/include/usb.h index 09e3f0cb30..3aafdc8bfd 100644 --- a/include/usb.h +++ b/include/usb.h @@ -49,6 +49,12 @@ extern bool usb_started; /* flag for the started/stopped USB status */ */ #define USB_TIMEOUT_MS(pipe) (usb_pipebulk(pipe) ? 5000 : 1000)
+/* + * The xhcd hcd driver prepares only a limited number interfaces / endpoints. + * Define this limit so that drivers do not exceed it. + */ +#define USB_MAX_ACTIVE_INTERFACES 2 + /* device request (setup) */ struct devrequest { __u8 requesttype;
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Janne Grunau j@jannau.net
Apple USB keyboards carry the HID keyboard boot protocol on the second interface. Using the second interface in the USB keyboard driver does not work since the xhci has not allocated a transfer ring.
So, what does this patch do ? That is not clear from the commit message.
btw. missing SoB line.
On Wed, Feb 21, 2024, at 13:39, Marek Vasut wrote:
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Janne Grunau j@jannau.net
Apple USB keyboards carry the HID keyboard boot protocol on the second interface. Using the second interface in the USB keyboard driver does not work since the xhci has not allocated a transfer ring.
So, what does this patch do ? That is not clear from the commit message.
rewritten for v2: | usb: xhci: Set up endpoints for the first 2 interfaces | | The xhci driver currently only does the necessary initialization for | endpoints found in the first interface descriptor. Apple USB keyboards | (released 2021) use the second interface descriptor for the HID keyboard | boot protocol. To allow USB drivers to use endpoints from other | interface descriptors the xhci driver needs to ensure these endpoints | are initialized as well. | Use USB_MAX_ACTIVE_INTERFACES to control how many interface descriptors | are initialized and useable. Currently defined to 2 as that is enough to | make the Apple keyboard usable.
btw. missing SoB line.
added
Thanks
Janne
On 2/25/24 4:28 PM, Janne Grunau wrote:
On Wed, Feb 21, 2024, at 13:39, Marek Vasut wrote:
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Janne Grunau j@jannau.net
Apple USB keyboards carry the HID keyboard boot protocol on the second interface. Using the second interface in the USB keyboard driver does not work since the xhci has not allocated a transfer ring.
So, what does this patch do ? That is not clear from the commit message.
rewritten for v2: | usb: xhci: Set up endpoints for the first 2 interfaces | | The xhci driver currently only does the necessary initialization for | endpoints found in the first interface descriptor. Apple USB keyboards | (released 2021) use the second interface descriptor for the HID keyboard | boot protocol. To allow USB drivers to use endpoints from other | interface descriptors the xhci driver needs to ensure these endpoints | are initialized as well. | Use USB_MAX_ACTIVE_INTERFACES to control how many interface descriptors | are initialized and useable. Currently defined to 2 as that is enough to | make the Apple keyboard usable.
Would it make sense to make this a tunable Kconfig option ?
Date: Sun, 25 Feb 2024 22:47:41 +0100 From: Marek Vasut marex@denx.de
On 2/25/24 4:28 PM, Janne Grunau wrote:
On Wed, Feb 21, 2024, at 13:39, Marek Vasut wrote:
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Janne Grunau j@jannau.net
Apple USB keyboards carry the HID keyboard boot protocol on the second interface. Using the second interface in the USB keyboard driver does not work since the xhci has not allocated a transfer ring.
So, what does this patch do ? That is not clear from the commit message.
rewritten for v2: | usb: xhci: Set up endpoints for the first 2 interfaces | | The xhci driver currently only does the necessary initialization for | endpoints found in the first interface descriptor. Apple USB keyboards | (released 2021) use the second interface descriptor for the HID keyboard | boot protocol. To allow USB drivers to use endpoints from other | interface descriptors the xhci driver needs to ensure these endpoints | are initialized as well. | Use USB_MAX_ACTIVE_INTERFACES to control how many interface descriptors | are initialized and useable. Currently defined to 2 as that is enough to | make the Apple keyboard usable.
Would it make sense to make this a tunable Kconfig option ?
Maybe, but it should probably be enabled everywhere where CONFIG_USB_KEYBOARD as folks will connect their Apple keyboard to non-Apple hardware as well. And I think someone mentioned on the #asahi irc channel that there are othe keyboards that have the boot protocol on the second interface descriptor.
Cheers,
Mark
Hej,
On Sun, Feb 25, 2024, at 22:47, Marek Vasut wrote:
On 2/25/24 4:28 PM, Janne Grunau wrote:
On Wed, Feb 21, 2024, at 13:39, Marek Vasut wrote:
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Janne Grunau j@jannau.net
Apple USB keyboards carry the HID keyboard boot protocol on the second interface. Using the second interface in the USB keyboard driver does not work since the xhci has not allocated a transfer ring.
So, what does this patch do ? That is not clear from the commit message.
rewritten for v2: | usb: xhci: Set up endpoints for the first 2 interfaces | | The xhci driver currently only does the necessary initialization for | endpoints found in the first interface descriptor. Apple USB keyboards | (released 2021) use the second interface descriptor for the HID keyboard | boot protocol. To allow USB drivers to use endpoints from other | interface descriptors the xhci driver needs to ensure these endpoints | are initialized as well. | Use USB_MAX_ACTIVE_INTERFACES to control how many interface descriptors | are initialized and useable. Currently defined to 2 as that is enough to | make the Apple keyboard usable.
Would it make sense to make this a tunable Kconfig option ?
I thought about that but I don't think it's worth it. We would have to default it to 2 (at least when the USB keyboard driver is enabled) since we can't predict which devices will be connected. Why would anyone chose a different value than the fixed value? I can't think of convincing reasons.
Janne
On 2/27/24 8:38 AM, Janne Grunau wrote:
Hej,
On Sun, Feb 25, 2024, at 22:47, Marek Vasut wrote:
On 2/25/24 4:28 PM, Janne Grunau wrote:
On Wed, Feb 21, 2024, at 13:39, Marek Vasut wrote:
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Janne Grunau j@jannau.net
Apple USB keyboards carry the HID keyboard boot protocol on the second interface. Using the second interface in the USB keyboard driver does not work since the xhci has not allocated a transfer ring.
So, what does this patch do ? That is not clear from the commit message.
rewritten for v2: | usb: xhci: Set up endpoints for the first 2 interfaces | | The xhci driver currently only does the necessary initialization for | endpoints found in the first interface descriptor. Apple USB keyboards | (released 2021) use the second interface descriptor for the HID keyboard | boot protocol. To allow USB drivers to use endpoints from other | interface descriptors the xhci driver needs to ensure these endpoints | are initialized as well. | Use USB_MAX_ACTIVE_INTERFACES to control how many interface descriptors | are initialized and useable. Currently defined to 2 as that is enough to | make the Apple keyboard usable.
Would it make sense to make this a tunable Kconfig option ?
I thought about that but I don't think it's worth it. We would have to default it to 2 (at least when the USB keyboard driver is enabled) since we can't predict which devices will be connected. Why would anyone chose a different value than the fixed value? I can't think of convincing reasons.
All right
Reviewed-by: Marek Vasut marex@denx.de
From: Janne Grunau j@jannau.net
Discovered while trying to use the second interface in the USB keyboard driver necessary on Apple USB keyboards.
Signed-off-by: Janne Grunau j@jannau.net --- drivers/usb/host/xhci-ring.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index b60661fe05..4446f5f098 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -685,6 +685,9 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, reset_ep(udev, ep_index);
ring = virt_dev->eps[ep_index].ring; + if (!ring) + return -1; + /* * How much data is (potentially) left before the 64KB boundary? * XHCI Spec puts restriction( TABLE 49 and 6.4.1 section of XHCI Spec) @@ -871,6 +874,8 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long pipe, ep_index = usb_pipe_ep_index(pipe);
ep_ring = virt_dev->eps[ep_index].ring; + if (!ep_ring) + return -1;
/* * Check to see if the max packet size for the default control
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Janne Grunau j@jannau.net
Discovered while trying to use the second interface in the USB keyboard driver necessary on Apple USB keyboards.
Signed-off-by: Janne Grunau j@jannau.net
drivers/usb/host/xhci-ring.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index b60661fe05..4446f5f098 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -685,6 +685,9 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, reset_ep(udev, ep_index);
ring = virt_dev->eps[ep_index].ring;
- if (!ring)
return -1;
Would it make sense to return e.g. -EINVAL or some other errno ?
Hej,
On Wed, Feb 21, 2024, at 13:40, Marek Vasut wrote:
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Janne Grunau j@jannau.net
Discovered while trying to use the second interface in the USB keyboard driver necessary on Apple USB keyboards.
Signed-off-by: Janne Grunau j@jannau.net
drivers/usb/host/xhci-ring.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index b60661fe05..4446f5f098 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -685,6 +685,9 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, reset_ep(udev, ep_index);
ring = virt_dev->eps[ep_index].ring;
- if (!ring)
return -1;Would it make sense to return e.g. -EINVAL or some other errno ?
Agreed, -EINVAL is good match as ep_index is directly derived from pipe argument.
Janne
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; + } + } + for (epNum = 0; epNum < iface->desc.bNumEndpoints; epNum++) { ep = &iface->ep_desc[epNum];
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 ?
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.
Janne
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.
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.
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.
On 2/26/24 9:47 PM, 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
Very far from perfect, yes.
and there are still USB devices that will prevent us from booting when connected. The list will provide a nice workaround for that issue.
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.
I agree.
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.
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
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.
From: Janne Grunau j@jannau.net
Apple USB keyboards (Magic Keyboard from 2021 (product id 0x029c)) carry the HID keyboard boot protocol on the second interface descriptor. Probe via vendor and product IDs since the class/subclass/protocol match uses the first interface descriptor. Probe the two first interface descriptors for the HID keyboard boot protocol.
USB configuration descriptor for reference:
| Bus 003 Device 002: ID 05ac:029c Apple, Inc. Magic Keyboard | Device Descriptor: | bLength 18 | bDescriptorType 1 | bcdUSB 2.00 | bDeviceClass 0 [unknown] | bDeviceSubClass 0 [unknown] | bDeviceProtocol 0 | bMaxPacketSize0 64 | idVendor 0x05ac Apple, Inc. | idProduct 0x029c Magic Keyboard | bcdDevice 3.90 | iManufacturer 1 Apple Inc. | iProduct 2 Magic Keyboard | iSerial 3 ... | bNumConfigurations 1 | Configuration Descriptor: | bLength 9 | bDescriptorType 2 | wTotalLength 0x003b | bNumInterfaces 2 | bConfigurationValue 1 | iConfiguration 4 Keyboard | bmAttributes 0xa0 | (Bus Powered) | Remote Wakeup | MaxPower 500mA | Interface Descriptor: | bLength 9 | bDescriptorType 4 | bInterfaceNumber 0 | bAlternateSetting 0 | bNumEndpoints 1 | bInterfaceClass 3 Human Interface Device | bInterfaceSubClass 0 [unknown] | bInterfaceProtocol 0 | iInterface 5 Device Management | HID Device Descriptor: | bLength 9 | bDescriptorType 33 | bcdHID 1.10 | bCountryCode 0 Not supported | bNumDescriptors 1 | bDescriptorType 34 Report | wDescriptorLength 83 | Report Descriptors: | ** UNAVAILABLE ** | Endpoint Descriptor: | bLength 7 | bDescriptorType 5 | bEndpointAddress 0x81 EP 1 IN | bmAttributes 3 | Transfer Type Interrupt | Synch Type None | Usage Type Data | wMaxPacketSize 0x0010 1x 16 bytes | bInterval 8 | Interface Descriptor: | bLength 9 | bDescriptorType 4 | bInterfaceNumber 1 | bAlternateSetting 0 | bNumEndpoints 1 | bInterfaceClass 3 Human Interface Device | bInterfaceSubClass 1 Boot Interface Subclass | bInterfaceProtocol 1 Keyboard | iInterface 6 Keyboard / Boot | HID Device Descriptor: | bLength 9 | bDescriptorType 33 | bcdHID 1.10 | bCountryCode 13 International (ISO) | bNumDescriptors 1 | bDescriptorType 34 Report | wDescriptorLength 207 | Report Descriptors: | ** UNAVAILABLE ** | Endpoint Descriptor: | bLength 7 | bDescriptorType 5 | bEndpointAddress 0x82 EP 2 IN | bmAttributes 3 | Transfer Type Interrupt | Synch Type None | Usage Type Data | wMaxPacketSize 0x0010 1x 16 bytes | bInterval 8
Signed-off-by: Janne Grunau j@jannau.net --- common/usb_kbd.c | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 774d3555d9..7aa803eb4e 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -23,6 +23,14 @@
#include <usb.h>
+/* + * USB vendor and product IDs used for quirks. + */ +#define USB_VENDOR_ID_APPLE 0x05ac +#define USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_2021 0x029c +#define USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_FINGERPRINT_2021 0x029a +#define USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_NUMPAD_2021 0x029f + /* * If overwrite_console returns 1, the stdin, stderr and stdout * are switched to the serial port, else the settings in the @@ -106,6 +114,8 @@ struct usb_kbd_pdata { unsigned long last_report; struct int_queue *intq;
+ uint32_t ifnum; + uint32_t repeat_delay;
uint32_t usb_in_pointer; @@ -159,8 +169,8 @@ static void usb_kbd_put_queue(struct usb_kbd_pdata *data, u8 c) */ static void usb_kbd_setled(struct usb_device *dev) { - struct usb_interface *iface = &dev->config.if_desc[0]; struct usb_kbd_pdata *data = dev->privptr; + struct usb_interface *iface = &dev->config.if_desc[data->ifnum]; ALLOC_ALIGN_BUFFER(uint32_t, leds, 1, USB_DMA_MINALIGN);
*leds = data->flags & USB_KBD_LEDMASK; @@ -374,7 +384,7 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev) #if defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP) struct usb_interface *iface; struct usb_kbd_pdata *data = dev->privptr; - iface = &dev->config.if_desc[0]; + iface = &dev->config.if_desc[data->ifnum]; usb_get_report(dev, iface->desc.bInterfaceNumber, 1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE); if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE)) { @@ -528,6 +538,8 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) data->new = memalign(USB_DMA_MINALIGN, roundup(USB_KBD_BOOT_REPORT_SIZE, USB_DMA_MINALIGN));
+ data->ifnum = ifnum; + /* Insert private data into USB device structure */ dev->privptr = data;
@@ -580,11 +592,18 @@ static int probe_usb_keyboard(struct usb_device *dev) { char *stdinname; struct stdio_dev usb_kbd_dev; + unsigned int ifnum; + unsigned int max_ifnum = min((unsigned int)USB_MAX_ACTIVE_INTERFACES, + (unsigned int)dev->config.no_of_if); int error;
/* Try probing the keyboard */ - if (usb_kbd_probe_dev(dev, 0) != 1) - return -ENOENT; + for (ifnum = 0; ifnum < max_ifnum; ifnum++) { + if (usb_kbd_probe_dev(dev, ifnum) == 1) + break; + } + if (ifnum >= max_ifnum) + return -ENOENT;
/* Register the keyboard */ debug("USB KBD: register.\n"); @@ -750,6 +769,18 @@ static const struct usb_device_id kbd_id_table[] = { .bInterfaceSubClass = USB_SUB_HID_BOOT, .bInterfaceProtocol = USB_PROT_HID_KEYBOARD, }, + { + USB_DEVICE(USB_VENDOR_ID_APPLE, + USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_2021), + }, + { + USB_DEVICE(USB_VENDOR_ID_APPLE, + USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_FINGERPRINT_2021), + }, + { + USB_DEVICE(USB_VENDOR_ID_APPLE, + USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_NUMPAD_2021), + }, { } /* Terminating entry */ };
From: Janne Grunau j@jannau.net
Those keyboards do not return the current device state. Polling will timeout unless there are key presses. This is not a problem during operation but the inital device state query during probing will fail. Skip this step in usb_kbd_probe_dev() to make these devices useable. Not all Apple keyboards behave like this. A keyboard with USB vendor/product ID 05ac:0221 is reported to work with the current code. Unfortunately some Keychron keyboards "re-use" Apple's vendor ID and show the same behavior (Keychron C2, 05ac:024f for example).
Signed-off-by: Janne Grunau j@jannau.net --- common/usb_kbd.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c index 7aa803eb4e..b0012ce7ad 100644 --- a/common/usb_kbd.c +++ b/common/usb_kbd.c @@ -31,6 +31,10 @@ #define USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_FINGERPRINT_2021 0x029a #define USB_DEVICE_ID_APPLE_MAGIC_KEYBOARD_NUMPAD_2021 0x029f
+#define USB_VENDOR_ID_KEYCHRON 0x3434 + +#define USB_HID_QUIRK_POLL_NO_REPORT_IDLE (1 << 0) + /* * If overwrite_console returns 1, the stdin, stderr and stdout * are switched to the serial port, else the settings in the @@ -483,6 +487,7 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) struct usb_interface *iface; struct usb_endpoint_descriptor *ep; struct usb_kbd_pdata *data; + unsigned int quirks = 0; int epNum; int i;
@@ -525,6 +530,15 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum)
debug("USB KBD: found interrupt EP: 0x%x\n", ep->bEndpointAddress);
+ switch (dev->descriptor.idVendor) { + case USB_VENDOR_ID_APPLE: + case USB_VENDOR_ID_KEYCHRON: + quirks |= USB_HID_QUIRK_POLL_NO_REPORT_IDLE; + break; + default: + break; + } + data = malloc(sizeof(struct usb_kbd_pdata)); if (!data) { printf("USB KBD: Error allocating private data\n"); @@ -565,6 +579,14 @@ static int usb_kbd_probe_dev(struct usb_device *dev, unsigned int ifnum) usb_set_idle(dev, iface->desc.bInterfaceNumber, 0, 0); #endif
+ /* + * Apple and Keychron keyboards do not report the device state. Reports + * are only returned during key presses. + */ + if (quirks & USB_HID_QUIRK_POLL_NO_REPORT_IDLE) { + debug("USB KBD: quirk: skip testing device state\n"); + return 1; + } debug("USB KBD: enable interrupt pipe...\n"); #ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE data->intq = create_int_queue(dev, data->intpipe, 1,
On 2/21/24 08:25, Janne Grunau via B4 Relay wrote:
From: Janne Grunau j@jannau.net
Those keyboards do not return the current device state. Polling will timeout unless there are key presses. This is not a problem during operation but the inital device state query during probing will fail. Skip this step in usb_kbd_probe_dev() to make these devices useable. Not all Apple keyboards behave like this. A keyboard with USB vendor/product ID 05ac:0221 is reported to work with the current code. Unfortunately some Keychron keyboards "re-use" Apple's vendor ID and show the same behavior (Keychron C2, 05ac:024f for example).
Uh, fun.
Reviewed-by: Marek Vasut marex@denx.de
Thanks !
participants (4)
-
Janne Grunau -
Janne Grunau via B4 Relay -
Marek Vasut -
Mark Kettenis