linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: gadget: f_hid: optional SETUP/SET_REPORT mode
@ 2021-08-13 11:45 Maxim Devaev
  2021-08-13 14:58 ` Alan Stern
  0 siblings, 1 reply; 5+ messages in thread
From: Maxim Devaev @ 2021-08-13 11:45 UTC (permalink / raw)
  To: balbi
  Cc: gregkh, mdevaev, ruslan.bilovol, mika.westerberg, maze,
	jj251510319013, linux-usb, linux-kernel

f_hid provides the OUT Endpoint for receiving reports from the host.
The USB HID standard describes the OUT Endpoint support as optional,
and hosts can ignore it if they don't support it.
However, this raises several problems.

(1) Some host OS drivers without OUT Endpoint support can't receive
    reports at all. In the case of the keyboard, it becomes impossible
    to get the status of the LEDs from the host OS.

(2) Some BIOSes and UEFIs not only don't support the OUT Endpoint,
    they cannot work with HID with this configuration at all.
    For example, absolutely all Apple UEFIs in all Macs can't handle
    the OUT Endpoint in accordance with the HID standard so it makes
    impossible to enter the Boot Menu using the hotkey at boot.
    This problem also occurs on HP and DELL BIOSes and in some dumb
    devices with primitive embedded firmware like KVM switches.

This patch adds option no_out_endpoint=1 to disable the OUT Endpoint
and allow f_hid to receive reports from the host via SETUP/SET_REPORT.

Previously, there was such a feature in f_hid, but it was replaced
by the OUT Endpoint ("usb: gadget: hidg: register OUT INT endpoint
for SET_REPORT"). It seems that no one knew at the time that it would
cause problems with BIOS. So this patch actually returns the removed
functionality making it optional. For backward compatibility reasons,
the OUT Endpoint mode remains the default behaviour.

If the SETUP/SET_REPORT mode is used, there is no event processing queue,
so the user will only read the last report. For classic HID devices
like keyboard this is not a problem, since it is intended to transmit
the status of the LEDs and only the last report is important.

Both modes pass USBCV tests. Checking with the USB protocol analyzer
also confirms that everything is working as it should and the new mode
ensures operability in all of the described cases.

Signed-off-by: Maxim Devaev <mdevaev@gmail.com>
---
 drivers/usb/gadget/function/f_hid.c | 217 +++++++++++++++++++++++-----
 drivers/usb/gadget/function/u_hid.h |   1 +
 2 files changed, 185 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index bb476e121eae..e3fb73ed696d 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -45,12 +45,17 @@ struct f_hidg {
 	unsigned short			report_desc_length;
 	char				*report_desc;
 	unsigned short			report_length;
+	bool				use_out_ep;
 
 	/* recv report */
-	struct list_head		completed_out_req;
 	spinlock_t			read_spinlock;
 	wait_queue_head_t		read_queue;
+	/* recv report - interrupt out only (use_out_ep == 1) */
+	struct list_head		completed_out_req;
 	unsigned int			qlen;
+	/* recv report - setup set_report only (use_out_ep == 0) */
+	char				*set_report_buf;
+	unsigned int			set_report_length;
 
 	/* send report */
 	spinlock_t			write_spinlock;
@@ -79,7 +84,7 @@ static struct usb_interface_descriptor hidg_interface_desc = {
 	.bDescriptorType	= USB_DT_INTERFACE,
 	/* .bInterfaceNumber	= DYNAMIC */
 	.bAlternateSetting	= 0,
-	.bNumEndpoints		= 2,
+	/* .bNumEndpoints	= DYNAMIC */
 	.bInterfaceClass	= USB_CLASS_HID,
 	/* .bInterfaceSubClass	= DYNAMIC */
 	/* .bInterfaceProtocol	= DYNAMIC */
@@ -140,7 +145,7 @@ static struct usb_ss_ep_comp_descriptor hidg_ss_out_comp_desc = {
 	/* .wBytesPerInterval   = DYNAMIC */
 };
 
-static struct usb_descriptor_header *hidg_ss_descriptors[] = {
+static struct usb_descriptor_header *hidg_ss_descriptors_intout[] = {
 	(struct usb_descriptor_header *)&hidg_interface_desc,
 	(struct usb_descriptor_header *)&hidg_desc,
 	(struct usb_descriptor_header *)&hidg_ss_in_ep_desc,
@@ -150,6 +155,14 @@ static struct usb_descriptor_header *hidg_ss_descriptors[] = {
 	NULL,
 };
 
+static struct usb_descriptor_header *hidg_ss_descriptors_ssreport[] = {
+	(struct usb_descriptor_header *)&hidg_interface_desc,
+	(struct usb_descriptor_header *)&hidg_desc,
+	(struct usb_descriptor_header *)&hidg_ss_in_ep_desc,
+	(struct usb_descriptor_header *)&hidg_ss_in_comp_desc,
+	NULL,
+};
+
 /* High-Speed Support */
 
 static struct usb_endpoint_descriptor hidg_hs_in_ep_desc = {
@@ -176,7 +189,7 @@ static struct usb_endpoint_descriptor hidg_hs_out_ep_desc = {
 				      */
 };
 
-static struct usb_descriptor_header *hidg_hs_descriptors[] = {
+static struct usb_descriptor_header *hidg_hs_descriptors_intout[] = {
 	(struct usb_descriptor_header *)&hidg_interface_desc,
 	(struct usb_descriptor_header *)&hidg_desc,
 	(struct usb_descriptor_header *)&hidg_hs_in_ep_desc,
@@ -184,6 +197,13 @@ static struct usb_descriptor_header *hidg_hs_descriptors[] = {
 	NULL,
 };
 
+static struct usb_descriptor_header *hidg_hs_descriptors_ssreport[] = {
+	(struct usb_descriptor_header *)&hidg_interface_desc,
+	(struct usb_descriptor_header *)&hidg_desc,
+	(struct usb_descriptor_header *)&hidg_hs_in_ep_desc,
+	NULL,
+};
+
 /* Full-Speed Support */
 
 static struct usb_endpoint_descriptor hidg_fs_in_ep_desc = {
@@ -210,7 +230,7 @@ static struct usb_endpoint_descriptor hidg_fs_out_ep_desc = {
 				       */
 };
 
-static struct usb_descriptor_header *hidg_fs_descriptors[] = {
+static struct usb_descriptor_header *hidg_fs_descriptors_intout[] = {
 	(struct usb_descriptor_header *)&hidg_interface_desc,
 	(struct usb_descriptor_header *)&hidg_desc,
 	(struct usb_descriptor_header *)&hidg_fs_in_ep_desc,
@@ -218,6 +238,13 @@ static struct usb_descriptor_header *hidg_fs_descriptors[] = {
 	NULL,
 };
 
+static struct usb_descriptor_header *hidg_fs_descriptors_ssreport[] = {
+	(struct usb_descriptor_header *)&hidg_interface_desc,
+	(struct usb_descriptor_header *)&hidg_desc,
+	(struct usb_descriptor_header *)&hidg_fs_in_ep_desc,
+	NULL,
+};
+
 /*-------------------------------------------------------------------------*/
 /*                                 Strings                                 */
 
@@ -241,9 +268,11 @@ static struct usb_gadget_strings *ct_func_strings[] = {
 /*-------------------------------------------------------------------------*/
 /*                              Char Device                                */
 
-static ssize_t f_hidg_read(struct file *file, char __user *buffer,
-			size_t count, loff_t *ptr)
+static ssize_t f_hidg_intout_read(struct file *file, char __user *buffer,
+				  size_t count, loff_t *ptr)
 {
+	/* used only if the OUT endpoint is configured */
+
 	struct f_hidg *hidg = file->private_data;
 	struct f_hidg_req_list *list;
 	struct usb_request *req;
@@ -255,15 +284,15 @@ static ssize_t f_hidg_read(struct file *file, char __user *buffer,
 
 	spin_lock_irqsave(&hidg->read_spinlock, flags);
 
-#define READ_COND (!list_empty(&hidg->completed_out_req))
+#define READ_COND_INTOUT (!list_empty(&hidg->completed_out_req))
 
 	/* wait for at least one buffer to complete */
-	while (!READ_COND) {
+	while (!READ_COND_INTOUT) {
 		spin_unlock_irqrestore(&hidg->read_spinlock, flags);
 		if (file->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 
-		if (wait_event_interruptible(hidg->read_queue, READ_COND))
+		if (wait_event_interruptible(hidg->read_queue, READ_COND_INTOUT))
 			return -ERESTARTSYS;
 
 		spin_lock_irqsave(&hidg->read_spinlock, flags);
@@ -313,6 +342,62 @@ static ssize_t f_hidg_read(struct file *file, char __user *buffer,
 	return count;
 }
 
+#define READ_COND_SSREPORT (hidg->set_report_buf != NULL)
+
+static ssize_t f_hidg_ssreport_read(struct file *file, char __user *buffer,
+				    size_t count, loff_t *ptr)
+{
+	/* used only if the OUT endpoint is NOT configured */
+
+	struct f_hidg *hidg = file->private_data;
+	char *tmp_buf = NULL;
+	unsigned long flags;
+
+	if (!count)
+		return 0;
+
+	spin_lock_irqsave(&hidg->read_spinlock, flags);
+
+	while (!READ_COND_SSREPORT) {
+		spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		if (wait_event_interruptible(hidg->read_queue, READ_COND_SSREPORT))
+			return -ERESTARTSYS;
+
+		spin_lock_irqsave(&hidg->read_spinlock, flags);
+	}
+
+	count = min_t(unsigned int, count, hidg->set_report_length);
+	tmp_buf = hidg->set_report_buf;
+	hidg->set_report_buf = NULL;
+
+	spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+
+	if (tmp_buf != NULL) {
+		count -= copy_to_user(buffer, tmp_buf, count);
+		kfree(tmp_buf);
+	} else {
+		count = -ENOMEM;
+	}
+
+	wake_up(&hidg->read_queue);
+
+	return count;
+}
+
+static ssize_t f_hidg_read(struct file *file, char __user *buffer,
+			   size_t count, loff_t *ptr)
+{
+	struct f_hidg *hidg = file->private_data;
+
+	if (hidg->use_out_ep)
+		return f_hidg_intout_read(file, buffer, count, ptr);
+	else
+		return f_hidg_ssreport_read(file, buffer, count, ptr);
+}
+
 static void f_hidg_req_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	struct f_hidg *hidg = (struct f_hidg *)ep->driver_data;
@@ -433,14 +518,20 @@ static __poll_t f_hidg_poll(struct file *file, poll_table *wait)
 	if (WRITE_COND)
 		ret |= EPOLLOUT | EPOLLWRNORM;
 
-	if (READ_COND)
-		ret |= EPOLLIN | EPOLLRDNORM;
+	if (hidg->use_out_ep) {
+		if (READ_COND_INTOUT)
+			ret |= EPOLLIN | EPOLLRDNORM;
+	} else {
+		if (READ_COND_SSREPORT)
+			ret |= EPOLLIN | EPOLLRDNORM;
+	}
 
 	return ret;
 }
 
 #undef WRITE_COND
-#undef READ_COND
+#undef READ_COND_SSREPORT
+#undef READ_COND_INTOUT
 
 static int f_hidg_release(struct inode *inode, struct file *fd)
 {
@@ -467,8 +558,10 @@ static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
 	return alloc_ep_req(ep, length);
 }
 
-static void hidg_set_report_complete(struct usb_ep *ep, struct usb_request *req)
+static void hidg_intout_complete(struct usb_ep *ep, struct usb_request *req)
 {
+	/* used only if the OUT endpoint is configured */
+
 	struct f_hidg *hidg = (struct f_hidg *) req->context;
 	struct usb_composite_dev *cdev = hidg->func.config->cdev;
 	struct f_hidg_req_list *req_list;
@@ -502,6 +595,39 @@ static void hidg_set_report_complete(struct usb_ep *ep, struct usb_request *req)
 	}
 }
 
+static void hidg_ssreport_complete(struct usb_ep *ep, struct usb_request *req)
+{
+	/* used only if the OUT endpoint is NOT configured */
+
+	struct f_hidg *hidg = (struct f_hidg *)req->context;
+	struct usb_composite_dev *cdev = hidg->func.config->cdev;
+	char *new_buf = NULL;
+	unsigned long flags;
+
+	if (req->status != 0 || req->buf == NULL || req->actual == 0) {
+		ERROR(cdev,
+		      "%s FAILED: status=%d, buf=%p, actual=%d\n",
+		      __func__, req->status, req->buf, req->actual);
+		return;
+	}
+
+	spin_lock_irqsave(&hidg->read_spinlock, flags);
+
+	new_buf = krealloc(hidg->set_report_buf, req->actual, GFP_ATOMIC);
+	if (new_buf == NULL) {
+		spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+		return;
+	}
+	hidg->set_report_buf = new_buf;
+
+	hidg->set_report_length = req->actual;
+	memcpy(hidg->set_report_buf, req->buf, req->actual);
+
+	spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+
+	wake_up(&hidg->read_queue);
+}
+
 static int hidg_setup(struct usb_function *f,
 		const struct usb_ctrlrequest *ctrl)
 {
@@ -549,7 +675,11 @@ static int hidg_setup(struct usb_function *f,
 	case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
 		  | HID_REQ_SET_REPORT):
 		VDBG(cdev, "set_report | wLength=%d\n", ctrl->wLength);
-		goto stall;
+		if (hidg->use_out_ep)
+			goto stall;
+		req->complete = hidg_ssreport_complete;
+		req->context  = hidg;
+		goto respond;
 		break;
 
 	case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
@@ -637,15 +767,18 @@ static void hidg_disable(struct usb_function *f)
 	unsigned long flags;
 
 	usb_ep_disable(hidg->in_ep);
-	usb_ep_disable(hidg->out_ep);
 
-	spin_lock_irqsave(&hidg->read_spinlock, flags);
-	list_for_each_entry_safe(list, next, &hidg->completed_out_req, list) {
-		free_ep_req(hidg->out_ep, list->req);
-		list_del(&list->list);
-		kfree(list);
+	if (hidg->out_ep) {
+		usb_ep_disable(hidg->out_ep);
+
+		spin_lock_irqsave(&hidg->read_spinlock, flags);
+		list_for_each_entry_safe(list, next, &hidg->completed_out_req, list) {
+			free_ep_req(hidg->out_ep, list->req);
+			list_del(&list->list);
+			kfree(list);
+		}
+		spin_unlock_irqrestore(&hidg->read_spinlock, flags);
 	}
-	spin_unlock_irqrestore(&hidg->read_spinlock, flags);
 
 	spin_lock_irqsave(&hidg->write_spinlock, flags);
 	if (!hidg->write_pending) {
@@ -691,8 +824,7 @@ static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 		}
 	}
 
-
-	if (hidg->out_ep != NULL) {
+	if (hidg->use_out_ep && hidg->out_ep != NULL) {
 		/* restart endpoint */
 		usb_ep_disable(hidg->out_ep);
 
@@ -717,7 +849,7 @@ static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 					hidg_alloc_ep_req(hidg->out_ep,
 							  hidg->report_length);
 			if (req) {
-				req->complete = hidg_set_report_complete;
+				req->complete = hidg_intout_complete;
 				req->context  = hidg;
 				status = usb_ep_queue(hidg->out_ep, req,
 						      GFP_ATOMIC);
@@ -743,7 +875,8 @@ static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	}
 	return 0;
 disable_out_ep:
-	usb_ep_disable(hidg->out_ep);
+	if (hidg->out_ep)
+		usb_ep_disable(hidg->out_ep);
 free_req_in:
 	if (req_in)
 		free_ep_req(hidg->in_ep, req_in);
@@ -795,14 +928,21 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 		goto fail;
 	hidg->in_ep = ep;
 
-	ep = usb_ep_autoconfig(c->cdev->gadget, &hidg_fs_out_ep_desc);
-	if (!ep)
-		goto fail;
-	hidg->out_ep = ep;
+	hidg->out_ep = NULL;
+	if (hidg->use_out_ep) {
+		ep = usb_ep_autoconfig(c->cdev->gadget, &hidg_fs_out_ep_desc);
+		if (!ep)
+			goto fail;
+		hidg->out_ep = ep;
+	}
+
+	/* used only if use_out_ep == 1 */
+	hidg->set_report_buf = NULL;
 
 	/* set descriptor dynamic values */
 	hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
 	hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
+	hidg_interface_desc.bNumEndpoints = hidg->use_out_ep ? 2 : 1;
 	hidg->protocol = HID_REPORT_PROTOCOL;
 	hidg->idle = 1;
 	hidg_ss_in_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length);
@@ -833,12 +973,19 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	hidg_ss_out_ep_desc.bEndpointAddress =
 		hidg_fs_out_ep_desc.bEndpointAddress;
 
-	status = usb_assign_descriptors(f, hidg_fs_descriptors,
-			hidg_hs_descriptors, hidg_ss_descriptors,
-			hidg_ss_descriptors);
+#define CHOOSE_DESC(prefix)	\
+	(hidg->use_out_ep ? prefix##_intout : prefix##_ssreport)
+
+	status = usb_assign_descriptors(f,
+		CHOOSE_DESC(hidg_fs_descriptors),
+		CHOOSE_DESC(hidg_hs_descriptors),
+		CHOOSE_DESC(hidg_ss_descriptors),
+		CHOOSE_DESC(hidg_ss_descriptors));
 	if (status)
 		goto fail;
 
+#undef CHOOSE_DESC
+
 	spin_lock_init(&hidg->write_spinlock);
 	hidg->write_pending = 1;
 	hidg->req = NULL;
@@ -950,6 +1097,7 @@ CONFIGFS_ATTR(f_hid_opts_, name)
 
 F_HID_OPT(subclass, 8, 255);
 F_HID_OPT(protocol, 8, 255);
+F_HID_OPT(no_out_endpoint, 8, 1);
 F_HID_OPT(report_length, 16, 65535);
 
 static ssize_t f_hid_opts_report_desc_show(struct config_item *item, char *page)
@@ -1009,6 +1157,7 @@ CONFIGFS_ATTR_RO(f_hid_opts_, dev);
 static struct configfs_attribute *hid_attrs[] = {
 	&f_hid_opts_attr_subclass,
 	&f_hid_opts_attr_protocol,
+	&f_hid_opts_attr_no_out_endpoint,
 	&f_hid_opts_attr_report_length,
 	&f_hid_opts_attr_report_desc,
 	&f_hid_opts_attr_dev,
@@ -1093,6 +1242,7 @@ static void hidg_free(struct usb_function *f)
 	hidg = func_to_hidg(f);
 	opts = container_of(f->fi, struct f_hid_opts, func_inst);
 	kfree(hidg->report_desc);
+	kfree(hidg->set_report_buf);
 	kfree(hidg);
 	mutex_lock(&opts->lock);
 	--opts->refcnt;
@@ -1139,6 +1289,7 @@ static struct usb_function *hidg_alloc(struct usb_function_instance *fi)
 			return ERR_PTR(-ENOMEM);
 		}
 	}
+	hidg->use_out_ep = !opts->no_out_endpoint;
 
 	mutex_unlock(&opts->lock);
 
diff --git a/drivers/usb/gadget/function/u_hid.h b/drivers/usb/gadget/function/u_hid.h
index 98d6af558c03..84bb70292855 100644
--- a/drivers/usb/gadget/function/u_hid.h
+++ b/drivers/usb/gadget/function/u_hid.h
@@ -20,6 +20,7 @@ struct f_hid_opts {
 	int				minor;
 	unsigned char			subclass;
 	unsigned char			protocol;
+	unsigned char			no_out_endpoint;
 	unsigned short			report_length;
 	unsigned short			report_desc_length;
 	unsigned char			*report_desc;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: gadget: f_hid: optional SETUP/SET_REPORT mode
  2021-08-13 11:45 [PATCH] usb: gadget: f_hid: optional SETUP/SET_REPORT mode Maxim Devaev
@ 2021-08-13 14:58 ` Alan Stern
  2021-08-13 20:22   ` Maxim Devaev
  0 siblings, 1 reply; 5+ messages in thread
From: Alan Stern @ 2021-08-13 14:58 UTC (permalink / raw)
  To: Maxim Devaev
  Cc: balbi, gregkh, ruslan.bilovol, mika.westerberg, maze,
	jj251510319013, linux-usb, linux-kernel

On Fri, Aug 13, 2021 at 02:45:51PM +0300, Maxim Devaev wrote:
> f_hid provides the OUT Endpoint for receiving reports from the host.
> The USB HID standard describes the OUT Endpoint support as optional,
> and hosts can ignore it if they don't support it.

No.  The HID standard (version 1.11 -- I may be out of date) actually 
says this (section 4.4):

	The Interrupt Out pipe is optional. If a device declares an 
	Interrupt Out endpoint then Output reports are transmitted by 
	the host to the device through the Interrupt Out endpoint. If 
	no Interrupt Out endpoint is declared then Output reports are
	transmitted to a device through the Control endpoint, using 
	Set_Report(Output) requests.

In other words, a device does not need to have an interrupt-OUT 
endpoint, but if it does have one then the host must use it.

> However, this raises several problems.
> 
> (1) Some host OS drivers without OUT Endpoint support can't receive

Can't _transmit_ output reports.  This is understandable, since such 
hosts aren't compliant with the standard.

>     reports at all. In the case of the keyboard, it becomes impossible
>     to get the status of the LEDs from the host OS.
> 
> (2) Some BIOSes and UEFIs not only don't support the OUT Endpoint,
>     they cannot work with HID with this configuration at all.

What configuration, exactly?  Do you mean that they can't work with 
HID interfaces that include an interrupt-OUT endpoint?

>     For example, absolutely all Apple UEFIs in all Macs can't handle
>     the OUT Endpoint in accordance with the HID standard so it makes
>     impossible to enter the Boot Menu using the hotkey at boot.

These hosts simply give up when they see an HID interface with an 
interrupt-OUT endpoint?  They don't just ignore it and continue on?

>     This problem also occurs on HP and DELL BIOSes and in some dumb
>     devices with primitive embedded firmware like KVM switches.
> 
> This patch adds option no_out_endpoint=1 to disable the OUT Endpoint
> and allow f_hid to receive reports from the host via SETUP/SET_REPORT.

Why not always allow f_hid to receive reports over ep0?  The HID 
standard doesn't forbid this.

> Previously, there was such a feature in f_hid, but it was replaced
> by the OUT Endpoint ("usb: gadget: hidg: register OUT INT endpoint
> for SET_REPORT").

Missing the SHA value of the commit.

Alan Stern

>  It seems that no one knew at the time that it would
> cause problems with BIOS. So this patch actually returns the removed
> functionality making it optional. For backward compatibility reasons,
> the OUT Endpoint mode remains the default behaviour.
> 
> If the SETUP/SET_REPORT mode is used, there is no event processing queue,
> so the user will only read the last report. For classic HID devices
> like keyboard this is not a problem, since it is intended to transmit
> the status of the LEDs and only the last report is important.
> 
> Both modes pass USBCV tests. Checking with the USB protocol analyzer
> also confirms that everything is working as it should and the new mode
> ensures operability in all of the described cases.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: gadget: f_hid: optional SETUP/SET_REPORT mode
  2021-08-13 14:58 ` Alan Stern
