[PATCH] USB: usb_set_configuration() rework (v2)

This is the latest update of the patch resolving bugs in how device
configurations were reflected in the driver model.  It addresses
the last significant problems I know about in that area.

    - Moves code around so that usb_set_configuration() updates sysfs
      to reflect the current configuration.  Previously, that only
      worked right for the initial configuration chosen by khubd.

        * Previous interfaces are inaccessible.  The code to handle this
          moved from usb_disconnect() into usb_disable_device(), which
          is now called both on disconnect and set_configuration paths.

        * There are new interfaces.  The code to handle this moved
          from usb_new_device() into usb_set_configuration().

        * Resolves a double-refcount problem with USB interfaces,
          by not getting the extra reference in the first place
          and switching to use device_del() to disable interfaces.

        * Comments a similar double-refcount problem with usb
          devices (not interfaces).

      Its kerneldoc is updated appropriately.  The main point being
      that calling usb_set_configuration() in driver probe() methods
      is even more of a no-no, since it'll self-deadlock.

    - Sysfs names for USB interfaces now include the configuation
      number, so that user mode code can't get as easily confused.

      Old style:  "3-1:0" for configs 2 and 3 (interface zero).
      New style:  "3-1:2.0" for config 2, "3-3:3.0" for config 3.

    - Moves usb_new_device() code around a bit, so that the device
      is visible in sysfs before usb_set_configuration() is called.
      (Before the devices for that config's interfaces appear.)

    - Makes the bConfigurationValue be writable through sysfs, so
      device configurations can be easily changed from user mode.
      (Or devices can be de-configured, by setting config 0.)

      There are devices that can benefit from this functionality;
      notably, cdc-acm modems need it right now, so that they can
      be told to use the non-proprietary configuration.  (Since
      the old "change config in probe callback" trick won't work.)

drivers/usb/core/config.c

@@ -237,9 +237,6 @@ int usb_parse_configuration(struct usb_host_config *config, char *buffer, int si
 		memset(interface, 0, sizeof(struct usb_interface));
 		interface->dev.release = usb_release_intf;
 		device_initialize(&interface->dev);
-
-		/* put happens in usb_destroy_configuration */
-		get_device(&interface->dev);
 	}
 
 	/* Go through the descriptors, checking their length and counting the

drivers/usb/core/driverfs.c

@@ -23,22 +23,44 @@
 #include "usb.h"
 
 /* Active configuration fields */
-#define usb_actconfig_attr(field, format_string)			\
+#define usb_actconfig_show(field, format_string)			\
 static ssize_t								\
 show_##field (struct device *dev, char *buf)				\
 {									\
 	struct usb_device *udev;					\
 									\
 	udev = to_usb_device (dev);					\
+	if (udev->actconfig) \
 	return sprintf (buf, format_string, udev->actconfig->desc.field); \
+	else return 0;							\
 }									\
+
+#define usb_actconfig_attr(field, format_string)			\
+usb_actconfig_show(field,format_string)					\
 static DEVICE_ATTR(field, S_IRUGO, show_##field, NULL);
 
 usb_actconfig_attr (bNumInterfaces, "%2d\n")
-usb_actconfig_attr (bConfigurationValue, "%2d\n")
 usb_actconfig_attr (bmAttributes, "%2x\n")
 usb_actconfig_attr (bMaxPower, "%3dmA\n")
 
+/* configuration value is always present, and r/w */
+usb_actconfig_show(bConfigurationValue,"%u\n");
+
+static ssize_t
+set_bConfigurationValue (struct device *dev, const char *buf, size_t count)
+{
+	struct usb_device	*udev = udev = to_usb_device (dev);
+	int			config, value;
+
+	if (sscanf (buf, "%u", &config) != 1 || config > 255)
+		return -EINVAL;
+	value = usb_set_configuration (udev, config);
+	return (value < 0) ? value : count;
+}
+
+static DEVICE_ATTR(bConfigurationValue, S_IRUGO | S_IWUSR, 
+		show_bConfigurationValue, set_bConfigurationValue);
+
 /* String fields */
 static ssize_t show_product (struct device *dev, char *buf)
 {

drivers/usb/core/message.c

@@ -781,18 +781,40 @@ void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf)
  * @skip_ep0: 0 to disable endpoint 0, 1 to skip it.
  *
  * Disables all the device's endpoints, potentially including endpoint 0.
- * Deallocates hcd/hardware state for the endpoints ... and nukes all
- * pending urbs.
+ * Deallocates hcd/hardware state for the endpoints (nuking all or most
+ * pending urbs) and usbcore state for the interfaces, so that usbcore
+ * must usb_set_configuration() before any interfaces could be used.
  */
 void usb_disable_device(struct usb_device *dev, int skip_ep0)
 {
 	int i;
 
-	dbg("nuking URBs for device %s", dev->dev.bus_id);
+	dev_dbg(&dev->dev, "%s nuking %s URBs\n", __FUNCTION__,
+			skip_ep0 ? "non-ep0" : "all");
 	for (i = skip_ep0; i < 16; ++i) {
 		usb_disable_endpoint(dev, i);
 		usb_disable_endpoint(dev, i + USB_DIR_IN);
 	}
+	dev->toggle[0] = dev->toggle[1] = 0;
+	dev->halted[0] = dev->halted[1] = 0;
+
+	/* getting rid of interfaces will disconnect
+	 * any drivers bound to them (a key side effect)
+	 */
+	if (dev->actconfig) {
+		for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
+			struct usb_interface	*interface;
+
+			/* remove this interface */
+			interface = dev->actconfig->interface[i];
+			dev_dbg (&dev->dev, "unregistering interface %s\n",
+				interface->dev.bus_id);
+			device_del(&interface->dev);
+		}
+		dev->actconfig = 0;
+		if (dev->state == USB_STATE_CONFIGURED)
+			dev->state = USB_STATE_ADDRESS;
+	}
 }
 
 
@@ -979,6 +1001,9 @@ int usb_reset_configuration(struct usb_device *dev)
 	int			i, retval;
 	struct usb_host_config	*config;
 
+	/* dev->serialize guards all config changes */
+	down(&dev->serialize);
+
 	for (i = 1; i < 16; ++i) {
 		usb_disable_endpoint(dev, i);
 		usb_disable_endpoint(dev, i + USB_DIR_IN);
@@ -989,8 +1014,10 @@ int usb_reset_configuration(struct usb_device *dev)
 			USB_REQ_SET_CONFIGURATION, 0,
 			config->desc.bConfigurationValue, 0,
 			NULL, 0, HZ * USB_CTRL_SET_TIMEOUT);
-	if (retval < 0)
-		return retval;
+	if (retval < 0) {
+		dev->state = USB_STATE_ADDRESS;
+		goto done;
+	}
 
 	dev->toggle[0] = dev->toggle[1] = 0;
 	dev->halted[0] = dev->halted[1] = 0;
@@ -1002,7 +1029,9 @@ int usb_reset_configuration(struct usb_device *dev)
 		intf->act_altsetting = 0;
 		usb_enable_interface(dev, intf);
 	}
-	return 0;
+done:
+	up(&dev->serialize);
+	return (retval < 0) ? retval : 0;
 }
 
 /**
@@ -1012,72 +1041,105 @@ int usb_reset_configuration(struct usb_device *dev)
  * Context: !in_interrupt ()
  *
  * This is used to enable non-default device modes.  Not all devices
- * support this kind of configurability.  By default, configuration
- * zero is selected after enumeration; many devices only have a single
+ * use this kind of configurability; many devices only have one
  * configuration.
  *
- * USB devices may support one or more configurations, which affect
+ * USB device configurations may affect Linux interoperability,
  * power consumption and the functionality available.  For example,
  * the default configuration is limited to using 100mA of bus power,
  * so that when certain device functionality requires more power,
- * and the device is bus powered, that functionality will be in some
+ * and the device is bus powered, that functionality should be in some
  * non-default device configuration.  Other device modes may also be
  * reflected as configuration options, such as whether two ISDN
- * channels are presented as independent 64Kb/s interfaces or as one
- * bonded 128Kb/s interface.
+ * channels are available independently; and choosing between open
+ * standard device protocols (like CDC) or proprietary ones.
  *
  * Note that USB has an additional level of device configurability,
  * associated with interfaces.  That configurability is accessed using
  * usb_set_interface().
  *
- * This call is synchronous, and may not be used in an interrupt context.
+ * This call is synchronous. The calling context must be able to sleep,
+ * and must not hold the driver model lock for USB; usb device driver
+ * probe() methods may not use this routine.
  *
  * Returns zero on success, or else the status code returned by the
- * underlying usb_control_msg() call.
+ * underlying call that failed.  On succesful completion, each interface
+ * in the original device configuration has been destroyed, and each one
+ * in the new configuration has been probed by all relevant usb device
+ * drivers currently known to the kernel.
  */
 int usb_set_configuration(struct usb_device *dev, int configuration)
 {
 	int i, ret;
 	struct usb_host_config *cp = NULL;
 	
+	/* dev->serialize guards all config changes */
+	down(&dev->serialize);
+
 	for (i=0; i<dev->descriptor.bNumConfigurations; i++) {
 		if (dev->config[i].desc.bConfigurationValue == configuration) {
 			cp = &dev->config[i];
 			break;
 		}
 	}
-	if ((!cp && configuration != 0) || (cp && configuration == 0)) {
-		warn("selecting invalid configuration %d", configuration);
-		return -EINVAL;
+	if ((!cp && configuration != 0)) {
+		ret = -EINVAL;
+		goto out;
 	}
+	if (cp && configuration == 0)
+		dev_warn(&dev->dev, "config 0 descriptor??\n");
 
-	/* if it's already configured, clear out old state first. */
+	/* if it's already configured, clear out old state first.
+	 * getting rid of old interfaces means unbinding their drivers.
+	 */
 	if (dev->state != USB_STATE_ADDRESS)
 		usb_disable_device (dev, 1);	// Skip ep0
-	dev->toggle[0] = dev->toggle[1] = 0;
-	dev->halted[0] = dev->halted[1] = 0;
-	dev->state = USB_STATE_ADDRESS;
 
 	if ((ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
 			USB_REQ_SET_CONFIGURATION, 0, configuration, 0,
 			NULL, 0, HZ * USB_CTRL_SET_TIMEOUT)) < 0)
-		return ret;
-	if (configuration)
-		dev->state = USB_STATE_CONFIGURED;
+		goto out;
+
 	dev->actconfig = cp;
+	if (!configuration)
+		dev->state = USB_STATE_ADDRESS;
+	else {
+		dev->state = USB_STATE_CONFIGURED;
 
-	/* reset more hc/hcd interface/endpoint state */
+		/* re-initialize hc/hcd/usbcore interface/endpoint state.
+		 * this triggers binding of drivers to interfaces; and
+		 * maybe probe() calls will choose different altsettings.
+		 */
 		for (i = 0; i < cp->desc.bNumInterfaces; ++i) {
 			struct usb_interface *intf = cp->interface[i];
+			struct usb_interface_descriptor *desc;
 
 			intf->act_altsetting = 0;
+			desc = &intf->altsetting [0].desc;
 			usb_enable_interface(dev, intf);
+
+			intf->dev.parent = &dev->dev;
+			intf->dev.driver = NULL;
+			intf->dev.bus = &usb_bus_type;
+			intf->dev.dma_mask = dev->dev.dma_mask;
+			sprintf (&intf->dev.bus_id[0], "%d-%s:%d.%d",
+				 dev->bus->busnum, dev->devpath,
+				 configuration,
+				 desc->bInterfaceNumber);
+			dev_dbg (&dev->dev,
+				"registering %s (config #%d, interface %d)\n",
+				intf->dev.bus_id, configuration,
+				desc->bInterfaceNumber);
+			device_add (&intf->dev);
+			usb_create_driverfs_intf_files (intf);
+		}
 	}
 
-	return 0;
+out:
+	up(&dev->serialize);
+	return ret;
 }
 
