On Wed, 27 May 2015, Golmer Palmer wrote: > Hi Alan, > > Thank you for the new patch! > However, a comment: > > * At page 74 of the specifications, > section F.3 Boot Keyboard Requirements, > (http://www.usb.org/developers/hidpage/HID1_11.pdf) > is defined that: > "The Boot Keyboard shall, upon reset, return to the non- boot protocol > which is > described in its Report descriptor. That is, the Report descriptor for a > Boot > Keyboard does not necessarily match the boot protocol. The Report > descriptor > for a Boot Keyboard is the non-boot protocol descriptor. " > > This enforces that the default mode (protocol) is REPORT, not BOOT. > So why initialize in the patch to "1" for devices without boot support > and "0" for devices with boot support? > > I suggest to always initialize to "1" (=REPORT mode). You're right; that was a mistake in the patch. Also, the protocol has to return to the default whenever there is a new connection. > Remember that the assumption is that the DESCRIPTOR of the BOOT protocol > is IDENTICAL to the descriptor of the REPORT protocol, so they are the > same. This is the key point! > > Moreover, if a device don't supports BOOT mode, then the function > Get_Protocol() needs to be unimplemented. See page 54, section > 7.2.5 Get_Protocol Request. That section doesn't say what should happen for devices that don't support the Boot protocol. All it says is that the request must be supported by devices in the Boot subclass. There's no harm in supporting it for all devices. > So, I suggest to maintain the old behaviour > ("goto stall" for returning ERROR) in the Get_Protocol() in case of > bInterfaceSubClass != USB_INTERFACE_SUBCLASS_BOOT. This can be in the > same way as you done in the Set_Protocol function. Not necessary. Here's the next version of the patch. Alan Stern Index: usb-4.0/drivers/usb/gadget/function/f_hid.c =================================================================== --- usb-4.0.orig/drivers/usb/gadget/function/f_hid.c +++ usb-4.0/drivers/usb/gadget/function/f_hid.c @@ -44,6 +44,7 @@ struct f_hidg { /* configuration */ unsigned char bInterfaceSubClass; unsigned char bInterfaceProtocol; + unsigned char protocol_is_report; unsigned short report_desc_length; char *report_desc; unsigned short report_length; @@ -418,7 +419,9 @@ static int hidg_setup(struct usb_functio case ((USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8 | HID_REQ_GET_PROTOCOL): VDBG(cdev, "get_protocol\n"); - goto stall; + length = min_t(unsigned, length, 1); + ((u8 *) req->buf)[0] = hidg->protocol_is_report; + goto respond; break; case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8 @@ -430,6 +433,17 @@ static int hidg_setup(struct usb_functio case ((USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE) << 8 | HID_REQ_SET_PROTOCOL): VDBG(cdev, "set_protocol\n"); + if (value > 1) + goto stall; + length = 0; + /* + * We assume that programs implementing the Boot protocol + * are also compatible with the Report protocol. + */ + if (hidg->bInterfaceSubClass == USB_INTERFACE_SUBCLASS_BOOT) { + hidg->protocol_is_report = value; + goto respond; + } goto stall; break; @@ -628,6 +642,7 @@ static int hidg_bind(struct usb_configur /* set descriptor dynamic values */ hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass; hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol; + hidg->protocol_is_report = 1; hidg_hs_in_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length); hidg_fs_in_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length); hidg_hs_out_ep_desc.wMaxPacketSize = cpu_to_le16(hidg->report_length); -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
