Re: [PATCH v2] usb: gadget: f_hid: optional SETUP/SET_REPORT mode

From: Maciej Żenczykowski
Date: Thu Aug 19 2021 - 17:14:05 EST

Next message: kernel test robot: "[dhowells-fs:netfs-folio-regions 18/28] fs/netfs/write_prep.c:78:31: sparse: sparse: incompatible types in comparison expression (different type sizes):"
Previous message: kernel test robot: "Re: [PATCH v2] mei: improve Denverton HSM & IFSI support"
Next in thread: Maxim Devaev: "Re: [PATCH v2] usb: gadget: f_hid: optional SETUP/SET_REPORT mode"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

I'm a noob when it comes to usb, so some of my questions may well be stupid...

On Sat, Aug 14, 2021 at 5:13 AM Maxim Devaev <mdevaev@xxxxxxxxx> wrote:
>
> f_hid provides the OUT Endpoint as only way for receiving reports
> from the host. SETUP/SET_REPORT method is not supported, and this causes
> a number of compatibility problems with various host drivers, especially
> in the case of keyboard emulation using f_hid.
>
> - Some hosts do not support the OUT Endpoint and ignore it,
> so it becomes impossible for the gadget to receive a report
> from the host. In the case of a keyboard, the gadget loses the ability
> to get the status of the LEDs.

perhaps better to rephrase as 'the host ceases to change the status of
the gadget/keyboard LEDs',
unless this is actually driven by the keyboard as opposed to the other
way round (which is what I'd expect from AT, PS/2 keyboards).

> - Some BIOSes/UEFIs can't work with HID with the OUT Endpoint at all.

HID devices with an OUT Endpoint

> This may be due to their bugs or incomplete implementation
> of the HID standard.
> For example, absolutely all Apple UEFIs can't handle the OUT Endpoint
> if it goes after IN Endpoint in the descriptor and require the reverse
> order (OUT, IN) which is a violation of the standard.
> Other hosts either do not initialize gadgets with a descriptor
> containing the OUT Endpoint completely (like some HP and DELL BIOSes
> and embedded firmwares like on KVM switches), or initialize them,
> but will not poll.

Not clear what 'not poll' means here. Why would they (the host) need
to poll an OUT endpoint?

Additionally it seems like any keyboard gadget would want to default
to the older more compatible mode?
Or are there compatibility problems with it as well?

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

allows

Is this a kcmdline option? module param? sysfs option?
Can one dynamically change this as one reconfigures the gadget for a new mode?

>
> Previously, there was such a feature in f_hid, but it was replaced
> by the OUT Endpoint [1] in the commit 99c515005857 ("usb: gadget: hidg:
> register OUT INT endpoint for SET_REPORT"). So this patch actually returns
> the removed functionality making it optional. For backward compatibility

while making it optional

> reasons, the OUT Endpoint mode remains the default behaviour.
>
> - The OUT Endpoint mode provides the report queue and reduces USB overhead
> (eliminating SETUP routine) on transmitting a report from the host.
>
> - If the SETUP/SET_REPORT mode is used, there is no report queue,
> so the userspace will only read the last report. For classic HID

without 'the'

> devices like keyboard this is not a problem, since it is intended

keyboards

> to transmit the status of the LEDs and only the last report
> is important. This mode provides better compatibility with strange
> and buggy host drivers.
>
> Both modes passed USBCV tests. Checking with the USB protocol analyzer
> also confirmed 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@xxxxxxxxx>
> Link: https://www.spinics.net/lists/linux-usb/msg65494.html [1]
> ---
> 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;

this could use a comment along the lines of /* as opposed to SETUP/SET_REPORT */

>
> /* 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 */

if you look down below, this isn't actually dynamic, should we just
have hidg_interface_desc_{intout,ssreport} structs?

> .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 */
> +

comment probably belongs above the function instead of inside it

> 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 */
> +

ditto

> 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 */
> +

ditto

> + 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));

may be better to just use an if (hidg->use_out_ep) status =
usb_assign_descriptors(...) else status = usb_assign_descriptors(...)

(or just status = hidg->use_out_ep ? usb_assign_descriptors(...) :
usb_assign_descriptors(...)

it's not really any longer (when you count up the extra lines adding
and undefing the macro) and eliminates a macro making things easier to
grep for...

> 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;

maybe it would be better to use consistent naming...

hidg->no_out_endpoint = opts->no_out_endpoint

or call the option 'use_ssreport' instead of 'no_out_endpoint'
(negatives are harder to think about)

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

Anyway, nothing in here is particularly important, just loose thoughts.
In general this seems pretty nice.

- Maciej

Next message: kernel test robot: "[dhowells-fs:netfs-folio-regions 18/28] fs/netfs/write_prep.c:78:31: sparse: sparse: incompatible types in comparison expression (different type sizes):"
Previous message: kernel test robot: "Re: [PATCH v2] mei: improve Denverton HSM & IFSI support"
Next in thread: Maxim Devaev: "Re: [PATCH v2] usb: gadget: f_hid: optional SETUP/SET_REPORT mode"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]