Hi Alan,

> I've just started looking in detail at your patch, and there is at least
> one error plus a couple of other things that perhaps could be cleaned up.

No surprise on either count!  Thanks for your careful review;
it's a big help when re-working mainline code like this.


> The error is in usb_alloc_dev. Instead of > > dev->dev.parent = parent->dev.parent; > > it should say > > dev->dev.parent = &parent->dev;

Yes, a bug!  I fixed it in the updated patch which I've attached;
it applies cleanly to Greg's current usb-devel-2.6 BK.

[ Split out patches will coming out too, in dribs and drabs. ]


> Otherwise everything ends up with the same parent! For that matter, does > it really make sense to have a parent pointer in struct usb_device when it > will always echo the parent pointer in the embedded struct device?

Not that I can tell, so long as there's a different cheap way to detect
root hubs.  That's another part of usbcore that hasn't yet replaced the
original homebrew solution by using the driver model:  if the parent is
really a usb_device, dev->parent == to_usb_device(dev->dev.parent).

Likely dev->children[] could be done away with at some point too.
That'd seem a bit trickier to change.


> In usb_new_device() the failure pathway does not call > usb_destroy_configuration(). It does change the device state incorrectly > and remove the device address bit (which it shouldn't -- the caller should > take care of that.)

Right, the state after faults should be ADDRESS.  Fixed in the attached
version.  The two callers are for root hub init, which doesn't need to
clear that bit; and hub_port_connect_change(), which should clear it
before freeing the device.

The new device logic is finally starting to look like it'll clean up
most of its litter ... :)


> hub_port_connect_change() doesn't acquire dev->serialize.


Not needed before usb_new_device(), surely -- nobody else can even see
the device, until it's published part way through usb_new_device().
I suspect the simplest fix is to just grab it early in usb_new_device(),
and drop it so that usb_set_configuration() can re-acquire that lock.

Maybe by the time we finish, usb_set_configuration() will rely on callers
to grab the lock ... easy to do through sysfs, and usbfs needs changes
in any case.  At this point I'm not even sure we should allow device
drivers to call usb_set_configuration ... sysfs is sufficient, or even
usbfs.


Hmm, the device isn't published in hub->children[] until after it's configured. That was a recent change Greg merged; it changed from one "half visible" device state to another one. (Now the driver model shows the device during new_device, but usb doesn't....) Of course if dev->children[] went away, or became hub-internal, that's be simplest.


> Is the SET_CONFIG_TRIES loop in hub_port_connect_change() really needed? > It was in the old code, and it probably was unnecessary there too, though > I can't be sure of that. There's no comment indicating why it should be > there, and hub_port_init() has its own retry loops.

I don't know that it's needed, and added a comment to that effect.

This loop acts differently though ... it includes config setting,
so it retries things the port initialization doesn't.  Some devices
might need it.  (Though I've never seen that loop run, and since
it was originally misnamed as a HUB_PROBE_TRIES, I wonder if maybe
it's always been some kind of misunderstanding.)


> Speaking of which, in hub_port_init() there's no reason to have > usb_set_address() called within the GET_DESCRIPTOR_TRIES loop. If it > needs a retry loop, it should have one of its own.

Hmm, not exactly.  I added a comment in this version ... device
hardware (!) and firmware sometimes has bugs in this area, and
I think that logic was a partial workaround for some of them.
Bugs at the level of "address 3 doesn't work" ... MS-Windows
and Linux don't act quite the same here (which hasn't been a
real issue, unlike with usb-storage and SCSI support).


> In hub_port_init(), the failure paths don't consistently set retval.


Now they should!


> Nowhere does the core call disable_endpoint for ep0 at address 0. I > suppose the host drivers are supposed to take care of that themselves when > they exit.

Actually that's one of the few remaining bits of automagic in the
interface to an HCD:  they're expected to morph that address's ep0
into a descriptor for the "real" address.  It's a UHCI-ism that
OHCI (and now EHCI) have had to cope with forever.

Removing that would be an excuse to clean up some minor cruft in
OHCI and EHCI.

- Dave



> Alan Stern
>
>
--- 1.75/drivers/usb/core/hcd.c Sun Dec  7 04:29:05 2003
+++ edited/drivers/usb/core/hcd.c       Sat Dec 27 11:45:48 2003
@@ -735,7 +735,7 @@
  *
  * The USB host controller calls this function to register the root hub
  * properly with the USB subsystem.  It sets up the device properly in
- * the driverfs tree, and then calls usb_new_device() to register the
+ * the device model tree, and then calls usb_new_device() to register the
  * usb device.  It also assigns the root hub's USB address (always 1).
  */
 int usb_register_root_hub (struct usb_device *usb_dev, struct device *parent_dev)
@@ -743,14 +743,22 @@
        const int devnum = 1;
        int retval;
 
-       sprintf (&usb_dev->dev.bus_id[0], "usb%d", usb_dev->bus->busnum);
-       usb_dev->state = USB_STATE_DEFAULT;
-
        usb_dev->devnum = devnum;
        usb_dev->bus->devnum_next = devnum + 1;
+       memset (&usb_dev->bus->devmap.devicemap, 0,
+                       sizeof usb_dev->bus->devmap.devicemap);
        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);
