[PATCH] USB: remove usb_interface.driver field

Remove usb_interface.driver, and along with it the "half bound" state
previously associated with drivers binding with claim() instead of probe().
This changes usb_driver_claim_interface() semantics slightly: drivers must
now be prepared to accept disconnect() callbacks.

Fixes more locking bugs, and a claim() oops that snuck in with a
recent patch.

drivers/usb/core/devices.c

@@ -247,7 +247,9 @@ static char *usb_dump_interface_descriptor(char *start, char *end, const struct
 			 class_decode(desc->bInterfaceClass),
 			 desc->bInterfaceSubClass,
 			 desc->bInterfaceProtocol,
-			 iface->driver ? iface->driver->name : "(none)");
+			 iface->dev.driver
+				? iface->dev.driver->name
+				: "(none)");
 	up_read(&usb_bus_type.subsys.rwsem);
 	return start;
 }

drivers/usb/core/devio.c

@@ -704,9 +704,9 @@ static int proc_getdriver(struct dev_state *ps, void __user *arg)
 	if ((ret = findintfif(ps->dev, gd.interface)) < 0)
 		return ret;
 	interface = ps->dev->actconfig->interface[ret];
-	if (!interface->driver)
+	if (!interface->dev.driver)
 		return -ENODATA;
-	strcpy(gd.driver, interface->driver->name);
+	strncpy(gd.driver, interface->dev.driver->name, sizeof(gd.driver));
 	if (copy_to_user(arg, &gd, sizeof(gd)))
 		return -EFAULT;
 	return 0;
@@ -725,26 +725,11 @@ static int proc_connectinfo(struct dev_state *ps, void __user *arg)
 
 static int proc_resetdevice(struct dev_state *ps)
 {
-	int i, ret;
-
-	ret = usb_reset_device(ps->dev);
-	if (ret < 0)
-		return ret;
-
-	for (i = 0; i < ps->dev->actconfig->desc.bNumInterfaces; i++) {
-		struct usb_interface *intf = ps->dev->actconfig->interface[i];
-
-		/* Don't simulate interfaces we've claimed */
-		if (test_bit(i, &ps->ifclaimed))
-			continue;
-
-		err ("%s - this function is broken", __FUNCTION__);
-		if (intf->driver && ps->dev) {
-			usb_probe_interface (&intf->dev);
-		}
-	}
+	/* FIXME when usb_reset_device() is fixed we'll need to grab
+	 * ps->dev->serialize before calling it.
+	 */
+	return usb_reset_device(ps->dev);
 
-	return 0;
 }
 
 static int proc_setintf(struct dev_state *ps, void __user *arg)
@@ -758,7 +743,7 @@ static int proc_setintf(struct dev_state *ps, void __user *arg)
 	if ((ret = findintfif(ps->dev, setintf.interface)) < 0)
 		return ret;
 	interface = ps->dev->actconfig->interface[ret];
-	if (interface->driver) {
+	if (interface->dev.driver) {
 		if ((ret = checkintf(ps, ret)))
 			return ret;
 	}
@@ -1141,58 +1126,51 @@ static int proc_ioctl (struct dev_state *ps, void __user *arg)
 		}
 	}
 
-       if (!ps->dev)
-               retval = -ENODEV;
-       else if (!(ifp = usb_ifnum_to_if (ps->dev, ctrl.ifno)))
+	if (!ps->dev) {
+		if (buf)
+			kfree(buf);
+		return -ENODEV;
+	}
+
+	down(&ps->dev->serialize);
+	if (ps->dev->state != USB_STATE_CONFIGURED)
+		retval = -ENODEV;
+	else if (!(ifp = usb_ifnum_to_if (ps->dev, ctrl.ifno)))
                retval = -EINVAL;
