[PATCH] USB: usb driver binding fixes

There are problems lurking in the driver binding code for usb,
with highlights being disagreements about:

    (a) locks: usb bus writelock v. BKL v. driver->serialize
    (b) driver: interface.driver v. interface.dev.driver

Fixing those is going to take multiple patches, and I thought
I'd start out with a small one that's relatively simple.  This:

    - Cleans up locking.

        * Updates comments and kerneldoc to reflect that the
          usb bus writelock is what protects against conflicts
          when binding/unbinding drivers to devices.

        * Removes driver->serialize ... not needed, since
          it's only gotten when the bus writelock is held.

        * Removes incorrect "must have BKL" comments, and one
          bit of code that tried to use BKL not the writelock.

    - Removes inconsistencies about what driver is bound to the
      interface ... for now "interface.driver" is "the truth".

        * usb_probe_interface() will no longer clobber bindings
          established with usb_driver_claim_interface().

        * usb_driver_release_interface() calls device_release_driver()
          for bindings established with probe(), so the driver model
          updates (sysfs etc) are done as expected.

        * usb_unbind_interface() doesn't usb_driver_release_interface(),
          since release() should eventually _always_ call unbind()
          (indirectly through device_release_driver).

Essentially there are two driver binding models in USB today,
and this patch makes them start to cooperate properly:

   - probe()/disconnect(), used by most drivers.  This goes
     through the driver model core.

   - claim()/release(), used by CDC drivers (ACM, Ethernet)
     and audio to claim extra interfaces and by usbfs since it
     can't come in through probe().  Bypasses driver model.

That interface.driver pointer can be removed by changing the
claim()/release() code to use the driver model calls added
for that purpose:  device_{bind,release}_driver().  I didn't
do that in this patch, since it'll have side effects like
extra disconnect() calls that drivers will need to handle.

