[PATCH 1/2] USB: gadget: f_hid: Add Get-Feature report

[PATCH 1/2] USB: gadget: f_hid: Add Get-Feature report

[Vicki Pfau]

While the HID gadget implementation has been sufficient for devices that only
use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
Get-Feature report CONTROL transfers that go over endpoint 0. These were
previously impossible with the existing implementation, and would either send
an empty reply, or stall out.

As the feature is a standard part of USB HID, it stands to reason that devices
would use it, and that the HID gadget should support it. This patch adds
support for (polled) device-to-host Get-Feature reports through a new ioctl
interface to the hidg class dev nodes.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/usb/gadget/function/f_hid.c | 121 ++++++++++++++++++++++++++--
 include/uapi/linux/usb/g_hid.h      |  38 +++++++++
 include/uapi/linux/usb/gadgetfs.h   |   2 +-
 3 files changed, 154 insertions(+), 7 deletions(-)
 create mode 100644 include/uapi/linux/usb/g_hid.h

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index ca0a7d9eaa34..7e70e4a168e6 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -16,6 +16,7 @@
 #include <linux/wait.h>
 #include <linux/sched.h>
 #include <linux/usb/g_hid.h>
+#include <uapi/linux/usb/g_hid.h>
 
 #include "u_f.h"
 #include "u_hid.h"
@@ -71,6 +72,13 @@ struct f_hidg {
 	wait_queue_head_t		write_queue;
 	struct usb_request		*req;
 
+	/* get report */
+	struct usb_request		*get_req;
+	struct usb_hidg_report		get_report;
+	spinlock_t			get_spinlock;
+	bool				get_pending;
+	wait_queue_head_t		get_queue;
+
 	int				minor;
 	struct cdev			cdev;
 	struct usb_function		func;
@@ -511,6 +519,64 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 	return status;
 }
 