+       if (retval != sizeof usb_dev->descriptor) {
+               dev_err (parent_dev, "can't read %s device descriptor %d\n",
+                               usb_dev->dev.bus_id, retval);
+               return retval;
+       }
 
-       retval = usb_new_device (usb_dev, parent_dev);
+       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);
--- 1.34/drivers/usb/core/hcd.h Fri Aug 29 11:06:37 2003
+++ edited/drivers/usb/core/hcd.h       Sat Dec 27 11:45:48 2003
@@ -241,14 +241,12 @@
 /* -------------------------------------------------------------------------- */
 
 /* Enumeration is only for the hub driver, or HCD virtual root hubs */
-extern int usb_new_device(struct usb_device *dev, struct device *parent);
-extern void usb_choose_address(struct usb_device *dev);
+extern int usb_new_device(struct usb_device *dev);
 extern void usb_disconnect(struct usb_device **);
 
 /* exported to hub driver ONLY to support usb_reset_device () */
 extern int usb_get_configuration(struct usb_device *dev);
 extern void usb_destroy_configuration(struct usb_device *dev);
-extern int usb_set_address(struct usb_device *dev);
 
 /* use these only before the device's address has been set */
 #define usb_snddefctrl(dev)            ((PIPE_CONTROL << 30))
--- 1.83/drivers/usb/core/hub.c Tue Dec 16 10:29:53 2003
+++ edited/drivers/usb/core/hub.c       Sat Dec 27 11:45:48 2003
@@ -33,10 +33,10 @@
 
 #include "hcd.h"
 #include "hub.h"
+#include "usb.h"
 
 /* Wakes up khubd */
 static spinlock_t hub_event_lock = SPIN_LOCK_UNLOCKED;
-static DECLARE_MUTEX(usb_address0_sem);
 
 static LIST_HEAD(hub_event_list);      /* List of hubs needing servicing */
 static LIST_HEAD(hub_list);            /* List of all hubs (for cleanup) */
@@ -718,8 +718,43 @@
        return ret;
 }
 
-#define HUB_RESET_TRIES                5
-#define HUB_PROBE_TRIES                2
+static inline void usb_choose_address(struct usb_device *dev)
+{
+       struct usb_bus  *bus = dev->bus;
+       int             devnum;
+
+       /* Try to allocate the next devnum beginning at bus->devnum_next. */
+       devnum = find_next_zero_bit(bus->devmap.devicemap, 128, bus->devnum_next);
+       if (devnum >= 128)
+               devnum = find_next_zero_bit(bus->devmap.devicemap, 128, 1);
+       if (devnum >= 128)
+               return;
+
+       bus->devnum_next = (devnum == 127) ? 1 : devnum + 1;
+       set_bit(devnum, bus->devmap.devicemap);
+       dev->devnum = devnum;
+}
+
+static inline int usb_set_address(struct usb_device *dev)
+{
+       int retval;
+
+       if (dev->devnum <= 0)
+               return -EINVAL;
+       if (dev->state != USB_STATE_DEFAULT && dev->state != USB_STATE_ADDRESS)
+               return -EINVAL;
+       retval = usb_control_msg(dev, usb_snddefctrl(dev), USB_REQ_SET_ADDRESS,
+               0, dev->devnum, 0, NULL, 0, HZ * USB_CTRL_SET_TIMEOUT);
+       if (retval == 0)
+               dev->state = USB_STATE_ADDRESS;
+       return retval;
+}
+
+#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
@@ -784,7 +819,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 */
@@ -811,7 +846,7 @@
        return -1;
 }
 
