I had a chance to take look at your patch(es). They didn't apply cleanly on my tree, and I noticed some issues ... so here's an updated version, without the driver-specific patches.
I've tested this with a cdc-ether device and "usbtest", as well as drivers that shouldn't use any of the new codepaths. They behaved just fine.
- Dave
[USB] usbcore driver model updates: remove usb_interface.driver
This updates Duncan Sands' patch to remove usb_interface.driver. After
applying this patch, I think usbcore will no longer have significant
disconnects with driver model binding, or the locking related to it.
This affects usbfs and the other multi-interface drivers (cdc ethernet and
acm; oss and alsa audio). They previously relied on a strange "half-bound"
interface state, which didn't get disconnect() calls. Patches for those
other drivers should be integrated before this one.
The changes are:
- remove usb_interface.driver (duplicates device.driver)
- more fixes to usbfs locking:
* fix places where bus rwsem should have been used
* removes more BKL abuse
- saner reset processing
* replaces err("this is broken") codepath
* depends on my reset-01xx.patch
- completes the proc_ioctl() update
The usbfs locking and proc_ioctl updates are mostly from a previous patch
I sent around; but I removed most of its dev->serialize fixes. (Duncan
has other usbfs updates pending, many relating to that semaphore.)
--- 1.22/drivers/usb/core/devices.c Tue Jul 29 04:28:37 2003
+++ edited/drivers/usb/core/devices.c Mon Jan 19 15:00:33 2004
@@ -238,7 +238,7 @@
if (start > end)
return start;
- lock_kernel(); /* driver might be unloaded */
+ down_read(&usb_bus_type.subsys.rwsem);
start += sprintf(start, format_iface,
desc->bInterfaceNumber,
desc->bAlternateSetting,
@@ -247,8 +247,10 @@
class_decode(desc->bInterfaceClass),
desc->bInterfaceSubClass,
desc->bInterfaceProtocol,
- iface->driver ? iface->driver->name : "(none)");
- unlock_kernel();
+ iface->dev.driver
+ ? iface->dev.driver->name
+ : "(none)");
+ up_read(&usb_bus_type.subsys.rwsem);
return start;
}
--- 1.55/drivers/usb/core/devio.c Fri Dec 5 14:42:24 2003
+++ edited/drivers/usb/core/devio.c Mon Jan 19 18:19:33 2004
@@ -363,13 +363,15 @@
return 0;
iface = dev->actconfig->interface[intf];
err = -EBUSY;
- lock_kernel();
+
+ /* lock against other changes to driver bindings */
+ down_write(&usb_bus_type.subsys.rwsem);
if (!usb_interface_claimed(iface)) {
usb_driver_claim_interface(&usbdevfs_driver, iface, ps);
set_bit(intf, &ps->ifclaimed);
err = 0;
}
- unlock_kernel();
+ up_write(&usb_bus_type.subsys.rwsem);
return err;
}
@@ -384,11 +386,14 @@
err = -EINVAL;
dev = ps->dev;
down(&dev->serialize);
+ /* lock against other changes to driver bindings */
+ down_write(&usb_bus_type.subsys.rwsem);
if (test_and_clear_bit(intf, &ps->ifclaimed)) {
iface = dev->actconfig->interface[intf];
usb_driver_release_interface(&usbdevfs_driver, iface);
err = 0;
}
+ up_write(&usb_bus_type.subsys.rwsem);
up(&dev->serialize);
return err;
}
@@ -691,9 +696,9 @@
interface = usb_ifnum_to_if(ps->dev, gd.interface);
if (!interface)
return -EINVAL;
- 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;
@@ -710,28 +715,26 @@
return 0;
}
+extern int __usb_reset_device(struct usb_device *);
+
static int proc_resetdevice(struct dev_state *ps)
{
- int i, ret;
-
- ret = usb_reset_device(ps->dev);
- if (ret < 0)
- return ret;
+ int i, ret = -EBUSY;
+ /* don't reset unless no other driver is involved */
+ down(&ps->dev->serialize);
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);
- }
- }
-
- return 0;
+ if (usb_interface_claimed(intf)
+ && !test_bit(i, &ps->ifclaimed))
+ goto done;
+ }
+ ret = __usb_reset_device(ps->dev);
+ /* device might have vanished ... */
+done:
+ up(&ps->dev->serialize);
+ return ret;
}
static int proc_setintf(struct dev_state *ps, void __user *arg)
@@ -747,7 +750,7 @@
interface = usb_ifnum_to_if(ps->dev, setintf.interface);
if (!interface)
return -EINVAL;
- if (interface->driver) {
+ if (interface->dev.driver) {
if ((ret = checkintf(ps, ret)))
return ret;
}
@@ -1089,58 +1092,52 @@
}
}
- 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;
+ /* 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);
if (driver) {
- dbg ("disconnect '%s' from dev %d interface %d",
- driver->name, ps->dev->devnum, ctrl.ifno);
- usb_unbind_interface(&ifp->dev);
+ 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.40/drivers/usb/core/message.c Sun Jan 4 11:46:27 2004
+++ edited/drivers/usb/core/message.c Mon Jan 19 15:00:33 2004
@@ -1141,6 +1141,15 @@
dev->bus->busnum, dev->devpath,
configuration,
desc->bInterfaceNumber);
+ }
+ /* Now that all interfaces are setup, any of them can be
+ * meaningfully accessed from the probe() call for another
+ */
+ 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",
intf->dev.bus_id, configuration,
--- 1.151/drivers/usb/core/usb.c Sun Jan 4 14:08:50 2004
+++ edited/drivers/usb/core/usb.c Mon Jan 19 15:00:33 2004
@@ -93,17 +93,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;
}
@@ -112,7 +106,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);
@@ -125,7 +119,6 @@
intf->altsetting[0].desc.bInterfaceNumber,
0);
usb_set_intfdata(intf, NULL);
- intf->driver = NULL;
return 0;
}
@@ -254,7 +247,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
@@ -272,75 +266,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 registered, 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. Since it calls disconnect,
+ * drivers that use it from their disconnect method need to protect
+ * themselves against infinite recursion.
*
* 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)
{
+ 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;
- }
+ /* in case we're called before dev_add() */
+ if (!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;
}
/**
@@ -1487,7 +1458,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.93/include/linux/usb.h Sat Jan 3 16:18:59 2004
+++ edited/include/linux/usb.h Mon Jan 19 15:00:33 2004
@@ -31,6 +31,7 @@
}
struct usb_device;
+struct usb_driver;
/*-------------------------------------------------------------------------*/
@@ -118,7 +119,6 @@
unsigned act_altsetting; /* 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;
@@ -288,7 +288,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,