@ 2021-08-13 20:22   ` Maxim Devaev
  2021-08-13 20:40     ` Maxim Devaev
  0 siblings, 1 reply; 5+ messages in thread
From: Maxim Devaev @ 2021-08-13 20:22 UTC (permalink / raw)
  To: Alan Stern
  Cc: balbi, gregkh, ruslan.bilovol, mika.westerberg, maze,
	jj251510319013, linux-usb, linux-kernel

Alan Stern <stern@rowland.harvard.edu> wrote:
> In other words, a device does not need to have an interrupt-OUT 
> endpoint, but if it does have one then the host must use it.

You're right. Although the actual behavior of the hosts is not different
from what I wrote - they really just ignore out endpoint.
I will eventually fix this in the patch description.

> > However, this raises several problems.
> > 
> > (1) Some host OS drivers without OUT Endpoint support can't receive  
> 
> Can't _transmit_ output reports.  This is understandable, since such 
> hosts aren't compliant with the standard.

Thank you, this is a typo. With the replacement of the word, the meaning
is fixed. They really don't work. Hosts do not use OUT endpoint
and transmit reports.

> >     reports at all. In the case of the keyboard, it becomes impossible
> >     to get the status of the LEDs from the host OS.
> > 
> > (2) Some BIOSes and UEFIs not only don't support the OUT Endpoint,
> >     they cannot work with HID with this configuration at all.  
> 
> What configuration, exactly?  Do you mean that they can't work with 
> HID interfaces that include an interrupt-OUT endpoint?