-int hub_port_disable(struct usb_device *hub, int port)
+static int hub_port_disable(struct usb_device *hub, int port)
 {
        int ret;
 
@@ -880,40 +915,21 @@
        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.
+ *
+ * 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;
+       static DECLARE_MUTEX(usb_address0_sem);
 
-       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)
@@ -923,33 +939,47 @@
 
        /* 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++) {
-               struct usb_device *pdev;
-               int     len;
-
-               /* Allocate a new device struct */
-               dev = usb_alloc_dev(hub, hub->bus);
-               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 */
+       retval = hub_port_reset(hub, port, dev, delay);
+       if (retval < 0)
+               goto fail;
+       if (oldspeed != USB_SPEED_UNKNOWN && oldspeed != dev->speed) {
+               dev_warn(&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) {
@@ -957,56 +987,167 @@
                        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;
                }
 
-               /* Save readable and stable topology id, distinguishing
-                * devices by location for diagnostics, tools, etc.  The
-                * string is a path along hub ports, from the root.  Each
-                * device's id will be stable until USB is re-cabled, and
-                * hubs are often labeled with these port numbers.
-                *
-                * Initial size: ".NN" times five hubs + NUL = 16 bytes max
-                * (quite rare, since most hubs have 4-6 ports).
-                */
-               pdev = dev->parent;
-               if (pdev->devpath [0] != '0')   /* parent not root? */
-                       len = snprintf (dev->devpath, sizeof dev->devpath,
-                               "%s.%d", pdev->devpath, port + 1);
-               /* root == "0", root port 2 == "2", port 3 that hub "2.3" */
-               else
-                       len = snprintf (dev->devpath, sizeof dev->devpath,
-                               "%d", port + 1);
-               if (len == sizeof dev->devpath)
-                       dev_err (&hubstate->intf->dev,
-                               "devpath size! usb/%03d/%03d path %s\n",
-                               dev->bus->busnum, dev->devnum, dev->devpath);
-               dev_info (&hubstate->intf->dev,
-                       "new USB device on port %d, assigned address %d\n",
-                       port + 1, dev->devnum);
-
-               /* put the device in the global device tree. the hub port
-                * is the "bus_id"; hubs show in hierarchy like bridges
-                */
-               dev->dev.parent = dev->parent->dev.parent->parent;
-
-               /* Run it through the hoops (find a driver, etc) */
-               if (!usb_new_device(dev, &hub->dev)) {
-                       hub->children[port] = dev;
-                       goto done;
+               /* force the right log message (below) at low speed */
+               oldspeed = USB_SPEED_UNKNOWN;
+       }
+ 
+       dev_info (&dev->dev,
+               "%s USB %s speed 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;
+               case USB_SPEED_HIGH:    speed = "high"; break;
+               default:                speed = "?";    break;
+               }; speed;}),
+               dev->devnum);
+ 
+       /* 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);
                }
-
-               /* Free the configuration if there was an error */
-               usb_put_dev(dev);
-
-               /* Switch to a long reset time */
-               delay = HUB_LONG_RESET_TIME;
+               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;
+                       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,
+                *
+                * FIXME assumes device->descriptor cacheline is dma-coherent
+                * and so does usb_get_device_descriptor
+                */
+               wait_ms(10);
+               retval = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
+                               &dev->descriptor, 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);
+       if (retval < (signed)sizeof(dev->descriptor)) {
+               dev_err(&dev->dev, "device descriptor read/%s, error %d\n",
+                       "all", retval);
+               goto fail;
+       }
+  
+       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);
+       struct usb_device *dev;
+       int status, 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)
+                       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;
+       }
+  
+       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;
+ 
+       /* XXX it's not clear why we'd need to loop here */
+       for (i = 0; i < SET_CONFIG_TRIES; i++) {
+               status = hub_port_init(hub, dev, port);
+               if (status < 0)
+                       continue;
+ 
+               /* Run it through the hoops (find a driver, etc) */
+               if (usb_new_device(dev) != 0)
+                       continue;
 
-       hub_port_disable(hub, port);
+               /* FIXME the device is visible in sysfs before now, so
+                * this delay just creates a funky state ...
+                */
+               hub->children[port] = dev;
+               return;
+       }
+ 
+       if (dev->devnum != -1)
+               clear_bit(dev->devnum, dev->bus->devmap.devicemap);
+       usb_put_dev(dev);
 done:
-       up(&usb_address0_sem);
+       hub_port_disable(hub, port);
 }
 
 static void hub_events(void)
@@ -1229,25 +1370,65 @@
        usb_deregister(&hub_driver);
 } /* usb_hub_cleanup() */
 
-/*
- * WARNING - If a driver calls usb_reset_device, you should simulate a
- * disconnect() and probe() for other interfaces you doesn't claim. This
- * is left up to the driver writer right now. This insures other drivers
- * have a chance to re-setup their interface.
+
+static int config_descriptors_changed(struct usb_device *dev)
+{
+       unsigned index;
+       char *buf;
+       unsigned len = 0;
+
+       for (index = 0; index < dev->descriptor.bNumConfigurations; index++) {
+               if (len < dev->config[index].desc.wTotalLength)
+                       len = dev->config[index].desc.wTotalLength;
+       }
+       buf = kmalloc (len, SLAB_KERNEL);
+       if (buf == 0) {
+               dev_err(&dev->dev, "can't re-read configs after reset\n");
+               /* assume the worst */
+               return 1;
+       }
+       for (index = 0; index < dev->descriptor.bNumConfigurations; index++) {
+               int length;
+
+               length = usb_get_descriptor(dev, USB_DT_CONFIG, index, buf, len);
+               if (length < sizeof (struct usb_config_descriptor)) {
+                       dev_dbg(&dev->dev, "config index %d, error %d\n",
+                                       index, length);
+                       break;
+               }
+               if (memcmp (buf, dev->rawdescriptors[index], length) != 0) {
+                       dev_dbg(&dev->dev, "config index %d changed\n", index);
+/* FIXME enable this when we can re-enumerate after reset;
+ * until then DFU-ish drivers need this and other workarounds
+ */
+//                     break;
+               }
+       }
+       kfree(buf);
+       return (index < dev->descriptor.bNumConfigurations) ? 1 : 0;
+}
+
+/**
+ * __usb_reset_device - unlocked version of usb_reset_device
+ * @dev: the usb device to be reset
  *
- * Take a look at proc_resetdevice in devio.c for some sample code to
- * do this.
- * Use this only from within your probe function, otherwise use
- * usb_reset_device() below, which ensure proper locking
+ * This is the same as usb_reset_device() except that the caller
+ * already holds @dev->serialize.  For example, it's safe to use
+ * this from a driver probe() routine after downloading new firmware.
  */