+
+static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *buffer)
+{
+	struct f_hidg			*hidg = file->private_data;
+	struct usb_composite_dev	*cdev = hidg->func.config->cdev;
+
+	int		status = 0;
+	unsigned long	flags;
+
+	spin_lock_irqsave(&hidg->get_spinlock, flags);
+
+#define GET_REPORT_COND (!hidg->get_pending)
+
+	while (!GET_REPORT_COND) {
+		spin_unlock_irqrestore(&hidg->get_spinlock, flags);
+
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		if (wait_event_interruptible_exclusive(hidg->get_queue,
+						       GET_REPORT_COND))
+			return -ERESTARTSYS;
+
+		spin_lock_irqsave(&hidg->get_spinlock, flags);
+		if (!hidg->get_pending) {
+			spin_unlock_irqrestore(&hidg->get_spinlock, flags);
+			return -EINVAL;
+		}
+	}
+
+	hidg->get_pending = true;
+	spin_unlock_irqrestore(&hidg->get_spinlock, flags);
+
+	status = copy_from_user(&hidg->get_report, buffer,
+				sizeof(struct usb_hidg_report));
+	if (status != 0) {
+		ERROR(cdev, "copy_from_user error\n");
+		status = -EINVAL;
+	}
+
+	spin_lock_irqsave(&hidg->get_spinlock, flags);
+	hidg->get_pending = false;
+	spin_unlock_irqrestore(&hidg->get_spinlock, flags);
+
+	wake_up(&hidg->get_queue);
+	return status;
+}
+
+static long f_hidg_ioctl(struct file *file, unsigned int code, unsigned long arg)
+{
+	switch (code) {
+	case GADGET_HID_WRITE_GET_REPORT:
+		return f_hidg_get_report(file, (struct usb_hidg_report __user *)arg);
+	default:
+		return -ENOTTY;
+	}
+}
+
 static __poll_t f_hidg_poll(struct file *file, poll_table *wait)
 {
 	struct f_hidg	*hidg  = file->private_data;
@@ -536,6 +602,7 @@ static __poll_t f_hidg_poll(struct file *file, poll_table *wait)
 #undef WRITE_COND
 #undef READ_COND_SSREPORT
 #undef READ_COND_INTOUT
+#undef GET_REPORT_COND
 
 static int f_hidg_release(struct inode *inode, struct file *fd)
 {
@@ -628,6 +695,10 @@ static void hidg_ssreport_complete(struct usb_ep *ep, struct usb_request *req)
 	wake_up(&hidg->read_queue);
 }
 
+static void hidg_get_report_complete(struct usb_ep *ep, struct usb_request *req)
+{
+}
+
 static int hidg_setup(struct usb_function *f,
 		const struct usb_ctrlrequest *ctrl)
 {
@@ -635,6 +706,8 @@ static int hidg_setup(struct usb_function *f,
 	struct usb_composite_dev	*cdev = f->config->cdev;
 	struct usb_request		*req  = cdev->req;
 	int status = 0;
+	unsigned long flags;
+	bool do_wake = false;
 	__u16 value, length;
 
 	value	= __le16_to_cpu(ctrl->wValue);
@@ -647,14 +720,29 @@ static int hidg_setup(struct usb_function *f,
 	switch ((ctrl->bRequestType << 8) | ctrl->bRequest) {
 	case ((USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
 		  | HID_REQ_GET_REPORT):
-		VDBG(cdev, "get_report\n");
+		VDBG(cdev, "get_report | wLength=%d\n", ctrl->wLength);
 
-		/* send an empty report */
-		length = min_t(unsigned, length, hidg->report_length);
-		memset(req->buf, 0x0, length);
+		req = hidg->get_req;
+		req->zero = 0;
+		req->length = min_t(unsigned, length, hidg->report_length);
+		status = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
+		if (status < 0) {
+			ERROR(cdev, "usb_ep_queue error on get_report %d\n",
+			      status);
 
-		goto respond;
-		break;
+			spin_lock_irqsave(&hidg->get_spinlock, flags);
+			if (hidg->get_pending) {
+				hidg->get_pending = false;
+				do_wake = true;
+			}
+			spin_unlock_irqrestore(&hidg->get_spinlock, flags);
+
+			if (do_wake) {
+				wake_up(&hidg->get_queue);
+			}
+		}
+
+		return status;
 
 	case ((USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
 		  | HID_REQ_GET_PROTOCOL):
@@ -788,6 +876,14 @@ static void hidg_disable(struct usb_function *f)
 
 	hidg->req = NULL;
 	spin_unlock_irqrestore(&hidg->write_spinlock, flags);
+
+	spin_lock_irqsave(&hidg->get_spinlock, flags);
+	if (!hidg->get_pending) {
+		usb_ep_free_request(f->config->cdev->gadget->ep0, hidg->get_req);
+		hidg->get_pending = true;
+	}
+	hidg->get_req = NULL;
+	spin_unlock_irqrestore(&hidg->get_spinlock, flags);
 }
 
 static int hidg_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
@@ -896,6 +992,7 @@ static const struct file_operations f_hidg_fops = {
 	.write		= f_hidg_write,
 	.read		= f_hidg_read,
 	.poll		= f_hidg_poll,
+	.unlocked_ioctl	= f_hidg_ioctl,
 	.llseek		= noop_llseek,
 };
 
@@ -908,6 +1005,14 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	int			status;
 	dev_t			dev;
 
+	hidg->get_req = usb_ep_alloc_request(c->cdev->gadget->ep0, GFP_ATOMIC);
+	if (!hidg->get_req)
+		return -ENOMEM;
+	hidg->get_req->buf = hidg->get_report.data;
+	hidg->get_req->zero = 0;
+	hidg->get_req->complete = hidg_get_report_complete;
+	hidg->get_req->context = hidg;
+
 	/* maybe allocate device-global string IDs, and patch descriptors */
 	us = usb_gstrings_attach(c->cdev, ct_func_strings,
 				 ARRAY_SIZE(ct_func_string_defs));
@@ -993,8 +1098,10 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	hidg->write_pending = 1;
 	hidg->req = NULL;
 	spin_lock_init(&hidg->read_spinlock);
+	spin_lock_init(&hidg->get_spinlock);
 	init_waitqueue_head(&hidg->write_queue);
 	init_waitqueue_head(&hidg->read_queue);
+	init_waitqueue_head(&hidg->get_queue);
 	INIT_LIST_HEAD(&hidg->completed_out_req);
 
 	/* create char device */
@@ -1021,6 +1128,8 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	if (hidg->req != NULL)
 		free_ep_req(hidg->in_ep, hidg->req);
 
+	usb_ep_free_request(c->cdev->gadget->ep0, hidg->get_req);
+
 	return status;
 }
 
diff --git a/include/uapi/linux/usb/g_hid.h b/include/uapi/linux/usb/g_hid.h
new file mode 100644
index 000000000000..c6068b486354
--- /dev/null
+++ b/include/uapi/linux/usb/g_hid.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
+/*
+ * g_hid.h -- Header file for USB HID gadget driver
+ *
+ * Copyright (C) 2022 Valve Software
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef __UAPI_LINUX_USB_G_HID_H
+#define __UAPI_LINUX_USB_G_HID_H
+
+#include <linux/types.h>
+
+struct usb_hidg_report {
+	__u16 length;
+	__u8 data[512];
+};
+
+/* The 'g' code is also used by gadgetfs and hid gadget ioctl requests.
+ * Don't add any colliding codes to either driver, and keep
+ * them in unique ranges (size 0x20 for now).
+ */
+#define GADGET_HID_WRITE_GET_REPORT	_IOW('g', 0x42, struct usb_hidg_report)
+
+#endif /* __UAPI_LINUX_USB_G_HID_H */
diff --git a/include/uapi/linux/usb/gadgetfs.h b/include/uapi/linux/usb/gadgetfs.h
index 835473910a49..9754822b2a40 100644
--- a/include/uapi/linux/usb/gadgetfs.h
+++ b/include/uapi/linux/usb/gadgetfs.h
@@ -62,7 +62,7 @@ struct usb_gadgetfs_event {
 };
 
 
-/* The 'g' code is also used by printer gadget ioctl requests.
+/* The 'g' code is also used by printer and hid gadget ioctl requests.
  * Don't add any colliding codes to either driver, and keep
  * them in unique ranges (size 0x20 for now).
  */
-- 
2.37.1

[PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Vicki Pfau]

While the HID gadget implementation has been sufficient for devices that only
use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
Get-Feature report CONTROL transfers that go over endpoint 0. These were
previously impossible with the existing implementation, and would either send
an empty reply, or stall out.

As the feature is a standard part of USB HID, it stands to reason that devices
would use it, and that the HID gadget should support it. This patch adds
support for host-to-device Set-Feature reports through a new ioctl
interface to the hidg class dev nodes.

Signed-off-by: Vicki Pfau <vi@endrift.com>
---
 drivers/usb/gadget/function/f_hid.c | 112 ++++++++++++++++++++++++++--
 include/uapi/linux/usb/g_hid.h      |   1 +
 2 files changed, 105 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
index 7e70e4a168e6..97aa56206020 100644
--- a/drivers/usb/gadget/function/f_hid.c
+++ b/drivers/usb/gadget/function/f_hid.c
@@ -72,6 +72,11 @@ struct f_hidg {
 	wait_queue_head_t		write_queue;
 	struct usb_request		*req;
 
+	/* set report */
+	struct list_head		completed_set_req;
+	spinlock_t			set_spinlock;
+	wait_queue_head_t		set_queue;
+
 	/* get report */
 	struct usb_request		*get_req;
 	struct usb_hidg_report		get_report;
@@ -519,6 +524,54 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 	return status;
 }
 
+static int f_hidg_set_report(struct file *file, struct usb_hidg_report __user *buffer)
+{
+	struct f_hidg		*hidg = file->private_data;
+	struct f_hidg_req_list	*list;
+	struct usb_request	*req;
+	unsigned long		flags;
+	unsigned short		length;
+	int			status;
+
+	spin_lock_irqsave(&hidg->set_spinlock, flags);
+
+#define SET_REPORT_COND (!list_empty(&hidg->completed_set_req))
+
+	/* wait for at least one buffer to complete */
+	while (!SET_REPORT_COND) {
+		spin_unlock_irqrestore(&hidg->set_spinlock, flags);
+		if (file->f_flags & O_NONBLOCK)
+			return -EAGAIN;
+
+		if (wait_event_interruptible(hidg->set_queue, SET_REPORT_COND))
+			return -ERESTARTSYS;
+
+		spin_lock_irqsave(&hidg->set_spinlock, flags);
+	}
+
+	/* pick the first one */
+	list = list_first_entry(&hidg->completed_set_req,
+				struct f_hidg_req_list, list);
+
+	/*
+	 * Remove this from list to protect it from being free()
+	 * while host disables our function
+	 */
+	list_del(&list->list);
+
+	req = list->req;
+	spin_unlock_irqrestore(&hidg->set_spinlock, flags);
+
+	/* copy to user outside spinlock */
+	length = min_t(unsigned short, sizeof(buffer->data), req->actual);
+	status = copy_to_user(&buffer->length, &length, sizeof(buffer->length));
+	if (!status) {
+	    status = copy_to_user(&buffer->data, req->buf, length);
+	}
+	kfree(list);
+	free_ep_req(hidg->func.config->cdev->gadget->ep0, req);
+	return status;
+}
 
 static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *buffer)
 {
@@ -570,6 +623,8 @@ static int f_hidg_get_report(struct file *file, struct usb_hidg_report __user *b
 static long f_hidg_ioctl(struct file *file, unsigned int code, unsigned long arg)
 {
 	switch (code) {
+	case GADGET_HID_READ_SET_REPORT:
+		return f_hidg_set_report(file, (struct usb_hidg_report __user *)arg);
 	case GADGET_HID_WRITE_GET_REPORT:
 		return f_hidg_get_report(file, (struct usb_hidg_report __user *)arg);
 	default:
@@ -584,6 +639,7 @@ static __poll_t f_hidg_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &hidg->read_queue, wait);
 	poll_wait(file, &hidg->write_queue, wait);
+	poll_wait(file, &hidg->set_queue, wait);
 
 	if (WRITE_COND)
 		ret |= EPOLLOUT | EPOLLWRNORM;
@@ -596,12 +652,16 @@ static __poll_t f_hidg_poll(struct file *file, poll_table *wait)
 			ret |= EPOLLIN | EPOLLRDNORM;
 	}
 
+	if (SET_REPORT_COND)
+		ret |= EPOLLPRI;
+
 	return ret;
 }
 
 #undef WRITE_COND
 #undef READ_COND_SSREPORT
 #undef READ_COND_INTOUT
+#undef SET_REPORT_COND
 #undef GET_REPORT_COND
 
 static int f_hidg_release(struct inode *inode, struct file *fd)
@@ -646,11 +706,19 @@ static void hidg_intout_complete(struct usb_ep *ep, struct usb_request *req)
 
 		req_list->req = req;
 
-		spin_lock_irqsave(&hidg->read_spinlock, flags);
-		list_add_tail(&req_list->list, &hidg->completed_out_req);
-		spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+		if (ep == cdev->gadget->ep0) {
+			spin_lock_irqsave(&hidg->set_spinlock, flags);
+			list_add_tail(&req_list->list, &hidg->completed_set_req);
+			spin_unlock_irqrestore(&hidg->set_spinlock, flags);
 
-		wake_up(&hidg->read_queue);
+			wake_up(&hidg->set_queue);
+		} else {
+			spin_lock_irqsave(&hidg->read_spinlock, flags);
+			list_add_tail(&req_list->list, &hidg->completed_out_req);
+			spin_unlock_irqrestore(&hidg->read_spinlock, flags);
+
+			wake_up(&hidg->read_queue);
+		}
 		break;
 	default:
 		ERROR(cdev, "Set report failed %d\n", req->status);
@@ -763,12 +831,29 @@ 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);
-		if (hidg->use_out_ep)
+		if (!hidg->use_out_ep) {
+			req->complete = hidg_ssreport_complete;
+			req->context  = hidg;
+			goto respond;
+		}
+		if (!length) {
 			goto stall;
-		req->complete = hidg_ssreport_complete;
+		}
+		req = alloc_ep_req(cdev->gadget->ep0, GFP_ATOMIC);
+		if (!req) {
+			return -ENOMEM;
+		}
+		req->complete = hidg_intout_complete;
 		req->context  = hidg;
-		goto respond;
-		break;
+		req->zero = 0;
+		req->length = length;
+		status = usb_ep_queue(cdev->gadget->ep0, req, GFP_ATOMIC);
+		if (status < 0) {
+			ERROR(cdev, "usb_ep_queue error on set_report %d\n", status);
+			free_ep_req(cdev->gadget->ep0, req);
+		}
+
+		return status;
 
 	case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8
 		  | HID_REQ_SET_PROTOCOL):
@@ -868,6 +953,14 @@ static void hidg_disable(struct usb_function *f)
 		spin_unlock_irqrestore(&hidg->read_spinlock, flags);
 	}
 
+	spin_lock_irqsave(&hidg->set_spinlock, flags);
+	list_for_each_entry_safe(list, next, &hidg->completed_set_req, list) {
+		free_ep_req(f->config->cdev->gadget->ep0, list->req);
+		list_del(&list->list);
+		kfree(list);
+	}
+	spin_unlock_irqrestore(&hidg->set_spinlock, flags);
+
 	spin_lock_irqsave(&hidg->write_spinlock, flags);
 	if (!hidg->write_pending) {
 		free_ep_req(hidg->in_ep, hidg->req);
@@ -1098,11 +1191,14 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f)
 	hidg->write_pending = 1;
 	hidg->req = NULL;
 	spin_lock_init(&hidg->read_spinlock);
+	spin_lock_init(&hidg->set_spinlock);
 	spin_lock_init(&hidg->get_spinlock);
 	init_waitqueue_head(&hidg->write_queue);
 	init_waitqueue_head(&hidg->read_queue);
+	init_waitqueue_head(&hidg->set_queue);
 	init_waitqueue_head(&hidg->get_queue);
 	INIT_LIST_HEAD(&hidg->completed_out_req);
+	INIT_LIST_HEAD(&hidg->completed_set_req);
 
 	/* create char device */
 	cdev_init(&hidg->cdev, &f_hidg_fops);
diff --git a/include/uapi/linux/usb/g_hid.h b/include/uapi/linux/usb/g_hid.h
index c6068b486354..81416698e145 100644
--- a/include/uapi/linux/usb/g_hid.h
+++ b/include/uapi/linux/usb/g_hid.h
@@ -33,6 +33,7 @@ struct usb_hidg_report {
  * Don't add any colliding codes to either driver, and keep
  * them in unique ranges (size 0x20 for now).
  */
+#define GADGET_HID_READ_SET_REPORT	_IOR('g', 0x41, struct usb_hidg_report)
 #define GADGET_HID_WRITE_GET_REPORT	_IOW('g', 0x42, struct usb_hidg_report)
 
 #endif /* __UAPI_LINUX_USB_G_HID_H */
-- 
2.37.1

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Maxim Devaev]

В Mon, 25 Jul 2022 17:58:26 -0700
Vicki Pfau <vi@endrift.com> пишет:

> While the HID gadget implementation has been sufficient for devices that only
> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
> Get-Feature report CONTROL transfers that go over endpoint 0. These were
> previously impossible with the existing implementation, and would either send
> an empty reply, or stall out.
> 
> As the feature is a standard part of USB HID, it stands to reason that devices
> would use it, and that the HID gadget should support it. This patch adds
> support for host-to-device Set-Feature reports through a new ioctl
> interface to the hidg class dev nodes.
> 
> Signed-off-by: Vicki Pfau <vi@endrift.com>

Won't it break the logic of the existing software that works with /dev/hidgX?
Will it work if I want my gadget to work the old way?
It is important that the old behavior is the default without having to use
the new ioctls. As for these ioctls, since this is an addition to the new API,
they should be documented. I think it's also worth adding these ioctls
to the userspace example:
  - Documentation/usb/gadget_hid.rst
  - Documentation/usb/gadget-testing.rst

But it seems to me that extending the HID functionality to meet
the specifications is definitely a good idea :)

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Vicki Pfau]