Yep. They see OUT endpoint and refuse to use the device in various ways,
for example, they don't continue the initialization process after receiving
the descriptor. The failure behavior may differ, but it always leads to
a refuse or to the fact that the device simply does not poll IN endpoint.

> >     For example, absolutely all Apple UEFIs in all Macs can't handle
> >     the OUT Endpoint in accordance with the HID standard so it makes
> >     impossible to enter the Boot Menu using the hotkey at boot.  
> 
> These hosts simply give up when they see an HID interface with an 
> interrupt-OUT endpoint?  They don't just ignore it and continue on?

Exactly. Hosts will either stop the initialization process when they receive
a handle with an OUT endpoint, or they will not be able to work in any other way.

While investigating the problem with Apple, I realized that their UEFI
is presumably iterated over endpoints and simply does not break the loop
when an IN endpoint is detected. They reach OUT endpoint (which follows IN),
see that it is not IN endpoint, repeat the last initialization ops three times
(SET_IDLE, etc.) and refuse to work with our keyboard. If I change the order
of the endpoints in the descriptor and do OUT first and then IN, then everything
will work in the case of Apple, but this is obviously a violation of the standard,
since it explicitly describes the order of IN and then the optional OUT.

And it only helps with Apple. So the correct solution was to make SETUP/SET_REPORT
because this satisfies all devices. For the order of the endpoints, see 7.2 of HID 1.11:

