ChangeSet 1.1673.8.64, 2004/03/31 13:35:04-08:00, david-b@pacbell.net [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 | 4 +- drivers/usb/core/devio.c | 88 ++++++++++++++++----------------------------- drivers/usb/core/message.c | 25 +++++++++++- drivers/usb/core/usb.c | 87 ++++++++++++++------------------------------ include/linux/usb.h | 18 ++++++++- 5 files changed, 102 insertions(+), 120 deletions(-) diff -Nru a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c --- a/drivers/usb/core/devices.c Wed Apr 14 14:34:40 2004 +++ b/drivers/usb/core/devices.c Wed Apr 14 14:34: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; } diff -Nru a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c --- a/drivers/usb/core/devio.c Wed Apr 14 14:34:40 2004 +++ b/drivers/usb/core/devio.c Wed Apr 14 14:34:40 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 diff -Nru a/drivers/usb/core/message.c b/drivers/usb/core/message.c --- a/drivers/usb/core/message.c Wed Apr 14 14:34:40 2004 +++ b/drivers/usb/core/message.c Wed Apr 14 14:34:40 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); } } diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c --- a/drivers/usb/core/usb.c Wed Apr 14 14:34:40 2004 +++ b/drivers/usb/core/usb.c Wed Apr 14 14:34:40 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); diff -Nru a/include/linux/usb.h b/include/linux/usb.h --- a/include/linux/usb.h Wed Apr 14 14:34:40 2004 +++ b/include/linux/usb.h Wed Apr 14 14:34:40 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,