-
 /**
  * usb_string - returns ISO 8859-1 version of a string descriptor
  * @dev: the device whose string descriptor is being retrieved

drivers/usb/core/usb.c

@@ -898,6 +898,7 @@ void usb_disconnect(struct usb_device **pdev)
 	 * this device will fail.
 	 */
 	dev->state = USB_STATE_NOTATTACHED;
+	down(&dev->serialize);
 
 	dev_info (&dev->dev, "USB disconnect, address %d\n", dev->devnum);
 
@@ -908,31 +909,23 @@ void usb_disconnect(struct usb_device **pdev)
 			usb_disconnect(child);
 	}
 
-	/* deallocate hcd/hardware state ... and nuke all pending urbs */
+	/* deallocate hcd/hardware state ... nuking all pending urbs and
+	 * cleaning up all state associated with the current configuration
+	 */
 	usb_disable_device(dev, 0);
 
-	/* disconnect() drivers from interfaces (a key side effect) */
-	dev_dbg (&dev->dev, "unregistering interfaces\n");
-	if (dev->actconfig) {
-		for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
-			struct usb_interface	*interface;
-
-			/* remove this interface */
-			interface = dev->actconfig->interface[i];
-			device_unregister(&interface->dev);
-		}
-	}
-
 	dev_dbg (&dev->dev, "unregistering device\n");
 	/* Free the device number and remove the /proc/bus/usb entry */
 	if (dev->devnum > 0) {
 		clear_bit(dev->devnum, dev->bus->devmap.devicemap);
 		usbfs_remove_device(dev);
 	}