> The HID class uses the standard request Get_Descriptor as described in the USB
> Specification ... That is, the order shall be:
> Configuration descriptor
>   Interface descriptor (specifying HID Class)
>     HID descriptor (associated with above Interface)
>       Endpoint descriptor (for HID Interrupt In Endpoint)
>       Optional Endpoint descriptor (for HID Interrupt Out Endpoint)

> Why not always allow f_hid to receive reports over ep0?  The HID 
> standard doesn't forbid this.

Previously, this was the case, but then this functionality was removed
for two reasons: using OUT endpoint takes up a USB channel less than SETUP
and allows you to transfer more data from HID gadget, and also allows you
to organize a queue. The queue could have been organized without this change,
but the first point is more significant. I found the correspondence
so that you can read the history of the issue, link below.

That change was not a problem as long as f_hid was mainly used for the keyboard
at the OS level. Now I am making an open source KVM-over-IP based on this
and the compatibility hell have risen to full height.

> Missing the SHA value of the commit.

99c515005857ff7d6cd5c2ba272ccab5dc0ea648

Also the descussion: https://www.spinics.net/lists/linux-usb/msg65494.html

Sorry, I couldn't find this on lore.kernel.org. Maybe I didn't search well,
or this correspondence is too old, 2012.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: gadget: f_hid: optional SETUP/SET_REPORT mode
  2021-08-13 20:22   ` Maxim Devaev
@ 2021-08-13 20:40     ` Maxim Devaev
  2021-08-14  1:45       ` Alan Stern
  0 siblings, 1 reply; 5+ messages in thread
From: Maxim Devaev @ 2021-08-13 20:40 UTC (permalink / raw)
  To: Alan Stern
  Cc: balbi, gregkh, ruslan.bilovol, mika.westerberg, maze,
	jj251510319013, linux-usb, linux-kernel

В Fri, 13 Aug 2021 23:22:12 +0300
Maxim Devaev <mdevaev@gmail.com> пишет:

> Alan Stern <stern@rowland.harvard.edu> wrote:
> > In other words, a device does not need to have an interrupt-OUT 
> > endpoint, but if it does have one then the host must use it.
> 
> You're right. Although the actual behavior of the hosts is not different
> from what I wrote - they really just ignore out endpoint.
> I will eventually fix this in the patch description.

It seems that I have confused everything even more, sorry. I will explain.
There are three possible host behaviors:

(1) The host works with the OUT endpoint as it describes the standard
    and transmits reports through it.

(2) The host works with IN endpoint, but refuses to transmit reports
    via OUT endpoint at all. In the case of the keyboard, it will work,
    but it will not receive the status of the LEDs.

(3) The host sees OUT endpoint and either refuses to use such a device at all,
    or goes crazy in various ways.

In both cases (2) and (3), using SETUP/SET_REPORT solves the problem.
Therefore, I offer this as an option to solve compatibility problems.
Yes, in fact, this is not our problem, but it is impossible to fix the drivers
of all these proprietary devices. Moreover, I have never met a keyboard
with OUT endpoint, absolutely all of them use SETUP/SET_REPORT.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: gadget: f_hid: optional SETUP/SET_REPORT mode
  2021-08-13 20:40     ` Maxim Devaev
