ChangeSet 1.1608.24.35, 2004/03/03 12:49:44-08:00, stern@rowland.harvard.edu [PATCH] USB: Improve handling of altsettings On Sat, 21 Feb 2004, Greg KH wrote: > > One thing that would be good, whether this change gets made or not, is to > > remove all assumptions from drivers about the order in which interfaces > > are stored (use usb_ifnum_to_if()) and the order in which altsettings are > > stored (replace intf.act_altsetting with a pointer and create > > usb_altnum_to_alt() analogous to usb_ifnum_to_if()). There are plenty of > > drivers that will need to be fixed up. > > I'd be glad to take patches to fix up any drivers that still have this > problem right now. Here's a start. This patch begins the conversion process by adding usbcore support for cur_altsetting and deprecating act_altsetting. So long as we assumed that altsetting numbers range from 0 to num_altsetting-1 and that the number matches its index in the altsetting array, there was no harm in using act_altsetting. But without that assumption act_altsetting is merely an invitation to errors. Although the kerneldoc says that act_altsetting is the _index_ of the active altsetting, it's all too easy to confuse it with the _number_ of the active altsetting. Using cur_altsetting instead (a pointer rather than a number) will prevent that confusion. Until all the drivers have been converted to use cur_altsetting, the core will have to maintain act_altsetting in parallel with it. Eventually we will be able to remove act_altsetting, but fixing all the drivers will take a while. Included in this patch: Add cur_altsetting to struct usb_interface and deprecate act_altsetting. Add comments and kerneldoc explaining the changes. Also remove the comments in front of struct usb_host_config (they seem to have been left behind when usb_ch9.h was split out) and add kerneldoc for that structure. Add usb_altnum_to_altsetting() to help look up altsettings based on their number. Convert the usb_set_interface(), usb_set_configuration(), and usb_reset_configuration() routines to support cur_altsetting and act_altsetting in parallel. Convert a few others to use cur_altsetting rather than act_altsetting. Rename a few local variables to make their meaning a little clearer. It would be nice to change struct usb_host_interface to something like usb_host_altsetting, but that's a patch for another time. drivers/usb/core/message.c | 68 ++++++++++++++++++++++++++-------------- drivers/usb/core/usb.c | 42 ++++++++++++++++++++---- include/linux/usb.h | 76 +++++++++++++++++++++++++++++++++------------ 3 files changed, 135 insertions(+), 51 deletions(-) diff -Nru a/drivers/usb/core/message.c b/drivers/usb/core/message.c --- a/drivers/usb/core/message.c Tue Mar 16 15:04:41 2004 +++ b/drivers/usb/core/message.c Tue Mar 16 15:04:41 2004 @@ -783,13 +783,12 @@ */ void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf) { - struct usb_host_interface *hintf = - &intf->altsetting[intf->act_altsetting]; + struct usb_host_interface *alt = intf->cur_altsetting; int i; - for (i = 0; i < hintf->desc.bNumEndpoints; ++i) { + for (i = 0; i < alt->desc.bNumEndpoints; ++i) { usb_disable_endpoint(dev, - hintf->endpoint[i].desc.bEndpointAddress); + alt->endpoint[i].desc.bEndpointAddress); } } @@ -887,12 +886,11 @@ void usb_enable_interface(struct usb_device *dev, struct usb_interface *intf) { - struct usb_host_interface *hintf = - &intf->altsetting[intf->act_altsetting]; + struct usb_host_interface *alt = intf->cur_altsetting; int i; - for (i = 0; i < hintf->desc.bNumEndpoints; ++i) - usb_enable_endpoint(dev, &hintf->endpoint[i].desc); + for (i = 0; i < alt->desc.bNumEndpoints; ++i) + usb_enable_endpoint(dev, &alt->endpoint[i].desc); } /** @@ -931,6 +929,7 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) { struct usb_interface *iface; + struct usb_host_interface *alt; int ret; int manual = 0; @@ -940,14 +939,15 @@ return -EINVAL; } - if (alternate < 0 || alternate >= iface->num_altsetting) + alt = usb_altnum_to_altsetting(iface, alternate); + if (!alt) { + warn("selecting invalid altsetting %d", alternate); return -EINVAL; + } ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), USB_REQ_SET_INTERFACE, USB_RECIP_INTERFACE, - iface->altsetting[alternate] - .desc.bAlternateSetting, - interface, NULL, 0, HZ * 5); + alternate, interface, NULL, 0, HZ * 5); /* 9.4.10 says devices don't need this and are free to STALL the * request if the interface only has one alternate setting. @@ -968,7 +968,8 @@ /* prevent submissions using previous endpoint settings */ usb_disable_interface(dev, iface); - iface->act_altsetting = alternate; + iface->cur_altsetting = alt; + iface->act_altsetting = alt - iface->altsetting; /* If the interface only has one altsetting and the device didn't * accept the request, we attempt to carry out the equivalent action @@ -976,13 +977,11 @@ * new altsetting. */ if (manual) { - struct usb_host_interface *iface_as = - &iface->altsetting[alternate]; int i; - for (i = 0; i < iface_as->desc.bNumEndpoints; i++) { + for (i = 0; i < alt->desc.bNumEndpoints; i++) { unsigned int epaddr = - iface_as->endpoint[i].desc.bEndpointAddress; + alt->endpoint[i].desc.bEndpointAddress; unsigned int pipe = __create_pipe(dev, USB_ENDPOINT_NUMBER_MASK & epaddr) | (usb_endpoint_out(epaddr) ? USB_DIR_OUT : USB_DIR_IN); @@ -1056,8 +1055,20 @@ /* re-init hc/hcd interface/endpoint state */ for (i = 0; i < config->desc.bNumInterfaces; i++) { struct usb_interface *intf = config->interface[i]; + struct usb_host_interface *alt; + + alt = usb_altnum_to_altsetting(intf, 0); + + /* No altsetting 0? We'll assume the first altsetting. + * We could use a GetInterface call, but if a device is + * so non-compliant that it doesn't have altsetting 0 + * then I wouldn't trust its reply anyway. + */ + if (!alt) + alt = &intf->altsetting[0]; - intf->act_altsetting = 0; + intf->cur_altsetting = alt; + intf->act_altsetting = alt - intf->altsetting; usb_enable_interface(dev, intf); } return 0; @@ -1146,12 +1157,21 @@ */ for (i = 0; i < cp->desc.bNumInterfaces; ++i) { struct usb_interface *intf = cp->interface[i]; - struct usb_interface_descriptor *desc; + struct usb_host_interface *alt; - intf->act_altsetting = 0; - desc = &intf->altsetting [0].desc; - usb_enable_interface(dev, intf); + alt = usb_altnum_to_altsetting(intf, 0); + + /* No altsetting 0? We'll assume the first altsetting. + * We could use a GetInterface call, but if a device is + * so non-compliant that it doesn't have altsetting 0 + * then I wouldn't trust its reply anyway. + */ + if (!alt) + alt = &intf->altsetting[0]; + intf->cur_altsetting = alt; + intf->act_altsetting = alt - intf->altsetting; + usb_enable_interface(dev, intf); intf->dev.parent = &dev->dev; intf->dev.driver = NULL; intf->dev.bus = &usb_bus_type; @@ -1160,11 +1180,11 @@ sprintf (&intf->dev.bus_id[0], "%d-%s:%d.%d", dev->bus->busnum, dev->devpath, configuration, - desc->bInterfaceNumber); + alt->desc.bInterfaceNumber); dev_dbg (&dev->dev, "registering %s (config #%d, interface %d)\n", intf->dev.bus_id, configuration, - desc->bInterfaceNumber); + alt->desc.bInterfaceNumber); device_register (&intf->dev); 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 Tue Mar 16 15:04:41 2004 +++ b/drivers/usb/core/usb.c Tue Mar 16 15:04:41 2004 @@ -189,7 +189,7 @@ } /** - * usb_ifnum_to_if - get the interface object with a given interface number (usbcore-internal) + * usb_ifnum_to_if - get the interface object with a given interface number * @dev: the device whose current configuration is considered * @ifnum: the desired interface * @@ -220,6 +220,33 @@ } /** + * usb_altnum_to_altsetting - get the altsetting structure with a given + * alternate setting number. + * @intf: the interface containing the altsetting in question + * @altnum: the desired alternate setting number + * + * This searches the altsetting array of the specified interface for + * an entry with the correct bAlternateSetting value and returns a pointer + * to that entry, or null. + * + * Note that altsettings need not be stored sequentially by number, so + * it would be incorrect to assume that the first altsetting entry in + * the array corresponds to altsetting zero. This routine helps device + * drivers avoid such mistakes. + */ +struct usb_host_interface *usb_altnum_to_altsetting(struct usb_interface *intf, + unsigned int altnum) +{ + int i; + + for (i = 0; i < intf->num_altsetting; i++) { + if (intf->altsetting[i].desc.bAlternateSetting == altnum) + return &intf->altsetting[i]; + } + return NULL; +} + +/** * usb_epnum_to_ep_desc - get the endpoint object with a given endpoint number * @dev: the device whose current configuration+altsettings is considered * @epnum: the desired endpoint, masked with USB_DIR_IN as appropriate. @@ -247,7 +274,7 @@ /* only endpoints in current altsetting are active */ intf = config->interface[i]; - alt = intf->altsetting + intf->act_altsetting; + alt = intf->cur_altsetting; for (k = 0; k < alt->desc.bNumEndpoints; k++) if (epnum == alt->endpoint[k].desc.bEndpointAddress) @@ -421,7 +448,7 @@ if (id == NULL) return NULL; - intf = &interface->altsetting [interface->act_altsetting]; + intf = interface->cur_altsetting; dev = interface_to_usbdev(interface); /* It is important to check that id->driver_info is nonzero, @@ -624,7 +651,7 @@ scratch += length; if (usb_dev->descriptor.bDeviceClass == 0) { - int alt = intf->act_altsetting; + struct usb_host_interface *alt = intf->cur_altsetting; /* 2.4 only exposed interface zero. in 2.5, hotplug * agents are called for all interfaces, and can use @@ -633,9 +660,9 @@ envp [i++] = scratch; length += snprintf (scratch, buffer_size - length, "INTERFACE=%d/%d/%d", - intf->altsetting[alt].desc.bInterfaceClass, - intf->altsetting[alt].desc.bInterfaceSubClass, - intf->altsetting[alt].desc.bInterfaceProtocol); + alt->desc.bInterfaceClass, + alt->desc.bInterfaceSubClass, + alt->desc.bInterfaceProtocol); if ((buffer_size - length <= 0) || (i >= num_envp)) return -ENOMEM; ++length; @@ -1582,6 +1609,7 @@ EXPORT_SYMBOL(usb_match_id); EXPORT_SYMBOL(usb_find_interface); EXPORT_SYMBOL(usb_ifnum_to_if); +EXPORT_SYMBOL(usb_altnum_to_altsetting); EXPORT_SYMBOL(usb_reset_device); EXPORT_SYMBOL(usb_disconnect); diff -Nru a/include/linux/usb.h b/include/linux/usb.h --- a/include/linux/usb.h Tue Mar 16 15:04:41 2004 +++ b/include/linux/usb.h Tue Mar 16 15:04:41 2004 @@ -72,14 +72,15 @@ /** * struct usb_interface - what usb device drivers talk to - * @altsetting: array of interface descriptors, one for each alternate + * @altsetting: array of interface structures, one for each alternate * setting that may be selected. Each one includes a set of - * endpoint configurations and will be in numberic order, - * 0..num_altsetting. + * endpoint configurations. They will be in no particular order. * @num_altsetting: number of altsettings defined. - * @act_altsetting: index of current altsetting. this number is always - * less than num_altsetting. after the device is configured, each + * @cur_altsetting: the current altsetting. + * @act_altsetting: index of current altsetting. This number is always + * less than num_altsetting. After the device is configured, each * interface uses its default setting of zero. + * NOTE: act_altsetting is deprecated. Use cur_altsetting instead. * @driver: the USB driver that is bound to this interface. * @minor: the minor number assigned to this interface, if this * interface is bound to a driver that uses the USB major number. @@ -104,20 +105,27 @@ * calls such as dev_get_drvdata() on the dev member of this structure. * * Each interface may have alternate settings. The initial configuration - * of a device sets the first of these, but the device driver can change + * of a device sets altsetting 0, but the device driver can change * that setting using usb_set_interface(). Alternate settings are often * used to control the the use of periodic endpoints, such as by having * different endpoints use different amounts of reserved USB bandwidth. * All standards-conformant USB devices that use isochronous endpoints * will use them in non-default settings. + * + * The USB specification says that alternate setting numbers must run from + * 0 to one less than the total number of alternate settings. But some + * devices manage to mess this up, and the structures aren't necessarily + * stored in numerical order anyhow. Use usb_altnum_to_altsetting() to + * look up an alternate setting in the altsetting array based on its number. */ struct usb_interface { - /* array of alternate settings for this interface. - * these will be in numeric order, 0..num_altsettting - */ + /* array of alternate settings for this interface, + * stored in no particular order */ struct usb_host_interface *altsetting; - unsigned act_altsetting; /* active alternate setting */ + struct usb_host_interface *cur_altsetting; /* the currently + * active alternate setting */ + unsigned act_altsetting; /* index of active alternate setting: DEPRECATED */ unsigned num_altsetting; /* number of alternate settings */ struct usb_driver *driver; /* driver */ @@ -143,19 +151,43 @@ /* this maximum is arbitrary */ #define USB_MAXINTERFACES 32 -/* USB_DT_CONFIG: Configuration descriptor information. +/** + * struct usb_host_config - representation of a device's configuration + * @desc: the device's configuration descriptor. + * @interface: array of usb_interface structures, one for each interface + * in the configuration. The number of interfaces is stored in + * desc.bNumInterfaces. + * @extra: pointer to buffer containing all extra descriptors associated + * with this configuration (those preceding the first interface + * descriptor). + * @extralen: length of the extra descriptors buffer. + * + * USB devices may have multiple configurations, but only one can be active + * at any time. Each encapsulates a different operational environment; + * for example, a dual-speed device would have separate configurations for + * full-speed and high-speed operation. The number of configurations + * available is stored in the device descriptor as bNumConfigurations. * - * USB_DT_OTHER_SPEED_CONFIG is the same descriptor, except that the - * descriptor type is different. Highspeed-capable devices can look - * different depending on what speed they're currently running. Only - * devices with a USB_DT_DEVICE_QUALIFIER have an OTHER_SPEED_CONFIG. + * A configuration can contain multiple interfaces. Each corresponds to + * a different function of the USB device, and all are available whenever + * the configuration is active. The USB standard says that interfaces + * are supposed to be numbered from 0 to desc.bNumInterfaces-1, but a lot + * of devices get this wrong. In addition, the interface array is not + * guaranteed to be sorted in numerical order. Use usb_ifnum_to_if() to + * look up an interface entry based on its number. + * + * Device drivers should not attempt to activate configurations. The choice + * of which configuration to install is a policy decision based on such + * considerations as available power, functionality provided, and the user's + * desires (expressed through hotplug scripts). However, drivers can call + * usb_reset_configuration() to reinitialize the current configuration and + * all its interfaces. */ struct usb_host_config { struct usb_config_descriptor desc; - /* the interfaces associated with this configuration - * these will be in numeric order, 0..desc.bNumInterfaces - */ + /* the interfaces associated with this configuration, + * stored in no particular order */ struct usb_interface *interface[USB_MAXINTERFACES]; unsigned char *extra; /* Extra descriptors */ @@ -297,8 +329,12 @@ const struct usb_device_id *usb_match_id(struct usb_interface *interface, const struct usb_device_id *id); -extern struct usb_interface *usb_find_interface(struct usb_driver *drv, int minor); -extern struct usb_interface *usb_ifnum_to_if(struct usb_device *dev, unsigned ifnum); +extern struct usb_interface *usb_find_interface(struct usb_driver *drv, + int minor); +extern struct usb_interface *usb_ifnum_to_if(struct usb_device *dev, + unsigned ifnum); +extern struct usb_host_interface *usb_altnum_to_altsetting( + struct usb_interface *intf, unsigned int altnum); /**