On Mon, 9 Jan 2012, Sarah Sharp wrote:
> > > --- a/drivers/usb/core/hub.c
> > > +++ b/drivers/usb/core/hub.c
> > > @@ -3164,6 +3164,16 @@ check_highspeed (struct usb_hub *hub, struct usb_device *udev, int port1)
> > > kfree(qual);
> > > }
> > >
> > > +static void check_superspeed(struct usb_hub *hub, struct usb_device *udev,
> > > + int port1)
> > > +{
> > > + if (le16_to_cpu(udev->descriptor.bcdUSB) < 0x0300 &&
> >
> > What's the point of this check? What happens if a USB-3.0 device sets
> > bcdUSB to 0x0300 in its high-speed device descriptor?
>
> Why would it do that? I haven't seen any devices that do, and I'm
> pretty sure they would fail the USB-IF certification test if they did.
Lots of firmware has mistakes, and lots of devices get sold that can't
pass the USB CV tests.
Besides, what good does the check do? Was it meant to be a way of
determining the device's current speed? If it was, just use
udev->speed.
> I'm mostly copying the speed check from check_highspeed().
But you didn't copy the tests at the place where check_highspeed() is
called.
> I assumed it
> was checking the bcdUSB version rather than using the udev->speed
> because it wouldn't be set yet, although I'll admit I didn't look at the
> speed setting code too closely.
No, that's not the reason. We look at bcdUSB because if it is < 0x0200
then the device is not required to support the get-device-qualifier
request. Nothing analogous applies in your case.
Incidentally, udev->speed gets set as soon as the initial port reset is
done. (Or even sooner in the case of SuperSpeed.) That's when
connection speeds are determined in USB.
> > > + /* BOS descriptor should have already been fetched by now. */
> >
> > Is this comment really helpful?
>
> Eh, I can remove it. I thought it might be useful in case anyone tried
> to move the SuperSpeed before the descriptor fetches. The
> check_highspeed() function will work in that case, but not the
> check_superspeed() function.
It can't be moved so easily; the speed and the BOS descriptors are both
set in hub_port_init().
> > > + dev_info(&udev->dev, "not running at top speed; "
> > > + "connect to a SuperSpeed hub or host\n");
> >
> > The "or host" part doesn't seem very useful. What if the user doesn't
> > have a SuperSpeed host? Might as well leave it out.
>
> It might incentive them to procure one. ;) But in all seriousness, a
> user could plug a USB 3.0 device into a USB 3.0 hub, but still have the
> message print because they've plugged the hub into an EHCI port.
> Telling them to plug the device into a SuperSpeed hub doesn't help, they
> already do!
Technically not -- when a hub is connected to an EHCI port it's high
speed, not SuperSpeed. To put it another way, the speed is a property
of a particular connection, not an inherent property of a device. If
you like, change the message to "connect to a SuperSpeed port".
And telling the user to plug the device into a SuperSpeed port or host
when the computer doesn't have any is useless -- it's like saying "Use
a different computer". Pretty dumb if, for example, the device is a
drive that's going to be used to make a backup.
> > > @@ -3383,6 +3393,7 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1,
> > > && udev->speed == USB_SPEED_FULL
> > > && highspeed_hubs != 0)
> > > check_highspeed (hub, udev, port1);
> >
> > You need to check some things here: Is the device running at less than
> > SuperSpeed? Are there any USB-3.0 controllers in the system?
> >
> > Or add those tests to your check_super_speed routine.
>
> So should I add more warnings if there aren't any USB 3.0 hosts in the
> system (or CONFIG_USB_XHCI is not turned on)?
Nope. Think back to the days when high speed was still new; there were
plenty of computers around that didn't have EHCI controllers. What
good would it have done to log a message every time somebody plugged a
high-speed-capable device into such a computer?
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
