usb: wBytesPerInterval workaround and cleanups
v3 cover usb: wBytesPerInterval workaround and cleanups
This series provides a workaround for certain devices with bogus wBytesPerInterval field in interrupt endpoint descriptors and enables them to work on the latest pedantic xHCI controllers.
It also fixes USB3 root hub descriptors and improves error logging.
new in v3:
all: add cover letter
rebase on 7.1 to fix context conflicts
1/1: reduce wMaxPacketSize instead of raising wBytesPerInterval,
as pointed out by Mathias Nyman
new in v2:
2/3: added reporter's tested-by
use usb_endpoint_is_int_in() as suggested by Alan
3/3: new patch
Michal Pecio (3):
usb: core: Fix SuperSpeed root hub wMaxPacketSize
usb: core: Fix up Interrupt IN endpoints with bogus wBytesPerInterval
usb: core: Clean up SuperSpeed/eUSB2 descriptor validation logging
drivers/usb/core/config.c | 46 ++++++++++++++++----------------------- drivers/usb/core/hcd.c | 4 +--- 2 files changed, 20 insertions(+), 30 deletions(-)
Signature · 1 line
2.48.1
PATCH v3 1/3 usb: core: Fix SuperSpeed root hub wMaxPacketSize
There is no good reason to have wBytesPerInterval < wMaxPacketSize - either one is too low or the other too high, and we may want to warn about such descriptors. Start with cleaning up our own root hubs.
USB 3.2 section 10.15.1 sets wMaxPacketSize and wBytesPerInterval of SuperSpeed hub status endpoints at 2 bytes, so reduce wMaxPacketSize from its former value of 4, which was derived from USB 2.0 spec and the kernel's USB_MAXCHILDREN limit. They don't apply because USB 3.2 10.15.2.1 specifies SuperSpeed hubs to have up to 15 ports.
--- drivers/usb/core/hcd.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/usb/core/hcd.c +1 -3
@@ -328,9 +328,7 @@ static const u8 ss_rh_config_descriptor[] = {USB_DT_ENDPOINT ,/* __u8 ep_bDescriptorType; Endpoint */ 0x81 ,/* __u8 ep_bEndpointAddress; IN Endpoint 1 */ 0x03 ,/* __u8 ep_bmAttributes; Interrupt */ -/* __le16 ep_wMaxPacketSize; 1 + (MAX_ROOT_PORTS / 8) -* see hub.c:hub_configure() for details. */ - (USB_MAXCHILDREN +1 +7 ) /8 ,0x00 , +0x02 ,0x00 ,/* __le16 ep_wMaxPacketSize; 2 bytes per USB3 10.15.1 */ 0x0c ,/* __u8 ep_bInterval; (256ms -- usb 2.0 spec) */ /* one SuperSpeed endpoint companion descriptor */
Signature · 1 line
2.48.1
PATCH v3 2/3 usb: core: Fix up Interrupt IN endpoints with bogus wBytesPerInterval
In reply to Michal PecioTao Xue found that some common devices violate USB 3.x 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.
--- drivers/usb/core/config.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/usb/core/config.c +8 -1
@@ -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
PATCH v3 3/3 usb: core: Clean up SuperSpeed/eUSB2 descriptor validation logging
In reply to Michal PecioCore 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.
--- drivers/usb/core/config.c | 37 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 26 deletions(-)
drivers/usb/core/config.c +11 -26
@@ -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 (size <USB_DT_SSP_ISOC_EP_COMP_SIZE || desc->bDescriptorType !=USB_DT_SSP_ISOC_ENDPOINT_COMP ) { -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