[PATCH v2 0/6] USB keyboard improvements for asahi / desktop systems

[PATCH v2 0/6] USB keyboard improvements for asahi / desktop systems

[Janne Grunau via B4 Relay]

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>
---
Changes in v2:
- rewritten commit message for "[PATCH 2/6] usb: xhci: Set up endpoints for the first 2 interfaces"
- Replaced the usb keyboard Yubikey block with an env based USB device blocklist
- Use "-EINVAL" as return value in "[PATCH 3/6] usb: xhci: Abort transfers with unallocated rings"
- added "Reviewed-by:" tags
- Link to v1: https://lore.kernel.org/r/20240221-asahi-keyboards-v1-0-814b2e741790@jannau.net

---
Janne Grunau (6):
      usb: xhci: refactor xhci_set_configuration
      usb: xhci: Set up endpoints for the first 2 interfaces
      usb: xhci: Abort transfers with unallocated rings
      usb: Add environment based device blocklist
      usb: kbd: support Apple Magic Keyboards (2021)
      usb: kbd: Add probe quirk for Apple and Keychron keyboards

 common/usb.c                 |  56 +++++++++++++++++++
 common/usb_kbd.c             |  61 +++++++++++++++++++--
 doc/usage/environment.rst    |  12 +++++
 drivers/usb/host/xhci-ring.c |   5 ++
 drivers/usb/host/xhci.c      | 126 +++++++++++++++++++++++++++----------------
 include/env_default.h        |  11 ++++
 include/usb.h                |   6 +++
 7 files changed, 227 insertions(+), 50 deletions(-)
---
base-commit: 37345abb97ef0dd9c50a03b2a72617612dcae585
change-id: 20240218-asahi-keyboards-f2ddaf0022b2

Best regards,
-- 
Janne Grunau <j@jannau.net>

[PATCH v2 1/6] usb: xhci: refactor xhci_set_configuration

[Janne Grunau via B4 Relay]

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.

Reviewed-by: Marek Vasut <marex@denx.de>
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);
 }
 

-- 
2.44.0

[PATCH v2 2/6] usb: xhci: Set up endpoints for the first 2 interfaces

[Janne Grunau via B4 Relay]

From: Janne Grunau <j@jannau.net>

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 considered during endpoint initialisation.
For now define it to 2 as that is sufficient for supporting the Apple
keyboards.

Reviewed-by: Marek Vasut <marex@denx.de>
Signed-off-by: Janne Grunau <j@jannau.net>
---
 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;

-- 
2.44.0

[PATCH v2 3/6] usb: xhci: Abort transfers with unallocated rings