@ 2021-08-14  1:45       ` Alan Stern
  0 siblings, 0 replies; 5+ messages in thread
From: Alan Stern @ 2021-08-14  1:45 UTC (permalink / raw)
  To: Maxim Devaev
  Cc: balbi, gregkh, ruslan.bilovol, mika.westerberg, maze,
	jj251510319013, linux-usb, linux-kernel

On Fri, Aug 13, 2021 at 11:40:22PM +0300, Maxim Devaev wrote:
> В Fri, 13 Aug 2021 23:22:12 +0300
> Maxim Devaev <mdevaev@gmail.com> пишет:
> 
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> > > In other words, a device does not need to have an interrupt-OUT 
> > > endpoint, but if it does have one then the host must use it.
> > 
> > You're right. Although the actual behavior of the hosts is not different
> > from what I wrote - they really just ignore out endpoint.
> > I will eventually fix this in the patch description.
> 
> It seems that I have confused everything even more, sorry. I will explain.
> There are three possible host behaviors:
> 
> (1) The host works with the OUT endpoint as it describes the standard
>     and transmits reports through it.
> 
> (2) The host works with IN endpoint, but refuses to transmit reports
>     via OUT endpoint at all. In the case of the keyboard, it will work,
>     but it will not receive the status of the LEDs.
> 
> (3) The host sees OUT endpoint and either refuses to use such a device at all,
>     or goes crazy in various ways.
> 
> In both cases (2) and (3), using SETUP/SET_REPORT solves the problem.
> Therefore, I offer this as an option to solve compatibility problems.
> Yes, in fact, this is not our problem, but it is impossible to fix the drivers
> of all these proprietary devices. Moreover, I have never met a keyboard
> with OUT endpoint, absolutely all of them use SETUP/SET_REPORT.

Okay.  I appreciate the more detailed explanations; thanks.  Please 
resubmit the patch with appropriate changes to the description.

Alan Stern

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-14  1:45 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13 11:45 [PATCH] usb: gadget: f_hid: optional SETUP/SET_REPORT mode Maxim Devaev
2021-08-13 14:58 ` Alan Stern
2021-08-13 20:22   ` Maxim Devaev
2021-08-13 20:40     ` Maxim Devaev
2021-08-14  1:45       ` Alan Stern

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).