ChangeSet 1.1504.2.51, 2003/12/12 14:36:36-08:00, david-b@pacbell.net

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


 drivers/usb/core/usb.c |   58 +++++++++++++++++++++++++++----------------------
 include/linux/usb.h    |    5 ----
 2 files changed, 32 insertions(+), 31 deletions(-)


diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
--- a/drivers/usb/core/usb.c	Mon Dec 29 14:21:16 2003
+++ b/drivers/usb/core/usb.c	Mon Dec 29 14:21:16 2003
@@ -80,7 +80,7 @@
 
 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 @@
 	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 @@
 	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 @@
 	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 @@
 /**
  * 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 @@
  * 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,12 +320,21 @@
  * 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,
diff -Nru a/include/linux/usb.h b/include/linux/usb.h
--- a/include/linux/usb.h	Mon Dec 29 14:21:16 2003
+++ b/include/linux/usb.h	Mon Dec 29 14:21:16 2003
@@ -414,9 +414,6 @@
  *	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 @@
 	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)