+	up(&dev->serialize);
 	device_unregister(&dev->dev);
 
 	/* Decrement the reference count, it'll auto free everything when */
 	/* it hits 0 which could very well be now */
+	/* FIXME the decrement in device_unregister() should suffice ... */
 	usb_put_dev(dev);
 }
 
@@ -1090,27 +1083,12 @@ int usb_new_device(struct usb_device *dev, struct device *parent)
 
 	err = usb_get_configuration(dev);
 	if (err < 0) {
-		dev_err(&dev->dev, "unable to get device %d configuration (error=%d)\n",
-			dev->devnum, err);
-		goto fail;
-	}
-
-	/* choose and set the configuration here */
-	if (dev->descriptor.bNumConfigurations != 1) {
-		dev_info(&dev->dev,
-			"configuration #%d chosen from %d choices\n",
-			dev->config[0].desc.bConfigurationValue,
-			dev->descriptor.bNumConfigurations);
-	}
-	err = usb_set_configuration(dev, dev->config[0].desc.bConfigurationValue);
-	if (err) {
-		dev_err(&dev->dev, "failed to set device %d default configuration (error=%d)\n",
-			dev->devnum, err);
+		dev_err(&dev->dev, "can't read configurations, error %d\n",
+			err);
 		goto fail;
 	}
 