-int usb_physical_reset_device(struct usb_device *dev)
+int __usb_reset_device(struct usb_device *dev)
 {
        struct usb_device *parent = dev->parent;
-       struct usb_device_descriptor *descriptor;
+       struct usb_device_descriptor descriptor = dev->descriptor;
        int i, ret, port = -1;
 
+       /*
+        * WARNING - don't reset any device unless drivers for all
+        * its interfaces are expecting that reset!
+        */
+
        if (!parent) {
-               err("attempting to reset root hub!");
+               dev_dbg(&dev->dev, "attempting to reset root hub!\n");
                return -EINVAL;
        }
 
@@ -1260,132 +1441,69 @@
        if (port < 0)
                return -ENOENT;
 
-       descriptor = kmalloc(sizeof *descriptor, GFP_NOIO);
-       if (!descriptor) {
-               return -ENOMEM;
-       }
-
-       down(&usb_address0_sem);
-
-       /* Send a reset to the device */
-       if (hub_port_reset(parent, port, dev, HUB_SHORT_RESET_TIME)) {
-               hub_port_disable(parent, port);
-               up(&usb_address0_sem);
-               kfree(descriptor);
-               return(-ENODEV);
-       }
-
-       /* Reprogram the Address */
-       ret = usb_set_address(dev);
-       if (ret < 0) {
-               err("USB device not accepting new address (error=%d)", ret);
-               hub_port_disable(parent, port);
-               up(&usb_address0_sem);
-               kfree(descriptor);
-               return ret;
-       }
-
-       /* Let the SET_ADDRESS settle */
-       wait_ms(10);
-
-       up(&usb_address0_sem);
-
-       /*
-        * Now we fetch the configuration descriptors for the device and
-        * see if anything has changed. If it has, we dump the current
-        * parsed descriptors and reparse from scratch. Then we leave
-        * the device alone for the caller to finish setting up.
-        *
-        * If nothing changed, we reprogram the configuration and then
-        * the alternate settings.
-        */
-
-       ret = usb_get_descriptor(dev, USB_DT_DEVICE, 0, descriptor,
-                       sizeof(*descriptor));
-       if (ret < 0) {
-               kfree(descriptor);
-               return ret;
-       }
-
-       le16_to_cpus(&descriptor->bcdUSB);
-       le16_to_cpus(&descriptor->idVendor);
-       le16_to_cpus(&descriptor->idProduct);
-       le16_to_cpus(&descriptor->bcdDevice);
-
-       if (memcmp(&dev->descriptor, descriptor, sizeof(*descriptor))) {
-               kfree(descriptor);
-               usb_destroy_configuration(dev);
-
-               ret = usb_get_device_descriptor(dev);
-               if (ret < sizeof(dev->descriptor)) {
-                       if (ret < 0)
-                               err("unable to get device %s descriptor "
-                                       "(error=%d)", dev->devpath, ret);
-                       else
-                               err("USB device %s descriptor short read "
-                                       "(expected %Zi, got %i)",
-                                       dev->devpath,
-                                       sizeof(dev->descriptor), ret);
-
-                       clear_bit(dev->devnum, dev->bus->devmap.devicemap);
-                       dev->devnum = -1;
-                       return -EIO;
-               }
-
-               ret = usb_get_configuration(dev);
-               if (ret < 0) {
-                       err("unable to get configuration (error=%d)", ret);
-                       usb_destroy_configuration(dev);
-                       clear_bit(dev->devnum, dev->bus->devmap.devicemap);
-                       dev->devnum = -1;
-                       return 1;
-               }
-
-               usb_set_configuration(dev, dev->config[0].desc.bConfigurationValue);
-               return 1;
-       }
-
-       kfree(descriptor);
+       ret = hub_port_init(parent, dev, port);
+       if (ret < 0)
+               goto re_enumerate;
+ 
+       /* Device might have changed firmware (DFU or similar) */
+       if (memcmp(&dev->descriptor, &descriptor, sizeof descriptor)
+                       || config_descriptors_changed (dev)) {
+               dev_info(&dev->dev, "device firmware changed\n");
+               goto re_enumerate;
+       }
+  
+       if (!dev->actconfig)
+               return 0;
 
        ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
                        USB_REQ_SET_CONFIGURATION, 0,
                        dev->actconfig->desc.bConfigurationValue, 0,
                        NULL, 0, HZ * USB_CTRL_SET_TIMEOUT);
        if (ret < 0) {
-               err("failed to set dev %s active configuration (error=%d)",
-                       dev->devpath, ret);
-               return ret;
-       }
+               dev_err(&dev->dev, "can't restore configuration (error=%d)\n",
+                       ret);
+               goto re_enumerate;
+       }
        dev->state = USB_STATE_CONFIGURED;
 
        for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
                struct usb_interface *intf = dev->actconfig->interface[i];
                struct usb_interface_descriptor *as;
 
