Skip to contentSkip to conversation

usb: core: Fix root hub descriptor wBytesPerInterval

PATCH In review Michal Pecio 2 participants 6 messages May 4 – May 6 USB

lore
Share & tools
lore

Michal Pecio

author
#0 lore raw

PATCH v2 1/3 usb: core: Fix root hub descriptor wBytesPerInterval

Per USB3 9.6.7, it's "the total number of bytes this endpoint will transfer every service interval". There seems to be no good reason to have wBytesPerInterval < wMaxPacketSize - either one is too low or the other too high. Here, wBytesPerInterval is too low for hubs with more than 15 ports and xHCI spec allows such root hubs.

This is inconsequential for emulated root hubs, but we may want to override and log suspiciously low wBytesPerInterval in descriptors, so fix this to prevent nuisance warnings.

Signed-off-by Michal Pecio <[email protected]>

---

v2: same as v1

 drivers/usb/core/hcd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/core/hcd.c +2 -1
index 89221f1ce769..fc8130f94ca5 100644
@@ -339,7 +339,8 @@ static const u8 ss_rh_config_descriptor[] = {
 		     /* Companion */
 	0x00,        /* __u8 ss_bMaxBurst; allows 1 TX between ACKs */
 	0x00,        /* __u8 ss_bmAttributes; 1 packet per service interval */
-	0x02, 0x00   /* __le16 ss_wBytesPerInterval; 15 bits for max 15 ports */
+		     /* __le16 ss_wBytesPerInterval; same as wMaxPacketSize */
+	(USB_MAXCHILDREN + 1 + 7) / 8, 0x00,
 };
 
 /* authorized_default behaviour:
Signature · 1 line
2.48.1
author
#1 lore raw

PATCH v2 2/3 usb: core: Fix up Interrupt IN endpoints with bogus wBytesPerInterval

Tao Xue found that some common devices violate USB3 section 9.6.7 by reporting wBytesPerInterval lower than the size of packets they actually send. I confirmed that AX88179 may set it to 0 and RTL8153 CDC configuration sets it to 8 but sends both 8 and 16 byte packets:

S Ii:11:007:3 -115:128 16 <
C Ii:11:007:3 0:128 8 = a1000000 01000000
S Ii:11:007:3 -115:128 16 <
C Ii:11:007:3 0:128 16 = a12a0000 01000800 00000000 00000000

Most xHCI host controllers neglect interrupt bandwidth reservations and let such devices exceed theirs, some fail the URB with EOVERFLOW.

Assume that wBytesPerInterval lower than wMaxPacketSize is bogus and increase it to the worst case maximum on interrupt IN endpoints. This solves xHCI problems and appears to have no other effect. Interrupt transfers are not limited to one interval and drivers submit URBs of class defined size without looking at wBytesPerInterval. Any multi- interval transfer is considered terminated by a packet shorter than wMaxPacketSize regardless of wBytesPerInterval - see USB3 8.10.3.

Stay in spec on OUT endpoints and isochronous. No buggy devices are known and we don't want to risk sending more data than the device is prepared to handle or confusing isoc drivers regarding altsetting capacities guaranteed by the device itself. And don't complain when wMaxPacketSize <= wBytesPerInterval < wMaxPacketSize * (bMaxBurst+1) because enabling this seems to be the exact goal of the spec.

Reported-and-tested-by Tao Xue <[email protected]>
Closes https://lore.kernel.org/linux-usb/[email protected]/ open here
Cc [email protected]
Signed-off-by Michal Pecio <[email protected]>

---

v2:
- added reporter's tested-by
- use usb_endpoint_is_int_in() as suggested by Alan

 drivers/usb/core/config.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/usb/core/config.c +8 -1
index 6a1fd967e0a6..74945cd30cd2 100644
@@ -191,7 +191,14 @@ static void usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
 			(desc->bMaxBurst + 1);
 	else
 		max_tx = 999999;
-	if (le16_to_cpu(desc->wBytesPerInterval) > max_tx) {
+	/*
+	 * wBytesPerInterval > max_tx is bogus, but USB3 spec doesn't forbid the opposite.
+	 * Experience shows that wBytesPerInterval < wMaxPacketSize on common interrupt IN
+	 * endpoints is usually bogus too, and recent HCs enforce interrupt BW limits.
+	 */
+	if (le16_to_cpu(desc->wBytesPerInterval) > max_tx ||
+	    (le16_to_cpu(desc->wBytesPerInterval) < usb_endpoint_maxp(&ep->desc) &&
+	     usb_endpoint_is_int_in(&ep->desc))) {
 		dev_notice(ddev, "%s endpoint with wBytesPerInterval of %d in "
 				"config %d interface %d altsetting %d ep %d: "
 				"setting to %d\n",
Signature · 1 line
2.48.1
author
#2 lore raw

PATCH v2 3/3 usb: core: Clean up SuperSpeed/eUSB2 descriptor validation logging

In reply to Michal Pecio

Core usually prints endpoint addresses with 0x%X format. Change this code to use it too, instead of just %d. Particularly for IN, 0x83 seems more readable than 131.

While at that, fix checkpatch warnings about multi-line quoted strings, as well as missing or doubled whitespace in those strings.

Signed-off-by Michal Pecio <[email protected]>

---

v2: new patch in series

 drivers/usb/core/config.c | 37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)
drivers/usb/core/config.c +11 -26
index 74945cd30cd2..6b718f8a6697 100644
@@ -56,8 +56,7 @@ static void usb_parse_ssp_isoc_endpoint_companion(struct device *ddev,
 	desc = (struct usb_ssp_isoc_ep_comp_descriptor *) buffer;
 	if (desc->bDescriptorType != USB_DT_SSP_ISOC_ENDPOINT_COMP ||
 	    size < USB_DT_SSP_ISOC_EP_COMP_SIZE) {
-		dev_notice(ddev, "Invalid SuperSpeedPlus isoc endpoint companion"
-			 "for config %d interface %d altsetting %d ep %d.\n",
+		dev_notice(ddev, "Invalid SuperSpeedPlus isoc endpoint companion for config %d interface %d altsetting %d ep 0x%X.\n",
 			 cfgno, inum, asnum, ep->desc.bEndpointAddress);
 		return;
 	}

@@ -91,7 +90,7 @@ static void usb_parse_eusb2_isoc_endpoint_companion(struct device *ddev,
 		size -= h->bLength;
 	}
 
-	dev_notice(ddev, "No eUSB2 isoc ep %d companion for config %d interface %d altsetting %d\n",
+	dev_notice(ddev, "No eUSB2 isoc ep 0x%X companion for config %d interface %d altsetting %d\n",
 		   ep->desc.bEndpointAddress, cfgno, inum, asnum);
 }
 

@@ -115,9 +114,7 @@ static void usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
 	}
 
 	if (desc->bDescriptorType != USB_DT_SS_ENDPOINT_COMP) {
-		dev_notice(ddev, "No SuperSpeed endpoint companion for config %d "
-				" interface %d altsetting %d ep %d: "
-				"using minimum values\n",
+		dev_notice(ddev, "No SuperSpeed endpoint companion for config %d interface %d altsetting %d ep 0x%X: using minimum values\n",
 				cfgno, inum, asnum, ep->desc.bEndpointAddress);
 
 		/* Fill in some default values.

@@ -141,42 +138,32 @@ static void usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
 
 	/* Check the various values */
 	if (usb_endpoint_xfer_control(&ep->desc) && desc->bMaxBurst != 0) {
-		dev_notice(ddev, "Control endpoint with bMaxBurst = %d in "
-				"config %d interface %d altsetting %d ep %d: "
-				"setting to zero\n", desc->bMaxBurst,
-				cfgno, inum, asnum, ep->desc.bEndpointAddress);
+		dev_notice(ddev, "Control endpoint with bMaxBurst = %d in config %d interface %d altsetting %d ep 0x%X: setting to zero\n",
+				desc->bMaxBurst, cfgno, inum, asnum, ep->desc.bEndpointAddress);
 		ep->ss_ep_comp.bMaxBurst = 0;
 	} else if (desc->bMaxBurst > 15) {
-		dev_notice(ddev, "Endpoint with bMaxBurst = %d in "
-				"config %d interface %d altsetting %d ep %d: "
-				"setting to 15\n", desc->bMaxBurst,
-				cfgno, inum, asnum, ep->desc.bEndpointAddress);
+		dev_notice(ddev, "Endpoint with bMaxBurst = %d in config %d interface %d altsetting %d ep 0x%X: setting to 15\n",
+				desc->bMaxBurst, cfgno, inum, asnum, ep->desc.bEndpointAddress);
 		ep->ss_ep_comp.bMaxBurst = 15;
 	}
 
 	if ((usb_endpoint_xfer_control(&ep->desc) ||
 			usb_endpoint_xfer_int(&ep->desc)) &&
 				desc->bmAttributes != 0) {
-		dev_notice(ddev, "%s endpoint with bmAttributes = %d in "
-				"config %d interface %d altsetting %d ep %d: "
-				"setting to zero\n",
+		dev_notice(ddev, "%s endpoint with bmAttributes = %d in config %d interface %d altsetting %d ep 0x%X: setting to zero\n",
 				usb_endpoint_xfer_control(&ep->desc) ? "Control" : "Bulk",
 				desc->bmAttributes,
 				cfgno, inum, asnum, ep->desc.bEndpointAddress);
 		ep->ss_ep_comp.bmAttributes = 0;
 	} else if (usb_endpoint_xfer_bulk(&ep->desc) &&
 			desc->bmAttributes > 16) {
-		dev_notice(ddev, "Bulk endpoint with more than 65536 streams in "
-				"config %d interface %d altsetting %d ep %d: "
-				"setting to max\n",
+		dev_notice(ddev, "Bulk endpoint with more than 65536 streams in config %d interface %d altsetting %d ep 0x%X: setting to max\n",
 				cfgno, inum, asnum, ep->desc.bEndpointAddress);
 		ep->ss_ep_comp.bmAttributes = 16;
 	} else if (usb_endpoint_xfer_isoc(&ep->desc) &&
 		   !USB_SS_SSP_ISOC_COMP(desc->bmAttributes) &&
 		   USB_SS_MULT(desc->bmAttributes) > 3) {
-		dev_notice(ddev, "Isoc endpoint has Mult of %d in "
-				"config %d interface %d altsetting %d ep %d: "
-				"setting to 3\n",
+		dev_notice(ddev, "Isoc endpoint has Mult of %d in config %d interface %d altsetting %d ep 0x%X: setting to 3\n",
 				USB_SS_MULT(desc->bmAttributes),
 				cfgno, inum, asnum, ep->desc.bEndpointAddress);
 		ep->ss_ep_comp.bmAttributes = 2;

@@ -199,9 +186,7 @@ static void usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
 	if (le16_to_cpu(desc->wBytesPerInterval) > max_tx ||
 	    (le16_to_cpu(desc->wBytesPerInterval) < usb_endpoint_maxp(&ep->desc) &&
 	     usb_endpoint_is_int_in(&ep->desc))) {
-		dev_notice(ddev, "%s endpoint with wBytesPerInterval of %d in "
-				"config %d interface %d altsetting %d ep %d: "
-				"setting to %d\n",
+		dev_notice(ddev, "%s endpoint with wBytesPerInterval of %d in config %d interface %d altsetting %d ep 0x%X: setting to %d\n",
 				usb_endpoint_xfer_isoc(&ep->desc) ? "Isoc" : "Int",
 				le16_to_cpu(desc->wBytesPerInterval),
 				cfgno, inum, asnum, ep->desc.bEndpointAddress,
Signature · 1 line
2.48.1
#3 lore raw
In reply to Michal Pecio
Quoted 6 lines · Michal Pecio

Per USB3 9.6.7, it's "the total number of bytes this endpoint will transfer every service interval". There seems to be no good reason to have wBytesPerInterval < wMaxPacketSize - either one is too low or the other too high. Here, wBytesPerInterval is too low for hubs with more than 15 ports and xHCI spec allows such root hubs.

There shouldn't be a USB3 hub with more than 15 ports as USB 3.x specification limits USB3 ports to 15.

See USB 3.2 section 10.15.2.1 Hub Descriptor
"bNbrPorts: Number of downstream facing ports that this hub supports. The
maximum number of ports of ports a hub can support is 15."

Hub driver also fails if hub has more than 15 ports. hub.c hub_configure():

   maxchild = USB_MAXCHILDREN;
        if (hub_is_superspeed(hdev))
                maxchild = min_t(unsigned, maxchild, USB_SS_MAXPORTS);
        if (hub->descriptor->bNbrPorts > maxchild) {
                message = "hub has too many ports!";
                ret = -ENODEV;
                goto fail;

ch11.h:#define USB_SS_MAXPORTS 15

The USB 3.2 specification 10.15.1 'Standard Descriptors for Hub Class' also shows hardcoded values of '2' for both the wBytesPerInterval and wMaxPacketSize for the hub interrupt endpoint.

Thanks
Mathias

author
#4 lore raw
Quoted 28 lines · Mathias Nyman
Quoted 6 lines · Michal Pecio

Per USB3 9.6.7, it's "the total number of bytes this endpoint will transfer every service interval". There seems to be no good reason to have wBytesPerInterval < wMaxPacketSize - either one is too low or the other too high. Here, wBytesPerInterval is too low for hubs with more than 15 ports and xHCI spec allows such root hubs.

There shouldn't be a USB3 hub with more than 15 ports as USB 3.x specification limits USB3 ports to 15.

See USB 3.2 section 10.15.2.1 Hub Descriptor
"bNbrPorts: Number of downstream facing ports that this hub supports. The
maximum number of ports of ports a hub can support is 15."

Hub driver also fails if hub has more than 15 ports. hub.c hub_configure():

    maxchild = USB_MAXCHILDREN;
         if (hub_is_superspeed(hdev))
                 maxchild = min_t(unsigned, maxchild, USB_SS_MAXPORTS);

         if (hub->descriptor->bNbrPorts > maxchild) {
                 message = "hub has too many ports!";
                 ret = -ENODEV;
                 goto fail;
Quoted 5 lines

ch11.h:#define USB_SS_MAXPORTS 15

The USB 3.2 specification 10.15.1 'Standard Descriptors for Hub Class' also shows hardcoded values of '2' for both the wBytesPerInterval and wMaxPacketSize for the hub interrupt endpoint.

Thanks, looks like we should be able to get away with reducing wMaxPacketSize then. Currently it's calculated to fit USB_MAXCHILDREN i.e. 31 ports, supposedly dictated by the needs of (obsolete) WUSB.

Any preferences whether to replace USB_MAXCHILDREN with USB_SS_MAXPORTS in the existing formula for USB3 hubs or just hardcode 0x02?

Regards,
Michal

#5 lore raw
Quoted 35 lines · Michal Pecio
Quoted 28 lines · Mathias Nyman
Quoted 6 lines · Michal Pecio

Per USB3 9.6.7, it's "the total number of bytes this endpoint will transfer every service interval". There seems to be no good reason to have wBytesPerInterval < wMaxPacketSize - either one is too low or the other too high. Here, wBytesPerInterval is too low for hubs with more than 15 ports and xHCI spec allows such root hubs.

There shouldn't be a USB3 hub with more than 15 ports as USB 3.x specification limits USB3 ports to 15.

See USB 3.2 section 10.15.2.1 Hub Descriptor
"bNbrPorts: Number of downstream facing ports that this hub supports. The
maximum number of ports of ports a hub can support is 15."

Hub driver also fails if hub has more than 15 ports. hub.c hub_configure():

     maxchild = USB_MAXCHILDREN;
          if (hub_is_superspeed(hdev))
                  maxchild = min_t(unsigned, maxchild, USB_SS_MAXPORTS);

          if (hub->descriptor->bNbrPorts > maxchild) {
                  message = "hub has too many ports!";
                  ret = -ENODEV;
                  goto fail;
Quoted 5 lines

ch11.h:#define USB_SS_MAXPORTS 15

The USB 3.2 specification 10.15.1 'Standard Descriptors for Hub Class' also shows hardcoded values of '2' for both the wBytesPerInterval and wMaxPacketSize for the hub interrupt endpoint.

Quoted 6 lines

Thanks, looks like we should be able to get away with reducing wMaxPacketSize then. Currently it's calculated to fit USB_MAXCHILDREN i.e. 31 ports, supposedly dictated by the needs of (obsolete) WUSB.

Any preferences whether to replace USB_MAXCHILDREN with USB_SS_MAXPORTS in the existing formula for USB3 hubs or just hardcode 0x02?

Maybe prefer the hardcoded 0x02 value as spec does it as well

Thanks
Mathias