On Wed, Oct 2, 2013 at 9:14 AM, Sarah Sharp
<sarah.a.sh...@linux.intel.com> wrote:
> On Mon, Sep 23, 2013 at 04:29:40PM -0700, Dan Williams wrote:
>> Change the enumeration scheme for xhci attached devices from:
>>
>>    SetAddress
>>    GetDescriptor(8)
>>    GetDescriptor(18)
>>
>> ...to:
>>
>>    GetDescriptor(64)
>>    SetAddress
>>    GetDescriptor(18)
>
> Thinking about this sequence again, I'm trying to figure out if that
> first 64-byte descriptor fetch is going to be an issue.  We set up the
> control endpoint context with a max packet size of 8 bytes, but it can
> be larger, depending on the device.  We don't know what it is until we
> fetch the device descriptors.
>
> So if the USB core asks for 64-bytes of the device descriptor before the
> xHCI driver sets the real max packet size in the control endpoint
> context, won't the host controller expect to receive 8-byte packets, but
> may instead get larger packets?
>
> Alan, how would this work under EHCI?
>
> Dan, I was also testing under HSW-ULT with a (possibly buggy) device,
> and I managed to get the device context into a bad state.  The device
> refused to respond to the first 64-byte control transfer (it timed out
> three times), and then the USB core reset the device.  The xHCI driver
> then attempted to issue a Reset Device command, which failed because the
> device was in the default state.
>
> I think this meant the endpoint toggles in the control endpoint on the
> device were mis-matched to the host's toggle state.  That would cause
> protocol errors.  I'm not quite sure what the solution for this is yet,
> but I think it may be why you get the context state error on the second
> Set Address command.  Because the Reset Device command didn't complete
> successfully, the host thinks the device is still in the default state.
>
> Not sure why the Set Address with BSR=1 fails on a different system.
>

So this is working for me now with the following incremental patch.  I
simply disable new scheme detection for USB3 devices.  This should be
safe since:

1/ it was already the default case for USB3 devices (previously we
disabled it for all xHC attached devices)

2/ we don't need to worry about xHC implementation quirks with
handling USB3 devices in the default state, we just go straight to
addressed as before.

We can revisit this when/if a USB3 device shows up that needs
new_scheme detection.

--
Dan


diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 355a09d..a9eb65c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2514,6 +2514,14 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
 #define HUB_LONG_RESET_TIME    200
 #define HUB_RESET_TIMEOUT      800

+static bool use_new_scheme(struct usb_device *udev, int retry)
+{
+       if (udev->speed == USB_SPEED_SUPER)
+               return false;
+
+       return USE_NEW_SCHEME(retry);
+}
+
 static int hub_port_reset(struct usb_hub *hub, int port1,
                        struct usb_device *udev, unsigned int delay, bool warm);

@@ -4069,7 +4077,7 @@ hub_port_init (struct usb_hub *hub, struct
usb_device *udev, int port1,
         * this area, and this is how Linux has done it for ages.
         * Change it cautiously.
         *
-        * NOTE:  If USE_NEW_SCHEME() is true we will start by issuing
+        * NOTE:  If use_new_scheme() is true we will start by issuing
         * a 64-byte GET_DESCRIPTOR request.  This is what Windows does,
         * so it may help with some non-standards-compliant devices.
         * Otherwise we start with SET_ADDRESS and then try to read the
@@ -4077,10 +4085,12 @@ hub_port_init (struct usb_hub *hub, struct
usb_device *udev, int port1,
         * value.
         */
        for (i = 0; i < GET_DESCRIPTOR_TRIES; (++i, msleep(100))) {
-               if (USE_NEW_SCHEME(retry_counter)) {
-                       struct usb_device_descriptor *buf;
+               bool did_new_scheme = false;
+
+               if (use_new_scheme(udev, retry_counter)) {
                        int r = 0;

+                       did_new_scheme = true;
 #define GET_DESCRIPTOR_BUFSIZE 64
                        buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO);
                        if (!buf) {
@@ -4173,7 +4183,7 @@ hub_port_init (struct usb_hub *hub, struct
usb_device *udev, int port1,
                         *  - read ep0 maxpacket even for high and low speed,
                         */
                        msleep(10);
-                       if (USE_NEW_SCHEME(retry_counter))
+                       if (did_new_scheme)
                                break;
                }
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to