-       else switch (ctrl.ioctl_code) {
-
-       /* disconnect kernel driver from interface, leaving it unbound.  */
-       /* maybe unbound - you get no guarantee it stays unbound */
-       case USBDEVFS_DISCONNECT:
-		/* this function is misdesigned - retained for compatibility */
-		lock_kernel();
-		driver = ifp->driver;
-		if (driver) {
-			dbg ("disconnect '%s' from dev %d interface %d",
-			     driver->name, ps->dev->devnum, ctrl.ifno);
-			usb_unbind_interface(&ifp->dev);
+	else switch (ctrl.ioctl_code) {
+
+	/* disconnect kernel driver from interface */
+	case USBDEVFS_DISCONNECT:
+		down_write(&usb_bus_type.subsys.rwsem);
+		if (ifp->dev.driver) {
+			driver = to_usb_driver(ifp->dev.driver);
+			dev_dbg (&ifp->dev, "disconnect by usbfs\n");
+			usb_driver_release_interface(driver, ifp);
 		} else
 			retval = -ENODATA;
-		unlock_kernel();
+		up_write(&usb_bus_type.subsys.rwsem);
 		break;
 
 	/* let kernel drivers try to (re)bind to the interface */
 	case USBDEVFS_CONNECT:
-		lock_kernel();
-		retval = usb_probe_interface (&ifp->dev);
-		unlock_kernel();
+		bus_rescan_devices(ifp->dev.bus);
 		break;
 
 	/* talk directly to the interface's driver */
 	default:
-		/* BKL used here to protect against changing the binding
-		 * of this driver to this device, as well as unloading its
-		 * driver module.
-		 */
-		lock_kernel ();
-		driver = ifp->driver;
+		down_read(&usb_bus_type.subsys.rwsem);
+		if (ifp->dev.driver)
+			driver = to_usb_driver(ifp->dev.driver);
 		if (driver == 0 || driver->ioctl == 0) {
-			unlock_kernel();
-			retval = -ENOSYS;
+			retval = -ENOTTY;
 		} else {
-			if (!try_module_get (driver->owner)) {
-				unlock_kernel();
-				retval = -ENOSYS;
-				break;
-			}
-			unlock_kernel ();
 			retval = driver->ioctl (ifp, ctrl.ioctl_code, buf);
 			if (retval == -ENOIOCTLCMD)
 				retval = -ENOTTY;
-			module_put (driver->owner);
 		}
+		up_read(&usb_bus_type.subsys.rwsem);
 	}
+	up(&ps->dev->serialize);
 
 	/* cleanup and return */
 	if (retval >= 0

drivers/usb/core/message.c

@@ -1176,15 +1176,34 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 			intf->dev.bus = &usb_bus_type;
 			intf->dev.dma_mask = dev->dev.dma_mask;
 			intf->dev.release = release_interface;
+			device_initialize (&intf->dev);
 			sprintf (&intf->dev.bus_id[0], "%d-%s:%d.%d",
 				 dev->bus->busnum, dev->devpath,
 				 configuration,
 				 alt->desc.bInterfaceNumber);
+		}
+
+		/* Now that all interfaces are setup, probe() calls
+		 * may claim() any interface that's not yet bound.
+		 * Many class drivers need that: CDC, audio, video, etc.
+		 */
+		for (i = 0; i < cp->desc.bNumInterfaces; ++i) {
+			struct usb_interface *intf = cp->interface[i];
+			struct usb_interface_descriptor *desc;
+
+			desc = &intf->altsetting [0].desc;
 			dev_dbg (&dev->dev,
-				"registering %s (config #%d, interface %d)\n",
+				"adding %s (config #%d, interface %d)\n",
 				intf->dev.bus_id, configuration,
-				alt->desc.bInterfaceNumber);
-			device_register (&intf->dev);
+				desc->bInterfaceNumber);
+			ret = device_add (&intf->dev);
+			if (ret != 0) {
+				dev_err(&dev->dev,
+					"device_add(%s) --> %d\n",
+					intf->dev.bus_id,
+					ret);
+				continue;
+			}
 			usb_create_driverfs_intf_files (intf);
 		}
 	}

drivers/usb/core/usb.c

@@ -7,8 +7,7 @@
  * (C) Copyright Gregory P. Smith 1999
  * (C) Copyright Deti Fliegl 1999 (new USB architecture)
  * (C) Copyright Randy Dunlap 2000
- * (C) Copyright David Brownell 2000-2001 (kernel hotplug, usb_device_id,
- 	more docs, etc)
+ * (C) Copyright David Brownell 2000-2004
  * (C) Copyright Yggdrasil Computing, Inc. 2000
  *     (usb_device_id matching changes by Adam J. Richter)
  * (C) Copyright Greg Kroah-Hartman 2002-2003
@@ -95,17 +94,11 @@ 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__);
 		error = driver->probe (intf, id);
 	}
-	if (!error)
-		intf->driver = driver;
 
 	return error;
 }
@@ -114,7 +107,7 @@ int usb_probe_interface(struct device *dev)
 int usb_unbind_interface(struct device *dev)
 {
 	struct usb_interface *intf = to_usb_interface(dev);
-	struct usb_driver *driver = intf->driver;
+	struct usb_driver *driver = to_usb_driver(intf->dev.driver);
 
 	/* release all urbs for this interface */
 	usb_disable_interface(interface_to_usbdev(intf), intf);
@@ -127,7 +120,6 @@ int usb_unbind_interface(struct device *dev)
 			intf->altsetting[0].desc.bInterfaceNumber,
 			0);
 	usb_set_intfdata(intf, NULL);
-	intf->driver = NULL;
 
 	return 0;
 }
@@ -290,7 +282,8 @@ usb_epnum_to_ep_desc(struct usb_device *dev, unsigned epnum)
 /**
  * usb_driver_claim_interface - bind a driver to an interface
  * @driver: the driver to be bound
- * @iface: the interface to which it will be bound
+ * @iface: the interface to which it will be bound; must be in the
+ *	usb device's active configuration
  * @priv: driver data associated with that interface
  *
  * This is used by usb device drivers that need to claim more than one
@@ -308,75 +301,52 @@ usb_epnum_to_ep_desc(struct usb_device *dev, unsigned epnum)
  */
 int usb_driver_claim_interface(struct usb_driver *driver, struct usb_interface *iface, void* priv)
 {
-	if (!iface || !driver)
-		return -EINVAL;
+	struct device *dev = &iface->dev;
 
-	if (iface->driver)
+	if (dev->driver)
 		return -EBUSY;
 
-	/* FIXME should device_bind_driver() */
-	iface->driver = driver;
+	dev->driver = &driver->driver;
 	usb_set_intfdata(iface, priv);
-	return 0;
-}
 
