ChangeSet 1.1371.759.9, 2004/04/23 15:44:57-07:00, david-b@pacbell.net [PATCH] USB: re-factor enumeration logic 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. 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.) drivers/usb/core/hcd.c | 12 ++ drivers/usb/core/hub.c | 287 ++++++++++++++++++++++++++++++++++++------------- drivers/usb/core/usb.c | 92 ++------------- 3 files changed, 240 insertions(+), 151 deletions(-) diff -Nru a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c --- a/drivers/usb/core/hcd.c Fri May 14 15:34:09 2004 +++ b/drivers/usb/core/hcd.c Fri May 14 15:34:09 2004 @@ -779,10 +779,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); diff -Nru a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c --- a/drivers/usb/core/hub.c Fri May 14 15:34:09 2004 +++ b/drivers/usb/core/hub.c Fri May 14 15:34:09 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; diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c --- a/drivers/usb/core/usb.c Fri May 14 15:34:09 2004 +++ b/drivers/usb/core/usb.c Fri May 14 15:34:09 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; }