| Message ID | 20240404100635.3215340-1-peter@korsgaard.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ec6ce7075ef879b91a8710829016005dc8170f17 |
| Headers | show |
| Series | usb: gadget: composite: fix OS descriptors w_value logic | expand |
Hi Peter, W dniu 4.04.2024 o 12:06, Peter Korsgaard pisze: > The OS descriptors logic had the high/low byte of w_value inverted, causing > the extended properties to not be accessible for interface != 0. Out of curiosity - did you run into problems running some USB gadget, and if yes, what was it? Or is this patch a result of a code and documentation review without actually running a gadget? > > From the Microsoft documentation: > https://learn.microsoft.com/en-us/windows-hardware/drivers/usbcon/microsoft-os-1-0-descriptors-specification > > OS_Desc_CompatID.doc (w_index = 0x4): > > - wValue: > > High Byte = InterfaceNumber. InterfaceNumber is set to the number of the > interface or function that is associated with the descriptor, typically > 0x00. Because a device can have only one extended compat ID descriptor, > it should ignore InterfaceNumber, regardless of the value, and simply > return the descriptor. > > Low Byte = 0. PageNumber is used to retrieve descriptors that are larger > than 64 KB. The header section is 16 bytes, so PageNumber is set to 0 for > this request. > > We currently do not support >64KB compat ID descriptors, so verify that the > low byte is 0. > > OS_Desc_Ext_Prop.doc (w_index = 0x5): > > - wValue: > > High byte = InterfaceNumber. The high byte of wValue is set to the number > of the interface or function that is associated with the descriptor. > > Low byte = PageNumber. The low byte of wValue is used to retrieve > descriptors that are larger than 64 KB. The header section is 10 bytes, so > PageNumber is set to 0 for this request. > FWIW w_value is: u16 w_value = le16_to_cpu(ctrl->wValue); Regards, Andrzej > We also don't support >64KB extended properties, so verify that the low byte > is 0 and use the high byte for the interface number. > > Fixes: 37a3a533429e ("usb: gadget: OS Feature Descriptors support") > > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> > --- > drivers/usb/gadget/composite.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > index 0ace45b66a31..0e151b54aae8 100644 > --- a/drivers/usb/gadget/composite.c > +++ b/drivers/usb/gadget/composite.c > @@ -2112,7 +2112,7 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) > buf[5] = 0x01; > switch (ctrl->bRequestType & USB_RECIP_MASK) { > case USB_RECIP_DEVICE: > - if (w_index != 0x4 || (w_value >> 8)) > + if (w_index != 0x4 || (w_value & 0xff)) > break; > buf[6] = w_index; > /* Number of ext compat interfaces */ > @@ -2128,9 +2128,9 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) > } > break; > case USB_RECIP_INTERFACE: > - if (w_index != 0x5 || (w_value >> 8)) > + if (w_index != 0x5 || (w_value & 0xff)) > break; > - interface = w_value & 0xFF; > + interface = w_value >> 8; > if (interface >= MAX_CONFIG_INTERFACES || > !os_desc_cfg->interface[interface]) > break;
>>>>> "Andrzej" == Andrzej Pietrasiewicz <andrzej.p@collabora.com> writes: > Hi Peter, > W dniu 4.04.2024 o 12:06, Peter Korsgaard pisze: >> The OS descriptors logic had the high/low byte of w_value inverted, causing >> the extended properties to not be accessible for interface != 0. > Out of curiosity - did you run into problems running some USB gadget, > and if yes, what was it? Or is this patch a result of a code and documentation > review without actually running a gadget? I had issues with getting Windows to accept the OS descriptors when the function I wanted to use with WinUSB was not the first (= interface 0) function in a composite device together with HID and mass storage.
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 0ace45b66a31..0e151b54aae8 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -2112,7 +2112,7 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) buf[5] = 0x01; switch (ctrl->bRequestType & USB_RECIP_MASK) { case USB_RECIP_DEVICE: - if (w_index != 0x4 || (w_value >> 8)) + if (w_index != 0x4 || (w_value & 0xff)) break; buf[6] = w_index; /* Number of ext compat interfaces */ @@ -2128,9 +2128,9 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) } break; case USB_RECIP_INTERFACE: - if (w_index != 0x5 || (w_value >> 8)) + if (w_index != 0x5 || (w_value & 0xff)) break; - interface = w_value & 0xFF; + interface = w_value >> 8; if (interface >= MAX_CONFIG_INTERFACES || !os_desc_cfg->interface[interface]) break;
The OS descriptors logic had the high/low byte of w_value inverted, causing the extended properties to not be accessible for interface != 0. From the Microsoft documentation: https://learn.microsoft.com/en-us/windows-hardware/drivers/usbcon/microsoft-os-1-0-descriptors-specification OS_Desc_CompatID.doc (w_index = 0x4): - wValue: High Byte = InterfaceNumber. InterfaceNumber is set to the number of the interface or function that is associated with the descriptor, typically 0x00. Because a device can have only one extended compat ID descriptor, it should ignore InterfaceNumber, regardless of the value, and simply return the descriptor. Low Byte = 0. PageNumber is used to retrieve descriptors that are larger than 64 KB. The header section is 16 bytes, so PageNumber is set to 0 for this request. We currently do not support >64KB compat ID descriptors, so verify that the low byte is 0. OS_Desc_Ext_Prop.doc (w_index = 0x5): - wValue: High byte = InterfaceNumber. The high byte of wValue is set to the number of the interface or function that is associated with the descriptor. Low byte = PageNumber. The low byte of wValue is used to retrieve descriptors that are larger than 64 KB. The header section is 10 bytes, so PageNumber is set to 0 for this request. We also don't support >64KB extended properties, so verify that the low byte is 0 and use the high byte for the interface number. Fixes: 37a3a533429e ("usb: gadget: OS Feature Descriptors support") Signed-off-by: Peter Korsgaard <peter@korsgaard.com> --- drivers/usb/gadget/composite.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)