On Sun, Mar 28, 2004 at 12:37:25AM -0800, David Brownell wrote:
Greg, if you want me to pull my latest version of Duncan's patch to "remove usb_interface.driver" patch out of my BK, let me know. That's one chunk of confusion, not IMO related to this particular problem, but maybe something that you'd want to fix now if you want to fix much of this "for good".
Yes, I'd like that patch, it will help.
OK here it is. Things compile, and they worked for sanity tests with wo drivers that use the claim() mechanism: usbnet, audio (OSS). I suspect this will resolve a cdc-acm problem one person reported.
- Dave
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.
--- 1.23/drivers/usb/core/devices.c Mon Mar 29 09:18:25 2004
+++ edited/drivers/usb/core/devices.c Tue Mar 30 16:50:40 2004
@@ -247,7 +247,9 @@
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;
}
--- 1.59/drivers/usb/core/devio.c Tue Mar 30 01:04:28 2004
+++ edited/drivers/usb/core/devio.c Tue Mar 30 17:19:53 2004
@@ -704,9 +704,9 @@
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_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 @@
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 @@
}
}
- 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) {
+ 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);
+ /* 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
--- 1.47/drivers/usb/core/message.c Tue Mar 30 01:04:29 2004
+++ edited/drivers/usb/core/message.c Tue Mar 30 17:34:54 2004
@@ -1176,15 +1176,34 @@
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);
}
}
--- 1.156/drivers/usb/core/usb.c Tue Mar 30 01:04:29 2004
+++ edited/drivers/usb/core/usb.c Tue Mar 30 17:30:45 2004
@@ -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 @@
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_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 @@
intf->altsetting[0].desc.bInterfaceNumber,
0);
usb_set_intfdata(intf, NULL);
- intf->driver = NULL;
return 0;
}
@@ -290,7 +282,8 @@
/**
* 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 @@
*/
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_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);
--- 1.101/include/linux/usb.h Tue Mar 30 01:04:29 2004
+++ edited/include/linux/usb.h Tue Mar 30 16:49:55 2004
@@ -31,6 +31,7 @@
}
struct usb_device;
+struct usb_driver;
/*-------------------------------------------------------------------------*/
@@ -123,7 +124,6 @@
* 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 @@
/* 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,
