On Tue, 17 Aug 2010, Sarah Sharp wrote:
> Hi John,
>
> I've been testing your patch with a VIA USB 3.0 hub prototype.
>
> I've tested with a mouse, USB 3.0 hard drive, and high speed webcam.
> All devices seem to work fine under the hub with your patch. The only
> slightly concerning thing is that I see a bunch of stalls when I try to
> run `lsusb -v` for the USB 3.0 hub portion. The lsusb output says:
>
> can't get hub descriptor: Broken pipe
> Device Status: 0x0001
> Self Powered
>
> The USB 3.0 hub descriptor is a different descriptor code (and format)
> than USB 1.1 or 2.0 hubs. It looks like your patch changed the kernel
> code in get_hub_descriptor() to ask for the right descriptor, but I'm
> not sure if userspace is issuing a control transfer through usbfs and
> bypassing the kernel.
It is.
> Full USBmon, lsusb and dmesg output attached. I'm running lsusb
> (usbutils) version 0.82.
>
> The first failed transfer is
> ffff88012c161cc0 3629591912 S Ci:9:002:0 s a0 06 2900 0000 000d 13 <
> ffff88012c161cc0 3629595966 C Ci:9:002:0 -32 0
>
> bmRequestType = a0, bRequest = 06, wValue = 2900, wIndex = 0000, wLength
> = 000d
>
> That looks like a USB 2.0 GetHubDescriptor request to me.
>
> Alan, Daniel, or Greg, do you know if lsusb is calling through libusb to
> get the hub descriptor, or is it just getting cached descriptors from
> the kernel?
The kernel does not export the hub descriptor.
> John, I'm concerned that you're blindly assigning the USB 3.0 hub
> descriptor to a usb_hub_descriptor. The current structure is:
>
> struct usb_hub_descriptor {
> __u8 bDescLength;
> __u8 bDescriptorType;
> __u8 bNbrPorts;
> __le16 wHubCharacteristics;
> __u8 bPwrOn2PwrGood;
> __u8 bHubContrCurrent;
> /* add 1 bit for hub status change; round to bytes */
> __u8 DeviceRemovable[(USB_MAXCHILDREN + 1 + 7) / 8];
> __u8 PortPwrCtrlMask[(USB_MAXCHILDREN + 1 + 7) / 8];
> } __attribute__ ((packed));
>
> The USB 3.0 hub descriptor should look something like this:
>
> struct usb_hub_usb3_descriptor {
> __u8 bDescLength;
> __u8 bDescriptorType;
> __u8 bNbrPorts;
> __le16 wHubCharacteristics;
> __u8 bPwrOn2PwrGood;
> __u8 bHubContrCurrent;
> /* add 1 bit for hub status change; round to bytes */
> __u8 bHubHdrDecLat;
> __u16 wHubDelay;
> __u8 DeviceRemovable[2];
> } __attribute__ ((packed));
>
> The descriptor is longer, and the fields after bHubContrCurrent don't
> align properly. So I don't think your current approach of just
> assigning the new hub descriptor to a pointer of the old hub descriptor
> type is going to work very well.
It isn't.
> One solution might be to change usb_hub->descriptor into a void pointer
> and make sure all code that touches it casts it to either a
> usb_hub_descriptor or a usb_hub_usb3_descriptor.
>
> Alan, can you think of a better solution?
It's an ugly situation. A better approach might be to create an
in-kernel version of the descriptor that can be used for both types of
hub, and then populate it with the data from the actual descriptor.
That would reduce the need for casts and duplicate code.
> John, a couple more comments below.
>
> Sarah Sharp
...
> > @@ -365,6 +376,17 @@ static int hub_port_status(struct usb_hub *hub, int port1,
> > } else {
> > *status = le16_to_cpu(hub->status->port.wPortStatus);
> > *change = le16_to_cpu(hub->status->port.wPortChange);
> > +
> > + if ((hub->hdev->parent != NULL) && hub_is_superspeed(hub->hdev)) {
> > + /* Translate the USB 3 port status */
> > + u16 tmp = *status & USB3_PORT_STAT_MASK;
> > + if (*status & USB3_PORT_STAT_POWER)
> > + tmp |= USB_PORT_STAT_POWER;
> > + if ((*status & USB3_PORT_STAT_SPEED) == USB3_PORT_STAT_SPEED_SUPER)
> > + tmp |= USB_PORT_STAT_SUPER_SPEED;
> > + *status = tmp;
> > + }
> > +
This is pretty ugly too. I don't think you can safely mix USB-2 and
USB-3 port statuses into the same 16-bit value.
Alan Stern
--
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
