On Wed, May 10, 2017 at 04:31:19PM +0200, Johan Hovold wrote: > On Wed, May 10, 2017 at 10:12:56AM -0400, Alan Stern wrote: > > On Wed, 10 May 2017, Johan Hovold wrote: > > > /* USB 2.0 spec Section 11.24.4.5 */ > > > -static int get_hub_descriptor(struct usb_device *hdev, void *data) > > > +static int get_hub_descriptor(struct usb_device *hdev, > > > + struct usb_hub_descriptor *desc) > > > { > > > int i, ret, size; > > > unsigned dtype; > > > @@ -378,12 +379,16 @@ static int get_hub_descriptor(struct usb_device *hdev, void *data) > > > for (i = 0; i < 3; i++) { > > > ret = usb_control_msg(hdev, usb_rcvctrlpipe(hdev, 0), > > > USB_REQ_GET_DESCRIPTOR, USB_DIR_IN | USB_RT_HUB, > > > - dtype << 8, 0, data, size, > > > + dtype << 8, 0, desc, size, > > > USB_CTRL_GET_TIMEOUT); > > > if (hub_is_superspeed(hdev)) { > > > if (ret == size) > > > return ret; > > > - } else if (ret >= (USB_DT_HUB_NONVAR_SIZE + 2)) { > > > + } else if (ret >= USB_DT_HUB_NONVAR_SIZE + 2) { > > > + /* Make sure we have the DeviceRemovable field. */ > > > + size = USB_DT_HUB_NONVAR_SIZE + desc->bNbrPorts / 8 + 1; > > > + if (ret < size) > > > + return -EMSGSIZE; > > > > The logic could be simplified a little. Since we don't really care > > about the return code when an error occurs, you could just do: > > > > } else if (ret >= USB_DT_HUB_NONVAR_SIZE + > > desc->bNbrPorts / 8 + 1) { > > /* We have the entire DeviceRemovable field. */ > > return ret; > > } > > Sure, that would work, but I it doesn't feel right to access bNbrPorts > without first verifying we got the non-variable fields. > > I considered dropping the +2 bit, but decided to keep it in the unlikely > even that there are quirky devices out there that rely on it (e.g. first > read always return 7 bytes). Spelling it out makes it sound overly > conservative though. How about I drop that instead? Then again, a non-SS hub descriptor is always at least (USB_DT_HUB_NONVAR_SIZE + 2) bytes long so keeping it kind of makes sense anyway. Johan