[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

@@ -783,13 +783,12 @@ void usb_disable_endpoint(struct usb_device *dev, unsigned int epaddr)
  */
 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_endpoint(struct usb_device *dev,
 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 @@ void usb_enable_interface(struct usb_device *dev,
 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 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate)
 		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 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate)
 	/* 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 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate)
 	 * 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 @@ int usb_reset_configuration(struct usb_device *dev)
 	/* 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 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		 */
 		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 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 			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);
 		}

drivers/usb/core/usb.c

@@ -189,7 +189,7 @@ void usb_deregister(struct usb_driver *driver)
 }
 
 /**
- * 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
  *
@@ -219,6 +219,33 @@ struct usb_interface *usb_ifnum_to_if(struct usb_device *dev, unsigned ifnum)
 	return NULL;
 }
 
+/**
+ * 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
@@ -247,7 +274,7 @@ usb_epnum_to_ep_desc(struct usb_device *dev, unsigned epnum)
 
 		/* 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 @@ usb_match_id(struct usb_interface *interface, const struct usb_device_id *id)
 	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 @@ static int usb_hotplug (struct device *dev, char **envp, int num_envp,
 	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 @@ static int usb_hotplug (struct device *dev, char **envp, int num_envp,
 		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_driver_release_interface);
 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);

include/linux/usb.h

@@ -72,14 +72,15 @@ struct usb_host_interface {
 
 /**
  * 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 @@ struct usb_host_interface {
  * 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 @@ static inline void usb_set_intfdata (struct usb_interface *intf, void *data)
 /* 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_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.
+ * 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.
+ *
+ * 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 @@ extern void usb_driver_release_interface(struct usb_driver *driver,
 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);
 
 
 /**