+               /* set_interface is needed to reset host side toggle and
+                * halt status even for altsetting zero
+                */
                as = &intf->altsetting[intf->act_altsetting].desc;
                ret = usb_set_interface(dev, as->bInterfaceNumber,
                        as->bAlternateSetting);
                if (ret < 0) {
-                       err("failed to set active alternate setting "
-                               "for dev %s interface %d (error=%d)",
-                               dev->devpath, i, ret);
-                       return ret;
+                       dev_err(&dev->dev, "failed to restore interface %d "
+                               "altsetting %d (error=%d)\n",
+                               as->bInterfaceNumber, as->bAlternateSetting,
+                               ret);
+                       goto re_enumerate;
                }
        }
 
        return 0;
+ 
+re_enumerate:
+       /* FIXME make some task re-enumerate; don't just mark unusable */
+       dev->state = USB_STATE_NOTATTACHED;
+       return -ENODEV;
 }
+EXPORT_SYMBOL(__usb_reset_device);
 
 int usb_reset_device(struct usb_device *udev)
 {
-       struct device *gdev = &udev->dev;
        int r;
        
-       down_read(&gdev->bus->subsys.rwsem);
-       r = usb_physical_reset_device(udev);
-       up_read(&gdev->bus->subsys.rwsem);
+       down(&udev->serialize);
+       r = __usb_reset_device(udev);
+       up(&udev->serialize);
 
        return r;
 }
+EXPORT_SYMBOL(usb_reset_device);
 
 
--- 1.147/drivers/usb/core/usb.c        Tue Dec 16 10:33:47 2003
+++ edited/drivers/usb/core/usb.c       Sat Dec 27 11:45:48 2003
@@ -672,17 +672,19 @@
 }
 
 /**
- * usb_alloc_dev - allocate a usb device structure (usbcore-internal)
- * @parent: hub to which device is connected
+ * usb_alloc_dev - usb device constructor (usbcore-internal)
+ * @parent: hub to which device is connected; null to allocate a root hub
  * @bus: bus used to access the device
+ * @port: zero based index of port; ignored for root hubs
  * Context: !in_interrupt ()
  *
  * Only hub drivers (including virtual root hub drivers for host
  * controllers) should ever call this.
  *
- * This call is synchronous, and may not be used in an interrupt context.
+ * This call may not be used in a non-sleeping context.
  */
-struct usb_device *usb_alloc_dev(struct usb_device *parent, struct usb_bus *bus)
+struct usb_device *
+usb_alloc_dev(struct usb_device *parent, struct usb_bus *bus, int port)
 {
        struct usb_device *dev;
 
@@ -699,11 +701,42 @@
        }
 
        device_initialize(&dev->dev);
+       dev->dev.bus = &usb_bus_type;
+       dev->dev.dma_mask = bus->controller->dma_mask;
+       dev->dev.driver_data = &usb_generic_driver_data;
+       dev->dev.driver = &usb_generic_driver;
        dev->dev.release = usb_release_dev;
-       dev->state = USB_STATE_ATTACHED;
 
-       if (!parent)
+       /* Save readable and stable topology id, distinguishing devices
+        * by location for diagnostics, tools, driver model, etc.  The
+        * string is a path along hub ports, from the root.  Each device's
+        * dev->devpath will be stable until USB is re-cabled, and hubs
+        * are often labeled with these port numbers.  The bus_id isn't
+        * as stable, since bus->busnum changes easily from modprobe order,
+        * cardbus or pci hotplugging, and so on.
+        */
+       if (!parent) {
                dev->devpath [0] = '0';
+
+               dev->dev.parent = bus->controller;
+               sprintf (&dev->dev.bus_id[0], "usb%d", bus->busnum);
+       } else {
+               /* match any labeling on the hubs; it's one-based */
+               if (parent->devpath [0] == '0')
+                       snprintf (dev->devpath, sizeof dev->devpath,
+                               "%d", port + 1);
+               else
+                       snprintf (dev->devpath, sizeof dev->devpath,
+                               "%s.%d", parent->devpath, port + 1);
+
+               dev->dev.parent = &parent->dev;
+               sprintf (&dev->dev.bus_id[0], "%d-%s",
+                       bus->busnum, dev->devpath);
+
+               /* hub driver sets up TT records */
+       }
+
+       dev->state = USB_STATE_ATTACHED;
        dev->bus = bus;
        dev->parent = parent;
        INIT_LIST_HEAD(&dev->filelist);
