This is an update to some patches from the December/January
timeframe, which will help sort out some of the mess for
drivers that need to use the reset logic.  It's one of the
last significant patches in my gadget-2.6 tree that haven't
yet been merged into the main kernel tree.

Please merge.

- dave


More refactoring of the enumeration code paths:

 * The first half of usb_new_device() becomes the second half of a new
   hub_port_init() routine (resets, sets address, gets descriptor)

 * The middle chunk of hub_port_connect_change() becomes the first half
   of that new hub_port_init() routine.

 * Khubd uses that new routine in hub_port_connect_change().

 * Now usb_new_device() cleans up better after faults, and has
   a more useful locking policy (caller owns dev->serialize).

 * Has related minor cleanups including commenting some of
   the curious request sequences coming from khubd.

Refactoring means a lot of the current usb_reset_device() logic won't
need to stay an imperfect clone of the enumeration code ... soon, it
can just call hub_port_init().

Even without touching usb_reset_device(), this eliminates a deadlock.
Previously, address0_sem was used both during probe and during reset,
so probe routines can't implement DFU firmware download (involves a
reset; DFU also uncovers other problems) or safely recover from probe
faults by resetting (usb-storage can try that).  Now that lock is no
longer held during probe(); so those deadlocks are gone.  (And some
drivers, like at76c503, can start to remove ugly workarounds.)


--- 1.82/drivers/usb/core/hcd.c Thu Mar 25 12:58:45 2004
+++ edited/drivers/usb/core/hcd.c       Wed Apr 21 10:46:29 2004
@@ -762,10 +762,22 @@
        set_bit (devnum, usb_dev->bus->devmap.devicemap);
        usb_dev->state = USB_STATE_ADDRESS;
 
+       usb_dev->epmaxpacketin[0] = usb_dev->epmaxpacketout[0] = 64;
+       retval = usb_get_device_descriptor(usb_dev, USB_DT_DEVICE_SIZE);
+       if (retval != sizeof usb_dev->descriptor) {
+               dev_dbg (parent_dev, "can't read %s device descriptor %d\n",
+                               usb_dev->dev.bus_id, retval);
+               return (retval < 0) ? retval : -EMSGSIZE;
+       }
+
+       (void) usb_get_dev (usb_dev);
+       down (&usb_dev->serialize);
        retval = usb_new_device (usb_dev);
        if (retval)
                dev_err (parent_dev, "can't register root hub for %s, %d\n",
                                usb_dev->dev.bus_id, retval);
+       up (&usb_dev->serialize);
+       usb_put_dev (usb_dev);
        return retval;
 }
 EXPORT_SYMBOL (usb_register_root_hub);
--- 1.92/drivers/usb/core/hub.c Mon Apr 19 01:12:13 2004
+++ edited/drivers/usb/core/hub.c       Wed Apr 21 10:46:29 2004
@@ -841,8 +841,11 @@
        return ret;
 }
 
-#define HUB_RESET_TRIES                5
-#define HUB_PROBE_TRIES                2
+#define PORT_RESET_TRIES       5
+#define SET_ADDRESS_TRIES      2
+#define GET_DESCRIPTOR_TRIES   2
+#define SET_CONFIG_TRIES       2
+
 #define HUB_ROOT_RESET_TIME    50      /* times are in msec */
 #define HUB_SHORT_RESET_TIME   10
 #define HUB_LONG_RESET_TIME    200
@@ -907,7 +910,7 @@
        int i, status;
 
        /* Reset the port */