A separate usbfs patch is needed to fix its driver binding;
mostly just to use the right lock, but those changes were
more extensive and uncovered other issues.  (Like, I think,
some that Duncan has been noticing ...)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index ee6f1abdf0aa158f50341d0f588b915a93d43605..8a39c3b52803789555ec73c58bee1995e12ab071 100644 (file)
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -80,7 +80,7 @@ static struct device_driver usb_generic_driver = {
 
 static int usb_generic_driver_data;
 
-/* needs to be called with BKL held */
+/* called from driver core with usb_bus_type.subsys writelock */
 int usb_probe_interface(struct device *dev)
 {
        struct usb_interface * intf = to_usb_interface(dev);
@@ -93,12 +93,14 @@ int usb_probe_interface(struct device *dev)
        if (!driver->probe)
                return error;
 
+       /* driver claim() doesn't yet affect dev->driver... */
+       if (intf->driver)
+               return error;
+
        id = usb_match_id (intf, driver->id_table);
        if (id) {
                dev_dbg (dev, "%s - got id\n", __FUNCTION__);
-               down (&driver->serialize);
                error = driver->probe (intf, id);
-               up (&driver->serialize);
        }
        if (!error)
                intf->driver = driver;
@@ -106,23 +108,24 @@ int usb_probe_interface(struct device *dev)
        return error;
 }
 
+/* called from driver core with usb_bus_type.subsys writelock */
 int usb_unbind_interface(struct device *dev)
 {
        struct usb_interface *intf = to_usb_interface(dev);
-       struct usb_driver *driver = to_usb_driver(dev->driver);
-
-       down(&driver->serialize);
+       struct usb_driver *driver = intf->driver;
 
        /* release all urbs for this interface */
        usb_disable_interface(interface_to_usbdev(intf), intf);
 
-       if (intf->driver && intf->driver->disconnect)
-               intf->driver->disconnect(intf);
+       if (driver && driver->disconnect)
+               driver->disconnect(intf);
 
-       /* force a release and re-initialize the interface */
-       usb_driver_release_interface(driver, intf);
-
-       up(&driver->serialize);
+       /* reset other interface state */
+       usb_set_interface(interface_to_usbdev(intf),
+                       intf->altsetting[0].desc.bInterfaceNumber,
+                       0);
+       usb_set_intfdata(intf, NULL);
+       intf->driver = NULL;
 
        return 0;
 }
@@ -152,8 +155,6 @@ int usb_register(struct usb_driver *new_driver)
        new_driver->driver.probe = usb_probe_interface;
        new_driver->driver.remove = usb_unbind_interface;
 
-       init_MUTEX(&new_driver->serialize);
-
        retval = driver_register(&new_driver->driver);
 
        if (!retval) {
@@ -170,7 +171,7 @@ int usb_register(struct usb_driver *new_driver)
 /**
  * usb_deregister - unregister a USB driver
  * @driver: USB operations of the driver to unregister
- * Context: !in_interrupt (), must be called with BKL held
+ * Context: must be able to sleep
  *
  * Unlinks the specified driver from the internal USB driver list.
  * 
@@ -264,26 +265,22 @@ usb_epnum_to_ep_desc(struct usb_device *dev, unsigned epnum)
  * Few drivers should need to use this routine, since the most natural
  * way to bind to an interface is to return the private data from
  * the driver's probe() method.
+ *
+ * Callers must own the driver model's usb bus writelock.  So driver
+ * probe() entries don't need extra locking, but other call contexts
+ * may need to explicitly claim that lock.
  */
 int usb_driver_claim_interface(struct usb_driver *driver, struct usb_interface *iface, void* priv)
 {
        if (!iface || !driver)
                return -EINVAL;
 
-       /* this is mainly to lock against usbfs */
-       lock_kernel();
-       if (iface->driver) {
-               unlock_kernel();
-               err ("%s driver booted %s off interface %p",
-                       driver->name, iface->driver->name, iface);
+       if (iface->driver)
                return -EBUSY;
-       } else {
-           dbg("%s driver claimed interface %p", driver->name, iface);
-       }
 
+       /* FIXME should device_bind_driver() */
        iface->driver = driver;
        usb_set_intfdata(iface, priv);
-       unlock_kernel();
        return 0;
 }
 
@@ -323,13 +320,22 @@ int usb_interface_claimed(struct usb_interface *iface)
  * usually won't need to call this.
  *
  * This call is synchronous, and may not be used in an interrupt context.
+ * Callers must own the driver model's usb bus writelock.  So driver
+ * disconnect() entries don't need extra locking, but other call contexts
+ * may need to explicitly claim that lock.
  */
 void usb_driver_release_interface(struct usb_driver *driver, struct usb_interface *iface)
 {
        /* this should never happen, don't release something that's not ours */
-       if (iface->driver && iface->driver != driver)
+       if (!iface || !iface->driver || iface->driver != driver)
                return;
 
+       if (iface->dev.driver) {
+               /* FIXME should be the ONLY case here */
+               device_release_driver(&iface->dev);
+               return;
+       }
+
        usb_set_interface(interface_to_usbdev(iface),
                        iface->altsetting[0].desc.bInterfaceNumber,
                        0);

diff --git a/include/linux/usb.h b/include/linux/usb.h
index 41c2ca3dc629792681cc9afbee554229aa699695..5e49f861230d745f16216e7f64b9a8d064896883 100644 (file)
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -414,9 +414,6 @@ static inline int usb_make_path (struct usb_device *dev, char *buf, size_t size)
  *     Export this with MODULE_DEVICE_TABLE(usb,...).  This must be set
  *     or your driver's probe function will never get called.
  * @driver: the driver model core driver structure.
- * @serialize: a semaphore used to serialize access to this driver.  Used
- *     in the probe and disconnect functions.  Only the USB core should use
- *     this lock.
  *
  * USB drivers must provide a name, probe() and disconnect() methods,
  * and an id_table.  Other driver fields are optional.
@@ -451,8 +448,6 @@ struct usb_driver {
        const struct usb_device_id *id_table;
 
        struct device_driver driver;
-
-       struct semaphore serialize;
 };
 #define        to_usb_driver(d) container_of(d, struct usb_driver, driver)