Please, I would feel much more comfortable accepting this if it was split out into logical chunks.
OK here's a third chunk, depending on
- moving more init logic to usb_alloc_dev (reset-1230.patch) - an unlocked usb_set_configuration (setconf.patch)
This patch
* Moves the first half of the usb_new_device() logic into a new hub_port_init() routine, which will be shared between enumeration (khubd) and usb_reset_device().
* Khubd uses that new code in hub_port_connect_change().
* Now usb_new_device() cleans up better after faults.
* Has related minor cleanups including commenting some of the curious request sequences coming from khubd.
Refactoring in that way gets rid of address0_sem deadlocks from probe routines that download firmware need to reset the device, or when a usb-storage device somehow triggers a bus reset during that probe (I saw that once).
Please merge.
- Dave
--- 1.76/drivers/usb/core/hcd.c Sat Jan 3 16:18:59 2004
+++ edited/drivers/usb/core/hcd.c Sun Jan 4 10:58:02 2004
@@ -750,6 +750,14 @@
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_dbg (parent_dev, "can't read %s device descriptor %d\n",
+ usb_dev->dev.bus_id, retval);
+ return (retval < 0) ? retval : -EMSGSIZE;
+ }
+
retval = usb_new_device (usb_dev);
if (retval)
dev_err (parent_dev, "can't register root hub for %s, %d\n",
--- 1.84/drivers/usb/core/hub.c Sat Jan 3 16:18:59 2004
+++ edited/drivers/usb/core/hub.c Sun Jan 4 10:58:02 2004
@@ -33,6 +33,8 @@
#include "hcd.h"
#include "hub.h"
+#include "usb.h"
+
/* Wakes up khubd */
static spinlock_t hub_event_lock = SPIN_LOCK_UNLOCKED;
@@ -718,8 +720,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
@@ -784,7 +789,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 +816,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 +885,19 @@
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;
-
- 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,31 +907,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++) {
-
- /* 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 */
+ retval = hub_port_reset(hub, port, dev, delay);
+ if (retval < 0)
+ 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) {
@@ -955,12 +955,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;
@@ -968,23 +977,149 @@
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);
}
-
- /* 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;
+ down (&dev->serialize);
+
+ /* 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;
- hub_port_disable(hub, port);
+
+ 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) */
+ status = usb_new_device(dev);
+ if (status != 0)
+ continue;
+
+ /* FIXME the device was visible in sysfs before now;
+ * this delay just created a brief funky state ...
+ */
+ hub->children[port] = dev;
+ up (&dev->serialize);
+ return;
+ }
+ up (&dev->serialize);
+
+ 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)
--- 1.149/drivers/usb/core/usb.c Sat Jan 3 16:18:59 2004
+++ edited/drivers/usb/core/usb.c Sun Jan 4 10:58:02 2004
@@ -1011,93 +1011,31 @@
return retval;
}
+extern int __usb_set_configuration(struct usb_device *, int);
+
/*
- * 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)
+ *
+ * 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, != 0 for error.
+ * Returns 0 for success (device is configured and listed, with its
+ * interfaces, in sysfs); else a negative errno value.
*
* 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_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",
@@ -1148,7 +1086,7 @@
config,
dev->descriptor.bNumConfigurations);
}
- err = usb_set_configuration(dev, config);
+ err = __usb_set_configuration(dev, config);
if (err) {
dev_err(&dev->dev, "can't set config #%d, error %d\n",
config, err);
@@ -1163,9 +1101,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;
}
