ChangeSet 1.1325.4.18, 2003/09/24 16:57:17-07:00, david-b@pacbell.net [PATCH] USB: usb_set_configuration() rework (v2) This is the latest update of the patch resolving bugs in how device configurations were reflected in the driver model. It addresses the last significant problems I know about in that area. - Moves code around so that usb_set_configuration() updates sysfs to reflect the current configuration. Previously, that only worked right for the initial configuration chosen by khubd. * Previous interfaces are inaccessible. The code to handle this moved from usb_disconnect() into usb_disable_device(), which is now called both on disconnect and set_configuration paths. * There are new interfaces. The code to handle this moved from usb_new_device() into usb_set_configuration(). * Resolves a double-refcount problem with USB interfaces, by not getting the extra reference in the first place and switching to use device_del() to disable interfaces. * Comments a similar double-refcount problem with usb devices (not interfaces). Its kerneldoc is updated appropriately. The main point being that calling usb_set_configuration() in driver probe() methods is even more of a no-no, since it'll self-deadlock. - Sysfs names for USB interfaces now include the configuation number, so that user mode code can't get as easily confused. Old style: "3-1:0" for configs 2 and 3 (interface zero). New style: "3-1:2.0" for config 2, "3-3:3.0" for config 3. - Moves usb_new_device() code around a bit, so that the device is visible in sysfs before usb_set_configuration() is called. (Before the devices for that config's interfaces appear.) - Makes the bConfigurationValue be writable through sysfs, so device configurations can be easily changed from user mode. (Or devices can be de-configured, by setting config 0.) There are devices that can benefit from this functionality; notably, cdc-acm modems need it right now, so that they can be told to use the non-proprietary configuration. (Since the old "change config in probe callback" trick won't work.) drivers/usb/core/config.c | 3 - drivers/usb/core/driverfs.c | 26 ++++++++- drivers/usb/core/message.c | 126 ++++++++++++++++++++++++++++++++------------ drivers/usb/core/usb.c | 80 ++++++++++----------------- 4 files changed, 148 insertions(+), 87 deletions(-) diff -Nru a/drivers/usb/core/config.c b/drivers/usb/core/config.c --- a/drivers/usb/core/config.c Thu Sep 25 14:31:01 2003 +++ b/drivers/usb/core/config.c Thu Sep 25 14:31:01 2003 @@ -237,9 +237,6 @@ memset(interface, 0, sizeof(struct usb_interface)); interface->dev.release = usb_release_intf; device_initialize(&interface->dev); - - /* put happens in usb_destroy_configuration */ - get_device(&interface->dev); } /* Go through the descriptors, checking their length and counting the diff -Nru a/drivers/usb/core/driverfs.c b/drivers/usb/core/driverfs.c --- a/drivers/usb/core/driverfs.c Thu Sep 25 14:31:01 2003 +++ b/drivers/usb/core/driverfs.c Thu Sep 25 14:31:01 2003 @@ -23,21 +23,43 @@ #include "usb.h" /* Active configuration fields */ -#define usb_actconfig_attr(field, format_string) \ +#define usb_actconfig_show(field, format_string) \ static ssize_t \ show_##field (struct device *dev, char *buf) \ { \ struct usb_device *udev; \ \ udev = to_usb_device (dev); \ + if (udev->actconfig) \ return sprintf (buf, format_string, udev->actconfig->desc.field); \ + else return 0; \ } \ + +#define usb_actconfig_attr(field, format_string) \ +usb_actconfig_show(field,format_string) \ static DEVICE_ATTR(field, S_IRUGO, show_##field, NULL); usb_actconfig_attr (bNumInterfaces, "%2d\n") -usb_actconfig_attr (bConfigurationValue, "%2d\n") usb_actconfig_attr (bmAttributes, "%2x\n") usb_actconfig_attr (bMaxPower, "%3dmA\n") + +/* configuration value is always present, and r/w */ +usb_actconfig_show(bConfigurationValue,"%u\n"); + +static ssize_t +set_bConfigurationValue (struct device *dev, const char *buf, size_t count) +{ + struct usb_device *udev = udev = to_usb_device (dev); + int config, value; + + if (sscanf (buf, "%u", &config) != 1 || config > 255) + return -EINVAL; + value = usb_set_configuration (udev, config); + return (value < 0) ? value : count; +} + +static DEVICE_ATTR(bConfigurationValue, S_IRUGO | S_IWUSR, + show_bConfigurationValue, set_bConfigurationValue); /* String fields */ static ssize_t show_product (struct device *dev, char *buf) diff -Nru a/drivers/usb/core/message.c b/drivers/usb/core/message.c --- a/drivers/usb/core/message.c Thu Sep 25 14:31:01 2003 +++ b/drivers/usb/core/message.c Thu Sep 25 14:31:01 2003 @@ -781,18 +781,40 @@ * @skip_ep0: 0 to disable endpoint 0, 1 to skip it. * * Disables all the device's endpoints, potentially including endpoint 0. - * Deallocates hcd/hardware state for the endpoints ... and nukes all - * pending urbs. + * Deallocates hcd/hardware state for the endpoints (nuking all or most + * pending urbs) and usbcore state for the interfaces, so that usbcore + * must usb_set_configuration() before any interfaces could be used. */ void usb_disable_device(struct usb_device *dev, int skip_ep0) { int i; - dbg("nuking URBs for device %s", dev->dev.bus_id); + dev_dbg(&dev->dev, "%s nuking %s URBs\n", __FUNCTION__, + skip_ep0 ? "non-ep0" : "all"); for (i = skip_ep0; i < 16; ++i) { usb_disable_endpoint(dev, i); usb_disable_endpoint(dev, i + USB_DIR_IN); } + dev->toggle[0] = dev->toggle[1] = 0; + dev->halted[0] = dev->halted[1] = 0; + + /* getting rid of interfaces will disconnect + * any drivers bound to them (a key side effect) + */ + if (dev->actconfig) { + for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) { + struct usb_interface *interface; + + /* remove this interface */ + interface = dev->actconfig->interface[i]; + dev_dbg (&dev->dev, "unregistering interface %s\n", + interface->dev.bus_id); + device_del(&interface->dev); + } + dev->actconfig = 0; + if (dev->state == USB_STATE_CONFIGURED) + dev->state = USB_STATE_ADDRESS; + } } @@ -979,6 +1001,9 @@ int i, retval; struct usb_host_config *config; + /* dev->serialize guards all config changes */ + down(&dev->serialize); + for (i = 1; i < 16; ++i) { usb_disable_endpoint(dev, i); usb_disable_endpoint(dev, i + USB_DIR_IN); @@ -989,8 +1014,10 @@ USB_REQ_SET_CONFIGURATION, 0, config->desc.bConfigurationValue, 0, NULL, 0, HZ * USB_CTRL_SET_TIMEOUT); - if (retval < 0) - return retval; + if (retval < 0) { + dev->state = USB_STATE_ADDRESS; + goto done; + } dev->toggle[0] = dev->toggle[1] = 0; dev->halted[0] = dev->halted[1] = 0; @@ -1002,7 +1029,9 @@ intf->act_altsetting = 0; usb_enable_interface(dev, intf); } - return 0; +done: + up(&dev->serialize); + return (retval < 0) ? retval : 0; } /** @@ -1012,71 +1041,104 @@ * Context: !in_interrupt () * * This is used to enable non-default device modes. Not all devices - * support this kind of configurability. By default, configuration - * zero is selected after enumeration; many devices only have a single + * use this kind of configurability; many devices only have one * configuration. * - * USB devices may support one or more configurations, which affect + * USB device configurations may affect Linux interoperability, * power consumption and the functionality available. For example, * the default configuration is limited to using 100mA of bus power, * so that when certain device functionality requires more power, - * and the device is bus powered, that functionality will be in some + * and the device is bus powered, that functionality should be in some * non-default device configuration. Other device modes may also be * reflected as configuration options, such as whether two ISDN - * channels are presented as independent 64Kb/s interfaces or as one - * bonded 128Kb/s interface. + * channels are available independently; and choosing between open + * standard device protocols (like CDC) or proprietary ones. * * Note that USB has an additional level of device configurability, * associated with interfaces. That configurability is accessed using * usb_set_interface(). * - * This call is synchronous, and may not be used in an interrupt context. + * This call is synchronous. The calling context must be able to sleep, + * and must not hold the driver model lock for USB; usb device driver + * probe() methods may not use this routine. * * Returns zero on success, or else the status code returned by the - * underlying usb_control_msg() call. + * underlying call that failed. On succesful completion, each interface + * in the original device configuration has been destroyed, and each one + * in the new configuration has been probed by all relevant usb device + * drivers currently known to the kernel. */ int usb_set_configuration(struct usb_device *dev, int configuration) { int i, ret; struct usb_host_config *cp = NULL; + /* dev->serialize guards all config changes */ + down(&dev->serialize); + for (i=0; idescriptor.bNumConfigurations; i++) { if (dev->config[i].desc.bConfigurationValue == configuration) { cp = &dev->config[i]; break; } } - if ((!cp && configuration != 0) || (cp && configuration == 0)) { - warn("selecting invalid configuration %d", configuration); - return -EINVAL; + if ((!cp && configuration != 0)) { + ret = -EINVAL; + goto out; } + if (cp && configuration == 0) + dev_warn(&dev->dev, "config 0 descriptor??\n"); - /* if it's already configured, clear out old state first. */ + /* if it's already configured, clear out old state first. + * getting rid of old interfaces means unbinding their drivers. + */ if (dev->state != USB_STATE_ADDRESS) usb_disable_device (dev, 1); // Skip ep0 - dev->toggle[0] = dev->toggle[1] = 0; - dev->halted[0] = dev->halted[1] = 0; - dev->state = USB_STATE_ADDRESS; if ((ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), USB_REQ_SET_CONFIGURATION, 0, configuration, 0, NULL, 0, HZ * USB_CTRL_SET_TIMEOUT)) < 0) - return ret; - if (configuration) - dev->state = USB_STATE_CONFIGURED; - dev->actconfig = cp; + goto out; - /* reset more hc/hcd interface/endpoint state */ - for (i = 0; i < cp->desc.bNumInterfaces; ++i) { - struct usb_interface *intf = cp->interface[i]; + dev->actconfig = cp; + if (!configuration) + dev->state = USB_STATE_ADDRESS; + else { + dev->state = USB_STATE_CONFIGURED; - intf->act_altsetting = 0; - usb_enable_interface(dev, intf); + /* re-initialize hc/hcd/usbcore interface/endpoint state. + * this triggers binding of drivers to interfaces; and + * maybe probe() calls will choose different altsettings. + */ + for (i = 0; i < cp->desc.bNumInterfaces; ++i) { + struct usb_interface *intf = cp->interface[i]; + struct usb_interface_descriptor *desc; + + intf->act_altsetting = 0; + desc = &intf->altsetting [0].desc; + usb_enable_interface(dev, intf); + + intf->dev.parent = &dev->dev; + intf->dev.driver = NULL; + intf->dev.bus = &usb_bus_type; + intf->dev.dma_mask = dev->dev.dma_mask; + sprintf (&intf->dev.bus_id[0], "%d-%s:%d.%d", + dev->bus->busnum, dev->devpath, + configuration, + desc->bInterfaceNumber); + dev_dbg (&dev->dev, + "registering %s (config #%d, interface %d)\n", + intf->dev.bus_id, configuration, + desc->bInterfaceNumber); + device_add (&intf->dev); + usb_create_driverfs_intf_files (intf); + } } - return 0; +out: + up(&dev->serialize); + return ret; } - /** * usb_string - returns ISO 8859-1 version of a string descriptor diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c --- a/drivers/usb/core/usb.c Thu Sep 25 14:31:01 2003 +++ b/drivers/usb/core/usb.c Thu Sep 25 14:31:01 2003 @@ -898,6 +898,7 @@ * this device will fail. */ dev->state = USB_STATE_NOTATTACHED; + down(&dev->serialize); dev_info (&dev->dev, "USB disconnect, address %d\n", dev->devnum); @@ -908,31 +909,23 @@ usb_disconnect(child); } - /* deallocate hcd/hardware state ... and nuke all pending urbs */ + /* deallocate hcd/hardware state ... nuking all pending urbs and + * cleaning up all state associated with the current configuration + */ usb_disable_device(dev, 0); - /* disconnect() drivers from interfaces (a key side effect) */ - dev_dbg (&dev->dev, "unregistering interfaces\n"); - if (dev->actconfig) { - for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) { - struct usb_interface *interface; - - /* remove this interface */ - interface = dev->actconfig->interface[i]; - device_unregister(&interface->dev); - } - } - dev_dbg (&dev->dev, "unregistering device\n"); /* Free the device number and remove the /proc/bus/usb entry */ if (dev->devnum > 0) { clear_bit(dev->devnum, dev->bus->devmap.devicemap); usbfs_remove_device(dev); } + up(&dev->serialize); device_unregister(&dev->dev); /* Decrement the reference count, it'll auto free everything when */ /* it hits 0 which could very well be now */ + /* FIXME the decrement in device_unregister() should suffice ... */ usb_put_dev(dev); } @@ -1090,27 +1083,12 @@ err = usb_get_configuration(dev); if (err < 0) { - dev_err(&dev->dev, "unable to get device %d configuration (error=%d)\n", - dev->devnum, err); - goto fail; - } - - /* choose and set the configuration here */ - if (dev->descriptor.bNumConfigurations != 1) { - dev_info(&dev->dev, - "configuration #%d chosen from %d choices\n", - dev->config[0].desc.bConfigurationValue, - dev->descriptor.bNumConfigurations); - } - err = usb_set_configuration(dev, dev->config[0].desc.bConfigurationValue); - if (err) { - dev_err(&dev->dev, "failed to set device %d default configuration (error=%d)\n", - dev->devnum, err); + dev_err(&dev->dev, "can't read configurations, error %d\n", + err); goto fail; } - /* USB device state == configured ... tell the world! */ - + /* Tell the world! */ dev_dbg(&dev->dev, "new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n", dev->descriptor.iManufacturer, dev->descriptor.iProduct, dev->descriptor.iSerialNumber); @@ -1123,30 +1101,32 @@ usb_show_string(dev, "SerialNumber", dev->descriptor.iSerialNumber); #endif - /* put into sysfs, with device and config specific files */ + /* put device-specific files into sysfs */ err = device_add (&dev->dev); - if (err) + if (err) { + dev_err(&dev->dev, "can't device_add, error %d\n", err); goto fail; + } usb_create_driverfs_dev_files (dev); - /* Register all of the interfaces for this device with the driver core. - * Remember, interfaces get bound to drivers, not devices. */ - for (i = 0; i < dev->actconfig->desc.bNumInterfaces; i++) { - struct usb_interface *interface = dev->actconfig->interface[i]; - struct usb_interface_descriptor *desc; - - desc = &interface->altsetting [interface->act_altsetting].desc; - interface->dev.parent = &dev->dev; - interface->dev.driver = NULL; - interface->dev.bus = &usb_bus_type; - interface->dev.dma_mask = parent->dma_mask; - sprintf (&interface->dev.bus_id[0], "%d-%s:%d", - dev->bus->busnum, dev->devpath, - desc->bInterfaceNumber); - dev_dbg (&dev->dev, "%s - registering interface %s\n", __FUNCTION__, interface->dev.bus_id); - device_add (&interface->dev); - usb_create_driverfs_intf_files (interface); + /* choose and set the configuration. that registers the interfaces + * with the driver core, and lets usb device drivers bind to them. + */ + if (dev->descriptor.bNumConfigurations != 1) { + dev_info(&dev->dev, + "configuration #%d chosen from %d choices\n", + dev->config[0].desc.bConfigurationValue, + dev->descriptor.bNumConfigurations); + } + err = usb_set_configuration(dev, + dev->config[0].desc.bConfigurationValue); + if (err) { + dev_err(&dev->dev, "can't set config #%d, error %d\n", + dev->config[0].desc.bConfigurationValue, err); + goto fail; } + + /* USB device state == configured ... usable */ /* add a /proc/bus/usb entry */ usbfs_add_device(dev);