On 7/26/22 02:51, Maxim Devaev wrote:
> В Mon, 25 Jul 2022 17:58:26 -0700
> Vicki Pfau <vi@endrift.com> пишет:
> 
>> While the HID gadget implementation has been sufficient for devices that only
>> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
>> Get-Feature report CONTROL transfers that go over endpoint 0. These were
>> previously impossible with the existing implementation, and would either send
>> an empty reply, or stall out.
>>
>> As the feature is a standard part of USB HID, it stands to reason that devices
>> would use it, and that the HID gadget should support it. This patch adds
>> support for host-to-device Set-Feature reports through a new ioctl
>> interface to the hidg class dev nodes.
>>
>> Signed-off-by: Vicki Pfau <vi@endrift.com>
> 
> Won't it break the logic of the existing software that works with /dev/hidgX?
> Will it work if I want my gadget to work the old way?

For existing software to use SET_FEATURE at all it has to use an alternative mode, which seems to have only been added somewhat recently. That mode also appears to preclude use of INTERRUPT transfers at all, unless there's some way to set up two hidg nodes that map to the same interface, with one for INTERRUPT and one for SET_FEATURE. If this breaks that, I suppose that's a regression, but this is meant to augment the original, long-standing mode so you can mix INTERRUPT and SET/GET_FEATURE transfers, as there is no way to do that yet. Honestly, the alternate mode seems more like a workaround, as far as I can tell, and not an ideal implementation. I'm not sure when it was added, but as I was originally authoring this against 5.13 and didn't see it until I went to rebase onto master, it can't have been that long ago. So if it breaks any software (which I don't believe it does), it would only affect very new software.

As I alluded to, I'd thought about perhaps adding a second node per interface so one would act as INTERRUPT transfers and the other as SET/GET_FEATURE transfers, but I already had this code half written and wanted to get feedback first, especially since what I have now works (although it's not well-tested after rebasing).

> It is important that the old behavior is the default without having to use
> the new ioctls. As for these ioctls, since this is an addition to the new API,
> they should be documented. I think it's also worth adding these ioctls
> to the userspace example:
>   - Documentation/usb/gadget_hid.rst
>   - Documentation/usb/gadget-testing.rst

Yup, I should probably do that. Forgot about that before sending the patch, though I'm hoping to be told if my approach was even worthwhile before I write that, but I can work on that soon either way.

> 
> But it seems to me that extending the HID functionality to meet
> the specifications is definitely a good idea :)

Re: [PATCH 1/2] USB: gadget: f_hid: Add Get-Feature report

[Greg Kroah-Hartman]

On Mon, Jul 25, 2022 at 05:58:25PM -0700, Vicki Pfau wrote:
> --- /dev/null
> +++ b/include/uapi/linux/usb/g_hid.h
> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/*
> + * g_hid.h -- Header file for USB HID gadget driver
> + *
> + * Copyright (C) 2022 Valve Software
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA

This whole license "boilerplate" is not needed, and should be removed
(especially things like addresses, that's crazy).

Only thing that is needed is the SPDX line.

> + */
> +
> +#ifndef __UAPI_LINUX_USB_G_HID_H
> +#define __UAPI_LINUX_USB_G_HID_H
> +
> +#include <linux/types.h>
> +
> +struct usb_hidg_report {
> +	__u16 length;
> +	__u8 data[512];

Why 512?

> +};
> +
> +/* The 'g' code is also used by gadgetfs and hid gadget ioctl requests.
> + * Don't add any colliding codes to either driver, and keep
> + * them in unique ranges (size 0x20 for now).
> + */
> +#define GADGET_HID_WRITE_GET_REPORT	_IOW('g', 0x42, struct usb_hidg_report)

This should be in the same .h file so that we don't get confused and
accidentally use the same ioctl.