[Janne Grunau via B4 Relay]

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..910c5f3352 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 -EINVAL;
+
 	/*
 	 * 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 -EINVAL;
 
 	/*
 	 * Check to see if the max packet size for the default control

-- 
2.44.0

Re: [PATCH v2 3/6] usb: xhci: Abort transfers with unallocated rings

[Marek Vasut]

On 3/17/24 12:07 PM, 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>

Reviewed-by: Marek Vasut <marex@denx.de>

[PATCH v2 4/6] usb: Add environment based device blocklist

[Janne Grunau via B4 Relay]

From: Janne Grunau <j@jannau.net>

Add the environment variable "usb_blocklist" to prevent USB devices
listed in it from being used. This allows to ignore devices which
trigger bugs in u-boot's USB stack or are undesirable for other reasons.
Devices emulating keyboards are one example. U-boot currently supports
only one USB keyboard device. Most commonly, people run into this with
Yubikeys, so let's ignore those in the default environment.

Based on previous USB keyboard specific patches for the same purpose.

Link: https://lore.kernel.org/u-boot/7ab604fb-0fec-4f5e-8708-7a3a7e2cb568@denx.de/
Signed-off-by: Janne Grunau <j@jannau.net>
---
 common/usb.c              | 56 +++++++++++++++++++++++++++++++++++++++++++++++
 doc/usage/environment.rst | 12 ++++++++++
 include/env_default.h     | 11 ++++++++++
 3 files changed, 79 insertions(+)

diff --git a/common/usb.c b/common/usb.c
index 836506dcd9..73af5be066 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -1084,6 +1084,57 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
 	return 0;
 }
 
+static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
+{
+	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
+	       blocklist);
+	return 0;
+}
+
+static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
+{
+	ulong vid, pid;
+	char *end;
+	const char *block_str = env_get("usb_blocklist");
+	const char *cur = block_str;
+
+	/* parse "usb_blocklist" strictly */
+	while (cur && cur[0] != '\0') {
+		vid = simple_strtoul(cur, &end, 0);
+		if (cur == end || end[0] != ':')
+			return usb_blocklist_parse_error(block_str,
+							 cur - block_str);
+
+		cur = end + 1;
+		pid = simple_strtoul(cur, &end, 0);
+		if (cur == end && end[0] != '*')
+			return usb_blocklist_parse_error(block_str,
+							 cur - block_str);
+
+		if (end[0] == '*') {
+			/* use out of range idProduct as wildcard indication */
+			pid = U16_MAX + 1;
+			end++;
+		}
+		if (end[0] != ',' && end[0] != '\0')
+			return usb_blocklist_parse_error(block_str,
+							 cur - block_str);
+
+		if (id_vendor == vid && (pid > U16_MAX || id_product == pid)) {
+			printf("Ignoring USB device 0x%x:0x%x\n",id_vendor,
+			      id_product);
+			debug("Ignoring USB device 0x%x:0x%x\n",id_vendor,
+			      id_product);
+			return 1;
+		}
+		if (end[0] == '\0')
+			break;
+		cur = end + 1;
+	}
+
+	return 0;
+}
+
 int usb_select_config(struct usb_device *dev)
 {
 	unsigned char *tmpbuf = NULL;
@@ -1099,6 +1150,11 @@ int usb_select_config(struct usb_device *dev)
 	le16_to_cpus(&dev->descriptor.idProduct);
 	le16_to_cpus(&dev->descriptor.bcdDevice);
 
+	/* ignore devices from usb_blocklist */
+	if (usb_device_is_blocked(dev->descriptor.idVendor,
+				  dev->descriptor.idProduct))
+		return -ENODEV;
+
 	/*
 	 * Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive
 	 * about this first Get Descriptor request. If there are any other
diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
index ebf75fa948..e42702adb2 100644
--- a/doc/usage/environment.rst
+++ b/doc/usage/environment.rst
@@ -366,6 +366,18 @@ tftpwindowsize
     This means the count of blocks we can receive before
     sending ack to server.
 
+usb_blocklist
+    Block USB devices from being bound to an USB device driver. This can be used
+    to ignore devices which causes crashes in u-boot's USB stack or are
+    undesirable for other reasons.
+    The default environment blocks Yubico devices since they emulate USB
+    keyboards. U-boot currently supports only a single USB keyboard device and
+    it is undesirable that a Yubikey is used as keyboard.
+    Devices are matched by idVendor and idProduct. The variable contains a comma
+    separated list of idVendor:idProduct pairs as hexadecimal numbers joined
+    by a colon. '*' functions as a wildcard for idProduct to block all devices
+    with the specified idVendor.
+
 vlan
     When set to a value < 4095 the traffic over
     Ethernet is encapsulated/received over 802.1q
diff --git a/include/env_default.h b/include/env_default.h
index 2ca4a087d3..ba4c7516b4 100644
--- a/include/env_default.h
+++ b/include/env_default.h
@@ -99,6 +99,17 @@ const char default_environment[] = {
 #ifdef CONFIG_SYS_SOC
 	"soc="		CONFIG_SYS_SOC			"\0"
 #endif
+#ifdef CONFIG_USB_HOST
+	"usb_blocklist="
+#ifdef CONFIG_USB_KEYBOARD
+	/* Ignore Yubico devices. Currently only a single USB keyboard device is
+	 * supported and the emulated HID keyboard Yubikeys present is useless
+	 * as keyboard.
+	 */
+	"0x1050:*,"
+#endif
+	"\0"
+#endif
 #ifdef CONFIG_ENV_IMPORT_FDT
 	"env_fdt_path="	CONFIG_ENV_FDT_PATH		"\0"
 #endif

-- 
2.44.0

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[Janne Grunau]

Hej,

On Sun, Mar 17, 2024, at 12:07, Janne Grunau via B4 Relay wrote:
> From: Janne Grunau <j@jannau.net>
>
> Add the environment variable "usb_blocklist" to prevent USB devices
> listed in it from being used. This allows to ignore devices which
> trigger bugs in u-boot's USB stack or are undesirable for other reasons.
> Devices emulating keyboards are one example. U-boot currently supports
> only one USB keyboard device. Most commonly, people run into this with
> Yubikeys, so let's ignore those in the default environment.
>
> Based on previous USB keyboard specific patches for the same purpose.
>
> Link: 
> https://lore.kernel.org/u-boot/7ab604fb-0fec-4f5e-8708-7a3a7e2cb568@denx.de/
> Signed-off-by: Janne Grunau <j@jannau.net>
> ---
>  common/usb.c              | 56 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  doc/usage/environment.rst | 12 ++++++++++
>  include/env_default.h     | 11 ++++++++++
>  3 files changed, 79 insertions(+)
>
> diff --git a/common/usb.c b/common/usb.c
> index 836506dcd9..73af5be066 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1084,6 +1084,57 @@ static int usb_prepare_device(struct usb_device 
> *dev, int addr, bool do_read,
>  	return 0;
>  }
> 
> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
> +{
> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
> +	       blocklist);
> +	return 0;
> +}
> +
> +static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
> +{
> +	ulong vid, pid;
> +	char *end;
> +	const char *block_str = env_get("usb_blocklist");
> +	const char *cur = block_str;
> +
> +	/* parse "usb_blocklist" strictly */
> +	while (cur && cur[0] != '\0') {
> +		vid = simple_strtoul(cur, &end, 0);
> +		if (cur == end || end[0] != ':')
> +			return usb_blocklist_parse_error(block_str,
> +							 cur - block_str);
> +
> +		cur = end + 1;
> +		pid = simple_strtoul(cur, &end, 0);
> +		if (cur == end && end[0] != '*')
> +			return usb_blocklist_parse_error(block_str,
> +							 cur - block_str);
> +
> +		if (end[0] == '*') {
> +			/* use out of range idProduct as wildcard indication */
> +			pid = U16_MAX + 1;
> +			end++;
> +		}
> +		if (end[0] != ',' && end[0] != '\0')
> +			return usb_blocklist_parse_error(block_str,
> +							 cur - block_str);
> +
> +		if (id_vendor == vid && (pid > U16_MAX || id_product == pid)) {
> +			printf("Ignoring USB device 0x%x:0x%x\n",id_vendor,
> +			      id_product);

this is a leftover from testing, already removed locally

Janne

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[Marek Vasut]

On 3/17/24 12:34 PM, Janne Grunau wrote:
> Hej,
> 
> On Sun, Mar 17, 2024, at 12:07, Janne Grunau via B4 Relay wrote:
>> From: Janne Grunau <j@jannau.net>
>>
>> Add the environment variable "usb_blocklist" to prevent USB devices
>> listed in it from being used. This allows to ignore devices which
>> trigger bugs in u-boot's USB stack or are undesirable for other reasons.
>> Devices emulating keyboards are one example. U-boot currently supports
>> only one USB keyboard device. Most commonly, people run into this with
>> Yubikeys, so let's ignore those in the default environment.
>>
>> Based on previous USB keyboard specific patches for the same purpose.
>>
>> Link:
>> https://lore.kernel.org/u-boot/7ab604fb-0fec-4f5e-8708-7a3a7e2cb568@denx.de/
>> Signed-off-by: Janne Grunau <j@jannau.net>
>> ---
>>   common/usb.c              | 56
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   doc/usage/environment.rst | 12 ++++++++++
>>   include/env_default.h     | 11 ++++++++++
>>   3 files changed, 79 insertions(+)
>>
>> diff --git a/common/usb.c b/common/usb.c
>> index 836506dcd9..73af5be066 100644
>> --- a/common/usb.c
>> +++ b/common/usb.c
>> @@ -1084,6 +1084,57 @@ static int usb_prepare_device(struct usb_device
>> *dev, int addr, bool do_read,
>>   	return 0;
>>   }
>>
>> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
>> +{
>> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
>> +	       blocklist);
>> +	return 0;
>> +}
>> +
>> +static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
>> +{
>> +	ulong vid, pid;
>> +	char *end;
>> +	const char *block_str = env_get("usb_blocklist");
>> +	const char *cur = block_str;
>> +
>> +	/* parse "usb_blocklist" strictly */
>> +	while (cur && cur[0] != '\0') {
>> +		vid = simple_strtoul(cur, &end, 0);
>> +		if (cur == end || end[0] != ':')
>> +			return usb_blocklist_parse_error(block_str,
>> +							 cur - block_str);
>> +
>> +		cur = end + 1;
>> +		pid = simple_strtoul(cur, &end, 0);
>> +		if (cur == end && end[0] != '*')
>> +			return usb_blocklist_parse_error(block_str,
>> +							 cur - block_str);
>> +
>> +		if (end[0] == '*') {
>> +			/* use out of range idProduct as wildcard indication */
>> +			pid = U16_MAX + 1;
>> +			end++;
>> +		}
>> +		if (end[0] != ',' && end[0] != '\0')
>> +			return usb_blocklist_parse_error(block_str,
>> +							 cur - block_str);
>> +
>> +		if (id_vendor == vid && (pid > U16_MAX || id_product == pid)) {
>> +			printf("Ignoring USB device 0x%x:0x%x\n",id_vendor,
>> +			      id_product);
> 
> this is a leftover from testing, already removed locally

You could turn this into dev_dbg()

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[Marek Vasut]

On 3/17/24 12:07 PM, Janne Grunau via B4 Relay wrote:
> From: Janne Grunau <j@jannau.net>
> 
> Add the environment variable "usb_blocklist" to prevent USB devices
> listed in it from being used. This allows to ignore devices which
> trigger bugs in u-boot's USB stack or are undesirable for other reasons.
> Devices emulating keyboards are one example. U-boot currently supports
> only one USB keyboard device. Most commonly, people run into this with
> Yubikeys, so let's ignore those in the default environment.
> 
> Based on previous USB keyboard specific patches for the same purpose.
> 
> Link: https://lore.kernel.org/u-boot/7ab604fb-0fec-4f5e-8708-7a3a7e2cb568@denx.de/
> Signed-off-by: Janne Grunau <j@jannau.net>
> ---
>   common/usb.c              | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>   doc/usage/environment.rst | 12 ++++++++++
>   include/env_default.h     | 11 ++++++++++
>   3 files changed, 79 insertions(+)
> 
> diff --git a/common/usb.c b/common/usb.c
> index 836506dcd9..73af5be066 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1084,6 +1084,57 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
>   	return 0;
>   }
>   
> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
> +{
> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
> +	       blocklist);
> +	return 0;

This could be static void without return 0 at the end.

> +}
> +
> +static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
> +{
> +	ulong vid, pid;
> +	char *end;
> +	const char *block_str = env_get("usb_blocklist");
> +	const char *cur = block_str;
> +
> +	/* parse "usb_blocklist" strictly */
> +	while (cur && cur[0] != '\0') {

Have a look at strsep() , namely strsep(block_str, ","); This will split 
the string up for you at "," delimiters.

Example is in drivers/dfu/dfu.c dfu_config_interfaces() .

And then, on each token, you can try and run sscanf(token, "%04x:%04x", 
vid, pid);, that will parse the token format for you. See e.g. 
test/lib/sscanf.c for further examples.

That should simplify the parsing a lot.

[...]

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[Janne Grunau]

Hej,

On Sun, Mar 17, 2024, at 17:18, Marek Vasut wrote:
> On 3/17/24 12:07 PM, Janne Grunau via B4 Relay wrote:
>> From: Janne Grunau <j@jannau.net>
>> 
>> Add the environment variable "usb_blocklist" to prevent USB devices
>> listed in it from being used. This allows to ignore devices which
>> trigger bugs in u-boot's USB stack or are undesirable for other reasons.
>> Devices emulating keyboards are one example. U-boot currently supports
>> only one USB keyboard device. Most commonly, people run into this with
>> Yubikeys, so let's ignore those in the default environment.
>> 
>> Based on previous USB keyboard specific patches for the same purpose.
>> 
>> Link: https://lore.kernel.org/u-boot/7ab604fb-0fec-4f5e-8708-7a3a7e2cb568@denx.de/
>> Signed-off-by: Janne Grunau <j@jannau.net>
>> ---
>>   common/usb.c              | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>>   doc/usage/environment.rst | 12 ++++++++++
>>   include/env_default.h     | 11 ++++++++++
>>   3 files changed, 79 insertions(+)
>> 
>> diff --git a/common/usb.c b/common/usb.c
>> index 836506dcd9..73af5be066 100644
>> --- a/common/usb.c
>> +++ b/common/usb.c
>> @@ -1084,6 +1084,57 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
>>   	return 0;
>>   }
>>   
>> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
>> +{
>> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
>> +	       blocklist);
>> +	return 0;
>
> This could be static void without return 0 at the end.

the return is there to break out of the while loop on parsing errors in a single statement. This probably won't be necessary after using strsep and sscanf in the parsing function but see below.

>> +}
>> +
>> +static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
>> +{
>> +	ulong vid, pid;
>> +	char *end;
>> +	const char *block_str = env_get("usb_blocklist");
>> +	const char *cur = block_str;
>> +
>> +	/* parse "usb_blocklist" strictly */
>> +	while (cur && cur[0] != '\0') {
>
> Have a look at strsep() , namely strsep(block_str, ","); This will split 
> the string up for you at "," delimiters.
>
> Example is in drivers/dfu/dfu.c dfu_config_interfaces() .

strsep() is probably a good idea even if it alone won't make the code that much simpler for strict parsing.

> And then, on each token, you can try and run sscanf(token, "%04x:%04x", 
> vid, pid);, that will parse the token format for you. See e.g. 
> test/lib/sscanf.c for further examples.
>
> That should simplify the parsing a lot.

It would but sscanf() is optional and is only selected by CONFIG_XEN so I assumed there would be concerns over binary size increase if USB_HOST would require sscanf.

Janne

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[Marek Vasut]

On 3/17/24 7:15 PM, Janne Grunau wrote:
> Hej,

Hi,

> On Sun, Mar 17, 2024, at 17:18, Marek Vasut wrote:
>> On 3/17/24 12:07 PM, Janne Grunau via B4 Relay wrote:
>>> From: Janne Grunau <j@jannau.net>
>>>
>>> Add the environment variable "usb_blocklist" to prevent USB devices
>>> listed in it from being used. This allows to ignore devices which
>>> trigger bugs in u-boot's USB stack or are undesirable for other reasons.
>>> Devices emulating keyboards are one example. U-boot currently supports
>>> only one USB keyboard device. Most commonly, people run into this with
>>> Yubikeys, so let's ignore those in the default environment.
>>>
>>> Based on previous USB keyboard specific patches for the same purpose.
>>>
>>> Link: https://lore.kernel.org/u-boot/7ab604fb-0fec-4f5e-8708-7a3a7e2cb568@denx.de/
>>> Signed-off-by: Janne Grunau <j@jannau.net>
>>> ---
>>>    common/usb.c              | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>>>    doc/usage/environment.rst | 12 ++++++++++
>>>    include/env_default.h     | 11 ++++++++++
>>>    3 files changed, 79 insertions(+)
>>>
>>> diff --git a/common/usb.c b/common/usb.c
>>> index 836506dcd9..73af5be066 100644
>>> --- a/common/usb.c
>>> +++ b/common/usb.c
>>> @@ -1084,6 +1084,57 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
>>>    	return 0;
>>>    }
>>>    
>>> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
>>> +{
>>> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
>>> +	       blocklist);
>>> +	return 0;
>>
>> This could be static void without return 0 at the end.
> 
> the return is there to break out of the while loop on parsing errors in a single statement. This probably won't be necessary after using strsep and sscanf in the parsing function but see below.

Ahh, now I see it. But then, shouldn't this return -ENODEV here already ?

>>> +}
>>> +
>>> +static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
>>> +{
>>> +	ulong vid, pid;
>>> +	char *end;
>>> +	const char *block_str = env_get("usb_blocklist");
>>> +	const char *cur = block_str;
>>> +
>>> +	/* parse "usb_blocklist" strictly */
>>> +	while (cur && cur[0] != '\0') {
>>
>> Have a look at strsep() , namely strsep(block_str, ","); This will split
>> the string up for you at "," delimiters.
>>
>> Example is in drivers/dfu/dfu.c dfu_config_interfaces() .
> 
> strsep() is probably a good idea even if it alone won't make the code that much simpler for strict parsing.
> 
>> And then, on each token, you can try and run sscanf(token, "%04x:%04x",
>> vid, pid);, that will parse the token format for you. See e.g.
>> test/lib/sscanf.c for further examples.
>>
>> That should simplify the parsing a lot.
> 
> It would but sscanf() is optional and is only selected by CONFIG_XEN so I assumed there would be concerns over binary size increase if USB_HOST would require sscanf.

Good point, lets postpone sscanf() . Also, looking at it, sscanf would 
work for numbers, but not for the special characters. So ... do you want 
to try tweaking this further with strsep() or shall we go with this 
implementation ?

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[Janne Grunau]

On Mon, Mar 18, 2024, at 06:06, Marek Vasut wrote:
> On 3/17/24 7:15 PM, Janne Grunau wrote:
>> Hej,
>
> Hi,
>
>> On Sun, Mar 17, 2024, at 17:18, Marek Vasut wrote:
>>> On 3/17/24 12:07 PM, Janne Grunau via B4 Relay wrote:
>>>> From: Janne Grunau <j@jannau.net>
>>>>
>>>> Add the environment variable "usb_blocklist" to prevent USB devices
>>>> listed in it from being used. This allows to ignore devices which
>>>> trigger bugs in u-boot's USB stack or are undesirable for other reasons.
>>>> Devices emulating keyboards are one example. U-boot currently supports
>>>> only one USB keyboard device. Most commonly, people run into this with
>>>> Yubikeys, so let's ignore those in the default environment.
>>>>
>>>> Based on previous USB keyboard specific patches for the same purpose.
>>>>
>>>> Link: https://lore.kernel.org/u-boot/7ab604fb-0fec-4f5e-8708-7a3a7e2cb568@denx.de/
>>>> Signed-off-by: Janne Grunau <j@jannau.net>
>>>> ---
>>>>    common/usb.c              | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>    doc/usage/environment.rst | 12 ++++++++++
>>>>    include/env_default.h     | 11 ++++++++++
>>>>    3 files changed, 79 insertions(+)
>>>>
>>>> diff --git a/common/usb.c b/common/usb.c
>>>> index 836506dcd9..73af5be066 100644
>>>> --- a/common/usb.c
>>>> +++ b/common/usb.c
>>>> @@ -1084,6 +1084,57 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
>>>> +{
>>>> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
>>>> +	       blocklist);
>>>> +	return 0;
>>>
>>> This could be static void without return 0 at the end.
>> 
>> the return is there to break out of the while loop on parsing errors in a single statement. This probably won't be necessary after using strsep and sscanf in the parsing function but see below.
>
> Ahh, now I see it. But then, shouldn't this return -ENODEV here already ?

It returns 0 so that parsing errors in the blocklist do not result
in blocking every USB device. That looked to me like the
less bad error behavior to me.

>>>> +}
>>>> +
>>>> +static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
>>>> +{
>>>> +	ulong vid, pid;
>>>> +	char *end;
>>>> +	const char *block_str = env_get("usb_blocklist");
>>>> +	const char *cur = block_str;
>>>> +
>>>> +	/* parse "usb_blocklist" strictly */
>>>> +	while (cur && cur[0] != '\0') {
>>>
>>> Have a look at strsep() , namely strsep(block_str, ","); This will split
>>> the string up for you at "," delimiters.
>>>
>>> Example is in drivers/dfu/dfu.c dfu_config_interfaces() .
>> 
>> strsep() is probably a good idea even if it alone won't make the code that much simpler for strict parsing.
>> 
>>> And then, on each token, you can try and run sscanf(token, "%04x:%04x",
>>> vid, pid);, that will parse the token format for you. See e.g.
>>> test/lib/sscanf.c for further examples.
>>>
>>> That should simplify the parsing a lot.
>> 
>> It would but sscanf() is optional and is only selected by CONFIG_XEN so I assumed there would be concerns over binary size increase if USB_HOST would require sscanf.
>
> Good point, lets postpone sscanf() . Also, looking at it, sscanf would 
> work for numbers, but not for the special characters. So ... do you want 
> to try tweaking this further with strsep() or shall we go with this 
> implementation ?

I started converting it to strsep. It makes the intent clearer but it doesn't
simplify the implementation much. strsep() has the disadvantage that
it can't work in place and needs to copy the string. If we go with strsep
I would look into parsing the list once at USB init time and use a list of
numeric vendor, product ID pairs at device probe time.
If have a slight preference for the current implementation (with ignore
or deny instead of blocklist) as long as the list remains short.

Janne

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[Marek Vasut]

On 3/18/24 8:33 AM, Janne Grunau wrote:

>>>>> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
>>>>> +{
>>>>> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
>>>>> +	       blocklist);
>>>>> +	return 0;
>>>>
>>>> This could be static void without return 0 at the end.
>>>
>>> the return is there to break out of the while loop on parsing errors in a single statement. This probably won't be necessary after using strsep and sscanf in the parsing function but see below.
>>
>> Ahh, now I see it. But then, shouldn't this return -ENODEV here already ?
> 
> It returns 0 so that parsing errors in the blocklist do not result
> in blocking every USB device. That looked to me like the
> less bad error behavior to me.

In unix , 0 is considered success and non-zero failure .

How about this:

- Return -EINVAL here (parsing failed)
- Instead of 'return 1' in usb_device_is_blocked() return -ENODEV
- In usb_select_config(), check
   if usb_device_is_blocked returned 0, no device blocked OK
   if usb_device_is_blocked returned -ENODEV, device blocked,
     return -ENODEV
   if usb_device_is_blocked returned any other error, parsing error
     return that error

What do you think ?

>>>>> +}
>>>>> +
>>>>> +static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
>>>>> +{
>>>>> +	ulong vid, pid;
>>>>> +	char *end;
>>>>> +	const char *block_str = env_get("usb_blocklist");
>>>>> +	const char *cur = block_str;
>>>>> +
>>>>> +	/* parse "usb_blocklist" strictly */
>>>>> +	while (cur && cur[0] != '\0') {
>>>>
>>>> Have a look at strsep() , namely strsep(block_str, ","); This will split
>>>> the string up for you at "," delimiters.
>>>>
>>>> Example is in drivers/dfu/dfu.c dfu_config_interfaces() .
>>>
>>> strsep() is probably a good idea even if it alone won't make the code that much simpler for strict parsing.
>>>
>>>> And then, on each token, you can try and run sscanf(token, "%04x:%04x",
>>>> vid, pid);, that will parse the token format for you. See e.g.
>>>> test/lib/sscanf.c for further examples.
>>>>
>>>> That should simplify the parsing a lot.
>>>
>>> It would but sscanf() is optional and is only selected by CONFIG_XEN so I assumed there would be concerns over binary size increase if USB_HOST would require sscanf.
>>
>> Good point, lets postpone sscanf() . Also, looking at it, sscanf would
>> work for numbers, but not for the special characters. So ... do you want
>> to try tweaking this further with strsep() or shall we go with this
>> implementation ?
> 
> I started converting it to strsep. It makes the intent clearer but it doesn't
> simplify the implementation much. strsep() has the disadvantage that
> it can't work in place and needs to copy the string. If we go with strsep
> I would look into parsing the list once at USB init time and use a list of
> numeric vendor, product ID pairs at device probe time.
> If have a slight preference for the current implementation (with ignore
> or deny instead of blocklist) as long as the list remains short.

OK, we can always improve this later, I would also like this 
functionality in soon-ish.

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[Janne Grunau]

On Mon, Mar 18, 2024 at 03:17:33PM +0100, Marek Vasut wrote:
> On 3/18/24 8:33 AM, Janne Grunau wrote:
> 
> >>>>> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
> >>>>> +{
> >>>>> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
> >>>>> +	       blocklist);
> >>>>> +	return 0;
> >>>>
> >>>> This could be static void without return 0 at the end.
> >>>
> >>> the return is there to break out of the while loop on parsing errors in a single statement. This probably won't be necessary after using strsep and sscanf in the parsing function but see below.
> >>
> >> Ahh, now I see it. But then, shouldn't this return -ENODEV here already ?
> > 
> > It returns 0 so that parsing errors in the blocklist do not result
> > in blocking every USB device. That looked to me like the
> > less bad error behavior to me.
> 
> In unix , 0 is considered success and non-zero failure .
> 
> How about this:
> 
> - Return -EINVAL here (parsing failed)

If we return 0 / negated errors we do not need this function and we can
simply report the parsing error when usb_device_is_blocked() return
-EINVAL.

> - Instead of 'return 1' in usb_device_is_blocked() return -ENODEV
> - In usb_select_config(), check
>    if usb_device_is_blocked returned 0, no device blocked OK
>    if usb_device_is_blocked returned -ENODEV, device blocked,
>      return -ENODEV
>    if usb_device_is_blocked returned any other error, parsing error
>      return that error

I think the preferable option is to ignore parsing errors. If we
would propagate the error the result would be that every USB device is
ignored. 
 
> What do you think ?

Fine by me, -EINVAL makes the parsing error reporting less awkward. Only
open question is what should happen on parsing errors.

locally modified and ready to resend once we agree on the behavior on
parsing errors

Janne

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[Marek Vasut]

On 3/19/24 10:17 PM, Janne Grunau wrote:

Hi,

sorry for the abysmal delay in response.

> On Mon, Mar 18, 2024 at 03:17:33PM +0100, Marek Vasut wrote:
>> On 3/18/24 8:33 AM, Janne Grunau wrote:
>>
>>>>>>> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
>>>>>>> +{
>>>>>>> +	printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
>>>>>>> +	       blocklist);
>>>>>>> +	return 0;
>>>>>>
>>>>>> This could be static void without return 0 at the end.
>>>>>
>>>>> the return is there to break out of the while loop on parsing errors in a single statement. This probably won't be necessary after using strsep and sscanf in the parsing function but see below.
>>>>
>>>> Ahh, now I see it. But then, shouldn't this return -ENODEV here already ?
>>>
>>> It returns 0 so that parsing errors in the blocklist do not result
>>> in blocking every USB device. That looked to me like the
>>> less bad error behavior to me.
>>
>> In unix , 0 is considered success and non-zero failure .
>>
>> How about this:
>>
>> - Return -EINVAL here (parsing failed)
> 
> If we return 0 / negated errors we do not need this function and we can
> simply report the parsing error when usb_device_is_blocked() return
> -EINVAL.
> 
>> - Instead of 'return 1' in usb_device_is_blocked() return -ENODEV
>> - In usb_select_config(), check
>>     if usb_device_is_blocked returned 0, no device blocked OK
>>     if usb_device_is_blocked returned -ENODEV, device blocked,
>>       return -ENODEV
>>     if usb_device_is_blocked returned any other error, parsing error
>>       return that error
> 
> I think the preferable option is to ignore parsing errors. If we
> would propagate the error the result would be that every USB device is
> ignored.

Good point.

>> What do you think ?
> 
> Fine by me, -EINVAL makes the parsing error reporting less awkward. Only
> open question is what should happen on parsing errors.

I agree, ignore/skip the parsing errors and report to user that 
something couldn't be parsed, that works.

> locally modified and ready to resend once we agree on the behavior on
> parsing errors

Thanks, I think we are in agreement.

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[E Shattow]

Should be usb_denylist, since "block devices" is ambiguous meaning if
it is a list of data chunks (blocks) or if it blocks (deny). There is
no question what a denylist is.

On Sun, Mar 17, 2024 at 4:07 AM Janne Grunau via B4 Relay
<devnull+j.jannau.net@kernel.org> wrote:
>
> From: Janne Grunau <j@jannau.net>
>
> Add the environment variable "usb_blocklist" to prevent USB devices
> listed in it from being used. This allows to ignore devices which
> trigger bugs in u-boot's USB stack or are undesirable for other reasons.
> Devices emulating keyboards are one example. U-boot currently supports
> only one USB keyboard device. Most commonly, people run into this with
> Yubikeys, so let's ignore those in the default environment.
>
> Based on previous USB keyboard specific patches for the same purpose.
>
> Link: https://lore.kernel.org/u-boot/7ab604fb-0fec-4f5e-8708-7a3a7e2cb568@denx.de/
> Signed-off-by: Janne Grunau <j@jannau.net>
> ---
>  common/usb.c              | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>  doc/usage/environment.rst | 12 ++++++++++
>  include/env_default.h     | 11 ++++++++++
>  3 files changed, 79 insertions(+)
>
> diff --git a/common/usb.c b/common/usb.c
> index 836506dcd9..73af5be066 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -1084,6 +1084,57 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
>         return 0;
>  }
>
> +static int usb_blocklist_parse_error(const char *blocklist, size_t pos)
> +{
> +       printf("usb_blocklist parse error at char %zu in \"%s\"\n", pos,
> +              blocklist);
> +       return 0;
> +}
> +
> +static int usb_device_is_blocked(u16 id_vendor, u16 id_product)
> +{
> +       ulong vid, pid;
> +       char *end;
> +       const char *block_str = env_get("usb_blocklist");
> +       const char *cur = block_str;
> +
> +       /* parse "usb_blocklist" strictly */
> +       while (cur && cur[0] != '\0') {
> +               vid = simple_strtoul(cur, &end, 0);
> +               if (cur == end || end[0] != ':')
> +                       return usb_blocklist_parse_error(block_str,
> +                                                        cur - block_str);
> +
> +               cur = end + 1;
> +               pid = simple_strtoul(cur, &end, 0);
> +               if (cur == end && end[0] != '*')
> +                       return usb_blocklist_parse_error(block_str,
> +                                                        cur - block_str);
> +
> +               if (end[0] == '*') {
> +                       /* use out of range idProduct as wildcard indication */
> +                       pid = U16_MAX + 1;
> +                       end++;
> +               }
> +               if (end[0] != ',' && end[0] != '\0')
> +                       return usb_blocklist_parse_error(block_str,
> +                                                        cur - block_str);
> +
> +               if (id_vendor == vid && (pid > U16_MAX || id_product == pid)) {
> +                       printf("Ignoring USB device 0x%x:0x%x\n",id_vendor,
> +                             id_product);
> +                       debug("Ignoring USB device 0x%x:0x%x\n",id_vendor,
> +                             id_product);
> +                       return 1;
> +               }
> +               if (end[0] == '\0')
> +                       break;
> +               cur = end + 1;
> +       }
> +
> +       return 0;
> +}
> +
>  int usb_select_config(struct usb_device *dev)
>  {
>         unsigned char *tmpbuf = NULL;
> @@ -1099,6 +1150,11 @@ int usb_select_config(struct usb_device *dev)
>         le16_to_cpus(&dev->descriptor.idProduct);
>         le16_to_cpus(&dev->descriptor.bcdDevice);
>
> +       /* ignore devices from usb_blocklist */
> +       if (usb_device_is_blocked(dev->descriptor.idVendor,
> +                                 dev->descriptor.idProduct))
> +               return -ENODEV;
> +
>         /*
>          * Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive
>          * about this first Get Descriptor request. If there are any other
> diff --git a/doc/usage/environment.rst b/doc/usage/environment.rst
> index ebf75fa948..e42702adb2 100644
> --- a/doc/usage/environment.rst
> +++ b/doc/usage/environment.rst
> @@ -366,6 +366,18 @@ tftpwindowsize
>      This means the count of blocks we can receive before
>      sending ack to server.
>
> +usb_blocklist
> +    Block USB devices from being bound to an USB device driver. This can be used
> +    to ignore devices which causes crashes in u-boot's USB stack or are
> +    undesirable for other reasons.
> +    The default environment blocks Yubico devices since they emulate USB
> +    keyboards. U-boot currently supports only a single USB keyboard device and
> +    it is undesirable that a Yubikey is used as keyboard.
> +    Devices are matched by idVendor and idProduct. The variable contains a comma
> +    separated list of idVendor:idProduct pairs as hexadecimal numbers joined
> +    by a colon. '*' functions as a wildcard for idProduct to block all devices
> +    with the specified idVendor.
> +
>  vlan
>      When set to a value < 4095 the traffic over
>      Ethernet is encapsulated/received over 802.1q
> diff --git a/include/env_default.h b/include/env_default.h
> index 2ca4a087d3..ba4c7516b4 100644
> --- a/include/env_default.h
> +++ b/include/env_default.h
> @@ -99,6 +99,17 @@ const char default_environment[] = {
>  #ifdef CONFIG_SYS_SOC
>         "soc="          CONFIG_SYS_SOC                  "\0"
>  #endif
> +#ifdef CONFIG_USB_HOST
> +       "usb_blocklist="
> +#ifdef CONFIG_USB_KEYBOARD
> +       /* Ignore Yubico devices. Currently only a single USB keyboard device is
> +        * supported and the emulated HID keyboard Yubikeys present is useless
> +        * as keyboard.
> +        */
> +       "0x1050:*,"
> +#endif
> +       "\0"
> +#endif
>  #ifdef CONFIG_ENV_IMPORT_FDT
>         "env_fdt_path=" CONFIG_ENV_FDT_PATH             "\0"
>  #endif
>
> --
> 2.44.0
>

Re: [PATCH v2 4/6] usb: Add environment based device blocklist

[Janne Grunau]

Hej,

On Sun, Mar 17, 2024, at 22:39, E Shattow wrote:
> Should be usb_denylist, since "block devices" is ambiguous meaning if
> it is a list of data chunks (blocks) or if it blocks (deny). There is
> no question what a denylist is.

I briefly thought of that concluded that the spelling "blocklist" removed
the ambiguity. I not sure about "denylist". It sounds weird to me in the
context of probing USB devices. I'd rather use "ignorelist" as alternative.

Janne

[PATCH v2 5/6] usb: kbd: support Apple Magic Keyboards (2021)

[Janne Grunau via B4 Relay]

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 4cbc9acb73..8cc3345075 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;
@@ -150,8 +160,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;
@@ -365,7 +375,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)) {
@@ -509,6 +519,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;
 
@@ -561,11 +573,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");
@@ -731,6 +750,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 */
 };
 

-- 
2.44.0

Re: [PATCH v2 5/6] usb: kbd: support Apple Magic Keyboards (2021)

[Marek Vasut]

On 3/17/24 12:07 PM, Janne Grunau via B4 Relay wrote:
> 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:

Reviewed-by: Marek Vasut <marex@denx.de>

Thank you for the detailed commit message, that is real helpful !

[PATCH v2 6/6] usb: kbd: Add probe quirk for Apple and Keychron keyboards

[Janne Grunau via B4 Relay]

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).

Reviewed-by: Marek Vasut <marex@denx.de>
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 8cc3345075..43c7668671 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
@@ -474,6 +478,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;
 
 	if (dev->descriptor.bNumConfigurations != 1)
@@ -506,6 +511,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");
@@ -546,6 +560,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,

-- 
2.44.0

Re: [PATCH v2 6/6] usb: kbd: Add probe quirk for Apple and Keychron keyboards

[Marek Vasut]

On 3/17/24 12:07 PM, 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).
> 
> Reviewed-by: Marek Vasut <marex@denx.de>
> 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 8cc3345075..43c7668671 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)

Use BIT(0) instead of (1 << 0)

With that fixed:

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH v2 0/6] USB keyboard improvements for asahi / desktop systems

[Neal Gompa]

On Sun, Mar 17, 2024 at 4:07 AM Janne Grunau via B4 Relay
<devnull+j.jannau.net@kernel.org> wrote:
>
> 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>
> ---
> Changes in v2:
> - rewritten commit message for "[PATCH 2/6] usb: xhci: Set up endpoints for the first 2 interfaces"
> - Replaced the usb keyboard Yubikey block with an env based USB device blocklist
> - Use "-EINVAL" as return value in "[PATCH 3/6] usb: xhci: Abort transfers with unallocated rings"
> - added "Reviewed-by:" tags
> - Link to v1: https://lore.kernel.org/r/20240221-asahi-keyboards-v1-0-814b2e741790@jannau.net
>
> ---
> Janne Grunau (6):
>       usb: xhci: refactor xhci_set_configuration
>       usb: xhci: Set up endpoints for the first 2 interfaces
>       usb: xhci: Abort transfers with unallocated rings
>       usb: Add environment based device blocklist
>       usb: kbd: support Apple Magic Keyboards (2021)
>       usb: kbd: Add probe quirk for Apple and Keychron keyboards
>
>  common/usb.c                 |  56 +++++++++++++++++++
>  common/usb_kbd.c             |  61 +++++++++++++++++++--
>  doc/usage/environment.rst    |  12 +++++
>  drivers/usb/host/xhci-ring.c |   5 ++
>  drivers/usb/host/xhci.c      | 126 +++++++++++++++++++++++++++----------------
>  include/env_default.h        |  11 ++++
>  include/usb.h                |   6 +++
>  7 files changed, 227 insertions(+), 50 deletions(-)
> ---
> base-commit: 37345abb97ef0dd9c50a03b2a72617612dcae585
> change-id: 20240218-asahi-keyboards-f2ddaf0022b2
>

Series LGTM. Thanks for this! :)

Reviewed-by: Neal Gompa <neal@gompa.dev>


-- 
真実はいつも一つ！/ Always, there's only one truth!