@@ -930,58 +963,11 @@
        device_unregister(&dev->dev);
 }
 
-/**
- * usb_choose_address - pick device address (usbcore-internal)
- * @dev: newly detected device (in DEFAULT state)
- *
- * Picks a device address.  It's up to the hub (or root hub) driver
- * to handle and manage enumeration, starting from the DEFAULT state.
- * Only hub drivers (but not virtual root hub drivers for host
- * controllers) should ever call this.
- */
-void usb_choose_address(struct usb_device *dev)
-{
-       int devnum;
-       // FIXME needs locking for SMP!!
-       /* why? this is called only from the hub thread, 
-        * which hopefully doesn't run on multiple CPU's simultaneously 8-)
-        */
-
-       /* Try to allocate the next devnum beginning at bus->devnum_next. */
-       devnum = find_next_zero_bit(dev->bus->devmap.devicemap, 128, 
dev->bus->devnum_next);
-       if (devnum >= 128)
-               devnum = find_next_zero_bit(dev->bus->devmap.devicemap, 128, 1);
-
-       dev->bus->devnum_next = ( devnum >= 127 ? 1 : devnum + 1);
-
-       if (devnum < 128) {
-               set_bit(devnum, dev->bus->devmap.devicemap);
-               dev->devnum = devnum;
-       }
-}
-
-
-// hub-only!! ... and only exported for reset/reinit path.
-// otherwise used internally, for usb_new_device()
-int usb_set_address(struct usb_device *dev)
-{
-       int retval;
-
-       if (dev->devnum == 0)
-               return -EINVAL;
-       if (dev->state != USB_STATE_DEFAULT && dev->state != USB_STATE_ADDRESS)
-               return -EINVAL;
-       retval = usb_control_msg(dev, usb_snddefctrl(dev), USB_REQ_SET_ADDRESS,
-               0, dev->devnum, 0, NULL, 0, HZ * USB_CTRL_SET_TIMEOUT);
-       if (retval == 0)
-               dev->state = USB_STATE_ADDRESS;
-       return retval;
-}
 
 /*
- * 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..
+ * By the time we get here, the device is enumerated, but not yet
+ * configured:  USB_STATE_ADDRESS, and we have its device descriptor.
+ * On a successful return, the device is configured.
  *
  * Returns 0 for success, != 0 for error.
  *
@@ -990,101 +976,12 @@
  * 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, struct device *parent)
+int usb_new_device(struct usb_device *dev)
 {
-       int err = -EINVAL;
+       int err;
        int i;
-       int j;
        int config;
 
-       /*
-        * Set the driver for the usb device to point to the "generic" driver.
-        * This prevents the main usb device from being sent to the usb bus
-        * probe function.  Yes, it's a hack, but a nice one :)
-        *
-        * Do it asap, so more driver model stuff (like the device.h message
-        * utilities) can be used in hcd submit/unlink code paths.
-        */
-       usb_generic_driver.bus = &usb_bus_type;
-       dev->dev.parent = parent;
-       dev->dev.driver = &usb_generic_driver;
-       dev->dev.bus = &usb_bus_type;
-       dev->dev.driver_data = &usb_generic_driver_data;
-       if (dev->dev.bus_id[0] == 0)
-               sprintf (&dev->dev.bus_id[0], "%d-%s",
-                        dev->bus->busnum, dev->devpath);
-
-       /* dma masks come from the controller; readonly, except to hcd */
-       dev->dev.dma_mask = parent->dma_mask;
-
-       /* 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_descriptor(dev, USB_DT_DEVICE, 0, &dev->descriptor, 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);
-       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",
@@ -1105,6 +1002,10 @@
                usb_show_string(dev, "SerialNumber", dev->descriptor.iSerialNumber);
 #endif
 
+       /* FIXME dev->serialize should probably be held from
+        * here until the configuration is assigned.
+        */
+
        /* put device-specific files into sysfs */
        err = device_add (&dev->dev);
        if (err) {
@@ -1150,9 +1051,7 @@
 
        return 0;
 fail:
-       dev->state = USB_STATE_DEFAULT;
-       clear_bit(dev->devnum, dev->bus->devmap.devicemap);
-       dev->devnum = -1;
+       usb_destroy_configuration (dev);
        return err;
 }
 