> +
> +#endif /* __UAPI_LINUX_USB_G_HID_H */
> diff --git a/include/uapi/linux/usb/gadgetfs.h b/include/uapi/linux/usb/gadgetfs.h
> index 835473910a49..9754822b2a40 100644
> --- a/include/uapi/linux/usb/gadgetfs.h
> +++ b/include/uapi/linux/usb/gadgetfs.h
> @@ -62,7 +62,7 @@ struct usb_gadgetfs_event {
>  };
>  
>  
> -/* The 'g' code is also used by printer gadget ioctl requests.
> +/* The 'g' code is also used by printer and hid gadget ioctl requests.

Yeah, put the definition here.

thanks,

greg k-h

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Maxim Devaev]

В Tue, 26 Jul 2022 21:26:05 -0700
Vicki Pfau <vi@endrift.com> пишет:

> On 7/26/22 02:51, Maxim Devaev wrote:
> > В Mon, 25 Jul 2022 17:58:26 -0700
> > Vicki Pfau <vi@endrift.com> пишет:
> >   
> >> While the HID gadget implementation has been sufficient for devices that only
> >> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
> >> Get-Feature report CONTROL transfers that go over endpoint 0. These were
> >> previously impossible with the existing implementation, and would either send
> >> an empty reply, or stall out.
> >>
> >> As the feature is a standard part of USB HID, it stands to reason that devices
> >> would use it, and that the HID gadget should support it. This patch adds
> >> support for host-to-device Set-Feature reports through a new ioctl
> >> interface to the hidg class dev nodes.
> >>
> >> Signed-off-by: Vicki Pfau <vi@endrift.com>  
> > 
> > Won't it break the logic of the existing software that works with /dev/hidgX?
> > Will it work if I want my gadget to work the old way?  
> 
> For existing software to use SET_FEATURE at all it has to use an alternative mode, which seems to have only been added somewhat recently. That mode also appears to preclude use of INTERRUPT transfers at all, unless there's some way to set up two hidg nodes that map to the same interface, with one for INTERRUPT and one for SET_FEATURE. If this breaks that, I suppose that's a regression, but this is meant to augment the original, long-standing mode so you can mix INTERRUPT and SET/GET_FEATURE transfers, as there is no way to do that yet. Honestly, the alternate mode seems more like a workaround, as far as I can tell, and not an ideal implementation. I'm not sure when it was added, but as I was originally authoring this against 5.13 and didn't see it until I went to rebase onto master, it can't have been that long ago. So if it breaks any software (which I don't believe it does), it would only affect very new software.
> 
> As I alluded to, I'd thought about perhaps adding a second node per interface so one would act as INTERRUPT transfers and the other as SET/GET_FEATURE transfers, but I already had this code half written and wanted to get feedback first, especially since what I have now works (although it's not well-tested after rebasing).

I'm a little confused here about what you call an alternative mode.
Are we talking about use_out_ep=1 (default behavior with INTERRUPT)
or use_out_ep=0 (SETUP/SET_REPORT)? The last mode was added by me
to ensure strict compatibility with Apple UEFI and strange BIOS,
and this mode is actually actively used. It is important to me
that it is not broken, but unfortunately I cannot test your patch
on my kernel, as I temporarily do not have access to testing equipment.

Re: [PATCH 1/2] USB: gadget: f_hid: Add Get-Feature report

[Vicki Pfau]

On 7/28/22 01:08, Greg Kroah-Hartman wrote:
> On Mon, Jul 25, 2022 at 05:58:25PM -0700, Vicki Pfau wrote:
>> --- /dev/null
>> +++ b/include/uapi/linux/usb/g_hid.h
>> @@ -0,0 +1,38 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
>> +/*
>> + * g_hid.h -- Header file for USB HID gadget driver
>> + *
>> + * Copyright (C) 2022 Valve Software
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> 
> This whole license "boilerplate" is not needed, and should be removed
> (especially things like addresses, that's crazy).
> 
> Only thing that is needed is the SPDX line.

I was just copying the header from g_printer.h and changing as needed. It hasn't been touched since 2017, so if that's no longer the convention you might want to update it too.

> 
>> + */
>> +
>> +#ifndef __UAPI_LINUX_USB_G_HID_H
>> +#define __UAPI_LINUX_USB_G_HID_H
>> +
>> +#include <linux/types.h>
>> +
>> +struct usb_hidg_report {
>> +	__u16 length;
>> +	__u8 data[512];
> 
> Why 512?

I was reading the specs and one of them said the maximum report length for USB 3 (I believe) was 512 bytes (in contrast with USB 2's 64). I can try to find where it said this, or add a define for max report length.

> 
>> +};
>> +
>> +/* The 'g' code is also used by gadgetfs and hid gadget ioctl requests.
>> + * Don't add any colliding codes to either driver, and keep
>> + * them in unique ranges (size 0x20 for now).
>> + */
>> +#define GADGET_HID_WRITE_GET_REPORT	_IOW('g', 0x42, struct usb_hidg_report)
> 
> This should be in the same .h file so that we don't get confused and
> accidentally use the same ioctl.

The same .h file as which? g_printer.h and gadgetfs.h are separate files.

> 
>> +
>> +#endif /* __UAPI_LINUX_USB_G_HID_H */
>> diff --git a/include/uapi/linux/usb/gadgetfs.h b/include/uapi/linux/usb/gadgetfs.h
>> index 835473910a49..9754822b2a40 100644
>> --- a/include/uapi/linux/usb/gadgetfs.h
>> +++ b/include/uapi/linux/usb/gadgetfs.h
>> @@ -62,7 +62,7 @@ struct usb_gadgetfs_event {
>>  };
>>  
>>  
>> -/* The 'g' code is also used by printer gadget ioctl requests.
>> +/* The 'g' code is also used by printer and hid gadget ioctl requests.
> 
> Yeah, put the definition here.

Should I move the g_printer.h definitions here at the same time? Maybe stub out g_printer.h and make it include gadgetfs.h?

> 
> thanks,
> 
> greg k-h

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Vicki Pfau]

On 7/28/22 01:59, Maxim Devaev wrote:
> В Tue, 26 Jul 2022 21:26:05 -0700
> Vicki Pfau <vi@endrift.com> пишет:
> 
>> On 7/26/22 02:51, Maxim Devaev wrote:
>>> В Mon, 25 Jul 2022 17:58:26 -0700
>>> Vicki Pfau <vi@endrift.com> пишет:
>>>   
>>>> While the HID gadget implementation has been sufficient for devices that only
>>>> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
>>>> Get-Feature report CONTROL transfers that go over endpoint 0. These were
>>>> previously impossible with the existing implementation, and would either send
>>>> an empty reply, or stall out.
>>>>
>>>> As the feature is a standard part of USB HID, it stands to reason that devices
>>>> would use it, and that the HID gadget should support it. This patch adds
>>>> support for host-to-device Set-Feature reports through a new ioctl
>>>> interface to the hidg class dev nodes.
>>>>
>>>> Signed-off-by: Vicki Pfau <vi@endrift.com>  
>>>
>>> Won't it break the logic of the existing software that works with /dev/hidgX?
>>> Will it work if I want my gadget to work the old way?  
>>
>> For existing software to use SET_FEATURE at all it has to use an alternative mode, which seems to have only been added somewhat recently. That mode also appears to preclude use of INTERRUPT transfers at all, unless there's some way to set up two hidg nodes that map to the same interface, with one for INTERRUPT and one for SET_FEATURE. If this breaks that, I suppose that's a regression, but this is meant to augment the original, long-standing mode so you can mix INTERRUPT and SET/GET_FEATURE transfers, as there is no way to do that yet. Honestly, the alternate mode seems more like a workaround, as far as I can tell, and not an ideal implementation. I'm not sure when it was added, but as I was originally authoring this against 5.13 and didn't see it until I went to rebase onto master, it can't have been that long ago. So if it breaks any software (which I don't believe it does), it would only affect very new software.
>>
>> As I alluded to, I'd thought about perhaps adding a second node per interface so one would act as INTERRUPT transfers and the other as SET/GET_FEATURE transfers, but I already had this code half written and wanted to get feedback first, especially since what I have now works (although it's not well-tested after rebasing).
> 
> I'm a little confused here about what you call an alternative mode.
> Are we talking about use_out_ep=1 (default behavior with INTERRUPT)
> or use_out_ep=0 (SETUP/SET_REPORT)? The last mode was added by me
> to ensure strict compatibility with Apple UEFI and strange BIOS,
> and this mode is actually actively used. It is important to me
> that it is not broken, but unfortunately I cannot test your patch
> on my kernel, as I temporarily do not have access to testing equipment.

use_out_ep=0 is the alternate mode I was talking about. It didn't exist in 5.13, so as I said I wasn't aware of it until I rebased. As the device I'm using is still stuck on that old kernel (for now) and I don't know if I have any USB gadget capable devices on new kernels, I was unable to test it, and would very much like to make sure it doesn't regress before a patch is merged. I wasn't intending to break it, but I figured I'd get feedback I'd need to change before this was merged so if you could test it to ensure it doesn't regress any behavior that would be much appreciated and help me out. I will probably wait until then before submitting a v2.

Re: [PATCH 1/2] USB: gadget: f_hid: Add Get-Feature report

[Greg Kroah-Hartman]

On Thu, Jul 28, 2022 at 11:07:20AM -0700, Vicki Pfau wrote:
> 
> 
> On 7/28/22 01:08, Greg Kroah-Hartman wrote:
> > On Mon, Jul 25, 2022 at 05:58:25PM -0700, Vicki Pfau wrote:
> >> --- /dev/null
> >> +++ b/include/uapi/linux/usb/g_hid.h
> >> @@ -0,0 +1,38 @@
> >> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> >> +/*
> >> + * g_hid.h -- Header file for USB HID gadget driver
> >> + *
> >> + * Copyright (C) 2022 Valve Software
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program; if not, write to the Free Software
> >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> > 
> > This whole license "boilerplate" is not needed, and should be removed
> > (especially things like addresses, that's crazy).
> > 
> > Only thing that is needed is the SPDX line.
> 
> I was just copying the header from g_printer.h and changing as needed. It hasn't been touched since 2017, so if that's no longer the convention you might want to update it too.

Good point, I will look into that.

> >> + */
> >> +
> >> +#ifndef __UAPI_LINUX_USB_G_HID_H
> >> +#define __UAPI_LINUX_USB_G_HID_H
> >> +
> >> +#include <linux/types.h>
> >> +
> >> +struct usb_hidg_report {
> >> +	__u16 length;
> >> +	__u8 data[512];
> > 
> > Why 512?
> 
> I was reading the specs and one of them said the maximum report length for USB 3 (I believe) was 512 bytes (in contrast with USB 2's 64). I can try to find where it said this, or add a define for max report length.

Either is fine, magic values like this should always be documented
somehow.

> >> +};
> >> +
> >> +/* The 'g' code is also used by gadgetfs and hid gadget ioctl requests.
> >> + * Don't add any colliding codes to either driver, and keep
> >> + * them in unique ranges (size 0x20 for now).
> >> + */
> >> +#define GADGET_HID_WRITE_GET_REPORT	_IOW('g', 0x42, struct usb_hidg_report)
> > 
> > This should be in the same .h file so that we don't get confused and
> > accidentally use the same ioctl.
> 
> The same .h file as which? g_printer.h and gadgetfs.h are separate files.

Whatever the uapi .h file is, should have the list of ioctls, not
scattered around the core kernel files.

> 
> > 
> >> +
> >> +#endif /* __UAPI_LINUX_USB_G_HID_H */
> >> diff --git a/include/uapi/linux/usb/gadgetfs.h b/include/uapi/linux/usb/gadgetfs.h
> >> index 835473910a49..9754822b2a40 100644
> >> --- a/include/uapi/linux/usb/gadgetfs.h
> >> +++ b/include/uapi/linux/usb/gadgetfs.h
> >> @@ -62,7 +62,7 @@ struct usb_gadgetfs_event {
> >>  };
> >>  
> >>  
> >> -/* The 'g' code is also used by printer gadget ioctl requests.
> >> +/* The 'g' code is also used by printer and hid gadget ioctl requests.
> > 
> > Yeah, put the definition here.
> 
> Should I move the g_printer.h definitions here at the same time? Maybe stub out g_printer.h and make it include gadgetfs.h?

Move them in the first patch in the series would be great.  And no need
for stubbed out .h file, that's not needed for internal .h files.

thanks,

greg k-h

Re: [PATCH 1/2] USB: gadget: f_hid: Add Get-Feature report

[Vicki Pfau]

On 7/29/22 00:47, Greg Kroah-Hartman wrote:
> On Thu, Jul 28, 2022 at 11:07:20AM -0700, Vicki Pfau wrote:
>>
>>
>> On 7/28/22 01:08, Greg Kroah-Hartman wrote:
>>> On Mon, Jul 25, 2022 at 05:58:25PM -0700, Vicki Pfau wrote:
>>>> --- /dev/null
>>>> +++ b/include/uapi/linux/usb/g_hid.h
>>>> @@ -0,0 +1,38 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
>>>> +/*
>>>> + * g_hid.h -- Header file for USB HID gadget driver
>>>> + *
>>>> + * Copyright (C) 2022 Valve Software
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License as published by
>>>> + * the Free Software Foundation; either version 2 of the License, or
>>>> + * (at your option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License
>>>> + * along with this program; if not, write to the Free Software
>>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>>>
>>> This whole license "boilerplate" is not needed, and should be removed
>>> (especially things like addresses, that's crazy).
>>>
>>> Only thing that is needed is the SPDX line.
>>
>> I was just copying the header from g_printer.h and changing as needed. It hasn't been touched since 2017, so if that's no longer the convention you might want to update it too.
> 
> Good point, I will look into that.
> 
>>>> + */
>>>> +
>>>> +#ifndef __UAPI_LINUX_USB_G_HID_H
>>>> +#define __UAPI_LINUX_USB_G_HID_H
>>>> +
>>>> +#include <linux/types.h>
>>>> +
>>>> +struct usb_hidg_report {
>>>> +	__u16 length;
>>>> +	__u8 data[512];
>>>
>>> Why 512?
>>
>> I was reading the specs and one of them said the maximum report length for USB 3 (I believe) was 512 bytes (in contrast with USB 2's 64). I can try to find where it said this, or add a define for max report length.
> 
> Either is fine, magic values like this should always be documented
> somehow.
> 

I can't actually find where this is specified anymore, so I'm going to bump it down to 64 (as defined by bMaxPacketSize0 in the HID 1.11 spec) and define it as a constant.

>>>> +};
>>>> +
>>>> +/* The 'g' code is also used by gadgetfs and hid gadget ioctl requests.
>>>> + * Don't add any colliding codes to either driver, and keep
>>>> + * them in unique ranges (size 0x20 for now).
>>>> + */
>>>> +#define GADGET_HID_WRITE_GET_REPORT	_IOW('g', 0x42, struct usb_hidg_report)
>>>
>>> This should be in the same .h file so that we don't get confused and
>>> accidentally use the same ioctl.
>>
>> The same .h file as which? g_printer.h and gadgetfs.h are separate files.
> 
> Whatever the uapi .h file is, should have the list of ioctls, not
> scattered around the core kernel files.

There are three uapi .h files in quesiton: gadgetfs.h, g_printer.h and (now the new) g_hid.h. These all have distinct purposes and there's no natural "here are the ioctls for USB gadgets" file. I could make a gadget.h or g_ioctl.h file and move all the ioctls there, but that would still involve a new file (and possibly including it in the existing gadgetfs.h and g_printer.h files to maintain API compat).

> 
>>
>>>
>>>> +
>>>> +#endif /* __UAPI_LINUX_USB_G_HID_H */
>>>> diff --git a/include/uapi/linux/usb/gadgetfs.h b/include/uapi/linux/usb/gadgetfs.h
>>>> index 835473910a49..9754822b2a40 100644
>>>> --- a/include/uapi/linux/usb/gadgetfs.h
>>>> +++ b/include/uapi/linux/usb/gadgetfs.h
>>>> @@ -62,7 +62,7 @@ struct usb_gadgetfs_event {
>>>>  };
>>>>  
>>>>  
>>>> -/* The 'g' code is also used by printer gadget ioctl requests.
>>>> +/* The 'g' code is also used by printer and hid gadget ioctl requests.
>>>
>>> Yeah, put the definition here.
>>
>> Should I move the g_printer.h definitions here at the same time? Maybe stub out g_printer.h and make it include gadgetfs.h?
> 
> Move them in the first patch in the series would be great.  And no need
> for stubbed out .h file, that's not needed for internal .h files.

Is this internal? It's in the uapi so it's public to the kernel itself. Removing, e.g., g_printer.h if it was emptied out (though it looks like it'd still have two defines in it if the ioctls were moved) would break API compat and existing software would need to fix their code. Obviously it doesn't break ABI so maybe that's acceptable.

> 
> thanks,
> 
> greg k-h

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Maxim Devaev]

В Thu, 28 Jul 2022 11:11:31 -0700
Vicki Pfau <vi@endrift.com> пишет:

> On 7/28/22 01:59, Maxim Devaev wrote:
> > В Tue, 26 Jul 2022 21:26:05 -0700
> > Vicki Pfau <vi@endrift.com> пишет:
> >   
> >> On 7/26/22 02:51, Maxim Devaev wrote:  
> >>> В Mon, 25 Jul 2022 17:58:26 -0700
> >>> Vicki Pfau <vi@endrift.com> пишет:
> >>>     
> >>>> While the HID gadget implementation has been sufficient for devices that only
> >>>> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
> >>>> Get-Feature report CONTROL transfers that go over endpoint 0. These were
> >>>> previously impossible with the existing implementation, and would either send
> >>>> an empty reply, or stall out.
> >>>>
> >>>> As the feature is a standard part of USB HID, it stands to reason that devices
> >>>> would use it, and that the HID gadget should support it. This patch adds
> >>>> support for host-to-device Set-Feature reports through a new ioctl
> >>>> interface to the hidg class dev nodes.
> >>>>
> >>>> Signed-off-by: Vicki Pfau <vi@endrift.com>    
> >>>
> >>> Won't it break the logic of the existing software that works with /dev/hidgX?
> >>> Will it work if I want my gadget to work the old way?    
> >>
> >> For existing software to use SET_FEATURE at all it has to use an alternative mode, which seems to have only been added somewhat recently. That mode also appears to preclude use of INTERRUPT transfers at all, unless there's some way to set up two hidg nodes that map to the same interface, with one for INTERRUPT and one for SET_FEATURE. If this breaks that, I suppose that's a regression, but this is meant to augment the original, long-standing mode so you can mix INTERRUPT and SET/GET_FEATURE transfers, as there is no way to do that yet. Honestly, the alternate mode seems more like a workaround, as far as I can tell, and not an ideal implementation. I'm not sure when it was added, but as I was originally authoring this against 5.13 and didn't see it until I went to rebase onto master, it can't have been that long ago. So if it breaks any software (which I don't believe it does), it would only affect very new software.
> >>
> >> As I alluded to, I'd thought about perhaps adding a second node per interface so one would act as INTERRUPT transfers and the other as SET/GET_FEATURE transfers, but I already had this code half written and wanted to get feedback first, especially since what I have now works (although it's not well-tested after rebasing).  
> > 
> > I'm a little confused here about what you call an alternative mode.
> > Are we talking about use_out_ep=1 (default behavior with INTERRUPT)
> > or use_out_ep=0 (SETUP/SET_REPORT)? The last mode was added by me
> > to ensure strict compatibility with Apple UEFI and strange BIOS,
> > and this mode is actually actively used. It is important to me
> > that it is not broken, but unfortunately I cannot test your patch
> > on my kernel, as I temporarily do not have access to testing equipment.  
> 
> use_out_ep=0 is the alternate mode I was talking about. It didn't exist in 5.13, so as I said I wasn't aware of it until I rebased. As the device I'm using is still stuck on that old kernel (for now) and I don't know if I have any USB gadget capable devices on new kernels, I was unable to test it, and would very much like to make sure it doesn't regress before a patch is merged. I wasn't intending to break it, but I figured I'd get feedback I'd need to change before this was merged so if you could test it to ensure it doesn't regress any behavior that would be much appreciated and help me out. I will probably wait until then before submitting a v2.

I will get access to the USB analyzer and test environment in about a month,
if that suits you. You can write directly to my email after a month,
I will help you with testing.

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Vicki Pfau]

On 7/31/22 01:14, Maxim Devaev wrote:
> В Thu, 28 Jul 2022 11:11:31 -0700
> Vicki Pfau <vi@endrift.com> пишет:
> 
>> On 7/28/22 01:59, Maxim Devaev wrote:
>>> В Tue, 26 Jul 2022 21:26:05 -0700
>>> Vicki Pfau <vi@endrift.com> пишет:
>>>   
>>>> On 7/26/22 02:51, Maxim Devaev wrote:  
>>>>> В Mon, 25 Jul 2022 17:58:26 -0700
>>>>> Vicki Pfau <vi@endrift.com> пишет:
>>>>>     
>>>>>> While the HID gadget implementation has been sufficient for devices that only
>>>>>> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
>>>>>> Get-Feature report CONTROL transfers that go over endpoint 0. These were
>>>>>> previously impossible with the existing implementation, and would either send
>>>>>> an empty reply, or stall out.
>>>>>>
>>>>>> As the feature is a standard part of USB HID, it stands to reason that devices
>>>>>> would use it, and that the HID gadget should support it. This patch adds
>>>>>> support for host-to-device Set-Feature reports through a new ioctl
>>>>>> interface to the hidg class dev nodes.
>>>>>>
>>>>>> Signed-off-by: Vicki Pfau <vi@endrift.com>    
>>>>>
>>>>> Won't it break the logic of the existing software that works with /dev/hidgX?
>>>>> Will it work if I want my gadget to work the old way?    
>>>>
>>>> For existing software to use SET_FEATURE at all it has to use an alternative mode, which seems to have only been added somewhat recently. That mode also appears to preclude use of INTERRUPT transfers at all, unless there's some way to set up two hidg nodes that map to the same interface, with one for INTERRUPT and one for SET_FEATURE. If this breaks that, I suppose that's a regression, but this is meant to augment the original, long-standing mode so you can mix INTERRUPT and SET/GET_FEATURE transfers, as there is no way to do that yet. Honestly, the alternate mode seems more like a workaround, as far as I can tell, and not an ideal implementation. I'm not sure when it was added, but as I was originally authoring this against 5.13 and didn't see it until I went to rebase onto master, it can't have been that long ago. So if it breaks any software (which I don't believe it does), it would only affect very new software.
>>>>
>>>> As I alluded to, I'd thought about perhaps adding a second node per interface so one would act as INTERRUPT transfers and the other as SET/GET_FEATURE transfers, but I already had this code half written and wanted to get feedback first, especially since what I have now works (although it's not well-tested after rebasing).  
>>>
>>> I'm a little confused here about what you call an alternative mode.
>>> Are we talking about use_out_ep=1 (default behavior with INTERRUPT)
>>> or use_out_ep=0 (SETUP/SET_REPORT)? The last mode was added by me
>>> to ensure strict compatibility with Apple UEFI and strange BIOS,
>>> and this mode is actually actively used. It is important to me
>>> that it is not broken, but unfortunately I cannot test your patch
>>> on my kernel, as I temporarily do not have access to testing equipment.  
>>
>> use_out_ep=0 is the alternate mode I was talking about. It didn't exist in 5.13, so as I said I wasn't aware of it until I rebased. As the device I'm using is still stuck on that old kernel (for now) and I don't know if I have any USB gadget capable devices on new kernels, I was unable to test it, and would very much like to make sure it doesn't regress before a patch is merged. I wasn't intending to break it, but I figured I'd get feedback I'd need to change before this was merged so if you could test it to ensure it doesn't regress any behavior that would be much appreciated and help me out. I will probably wait until then before submitting a v2.
> 
> I will get access to the USB analyzer and test environment in about a month,
> if that suits you. You can write directly to my email after a month,
> I will help you with testing.

I'm not in a huge rush, though I might try to find some hardware I can test a modern kernel on to help test. What are you running the kernel on?

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Maxim Devaev]

В Wed, 3 Aug 2022 16:32:21 -0700
Vicki Pfau <vi@endrift.com> пишет:

> On 7/31/22 01:14, Maxim Devaev wrote:
> > В Thu, 28 Jul 2022 11:11:31 -0700
> > Vicki Pfau <vi@endrift.com> пишет:
> >   
> >> On 7/28/22 01:59, Maxim Devaev wrote:  
> >>> В Tue, 26 Jul 2022 21:26:05 -0700
> >>> Vicki Pfau <vi@endrift.com> пишет:
> >>>     
> >>>> On 7/26/22 02:51, Maxim Devaev wrote:    
> >>>>> В Mon, 25 Jul 2022 17:58:26 -0700
> >>>>> Vicki Pfau <vi@endrift.com> пишет:
> >>>>>       
> >>>>>> While the HID gadget implementation has been sufficient for devices that only
> >>>>>> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
> >>>>>> Get-Feature report CONTROL transfers that go over endpoint 0. These were
> >>>>>> previously impossible with the existing implementation, and would either send
> >>>>>> an empty reply, or stall out.
> >>>>>>
> >>>>>> As the feature is a standard part of USB HID, it stands to reason that devices
> >>>>>> would use it, and that the HID gadget should support it. This patch adds
> >>>>>> support for host-to-device Set-Feature reports through a new ioctl
> >>>>>> interface to the hidg class dev nodes.
> >>>>>>
> >>>>>> Signed-off-by: Vicki Pfau <vi@endrift.com>      
> >>>>>
> >>>>> Won't it break the logic of the existing software that works with /dev/hidgX?
> >>>>> Will it work if I want my gadget to work the old way?      
> >>>>
> >>>> For existing software to use SET_FEATURE at all it has to use an alternative mode, which seems to have only been added somewhat recently. That mode also appears to preclude use of INTERRUPT transfers at all, unless there's some way to set up two hidg nodes that map to the same interface, with one for INTERRUPT and one for SET_FEATURE. If this breaks that, I suppose that's a regression, but this is meant to augment the original, long-standing mode so you can mix INTERRUPT and SET/GET_FEATURE transfers, as there is no way to do that yet. Honestly, the alternate mode seems more like a workaround, as far as I can tell, and not an ideal implementation. I'm not sure when it was added, but as I was originally authoring this against 5.13 and didn't see it until I went to rebase onto master, it can't have been that long ago. So if it breaks any software (which I don't believe it does), it would only affect very new software.
> >>>>
> >>>> As I alluded to, I'd thought about perhaps adding a second node per interface so one would act as INTERRUPT transfers and the other as SET/GET_FEATURE transfers, but I already had this code half written and wanted to get feedback first, especially since what I have now works (although it's not well-tested after rebasing).    
> >>>
> >>> I'm a little confused here about what you call an alternative mode.
> >>> Are we talking about use_out_ep=1 (default behavior with INTERRUPT)
> >>> or use_out_ep=0 (SETUP/SET_REPORT)? The last mode was added by me
> >>> to ensure strict compatibility with Apple UEFI and strange BIOS,
> >>> and this mode is actually actively used. It is important to me
> >>> that it is not broken, but unfortunately I cannot test your patch
> >>> on my kernel, as I temporarily do not have access to testing equipment.    
> >>
> >> use_out_ep=0 is the alternate mode I was talking about. It didn't exist in 5.13, so as I said I wasn't aware of it until I rebased. As the device I'm using is still stuck on that old kernel (for now) and I don't know if I have any USB gadget capable devices on new kernels, I was unable to test it, and would very much like to make sure it doesn't regress before a patch is merged. I wasn't intending to break it, but I figured I'd get feedback I'd need to change before this was merged so if you could test it to ensure it doesn't regress any behavior that would be much appreciated and help me out. I will probably wait until then before submitting a v2.  
> > 
> > I will get access to the USB analyzer and test environment in about a month,
> > if that suits you. You can write directly to my email after a month,
> > I will help you with testing.  
> 
> I'm not in a huge rush, though I might try to find some hardware I can test a modern kernel on to help test. What are you running the kernel on?

I'm using the Raspberry Pi 4 with Mac Mini as host (checking the OS, UEFI and Recovery mode).

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Vicki Pfau]

Hi,

On 7/31/22 01:14, Maxim Devaev wrote:
> В Thu, 28 Jul 2022 11:11:31 -0700
> Vicki Pfau <vi@endrift.com> пишет:
> 
>> On 7/28/22 01:59, Maxim Devaev wrote:
>>> В Tue, 26 Jul 2022 21:26:05 -0700
>>> Vicki Pfau <vi@endrift.com> пишет:
>>>   
>>>> On 7/26/22 02:51, Maxim Devaev wrote:  
>>>>> В Mon, 25 Jul 2022 17:58:26 -0700
>>>>> Vicki Pfau <vi@endrift.com> пишет:
>>>>>     
>>>>>> While the HID gadget implementation has been sufficient for devices that only
>>>>>> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
>>>>>> Get-Feature report CONTROL transfers that go over endpoint 0. These were
>>>>>> previously impossible with the existing implementation, and would either send
>>>>>> an empty reply, or stall out.
>>>>>>
>>>>>> As the feature is a standard part of USB HID, it stands to reason that devices
>>>>>> would use it, and that the HID gadget should support it. This patch adds
>>>>>> support for host-to-device Set-Feature reports through a new ioctl
>>>>>> interface to the hidg class dev nodes.
>>>>>>
>>>>>> Signed-off-by: Vicki Pfau <vi@endrift.com>    
>>>>>
>>>>> Won't it break the logic of the existing software that works with /dev/hidgX?
>>>>> Will it work if I want my gadget to work the old way?    
>>>>
>>>> For existing software to use SET_FEATURE at all it has to use an alternative mode, which seems to have only been added somewhat recently. That mode also appears to preclude use of INTERRUPT transfers at all, unless there's some way to set up two hidg nodes that map to the same interface, with one for INTERRUPT and one for SET_FEATURE. If this breaks that, I suppose that's a regression, but this is meant to augment the original, long-standing mode so you can mix INTERRUPT and SET/GET_FEATURE transfers, as there is no way to do that yet. Honestly, the alternate mode seems more like a workaround, as far as I can tell, and not an ideal implementation. I'm not sure when it was added, but as I was originally authoring this against 5.13 and didn't see it until I went to rebase onto master, it can't have been that long ago. So if it breaks any software (which I don't believe it does), it would only affect very new software.
>>>>
>>>> As I alluded to, I'd thought about perhaps adding a second node per interface so one would act as INTERRUPT transfers and the other as SET/GET_FEATURE transfers, but I already had this code half written and wanted to get feedback first, especially since what I have now works (although it's not well-tested after rebasing).  
>>>
>>> I'm a little confused here about what you call an alternative mode.
>>> Are we talking about use_out_ep=1 (default behavior with INTERRUPT)
>>> or use_out_ep=0 (SETUP/SET_REPORT)? The last mode was added by me
>>> to ensure strict compatibility with Apple UEFI and strange BIOS,
>>> and this mode is actually actively used. It is important to me
>>> that it is not broken, but unfortunately I cannot test your patch
>>> on my kernel, as I temporarily do not have access to testing equipment.  
>>
>> use_out_ep=0 is the alternate mode I was talking about. It didn't exist in 5.13, so as I said I wasn't aware of it until I rebased. As the device I'm using is still stuck on that old kernel (for now) and I don't know if I have any USB gadget capable devices on new kernels, I was unable to test it, and would very much like to make sure it doesn't regress before a patch is merged. I wasn't intending to break it, but I figured I'd get feedback I'd need to change before this was merged so if you could test it to ensure it doesn't regress any behavior that would be much appreciated and help me out. I will probably wait until then before submitting a v2.
> 
> I will get access to the USB analyzer and test environment in about a month,
> if that suits you. You can write directly to my email after a month,
> I will help you with testing.

I wanted to check the status of this. We're in the middle of rebasing onto a newer kernel, so I might be able to test it myself soon. What software are you using?

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Maxim Devaev]

В Thu, 8 Sep 2022 16:57:36 -0700
Vicki Pfau <vi@endrift.com> пишет:

> Hi,
> 
> On 7/31/22 01:14, Maxim Devaev wrote:
> > В Thu, 28 Jul 2022 11:11:31 -0700
> > Vicki Pfau <vi@endrift.com> пишет:
> >   
> >> On 7/28/22 01:59, Maxim Devaev wrote:  
> >>> В Tue, 26 Jul 2022 21:26:05 -0700
> >>> Vicki Pfau <vi@endrift.com> пишет:
> >>>     
> >>>> On 7/26/22 02:51, Maxim Devaev wrote:    
> >>>>> В Mon, 25 Jul 2022 17:58:26 -0700
> >>>>> Vicki Pfau <vi@endrift.com> пишет:
> >>>>>       
> >>>>>> While the HID gadget implementation has been sufficient for devices that only
> >>>>>> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
> >>>>>> Get-Feature report CONTROL transfers that go over endpoint 0. These were
> >>>>>> previously impossible with the existing implementation, and would either send
> >>>>>> an empty reply, or stall out.
> >>>>>>
> >>>>>> As the feature is a standard part of USB HID, it stands to reason that devices
> >>>>>> would use it, and that the HID gadget should support it. This patch adds
> >>>>>> support for host-to-device Set-Feature reports through a new ioctl
> >>>>>> interface to the hidg class dev nodes.
> >>>>>>
> >>>>>> Signed-off-by: Vicki Pfau <vi@endrift.com>      
> >>>>>
> >>>>> Won't it break the logic of the existing software that works with /dev/hidgX?
> >>>>> Will it work if I want my gadget to work the old way?      
> >>>>
> >>>> For existing software to use SET_FEATURE at all it has to use an alternative mode, which seems to have only been added somewhat recently. That mode also appears to preclude use of INTERRUPT transfers at all, unless there's some way to set up two hidg nodes that map to the same interface, with one for INTERRUPT and one for SET_FEATURE. If this breaks that, I suppose that's a regression, but this is meant to augment the original, long-standing mode so you can mix INTERRUPT and SET/GET_FEATURE transfers, as there is no way to do that yet. Honestly, the alternate mode seems more like a workaround, as far as I can tell, and not an ideal implementation. I'm not sure when it was added, but as I was originally authoring this against 5.13 and didn't see it until I went to rebase onto master, it can't have been that long ago. So if it breaks any software (which I don't believe it does), it would only affect very new software.
> >>>>
> >>>> As I alluded to, I'd thought about perhaps adding a second node per interface so one would act as INTERRUPT transfers and the other as SET/GET_FEATURE transfers, but I already had this code half written and wanted to get feedback first, especially since what I have now works (although it's not well-tested after rebasing).    
> >>>
> >>> I'm a little confused here about what you call an alternative mode.
> >>> Are we talking about use_out_ep=1 (default behavior with INTERRUPT)
> >>> or use_out_ep=0 (SETUP/SET_REPORT)? The last mode was added by me
> >>> to ensure strict compatibility with Apple UEFI and strange BIOS,
> >>> and this mode is actually actively used. It is important to me
> >>> that it is not broken, but unfortunately I cannot test your patch
> >>> on my kernel, as I temporarily do not have access to testing equipment.    
> >>
> >> use_out_ep=0 is the alternate mode I was talking about. It didn't exist in 5.13, so as I said I wasn't aware of it until I rebased. As the device I'm using is still stuck on that old kernel (for now) and I don't know if I have any USB gadget capable devices on new kernels, I was unable to test it, and would very much like to make sure it doesn't regress before a patch is merged. I wasn't intending to break it, but I figured I'd get feedback I'd need to change before this was merged so if you could test it to ensure it doesn't regress any behavior that would be much appreciated and help me out. I will probably wait until then before submitting a v2.  
> > 
> > I will get access to the USB analyzer and test environment in about a month,
> > if that suits you. You can write directly to my email after a month,
> > I will help you with testing.  
> 
> I wanted to check the status of this. We're in the middle of rebasing onto a newer kernel, so I might be able to test it myself soon. What software are you using?

I'm using PiKVM on Raspberry Pi 4 (https://github.com/pikvm/pikvm) but for generic testing you need to make
the usual ways of using gadget work in both modes: https://www.isticktoit.net/?p=1383
Unfortunately, I'm still away and I can't use my equipment :/

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Vicki Pfau]

On 9/13/22 03:06, Maxim Devaev wrote:
> В Thu, 8 Sep 2022 16:57:36 -0700
> Vicki Pfau <vi@endrift.com> пишет:
> 
>> Hi,
>>
>> On 7/31/22 01:14, Maxim Devaev wrote:
>>> В Thu, 28 Jul 2022 11:11:31 -0700
>>> Vicki Pfau <vi@endrift.com> пишет:
>>>   
>>>> On 7/28/22 01:59, Maxim Devaev wrote:  
>>>>> В Tue, 26 Jul 2022 21:26:05 -0700
>>>>> Vicki Pfau <vi@endrift.com> пишет:
>>>>>     
>>>>>> On 7/26/22 02:51, Maxim Devaev wrote:    
>>>>>>> В Mon, 25 Jul 2022 17:58:26 -0700
>>>>>>> Vicki Pfau <vi@endrift.com> пишет:
>>>>>>>       
>>>>>>>> While the HID gadget implementation has been sufficient for devices that only
>>>>>>>> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
>>>>>>>> Get-Feature report CONTROL transfers that go over endpoint 0. These were
>>>>>>>> previously impossible with the existing implementation, and would either send
>>>>>>>> an empty reply, or stall out.
>>>>>>>>
>>>>>>>> As the feature is a standard part of USB HID, it stands to reason that devices
>>>>>>>> would use it, and that the HID gadget should support it. This patch adds
>>>>>>>> support for host-to-device Set-Feature reports through a new ioctl
>>>>>>>> interface to the hidg class dev nodes.
>>>>>>>>
>>>>>>>> Signed-off-by: Vicki Pfau <vi@endrift.com>      
>>>>>>>
>>>>>>> Won't it break the logic of the existing software that works with /dev/hidgX?
>>>>>>> Will it work if I want my gadget to work the old way?      
>>>>>>
>>>>>> For existing software to use SET_FEATURE at all it has to use an alternative mode, which seems to have only been added somewhat recently. That mode also appears to preclude use of INTERRUPT transfers at all, unless there's some way to set up two hidg nodes that map to the same interface, with one for INTERRUPT and one for SET_FEATURE. If this breaks that, I suppose that's a regression, but this is meant to augment the original, long-standing mode so you can mix INTERRUPT and SET/GET_FEATURE transfers, as there is no way to do that yet. Honestly, the alternate mode seems more like a workaround, as far as I can tell, and not an ideal implementation. I'm not sure when it was added, but as I was originally authoring this against 5.13 and didn't see it until I went to rebase onto master, it can't have been that long ago. So if it breaks any software (which I don't believe it does), it would only affect very new software.
>>>>>>
>>>>>> As I alluded to, I'd thought about perhaps adding a second node per interface so one would act as INTERRUPT transfers and the other as SET/GET_FEATURE transfers, but I already had this code half written and wanted to get feedback first, especially since what I have now works (although it's not well-tested after rebasing).    
>>>>>
>>>>> I'm a little confused here about what you call an alternative mode.
>>>>> Are we talking about use_out_ep=1 (default behavior with INTERRUPT)
>>>>> or use_out_ep=0 (SETUP/SET_REPORT)? The last mode was added by me
>>>>> to ensure strict compatibility with Apple UEFI and strange BIOS,
>>>>> and this mode is actually actively used. It is important to me
>>>>> that it is not broken, but unfortunately I cannot test your patch
>>>>> on my kernel, as I temporarily do not have access to testing equipment.    
>>>>
>>>> use_out_ep=0 is the alternate mode I was talking about. It didn't exist in 5.13, so as I said I wasn't aware of it until I rebased. As the device I'm using is still stuck on that old kernel (for now) and I don't know if I have any USB gadget capable devices on new kernels, I was unable to test it, and would very much like to make sure it doesn't regress before a patch is merged. I wasn't intending to break it, but I figured I'd get feedback I'd need to change before this was merged so if you could test it to ensure it doesn't regress any behavior that would be much appreciated and help me out. I will probably wait until then before submitting a v2.  
>>>
>>> I will get access to the USB analyzer and test environment in about a month,
>>> if that suits you. You can write directly to my email after a month,
>>> I will help you with testing.  
>>
>> I wanted to check the status of this. We're in the middle of rebasing onto a newer kernel, so I might be able to test it myself soon. What software are you using?
> 
> I'm using PiKVM on Raspberry Pi 4 (https://github.com/pikvm/pikvm) but for generic testing you need to make
> the usual ways of using gadget work in both modes: https://www.isticktoit.net/?p=1383
> Unfortunately, I'm still away and I can't use my equipment :/

I finally got a chance to test on 6.0, and I can confirm that it doesn't break use_out_ep=1 style read/write. You mention PiKVM, which I can test (I have a Pi 4 and bought the PiKVM v3 kit when it was on Kickstarter), but given that the lowest version of Linux it supports predates use_out_ep=0 style, I wasn't sure if that was sufficient to make sure I didn't break it. I'll prepare a v2 as soon as I can confirm how to test use_out_ep=0 style.

Re: [PATCH 2/2] USB: gadget: f_hid: Add Set-Feature report

[Maxim Devaev]

В Fri, 21 Oct 2022 17:28:25 -0700
Vicki Pfau <vi@endrift.com> пишет:

> On 9/13/22 03:06, Maxim Devaev wrote:
> > В Thu, 8 Sep 2022 16:57:36 -0700
> > Vicki Pfau <vi@endrift.com> пишет:
> >   
> >> Hi,
> >>
> >> On 7/31/22 01:14, Maxim Devaev wrote:  
> >>> В Thu, 28 Jul 2022 11:11:31 -0700
> >>> Vicki Pfau <vi@endrift.com> пишет:
> >>>     
> >>>> On 7/28/22 01:59, Maxim Devaev wrote:    
> >>>>> В Tue, 26 Jul 2022 21:26:05 -0700
> >>>>> Vicki Pfau <vi@endrift.com> пишет:
> >>>>>       
> >>>>>> On 7/26/22 02:51, Maxim Devaev wrote:      
> >>>>>>> В Mon, 25 Jul 2022 17:58:26 -0700
> >>>>>>> Vicki Pfau <vi@endrift.com> пишет:
> >>>>>>>         
> >>>>>>>> While the HID gadget implementation has been sufficient for devices that only
> >>>>>>>> use INTERRUPT transfers, the USB HID standard includes provisions for Set- and
> >>>>>>>> Get-Feature report CONTROL transfers that go over endpoint 0. These were
> >>>>>>>> previously impossible with the existing implementation, and would either send
> >>>>>>>> an empty reply, or stall out.
> >>>>>>>>
> >>>>>>>> As the feature is a standard part of USB HID, it stands to reason that devices
> >>>>>>>> would use it, and that the HID gadget should support it. This patch adds
> >>>>>>>> support for host-to-device Set-Feature reports through a new ioctl
> >>>>>>>> interface to the hidg class dev nodes.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Vicki Pfau <vi@endrift.com>        
> >>>>>>>
> >>>>>>> Won't it break the logic of the existing software that works with /dev/hidgX?
> >>>>>>> Will it work if I want my gadget to work the old way?        
> >>>>>>
> >>>>>> For existing software to use SET_FEATURE at all it has to use an alternative mode, which seems to have only been added somewhat recently. That mode also appears to preclude use of INTERRUPT transfers at all, unless there's some way to set up two hidg nodes that map to the same interface, with one for INTERRUPT and one for SET_FEATURE. If this breaks that, I suppose that's a regression, but this is meant to augment the original, long-standing mode so you can mix INTERRUPT and SET/GET_FEATURE transfers, as there is no way to do that yet. Honestly, the alternate mode seems more like a workaround, as far as I can tell, and not an ideal implementation. I'm not sure when it was added, but as I was originally authoring this against 5.13 and didn't see it until I went to rebase onto master, it can't have been that long ago. So if it breaks any software (which I don't believe it does), it would only affect very new software.
> >>>>>>
> >>>>>> As I alluded to, I'd thought about perhaps adding a second node per interface so one would act as INTERRUPT transfers and the other as SET/GET_FEATURE transfers, but I already had this code half written and wanted to get feedback first, especially since what I have now works (although it's not well-tested after rebasing).      
> >>>>>
> >>>>> I'm a little confused here about what you call an alternative mode.
> >>>>> Are we talking about use_out_ep=1 (default behavior with INTERRUPT)
> >>>>> or use_out_ep=0 (SETUP/SET_REPORT)? The last mode was added by me
> >>>>> to ensure strict compatibility with Apple UEFI and strange BIOS,
> >>>>> and this mode is actually actively used. It is important to me
> >>>>> that it is not broken, but unfortunately I cannot test your patch
> >>>>> on my kernel, as I temporarily do not have access to testing equipment.      
> >>>>
> >>>> use_out_ep=0 is the alternate mode I was talking about. It didn't exist in 5.13, so as I said I wasn't aware of it until I rebased. As the device I'm using is still stuck on that old kernel (for now) and I don't know if I have any USB gadget capable devices on new kernels, I was unable to test it, and would very much like to make sure it doesn't regress before a patch is merged. I wasn't intending to break it, but I figured I'd get feedback I'd need to change before this was merged so if you could test it to ensure it doesn't regress any behavior that would be much appreciated and help me out. I will probably wait until then before submitting a v2.    
> >>>
> >>> I will get access to the USB analyzer and test environment in about a month,
> >>> if that suits you. You can write directly to my email after a month,
> >>> I will help you with testing.    
> >>
> >> I wanted to check the status of this. We're in the middle of rebasing onto a newer kernel, so I might be able to test it myself soon. What software are you using?  
> > 
> > I'm using PiKVM on Raspberry Pi 4 (https://github.com/pikvm/pikvm) but for generic testing you need to make
> > the usual ways of using gadget work in both modes: https://www.isticktoit.net/?p=1383
> > Unfortunately, I'm still away and I can't use my equipment :/  
> 
> I finally got a chance to test on 6.0, and I can confirm that it doesn't break use_out_ep=1 style read/write. You mention PiKVM, which I can test (I have a Pi 4 and bought the PiKVM v3 kit when it was on Kickstarter), but given that the lowest version of Linux it supports predates use_out_ep=0 style, I wasn't sure if that was sufficient to make sure I didn't break it. I'll prepare a v2 as soon as I can confirm how to test use_out_ep=0 style.

Sounds great! BTW PiKVM uses 5.15 LTS kernel from Raspberry Pi repo with several patches. use_out_ep works there.
https://github.com/raspberrypi/linux