| Message ID | 20210721180351.129450-1-mdevaev@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | afcff6dc690e24d636a41fd4bee6057e7c70eebd |
| Headers | show |
| Series | usb: gadget: f_hid: added GET_IDLE and SET_IDLE handlers | expand |
Maxim Devaev <mdevaev@gmail.com> writes: > The USB HID standard declares mandatory support for GET_IDLE and SET_IDLE > requests for Boot Keyboard. Most hosts can handle their absence, but others > like some old/strange UEFIs and BIOSes consider this a critical error > and refuse to work with f_hid. > > This primitive implementation of saving and returning idle is sufficient > to meet the requirements of the standard and these devices. > > Signed-off-by: Maxim Devaev <mdevaev@gmail.com> yeah, I don't see any issues with this. If you have access to the tool, mind running USBCV on the f_hid gadget? Would be cool to get some confirmation that we're within spec. That shouldn't gate $subject though. Acked-by: Felipe Balbi <balbi@kernel.org>
> Felipe Balbi <balbi@kernel.org> writes: > > yeah, I don't see any issues with this. If you have access to the tool, > mind running USBCV on the f_hid gadget? Would be cool to get some > confirmation that we're within spec. Thanks for pointing to USBCV. I used a hardware USB protocol analyzer to understand what was wrong with f_hid, and my hosts only sent idle=0. Thanks to the test, I realized that I should only use the upper byte that contains duration. Here a fixed version of the patch, which successfully passes all HID tests. The idle part: Now Starting Test: HID Class GET/SET Idle Test (Configuration Index 0) Start time: Jul 22, 2021 - 20:29:40 No report IDs found in report descriptor for Interface : 0x0 GET/SETIdle test for report ID 0. Setting Idle rate to : 0x7F No report IDs found in report descriptor for Interface : 0x1 GET/SETIdle test for report ID 0. Setting Idle rate to : 0x7F Stop time: Jul 22, 2021 - 20:29:41 Duration: 1 second. Stopping Test [ HID Class GET/SET Idle Test (Configuration Index 0): Number of: Fails (0); Aborts (0); Warnings (0) ] From ac56ddc1ab2dfa599a12a3bf064e520d587e89fe Mon Sep 17 00:00:00 2001 From: Maxim Devaev <mdevaev@gmail.com> Date: Wed, 21 Jul 2021 20:48:28 +0300 Subject: [PATCH] usb: gadget: f_hid: added GET_IDLE and SET_IDLE handlers The USB HID standard declares mandatory support for GET_IDLE and SET_IDLE requests for Boot Keyboard. Most hosts can handle their absence, but others like some old/strange UEFIs and BIOSes consider this a critical error and refuse to work with f_hid. This primitive implementation of saving and returning idle is sufficient to meet the requirements of the standard and these devices. Signed-off-by: Maxim Devaev <mdevaev@gmail.com> --- drivers/usb/gadget/function/f_hid.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c index 02683ac07..1010f0a3e 100644 --- a/drivers/usb/gadget/function/f_hid.c +++ b/drivers/usb/gadget/function/f_hid.c @@ -41,6 +41,7 @@ struct f_hidg { unsigned char bInterfaceSubClass; unsigned char bInterfaceProtocol; unsigned char protocol; + unsigned char idle; unsigned short report_desc_length; char *report_desc; unsigned short report_length; @@ -523,6 +524,14 @@ static int hidg_setup(struct usb_function *f, goto respond; break; + case ((USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8 + | HID_REQ_GET_IDLE): + VDBG(cdev, "get_idle\n"); + length = min_t(unsigned int, length, 1); + ((u8 *) req->buf)[0] = hidg->idle; + goto respond; + break; + case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8 | HID_REQ_SET_REPORT): VDBG(cdev, "set_report | wLength=%d\n", ctrl->wLength); @@ -546,6 +555,14 @@ static int hidg_setup(struct usb_function *f, goto stall; break; + case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8 + | HID_REQ_SET_IDLE): + VDBG(cdev, "set_idle\n"); + length = 0; + hidg->idle = value >> 8; + goto respond; + break; + case ((USB_DIR_IN | USB_TYPE_STANDARD | USB_RECIP_INTERFACE) << 8 | USB_REQ_GET_DESCRIPTOR): switch (value >> 8) { @@ -773,6 +790,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass; hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol; hidg->protocol = HID_REPORT_PROTOCOL; + hidg->idle = 1; hidg_ss_in_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length); hidg_ss_in_comp_desc.wBytesPerInterval = cpu_to_le16(hidg->report_length);
Maxim Devaev <mdevaev@gmail.com> writes: >> Felipe Balbi <balbi@kernel.org> writes: >> >> yeah, I don't see any issues with this. If you have access to the tool, >> mind running USBCV on the f_hid gadget? Would be cool to get some >> confirmation that we're within spec. > > Thanks for pointing to USBCV. I used a hardware USB protocol analyzer > to understand what was wrong with f_hid, and my hosts only sent idle=0. > Thanks to the test, I realized that I should only use the upper byte > that contains duration. Here a fixed version of the patch, > which successfully passes all HID tests. The idle part: > > Now Starting Test: HID Class GET/SET Idle Test (Configuration Index 0) > Start time: Jul 22, 2021 - 20:29:40 > No report IDs found in report descriptor for Interface : 0x0 > GET/SETIdle test for report ID 0. Setting Idle rate to : 0x7F > No report IDs found in report descriptor for Interface : 0x1 > GET/SETIdle test for report ID 0. Setting Idle rate to : 0x7F > > Stop time: Jul 22, 2021 - 20:29:41 > Duration: 1 second. > Stopping Test [ HID Class GET/SET Idle Test (Configuration Index 0): > Number of: Fails (0); Aborts (0); Warnings (0) ] > > > From ac56ddc1ab2dfa599a12a3bf064e520d587e89fe Mon Sep 17 00:00:00 2001 > From: Maxim Devaev <mdevaev@gmail.com> > Date: Wed, 21 Jul 2021 20:48:28 +0300 > Subject: [PATCH] usb: gadget: f_hid: added GET_IDLE and SET_IDLE handlers > > The USB HID standard declares mandatory support for GET_IDLE and SET_IDLE > requests for Boot Keyboard. Most hosts can handle their absence, but others > like some old/strange UEFIs and BIOSes consider this a critical error > and refuse to work with f_hid. > > This primitive implementation of saving and returning idle is sufficient > to meet the requirements of the standard and these devices. > > Signed-off-by: Maxim Devaev <mdevaev@gmail.com> Great, thank you. Acked-by: Felipe Balbi <balbi@kernel.org>
diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c index 02683ac07..1e1d1ae38 100644 --- a/drivers/usb/gadget/function/f_hid.c +++ b/drivers/usb/gadget/function/f_hid.c @@ -41,6 +41,7 @@ struct f_hidg { unsigned char bInterfaceSubClass; unsigned char bInterfaceProtocol; unsigned char protocol; + unsigned char idle; unsigned short report_desc_length; char *report_desc; unsigned short report_length; @@ -523,6 +524,14 @@ static int hidg_setup(struct usb_function *f, goto respond; break; + case ((USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8 + | HID_REQ_GET_IDLE): + VDBG(cdev, "get_idle\n"); + length = min_t(unsigned int, length, 1); + ((u8 *) req->buf)[0] = hidg->idle; + goto respond; + break; + case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8 | HID_REQ_SET_REPORT): VDBG(cdev, "set_report | wLength=%d\n", ctrl->wLength); @@ -546,6 +555,14 @@ static int hidg_setup(struct usb_function *f, goto stall; break; + case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8 + | HID_REQ_SET_IDLE): + VDBG(cdev, "set_idle\n"); + length = 0; + hidg->idle = value; + goto respond; + break; + case ((USB_DIR_IN | USB_TYPE_STANDARD | USB_RECIP_INTERFACE) << 8 | USB_REQ_GET_DESCRIPTOR): switch (value >> 8) { @@ -773,6 +790,7 @@ static int hidg_bind(struct usb_configuration *c, struct usb_function *f) hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass; hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol; hidg->protocol = HID_REPORT_PROTOCOL; + hidg->idle = 1; hidg_ss_in_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length); hidg_ss_in_comp_desc.wBytesPerInterval = cpu_to_le16(hidg->report_length);
The USB HID standard declares mandatory support for GET_IDLE and SET_IDLE requests for Boot Keyboard. Most hosts can handle their absence, but others like some old/strange UEFIs and BIOSes consider this a critical error and refuse to work with f_hid. This primitive implementation of saving and returning idle is sufficient to meet the requirements of the standard and these devices. Signed-off-by: Maxim Devaev <mdevaev@gmail.com> --- drivers/usb/gadget/function/f_hid.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)