@@ -1544,7 +1443,6 @@
 EXPORT_SYMBOL(usb_find_interface);
 EXPORT_SYMBOL(usb_ifnum_to_if);
 
-EXPORT_SYMBOL(usb_reset_device);
 EXPORT_SYMBOL(usb_disconnect);
 
 EXPORT_SYMBOL(__usb_get_extra_descriptor);
--- 1.61/drivers/usb/host/ehci-hcd.c    Mon Nov  3 05:06:51 2003
+++ edited/drivers/usb/host/ehci-hcd.c  Sat Dec 27 11:45:48 2003
@@ -468,7 +468,7 @@
 
        /* wire up the root hub */
        bus = hcd_to_bus (hcd);
-       bus->root_hub = udev = usb_alloc_dev (NULL, bus);
+       bus->root_hub = udev = usb_alloc_dev (NULL, bus, 0);
        if (!udev) {
 done2:
                ehci_mem_cleanup (ehci);
--- 1.6/drivers/usb/host/hc_sl811_rh.c  Fri Jul 25 06:48:06 2003
+++ edited/drivers/usb/host/hc_sl811_rh.c       Sat Dec 27 11:45:48 2003
@@ -559,7 +559,7 @@
        struct usb_device *usb_dev;
 
        hci->rh.devnum = 0;
-       usb_dev = usb_alloc_dev (NULL, hci->bus);
+       usb_dev = usb_alloc_dev (NULL, hci->bus, 0);
        if (!usb_dev)
                return -ENOMEM;
 
--- 1.50/drivers/usb/host/ohci-hcd.c    Wed Dec  3 11:35:19 2003
+++ edited/drivers/usb/host/ohci-hcd.c  Sat Dec 27 11:45:48 2003
@@ -519,7 +519,7 @@
  
        /* connect the virtual root hub */
        bus = hcd_to_bus (&ohci->hcd);
-       bus->root_hub = udev = usb_alloc_dev (NULL, bus);
+       bus->root_hub = udev = usb_alloc_dev (NULL, bus, 0);
        ohci->hcd.state = USB_STATE_RUNNING;
        if (!udev) {
                disable (ohci);
--- 1.47/drivers/usb/host/uhci-hcd.c    Thu Oct  9 03:20:04 2003
+++ edited/drivers/usb/host/uhci-hcd.c  Sat Dec 27 11:45:48 2003
@@ -2303,7 +2303,7 @@
 
        uhci->rh_numports = port;
 
-       hcd->self.root_hub = udev = usb_alloc_dev(NULL, &hcd->self);
+       hcd->self.root_hub = udev = usb_alloc_dev(NULL, &hcd->self, 0);
        if (!udev) {
                err("unable to allocate root hub");
                goto err_alloc_root_hub;
--- 1.92/include/linux/usb.h    Mon Dec  8 09:39:26 2003
+++ edited/include/linux/usb.h  Sat Dec 27 11:45:48 2003
@@ -274,12 +274,16 @@
 };
 #define        to_usb_device(d) container_of(d, struct usb_device, dev)
 
-extern struct usb_device *usb_alloc_dev(struct usb_device *parent, struct usb_bus *);
+extern struct usb_device *
+usb_alloc_dev(struct usb_device *parent, struct usb_bus *, int port);
 extern struct usb_device *usb_get_dev(struct usb_device *dev);
 extern void usb_put_dev(struct usb_device *dev);
 
 /* mostly for devices emulating SCSI over USB */
 extern int usb_reset_device(struct usb_device *dev);
+
+/* unlocked version; use during probe() after firmware update */
+extern int __usb_reset_device(struct usb_device *dev);
 
 extern struct usb_device *usb_find_device(u16 vendor_id, u16 product_id);
 

Reply via email to