[PATCH] USB: set_configuration locking cleanups

I've posted all these before, the only notable change is
treating that one gphoto2 case as warn-and-continue rather
than return-with-failure.


usb_set_configuration() cleanup

 * Remove it from the USB kernel driver API.  No drivers need it now,
   and the sysadmin can change bConfigurationValue using sysfs (say,
   when hotplugging an otherwise problematic device).

 * Simpler/cleaner locking:  caller must own dev->serialize.

 * Access from usbfs now uses usb_reset_configuration() sometimes,
   preventing sysfs thrash, and warns about some dangerous usage
   (which gphoto2 and other programs may be relying on).  (This is
   from Alan Stern, but I morphed an error return into a warning.)

 * Prevent a couple potential "no configuration" oopses. (Alan's?)

 * Remove one broken call from usbcore,  in the "device morphed" path
   of usb_reset_device().  This should be more polite now, hanging
   that one device rather than khubd.

drivers/usb/core/devio.c

@@ -430,6 +430,8 @@ static int findintfep(struct usb_device *dev, unsigned int ep)
 
 	if (ep & ~(USB_DIR_IN|0xf))
 		return -EINVAL;
+	if (!dev->actconfig)
+		return -ESRCH;
 	for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
 		iface = dev->actconfig->interface[i];
 		for (j = 0; j < iface->num_altsetting; j++) {
@@ -450,6 +452,8 @@ static int findintfif(struct usb_device *dev, unsigned int ifn)
 
 	if (ifn & ~0xff)
 		return -EINVAL;
+	if (!dev->actconfig)
+		return -ESRCH;
 	for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) {
 		if (dev->actconfig->interface[i]->
 				altsetting[0].desc.bInterfaceNumber == ifn)
@@ -766,10 +770,51 @@ static int proc_setintf(struct dev_state *ps, void __user *arg)
 static int proc_setconfig(struct dev_state *ps, void __user *arg)
 {
 	unsigned int u;
+	int status = 0;
+ 	struct usb_host_config *actconfig;
 
 	if (get_user(u, (unsigned int __user *)arg))
 		return -EFAULT;
-	return usb_set_configuration(ps->dev, u);
+
+	down(&ps->dev->serialize);
+ 	actconfig = ps->dev->actconfig;
+ 
+ 	/* Don't touch the device if any interfaces are claimed.
+ 	 * It could interfere with other drivers' operations, and if
+	 * an interface is claimed by usbfs it could easily deadlock.
+	 */
+ 	if (actconfig) {
+ 		int i;
+ 
+ 		for (i = 0; i < actconfig->desc.bNumInterfaces; ++i) {
+ 			if (usb_interface_claimed(actconfig->interface[i])) {
+				dev_warn (&ps->dev->dev,
+					"usbfs: interface %d claimed "
+					"while '%s' sets config #%d\n",
+					actconfig->interface[i]
+						->cur_altsetting
+						->desc.bInterfaceNumber,
+					current->comm, u);
+#if 0	/* FIXME:  enable in 2.6.10 or so */
+ 				status = -EBUSY;
+				break;
+#endif
+			}
+ 		}
+ 	}
+
+	/* SET_CONFIGURATION is often abused as a "cheap" driver reset,
+	 * so avoid usb_set_configuration()'s kick to sysfs
+	 */
+	if (status == 0) {
+		if (actconfig && actconfig->desc.bConfigurationValue == u)
+			status = usb_reset_configuration(ps->dev);
+		else
+			status = usb_set_configuration(ps->dev, u);
+	}
+	up(&ps->dev->serialize);
+
+	return status;
 }
 
 static int proc_submiturb(struct dev_state *ps, void __user *arg)

drivers/usb/core/driverfs.c

@@ -55,7 +55,9 @@ set_bConfigurationValue (struct device *dev, const char *buf, size_t count)
 
 	if (sscanf (buf, "%u", &config) != 1 || config > 255)
 		return -EINVAL;
+	down(&udev->serialize);
 	value = usb_set_configuration (udev, config);
+	up(&udev->serialize);
 	return (value < 0) ? value : count;
 }
 

drivers/usb/core/hub.c

@@ -1299,33 +1299,15 @@ int usb_physical_reset_device(struct usb_device *dev)
 		kfree(descriptor);
 		usb_destroy_configuration(dev);
 
-		ret = usb_get_device_descriptor(dev, sizeof(dev->descriptor));
-		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;
-		}
+		/* FIXME Linux doesn't yet handle these "device morphed"
+		 * paths.  DFU variants need this to work ... and they
+		 * include the "config descriptors changed" case this
+		 * doesn't yet detect!
+		 */
+		dev->state = USB_STATE_NOTATTACHED;
+		dev_err(&dev->dev, "device morphed (DFU?), nyet supported\n");
 
-		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;
+		return -ENODEV;
 	}
 
 	kfree(descriptor);

drivers/usb/core/message.c

@@ -1075,11 +1075,11 @@ int usb_reset_configuration(struct usb_device *dev)
 	return 0;
 }
 
-/**
+/*
  * usb_set_configuration - Makes a particular device setting be current
  * @dev: the device whose configuration is being updated
  * @configuration: the configuration being chosen.
- * Context: !in_interrupt ()
+ * Context: !in_interrupt(), caller holds dev->serialize
  *
  * This is used to enable non-default device modes.  Not all devices
  * use this kind of configurability; many devices only have one
@@ -1115,7 +1115,6 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 	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) {
@@ -1191,7 +1190,6 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 	}
 
 out:
-	up(&dev->serialize);
 	return ret;
 }
 
@@ -1300,6 +1298,5 @@ EXPORT_SYMBOL(usb_string);
 // synchronous calls that also maintain usbcore state
 EXPORT_SYMBOL(usb_clear_halt);
 EXPORT_SYMBOL(usb_reset_configuration);
-EXPORT_SYMBOL(usb_set_configuration);
 EXPORT_SYMBOL(usb_set_interface);
 

drivers/usb/core/usb.c

@@ -1173,10 +1173,13 @@ int usb_new_device(struct usb_device *dev)
 		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,6 +1214,7 @@ int usb_new_device(struct usb_device *dev)
 			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);

drivers/usb/core/usb.h

@@ -17,6 +17,7 @@ extern void usb_enable_interface (struct usb_device *dev,
 
 extern int usb_get_device_descriptor(struct usb_device *dev,
 		unsigned int size);
+extern int usb_set_configuration(struct usb_device *dev, int configuration);
 
 /* for labeling diagnostics */
 extern const char *usbcore_name;

include/linux/usb.h

@@ -904,7 +904,6 @@ extern int usb_string(struct usb_device *dev, int index,
 /* wrappers that also update important state inside usbcore */
 extern int usb_clear_halt(struct usb_device *dev, int pipe);
 extern int usb_reset_configuration(struct usb_device *dev);
-extern int usb_set_configuration(struct usb_device *dev, int configuration);
 extern int usb_set_interface(struct usb_device *dev, int ifnum, int alternate);
 
 /*