-/**
- * usb_interface_claimed - returns true iff an interface is claimed
- * @iface: the interface being checked
- *
- * This should be used by drivers to check other interfaces to see if
- * they are available or not.  If another driver has claimed the interface,
- * they may not claim it.  Otherwise it's OK to claim it using
- * usb_driver_claim_interface().
- *
- * Returns true (nonzero) iff the interface is claimed, else false (zero).
- */
-int usb_interface_claimed(struct usb_interface *iface)
-{
-	if (!iface)
-		return 0;
+	/* if interface was already added, bind now; else let
+	 * the future device_add() bind it, bypassing probe()
+	 */
+	if (!list_empty (&dev->bus_list))
+		device_bind_driver(dev);
 
-	return (iface->driver != NULL);
-} /* usb_interface_claimed() */
+	return 0;
+}
 
 /**
  * usb_driver_release_interface - unbind a driver from an interface
  * @driver: the driver to be unbound
  * @iface: the interface from which it will be unbound
  *
- * In addition to unbinding the driver, this re-initializes the interface
- * by selecting altsetting 0, the default alternate setting.
- * 
  * This can be used by drivers to release an interface without waiting
- * for their disconnect() methods to be called.
- *
- * When the USB subsystem disconnect()s a driver from some interface,
- * it automatically invokes this method for that interface.  That
- * means that even drivers that used usb_driver_claim_interface()
- * usually won't need to call this.
+ * for their disconnect() methods to be called.  In typical cases this
+ * also causes the driver disconnect() method to be called.
  *
  * 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.
+ * Callers must own the usb_device serialize semaphore and 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 those locks.
  */
-void usb_driver_release_interface(struct usb_driver *driver, struct usb_interface *iface)
+void usb_driver_release_interface(struct usb_driver *driver,
+					struct usb_interface *iface)
 {
+	struct device *dev = &iface->dev;
+
 	/* this should never happen, don't release something that's not ours */
-	if (!iface || !iface->driver || iface->driver != driver)
+	if (!dev->driver || dev->driver != &driver->driver)
 		return;
 
-	if (iface->dev.driver) {
-		/* FIXME should be the ONLY case here */
-		device_release_driver(&iface->dev);
-		return;
-	}
+	/* don't disconnect from disconnect(), or before dev_add() */
+	if (!list_empty (&dev->driver_list) && !list_empty (&dev->bus_list))
+		device_release_driver(dev);
 
-	usb_set_interface(interface_to_usbdev(iface),
-			iface->altsetting[0].desc.bInterfaceNumber,
-			0);
+	dev->driver = NULL;
 	usb_set_intfdata(iface, NULL);
-	iface->driver = NULL;
 }
 
 /**
@@ -1633,7 +1603,6 @@ EXPORT_SYMBOL(usb_get_dev);
 EXPORT_SYMBOL(usb_hub_tt_clear_buffer);
 
 EXPORT_SYMBOL(usb_driver_claim_interface);
-EXPORT_SYMBOL(usb_interface_claimed);
 EXPORT_SYMBOL(usb_driver_release_interface);
 EXPORT_SYMBOL(usb_match_id);
 EXPORT_SYMBOL(usb_find_interface);

include/linux/usb.h

@@ -31,6 +31,7 @@ static __inline__ void wait_ms(unsigned int ms)
 }
 
 struct usb_device;
+struct usb_driver;
 
 /*-------------------------------------------------------------------------*/
 
@@ -123,7 +124,6 @@ struct usb_interface {
 					 * active alternate setting */
 	unsigned num_altsetting;	/* number of alternate settings */
 
-	struct usb_driver *driver;	/* driver */
 	int minor;			/* minor number this interface is bound to */
 	struct device dev;		/* interface specific device info */
 	struct class_device *class_dev;
@@ -318,7 +318,21 @@ extern int usb_get_current_frame_number (struct usb_device *usb_dev);
 /* used these for multi-interface device registration */
 extern int usb_driver_claim_interface(struct usb_driver *driver,
 			struct usb_interface *iface, void* priv);
-extern int usb_interface_claimed(struct usb_interface *iface);
+
+/**
+ * usb_interface_claimed - returns true iff an interface is claimed
+ * @iface: the interface being checked
+ *
+ * Returns true (nonzero) iff the interface is claimed, else false (zero).
+ * Callers must own the driver model's usb bus readlock.  So driver
+ * probe() entries don't need extra locking, but other call contexts
+ * may need to explicitly claim that lock.
+ *
+ */
+static int inline usb_interface_claimed(struct usb_interface *iface) {
+	return (iface->dev.driver != NULL);
+}
+
 extern void usb_driver_release_interface(struct usb_driver *driver,
 			struct usb_interface *iface);
 const struct usb_device_id *usb_match_id(struct usb_interface *interface,