-	/* USB device state == configured ... tell the world! */
-
+	/* Tell the world! */
 	dev_dbg(&dev->dev, "new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n",
 		dev->descriptor.iManufacturer, dev->descriptor.iProduct, dev->descriptor.iSerialNumber);
 
@@ -1123,30 +1101,32 @@ int usb_new_device(struct usb_device *dev, struct device *parent)
 		usb_show_string(dev, "SerialNumber", dev->descriptor.iSerialNumber);
 #endif
 
-	/* put into sysfs, with device and config specific files */
+	/* put device-specific files into sysfs */
 	err = device_add (&dev->dev);
-	if (err)
+	if (err) {
+		dev_err(&dev->dev, "can't device_add, error %d\n", err);
 		goto fail;
+	}
 	usb_create_driverfs_dev_files (dev);
 
-	/* Register all of the interfaces for this device with the driver core.
-	 * Remember, interfaces get bound to drivers, not devices. */
-	for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
-		struct usb_interface *interface = dev->actconfig->interface[i];
-		struct usb_interface_descriptor *desc;
-
-		desc = &interface->altsetting [interface->act_altsetting].desc;
-		interface->dev.parent = &dev->dev;
-		interface->dev.driver = NULL;
-		interface->dev.bus = &usb_bus_type;
-		interface->dev.dma_mask = parent->dma_mask;
-		sprintf (&interface->dev.bus_id[0], "%d-%s:%d",
-			 dev->bus->busnum, dev->devpath,
-			 desc->bInterfaceNumber);
-		dev_dbg (&dev->dev, "%s - registering interface %s\n", __FUNCTION__, interface->dev.bus_id);
-		device_add (&interface->dev);
-		usb_create_driverfs_intf_files (interface);
+	/* choose and set the configuration. that registers the interfaces
+	 * with the driver core, and lets usb device drivers bind to them.
+	 */
+	if (dev->descriptor.bNumConfigurations != 1) {
+		dev_info(&dev->dev,
+			"configuration #%d chosen from %d choices\n",
+			dev->config[0].desc.bConfigurationValue,
+			dev->descriptor.bNumConfigurations);
 	}
+	err = usb_set_configuration(dev,
+			dev->config[0].desc.bConfigurationValue);
+	if (err) {
+		dev_err(&dev->dev, "can't set config #%d, error %d\n",
+			dev->config[0].desc.bConfigurationValue, err);
+		goto fail;
+	}
+
+	/* USB device state == configured ... usable */
 
 	/* add a /proc/bus/usb entry */
 	usbfs_add_device(dev);