[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.

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 1a2650ac5e3c17b0d67dedd88383bef7594076be..a3655d0e7ccbbec67d83e031bcba5aaa44376303 100644 (file)
--- a/drivers/usb/core/devio.c
+++ b/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)

diff --git a/drivers/usb/core/driverfs.c b/drivers/usb/core/driverfs.c
index ed14bb6987b94d74bfb35f39523e844c96b7fe1e..2284803bdeed792639715f620e6aeaffbe38b581 100644 (file)
--- a/drivers/usb/core/driverfs.c
+++ b/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;
 }
 

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e8abb6929099762cd32388f40b6d5699a4bfd556..86adfdd5d439426b6b50997aef3ded2bb5344e41 100644 (file)
--- a/drivers/usb/core/hub.c
+++ b/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);

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 558c974ce5c98e54c758d5a1b0c6533b22cb69a9..5d9e3a88a80ef31525e0e6aeb18ea8e18e3e1cea 100644 (file)
--- a/drivers/usb/core/message.c
+++ b/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);
 

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 05dd49c24a619853f68c8772852e6a061ae444fc..0018f343c6c0a8ad88c019d43560c3553ceafb42 100644 (file)
--- a/drivers/usb/core/usb.c
+++ b/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);

diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 499d9ae5642ea19cc1dff3444755f79fd169f6e6..5126d551ce3909af52ceb9223f9b1cec34e0392e 100644 (file)
--- a/drivers/usb/core/usb.h
+++ b/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;

diff --git a/include/linux/usb.h b/include/linux/usb.h
index e4b60511fc46bce4fa87161c04b263d022e3eb92..f6c4c170750d61fd98dbf18982576ca564784023 100644 (file)
--- a/include/linux/usb.h
+++ b/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);
 
 /*