-       for (i = 0; i < HUB_RESET_TRIES; i++) {
+       for (i = 0; i < PORT_RESET_TRIES; i++) {
                set_port_feature(hub, port + 1, USB_PORT_FEAT_RESET);
 
                /* return on disconnect or reset */
@@ -1003,40 +1006,20 @@
        return ((portstatus&USB_PORT_STAT_CONNECTION)) ? 0 : 1;
 }
 
-static void hub_port_connect_change(struct usb_hub *hubstate, int port,
-                                       u16 portstatus, u16 portchange)
+/* reset device, (re)assign address, get device descriptor.
+ * device connection is stable, no more debouncing needed.
+ * returns device in USB_STATE_ADDRESS, except on error.
+ * on error return, device is no longer usable (ref dropped).
+ *
+ * caller owns dev->serialize for the device, guarding against
+ * config changes and disconnect processing.
+ */
+static int
+hub_port_init (struct usb_device *hub, struct usb_device *dev, int port)
 {
-       struct usb_device *hub = interface_to_usbdev(hubstate->intf);
-       struct usb_device *dev;
-       unsigned int delay = HUB_SHORT_RESET_TIME;
-       int i;
-
-       dev_dbg (&hubstate->intf->dev,
-               "port %d, status %x, change %x, %s\n",
-               port + 1, portstatus, portchange, portspeed (portstatus));
-
-       /* Clear the connection change status */
-       clear_port_feature(hub, port + 1, USB_PORT_FEAT_C_CONNECTION);
-
-       /* Disconnect any existing devices under this port */
-       if (hub->children[port])
-               usb_disconnect(&hub->children[port]);
-
-       /* Return now if nothing is connected */
-       if (!(portstatus & USB_PORT_STAT_CONNECTION)) {
-               if (portstatus & USB_PORT_STAT_ENABLE)
-                       hub_port_disable(hub, port);
-
-               return;
-       }
-
-       if (hub_port_debounce(hub, port)) {
-               dev_err (&hubstate->intf->dev,
-                       "connect-debounce failed, port %d disabled\n",
-                       port+1);
-               hub_port_disable(hub, port);
-               return;
-       }
+       int                     i, j, retval = -ENODEV;
+       unsigned                delay = HUB_SHORT_RESET_TIME;
+       enum usb_device_speed   oldspeed = dev->speed;
 
        /* root hub ports have a slightly longer reset period
         * (from USB 2.0 spec, section 7.1.7.5)
@@ -1046,31 +1029,53 @@
 
        /* Some low speed devices have problems with the quick delay, so */
        /*  be a bit pessimistic with those devices. RHbug #23670 */
-       if (portstatus & USB_PORT_STAT_LOW_SPEED)
+       if (oldspeed == USB_SPEED_LOW)
                delay = HUB_LONG_RESET_TIME;
 
        down(&usb_address0_sem);
 
-       for (i = 0; i < HUB_PROBE_TRIES; i++) {
-
-               /* Allocate a new device struct */
-               dev = usb_alloc_dev(hub, hub->bus, port);
-               if (!dev) {
-                       dev_err (&hubstate->intf->dev,
-                               "couldn't allocate usb_device\n");
-                       break;
-               }
-
-               dev->state = USB_STATE_POWERED;
-
-               /* Reset the device, and detect its speed */
-               if (hub_port_reset(hub, port, dev, delay)) {
-                       usb_put_dev(dev);
-                       break;
-               }
-
-               /* Find a new address for it */
+       /* Reset the device; full speed may morph to high speed */
+       switch (hub_port_reset(hub, port, dev, delay)) {
+       case 0:                 /* success, speed is known */
+               break;
+       case 1:                 /* disconnect, give to companion */
+               retval = -EBUSY;
+               /* FALL THROUGH */
+       default:                /* error */
+               goto fail;
+       }
+       if (oldspeed != USB_SPEED_UNKNOWN && oldspeed != dev->speed) {
+               dev_dbg(&dev->dev, "device reset changed speed!\n");
+               goto fail;
+       }
+  
+       /* USB 2.0 section 5.5.3 talks about ep0 maxpacket ...
+        * it's fixed size except for full speed devices.
+        */
+       switch (dev->speed) {
+       case USB_SPEED_HIGH:            /* fixed at 64 */
+               i = 64;
+               break;
+       case USB_SPEED_FULL:            /* 8, 16, 32, or 64 */
+               /* to determine the ep0 maxpacket size, read the first 8
+                * bytes from the device descriptor to get bMaxPacketSize0;
+                * then correct our initial (small) guess.
+                */
+               // FALLTHROUGH
+       case USB_SPEED_LOW:             /* fixed at 8 */
+               i = 8;
+               break;
+       default:
+               goto fail;
+       }
+       dev->epmaxpacketin [0] = i;
+       dev->epmaxpacketout[0] = i;
+ 
+       /* set the address */
+       if (dev->devnum <= 0) {
                usb_choose_address(dev);
+               if (dev->devnum <= 0)
+                       goto fail;
 
                /* Set up TT records, if needed  */
                if (hub->tt) {
@@ -1078,12 +1083,21 @@
                        dev->ttport = hub->ttport;
                } else if (dev->speed != USB_SPEED_HIGH
                                && hub->speed == USB_SPEED_HIGH) {
+                       struct usb_hub  *hubstate;
+ 
+                       hubstate = usb_get_intfdata (hub->actconfig
+                                                       ->interface[0]);
                        dev->tt = &hubstate->tt;
                        dev->ttport = port + 1;
                }
 
-               dev_info (&dev->dev,
-                       "new %s speed USB device using address %d\n",
+               /* force the right log message (below) at low speed */
+               oldspeed = USB_SPEED_UNKNOWN;
+       }
+ 
+       dev_info (&dev->dev,
+                       "%s %s speed USB device using address %d\n",
+                       (oldspeed == USB_SPEED_UNKNOWN) ? "new" : "reset",
                        ({ char *speed; switch (dev->speed) {
                        case USB_SPEED_LOW:     speed = "low";  break;
                        case USB_SPEED_FULL:    speed = "full"; break;
@@ -1091,23 +1105,155 @@
                        default:                speed = "?";    break;
                        }; speed;}),
                        dev->devnum);
-
-               /* Run it through the hoops (find a driver, etc) */
-               if (usb_new_device(dev) == 0) {
-                       hub->children[port] = dev;
-                       goto done;
+ 
+       /* Why interleave GET_DESCRIPTOR and SET_ADDRESS this way?
+        * Because device hardware and firmware is sometimes buggy in
+        * this area, and this is how Linux has done it for ages.
+        * Change it cautiously.
+        *
+        * NOTE:  Windows gets the descriptor first, seemingly to help
+        * work around device bugs like "can't use addresses with bit 3
+        * set in certain configurations".  Yes, really.
+        */
+       for (i = 0; i < GET_DESCRIPTOR_TRIES; ++i) {
+               for (j = 0; j < SET_ADDRESS_TRIES; ++j) {
+                       retval = usb_set_address(dev);
+                       if (retval >= 0)
+                               break;
+                       wait_ms(200);
+               }
+               if (retval < 0) {
+                       dev_err(&dev->dev,
+                               "device not accepting address %d, error %d\n",
+                               dev->devnum, retval);
+ fail:
+                       hub_port_disable(hub, port);
+                       clear_bit(dev->devnum, dev->bus->devmap.devicemap);
+                       dev->devnum = -1;
+                       usb_put_dev(dev);
+                       up(&usb_address0_sem);
+                       return retval;
                }
+ 
+               /* cope with hardware quirkiness:
+                *  - let SET_ADDRESS settle, some device hardware wants it
+                *  - read ep0 maxpacket even for high and low speed,
+                */
+               wait_ms(10);
+               retval = usb_get_device_descriptor(dev, 8);
+               if (retval >= 8)
+                       break;
+               wait_ms(100);
+       }
+       if (retval != 8) {
+               dev_err(&dev->dev, "device descriptor read/%s, error %d\n",
+                               "8", retval);
+               if (retval >= 0)
+                       retval = -EMSGSIZE;
+               goto fail;
+       }
+       if (dev->speed == USB_SPEED_FULL
+                       && (dev->epmaxpacketin [0]
+                               != dev->descriptor.bMaxPacketSize0)) {
+               usb_disable_endpoint(dev, 0);
+               usb_endpoint_running(dev, 0, 1);
+               usb_endpoint_running(dev, 0, 0);
+               dev->epmaxpacketin [0] = dev->descriptor.bMaxPacketSize0;
+               dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0;
+       }
+  
+       retval = usb_get_device_descriptor(dev, USB_DT_DEVICE_SIZE);
+       if (retval < (signed)sizeof(dev->descriptor)) {
+               dev_err(&dev->dev, "device descriptor read/%s, error %d\n",
+                       "all", retval);
+               if (retval >= 0)
+                       retval = -ENOMSG;
+               goto fail;
+       }
 
-               /* Free the configuration if there was an error */
-               usb_put_dev(dev);
+       /* now dev is visible to other tasks */
+       hub->children[port] = dev;
 
-               /* Switch to a long reset time */
-               delay = HUB_LONG_RESET_TIME;
+       up(&usb_address0_sem);
+       return 0;
+}
+
+static void hub_port_connect_change(struct usb_hub *hubstate, int port,
+                                       u16 portstatus, u16 portchange)
+{
+       struct usb_device *hub = interface_to_usbdev(hubstate->intf);
+       int status, i;
+ 
+       dev_dbg (&hubstate->intf->dev,
+               "port %d, status %04x, change %04x, %s\n",
+               port + 1, portstatus, portchange, portspeed (portstatus));
+ 
+       /* Clear the connection change status */
+       clear_port_feature(hub, port + 1, USB_PORT_FEAT_C_CONNECTION);
+
+       if (hubstate->has_indicators) {
+               set_port_led(hub, hubstate, port + 1, HUB_LED_AUTO);
+               hubstate->indicator[port] = INDICATOR_AUTO;
+       }
+ 
+       /* Disconnect any existing devices under this port */
+       if (hub->children[port])
+               usb_disconnect(&hub->children[port]);
+ 
+       /* Return now if nothing is connected */
+       if (!(portstatus & USB_PORT_STAT_CONNECTION)) {
+               if (portstatus & USB_PORT_STAT_ENABLE)
+                       goto done;
+               return;
+       }
+  
+       if (hub_port_debounce(hub, port)) {
+               dev_err (&hubstate->intf->dev,
+                       "connect-debounce failed, port %d disabled\n",
+                       port+1);
+               goto done;
        }
 
-       hub_port_disable(hub, port);
+       for (i = 0; i < SET_CONFIG_TRIES; i++) {
+               struct usb_device *dev;
+
+               /* reallocate for each attempt, since references
+                * to the previous one can escape in various ways
+                */
+               dev = usb_alloc_dev(hub, hub->bus, port);
+               if (!dev) {
+                       dev_err (&hubstate->intf->dev,
+                               "couldn't allocate port %d usb_device\n", port+1);
+                       goto done;
+               }
+               dev->state = USB_STATE_POWERED;
+         
+               /* hub can tell if it's lowspeed already:  D- pullup (not D+) */
+               if (portstatus & USB_PORT_STAT_LOW_SPEED)
+                       dev->speed = USB_SPEED_LOW;
+               else
+                       dev->speed = USB_SPEED_UNKNOWN;
+
+               /* reset, set address, get descriptor, add to hub's children */
+               down (&dev->serialize);
+               status = hub_port_init(hub, dev, port);
+               if (status == -EBUSY)
+                       break;
+               if (status < 0)
+                       continue;
+ 
+               /* Run it through the hoops (find a driver, etc) */
+               status = usb_new_device(dev);
+               if (status != 0) {
+                       hub->children[port] = NULL;
+                       continue;
+               }
+
+               up (&dev->serialize);
+               return;
+       }
 done:
-       up(&usb_address0_sem);
+       hub_port_disable(hub, port);
 }
 
 static void hub_events(void)
@@ -1285,9 +1431,6 @@
        .id_table =     hub_id_table,
 };
 
-/*
- * This should be a separate module.
- */
 int usb_hub_init(void)
 {
        pid_t pid;
--- 1.159/drivers/usb/core/usb.c        Wed Mar 31 06:17:00 2004
+++ edited/drivers/usb/core/usb.c       Wed Apr 21 10:46:29 2004
@@ -1064,92 +1064,29 @@
 }
 
 /*
- * By the time we get here, we chose a new device address
- * and is in the default state. We need to identify the thing and
- * get the ball rolling..
+ * usb_new_device - perform initial device setup (usbcore-internal)
+ * @dev: newly addressed device (in ADDRESS state)
  *
- * Returns 0 for success, != 0 for error.
+ * This is called with devices which have been enumerated, but not yet
+ * configured.  The device descriptor is available, but not descriptors
+ * for any device configuration.  The caller owns dev->serialize, and
+ * the device is not visible through sysfs or other filesystem code.
+ *
+ * Returns 0 for success (device is configured and listed, with its
+ * interfaces, in sysfs); else a negative errno value.  On error, one
+ * reference count to the device has been dropped.
  *
  * This call is synchronous, and may not be used in an interrupt context.
  *
  * Only the hub driver should ever call this; root hub registration
  * uses it only indirectly.
  */
-#define NEW_DEVICE_RETRYS      2
-#define SET_ADDRESS_RETRYS     2
 int usb_new_device(struct usb_device *dev)
 {
-       int err = -EINVAL;
+       int err;
        int i;
-       int j;
        int config;
 
-       /* USB 2.0 section 5.5.3 talks about ep0 maxpacket ...
-        * it's fixed size except for full speed devices.
-        */
-       switch (dev->speed) {
-       case USB_SPEED_HIGH:            /* fixed at 64 */
-               i = 64;
-               break;
-       case USB_SPEED_FULL:            /* 8, 16, 32, or 64 */
-               /* to determine the ep0 maxpacket size, read the first 8
-                * bytes from the device descriptor to get bMaxPacketSize0;
-                * then correct our initial (small) guess.
-                */
-               // FALLTHROUGH
-       case USB_SPEED_LOW:             /* fixed at 8 */
-               i = 8;
-               break;
-       default:
-               goto fail;
-       }
-       dev->epmaxpacketin [0] = i;
-       dev->epmaxpacketout[0] = i;
-
-       for (i = 0; i < NEW_DEVICE_RETRYS; ++i) {
-
-               for (j = 0; j < SET_ADDRESS_RETRYS; ++j) {
-                       err = usb_set_address(dev);
-                       if (err >= 0)
-                               break;
-                       wait_ms(200);
-               }
-               if (err < 0) {
-                       dev_err(&dev->dev,
-                               "device not accepting address %d, error %d\n",
-                               dev->devnum, err);
-                       goto fail;
-               }
-
-               wait_ms(10);    /* Let the SET_ADDRESS settle */
-
-               /* high and low speed devices don't need this... */
-               err = usb_get_device_descriptor(dev, 8);
-               if (err >= 8)
-                       break;
-               wait_ms(100);
-       }
-
-       if (err < 8) {
-               dev_err(&dev->dev, "device descriptor read/8, error %d\n", err);
-               goto fail;
-       }
-       if (dev->speed == USB_SPEED_FULL) {
-               usb_disable_endpoint(dev, 0);
-               usb_endpoint_running(dev, 0, 1);
-               usb_endpoint_running(dev, 0, 0);
-               dev->epmaxpacketin [0] = dev->descriptor.bMaxPacketSize0;
-               dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0;
-       }
-
-       /* USB device state == addressed ... still not usable */
-
-       err = usb_get_device_descriptor(dev, sizeof(dev->descriptor));
-       if (err != (signed)sizeof(dev->descriptor)) {
-               dev_err(&dev->dev, "device descriptor read/all, error %d\n", err);
-               goto fail;
-       }
-
        err = usb_get_configuration(dev);
        if (err < 0) {
                dev_err(&dev->dev, "can't read configurations, error %d\n",
@@ -1170,13 +1107,10 @@
                usb_show_string(dev, "SerialNumber", dev->descriptor.iSerialNumber);
 #endif
 
-       down(&dev->serialize);
-
        /* put device-specific files into sysfs */
        err = device_add (&dev->dev);
        if (err) {
                dev_err(&dev->dev, "can't device_add, error %d\n", err);
-               up(&dev->serialize);
                goto fail;
        }
        usb_create_driverfs_dev_files (dev);
@@ -1211,7 +1145,6 @@
                        dev->descriptor.bNumConfigurations);
        }
        err = usb_set_configuration(dev, config);
-       up(&dev->serialize);
        if (err) {
                dev_err(&dev->dev, "can't set config #%d, error %d\n",
                        config, err);
@@ -1226,9 +1159,10 @@
 
        return 0;
 fail:
-       dev->state = USB_STATE_DEFAULT;
+       dev->state = USB_STATE_NOTATTACHED;
        clear_bit(dev->devnum, dev->bus->devmap.devicemap);
        dev->devnum = -1;
+       usb_put_dev(dev);
        return err;
 }
 

Reply via email to