[PATCH] USB: Allocate interface structures dynamically

This is a revised version of an earlier patch; I feel a lot better about
this one.  Basically it does the same thing as before: allocate
interfaces dynamically to avoid the problems with reusing them.

The difference is that this patch adds a struct kref to the array of
usb_interface_cache's, so the array can persist if needed after the
device has been disconnected.  Each interface takes a reference to it
(along with the configuration itself), so as long as the interfaces
remain pinned in memory the altsettings will also remain.

Here is a slight revision of patch as246b.  This one allocates all the new
interfaces before changing any other state; otherwise it's the same.

drivers/usb/core/config.c

@@ -96,19 +96,14 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno, int inum,
 	return buffer - buffer0 + i;
 }
 
-static void usb_free_intf(struct usb_interface *intf)
+static void usb_release_interface_cache(struct kref *ref)
 {
+	struct usb_interface_cache *intfc = ref_to_usb_interface_cache(ref);
 	int j;
 
-	if (intf->altsetting) {
-		for (j = 0; j < intf->num_altsetting; j++) {
-			struct usb_host_interface *alt = &intf->altsetting[j];
-
-			kfree(alt->endpoint);
-		}
-		kfree(intf->altsetting);
-	}
-	kfree(intf);
+	for (j = 0; j < intfc->num_altsetting; j++)
+		kfree(intfc->altsetting[j].endpoint);
+	kfree(intfc);
 }
 
 static int usb_parse_interface(struct device *ddev, int cfgno,
@@ -117,7 +112,7 @@ static int usb_parse_interface(struct device *ddev, int cfgno,
 	unsigned char *buffer0 = buffer;
 	struct usb_interface_descriptor	*d;
 	int inum, asnum;
-	struct usb_interface *interface;
+	struct usb_interface_cache *intfc;
 	struct usb_host_interface *alt;
 	int i, n;
 	int len, retval;
@@ -137,16 +132,16 @@ static int usb_parse_interface(struct device *ddev, int cfgno,
 	if (inum >= config->desc.bNumInterfaces)
 		goto skip_to_next_interface_descriptor;
 
-	interface = config->interface[inum];
+	intfc = config->intf_cache[inum];
 	asnum = d->bAlternateSetting;
-	if (asnum >= interface->num_altsetting) {
+	if (asnum >= intfc->num_altsetting) {
 		dev_err(ddev, "config %d interface %d has an invalid "
 		    "alternate setting number: %d but max is %d\n",
-		    cfgno, inum, asnum, interface->num_altsetting - 1);
+		    cfgno, inum, asnum, intfc->num_altsetting - 1);
 		return -EINVAL;
 	}
 
-	alt = &interface->altsetting[asnum];
+	alt = &intfc->altsetting[asnum];
 	if (alt->desc.bLength) {
 		dev_err(ddev, "Duplicate descriptor for config %d "
 		    "interface %d altsetting %d\n", cfgno, inum, asnum);
@@ -210,11 +205,12 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 	int cfgno;
 	int nintf, nintf_orig;
 	int i, j, n;
-	struct usb_interface *interface;
+	struct usb_interface_cache *intfc;
 	unsigned char *buffer2;
 	int size2;
 	struct usb_descriptor_header *header;
 	int len, retval;
+	u8 nalts[USB_MAXINTERFACES];
 
 	memcpy(&config->desc, buffer, USB_DT_CONFIG_SIZE);
 	if (config->desc.bDescriptorType != USB_DT_CONFIG ||
@@ -237,14 +233,7 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 		    cfgno, nintf, USB_MAXINTERFACES);
 		config->desc.bNumInterfaces = nintf = USB_MAXINTERFACES;
 	}
-
-	for (i = 0; i < nintf; ++i) {
-		interface = config->interface[i] =
-		    kmalloc(sizeof(struct usb_interface), GFP_KERNEL);
-		if (!interface)
-			return -ENOMEM;
-		memset(interface, 0, sizeof(struct usb_interface));
-	}
+	memset(nalts, 0, nintf);
 
 	/* Go through the descriptors, checking their length and counting the
 	 * number of altsettings for each interface */
@@ -277,8 +266,8 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 				    cfgno, i, nintf_orig - 1);
 				return -EINVAL;
 			}
-			if (i < nintf)
-				++config->interface[i]->num_altsetting;
+			if (i < nintf && nalts[i] < 255)
+				++nalts[i];
 
 		} else if (header->bDescriptorType == USB_DT_DEVICE ||
 			    header->bDescriptorType == USB_DT_CONFIG) {
@@ -290,29 +279,29 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 
 	}	/* for ((buffer2 = buffer, size2 = size); ...) */
 
-	/* Allocate the altsetting arrays */
+	/* Allocate the usb_interface_caches and altsetting arrays */
 	for (i = 0; i < nintf; ++i) {
-		interface = config->interface[i];
-		if (interface->num_altsetting > USB_MAXALTSETTING) {
+		j = nalts[i];
+		if (j > USB_MAXALTSETTING) {
 			dev_err(ddev, "too many alternate settings for "
 			    "config %d interface %d: %d, "
 			    "maximum allowed: %d\n",
-			    cfgno, i, interface->num_altsetting,
-			    USB_MAXALTSETTING);
+			    cfgno, i, j, USB_MAXALTSETTING);
 			return -EINVAL;
 		}
-		if (interface->num_altsetting == 0) {
+		if (j == 0) {
 			dev_err(ddev, "config %d has no interface number "
 			    "%d\n", cfgno, i);
 			return -EINVAL;
 		}
 
-		len = sizeof(*interface->altsetting) *
-		    interface->num_altsetting;
-		interface->altsetting = kmalloc(len, GFP_KERNEL);
-		if (!interface->altsetting)
+		len = sizeof(*intfc) + sizeof(struct usb_host_interface) * j;
+		config->intf_cache[i] = intfc = kmalloc(len, GFP_KERNEL);
+		if (!intfc)
 			return -ENOMEM;
-		memset(interface->altsetting, 0, len);
+		memset(intfc, 0, len);
+		intfc->num_altsetting = j;
+		kref_init(&intfc->ref, usb_release_interface_cache);
 	}
 
 	/* Skip over any Class Specific or Vendor Specific descriptors;
@@ -340,9 +329,9 @@ int usb_parse_configuration(struct device *ddev, int cfgidx,
 
 	/* Check for missing altsettings */
 	for (i = 0; i < nintf; ++i) {
-		interface = config->interface[i];
-		for (j = 0; j < interface->num_altsetting; ++j) {
-			if (!interface->altsetting[j].desc.bLength) {
+		intfc = config->intf_cache[i];
+		for (j = 0; j < intfc->num_altsetting; ++j) {
+			if (!intfc->altsetting[j].desc.bLength) {
 				dev_err(ddev, "config %d interface %d has no "
 				    "altsetting %d\n", cfgno, i, j);
 				return -EINVAL;
@@ -374,10 +363,8 @@ void usb_destroy_configuration(struct usb_device *dev)
 		struct usb_host_config *cf = &dev->config[c];
 
 		for (i = 0; i < cf->desc.bNumInterfaces; i++) {
-			struct usb_interface *ifp = cf->interface[i];
-
-			if (ifp)
-				usb_free_intf(ifp);
+			if (cf->intf_cache[i])
+				kref_put(&cf->intf_cache[i]->ref);
 		}
 	}
 	kfree(dev->config);

drivers/usb/core/devices.c

@@ -232,13 +232,21 @@ static char *usb_dump_endpoint_descriptor (
 	return start;
 }
 
-static char *usb_dump_interface_descriptor(char *start, char *end, const struct usb_interface *iface, int setno)
+static char *usb_dump_interface_descriptor(char *start, char *end,
+	const struct usb_interface_cache *intfc,
+	const struct usb_interface *iface,
+	int setno)
 {
-	struct usb_interface_descriptor *desc = &iface->altsetting[setno].desc;
+	struct usb_interface_descriptor *desc = &intfc->altsetting[setno].desc;
+	char *driver_name = "";
 
 	if (start > end)
 		return start;
 	down_read(&usb_bus_type.subsys.rwsem);
+	if (iface)
+		driver_name = (iface->dev.driver
+				? iface->dev.driver->name
+				: "(none)");
 	start += sprintf(start, format_iface,
 			 desc->bInterfaceNumber,
 			 desc->bAlternateSetting,
@@ -247,9 +255,7 @@ static char *usb_dump_interface_descriptor(char *start, char *end, const struct
 			 class_decode(desc->bInterfaceClass),
 			 desc->bInterfaceSubClass,
 			 desc->bInterfaceProtocol,
-			 iface->dev.driver
-				? iface->dev.driver->name
-				: "(none)");
+			 driver_name);
 	up_read(&usb_bus_type.subsys.rwsem);
 	return start;
 }
@@ -258,13 +264,14 @@ static char *usb_dump_interface(
 	int speed,
 	char *start,
 	char *end,
+	const struct usb_interface_cache *intfc,
 	const struct usb_interface *iface,
 	int setno
 ) {
-	struct usb_host_interface *desc = &iface->altsetting[setno];
+	struct usb_host_interface *desc = &intfc->altsetting[setno];
 	int i;
 
-	start = usb_dump_interface_descriptor(start, end, iface, setno);
+	start = usb_dump_interface_descriptor(start, end, intfc, iface, setno);
 	for (i = 0; i < desc->desc.bNumEndpoints; i++) {
 		if (start > end)
 			return start;
@@ -303,6 +310,7 @@ static char *usb_dump_config (
 )
 {
 	int i, j;
+	struct usb_interface_cache *intfc;
 	struct usb_interface *interface;
 
 	if (start > end)
@@ -311,14 +319,13 @@ static char *usb_dump_config (
 		return start + sprintf(start, "(null Cfg. desc.)\n");
 	start = usb_dump_config_descriptor(start, end, &config->desc, active);
 	for (i = 0; i < config->desc.bNumInterfaces; i++) {
+		intfc = config->intf_cache[i];
 		interface = config->interface[i];
-		if (!interface)
-			break;
-		for (j = 0; j < interface->num_altsetting; j++) {
+		for (j = 0; j < intfc->num_altsetting; j++) {
 			if (start > end)
 				return start;
 			start = usb_dump_interface(speed,
-					start, end, interface, j);
+				start, end, intfc, interface, j);
 		}
 	}
 	return start;

drivers/usb/core/message.c

@@ -796,10 +796,6 @@ void usb_disable_interface(struct usb_device *dev, struct usb_interface *intf)
 	}
 }
 
-static void release_interface(struct device *dev)
-{
-}
-
 /*
  * usb_disable_device - Disable all the endpoints for a USB device
  * @dev: the device whose endpoints are being disabled
@@ -835,6 +831,7 @@ void usb_disable_device(struct usb_device *dev, int skip_ep0)
 			dev_dbg (&dev->dev, "unregistering interface %s\n",
 				interface->dev.bus_id);
 			device_unregister (&interface->dev);
+			dev->actconfig->interface[i] = NULL;
 		}
 		dev->actconfig = 0;
 		if (dev->state == USB_STATE_CONFIGURED)
@@ -1071,6 +1068,16 @@ int usb_reset_configuration(struct usb_device *dev)
 	return 0;
 }
 
+static void release_interface(struct device *dev)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct usb_interface_cache *intfc =
+			altsetting_to_usb_interface_cache(intf->altsetting);
+
+	kref_put(&intfc->ref);
+	kfree(intf);
+}
+
 /*
  * usb_set_configuration - Makes a particular device setting be current
  * @dev: the device whose configuration is being updated
@@ -1109,19 +1116,19 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 {
 	int i, ret;
 	struct usb_host_config *cp = NULL;
+	struct usb_interface *new_interfaces[USB_MAXINTERFACES];
+	int n;
 
 	/* dev->serialize guards all config changes */
 
-	for (i=0; i<dev->descriptor.bNumConfigurations; i++) {
+	for (i = 0; i < dev->descriptor.bNumConfigurations; i++) {
 		if (dev->config[i].desc.bConfigurationValue == configuration) {
 			cp = &dev->config[i];
 			break;
 		}
 	}
-	if ((!cp && configuration != 0)) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if ((!cp && configuration != 0))
+		return -EINVAL;
 
 	/* The USB spec says configuration 0 means unconfigured.
 	 * But if a device includes a configuration numbered 0,
@@ -1130,6 +1137,25 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 	if (cp && configuration == 0)
 		dev_warn(&dev->dev, "config 0 descriptor??\n");
 
+	/* Allocate memory for new interfaces before doing anything else,
+	 * so that if we run out then nothing will have changed. */
+	n = 0;
+	if (cp) {
+		for (; n < cp->desc.bNumInterfaces; ++n) {
+			new_interfaces[n] = kmalloc(
+					sizeof(struct usb_interface),
+					GFP_KERNEL);
+			if (!new_interfaces[n]) {
+				dev_err(&dev->dev, "Out of memory");
+				ret = -ENOMEM;
+free_interfaces:
+				while (--n >= 0)
+					kfree(new_interfaces[n]);
+				return ret;
+			}
+		}
+	}
+
 	/* if it's already configured, clear out old state first.
 	 * getting rid of old interfaces means unbinding their drivers.
 	 */
@@ -1139,7 +1165,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 	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)
-		goto out;
+		goto free_interfaces;
 
 	dev->actconfig = cp;
 	if (!cp)
@@ -1152,9 +1178,17 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		 * 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_cache *intfc;
+			struct usb_interface *intf;
 			struct usb_host_interface *alt;
 
+			cp->interface[i] = intf = new_interfaces[i];
+			memset(intf, 0, sizeof(*intf));
+			intfc = cp->intf_cache[i];
+			intf->altsetting = intfc->altsetting;
+			intf->num_altsetting = intfc->num_altsetting;
+			kref_get(&intfc->ref);
+
 			alt = usb_altnum_to_altsetting(intf, 0);
 
 			/* No altsetting 0?  We'll assume the first altsetting.
@@ -1204,7 +1238,6 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
 		}
 	}
 
-out:
 	return ret;
 }
 

drivers/usb/core/usb.c

@@ -1127,7 +1127,7 @@ int usb_new_device(struct usb_device *dev)
 			/* heuristic:  Linux is more likely to have class
 			 * drivers, so avoid vendor-specific interfaces.
 			 */
-			desc = &dev->config[i].interface[0]
+			desc = &dev->config[i].intf_cache[0]
 					->altsetting->desc;
 			if (desc->bInterfaceClass == USB_CLASS_VENDOR_SPEC)
 				continue;

include/linux/usb.h

@@ -147,12 +147,43 @@ void usb_put_intf(struct usb_interface *intf);
 /* this maximum is arbitrary */
 #define USB_MAXINTERFACES	32
 
+/**
+ * struct usb_interface_cache - long-term representation of a device interface
+ * @num_altsetting: number of altsettings defined.
+ * @ref: reference counter.
+ * @altsetting: variable-length array of interface structures, one for
+ *	each alternate setting that may be selected.  Each one includes a
+ *	set of endpoint configurations.  They will be in no particular order.
+ *
+ * These structures persist for the lifetime of a usb_device, unlike
+ * struct usb_interface (which persists only as long as its configuration
+ * is installed).  The altsetting arrays can be accessed through these
+ * structures at any time, permitting comparison of configurations and
+ * providing support for the /proc/bus/usb/devices pseudo-file.
+ */
+struct usb_interface_cache {
+	unsigned num_altsetting;	/* number of alternate settings */
+	struct kref ref;		/* reference counter */
+
+	/* variable-length array of alternate settings for this interface,
+	 * stored in no particular order */
+	struct usb_host_interface altsetting[0];
+};
+#define	ref_to_usb_interface_cache(r) \
+		container_of(r, struct usb_interface_cache, ref)
+#define	altsetting_to_usb_interface_cache(a) \
+		container_of(a, struct usb_interface_cache, altsetting[0])
+
 /**
  * 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.
+ * @interface: array of pointers to usb_interface structures, one for each
+ *	interface in the configuration.  The number of interfaces is stored
+ *	in desc.bNumInterfaces.  These pointers are valid only while the
+ *	the configuration is active.
+ * @intf_cache: array of pointers to usb_interface_cache structures, one
+ *	for each interface in the configuration.  These structures exist
+ *	for the entire life of the device.
  * @extra: pointer to buffer containing all extra descriptors associated
  *	with this configuration (those preceding the first interface
  *	descriptor).
@@ -186,6 +217,10 @@ struct usb_host_config {
 	 * stored in no particular order */
 	struct usb_interface *interface[USB_MAXINTERFACES];
 
+	/* Interface information available even when this is not the
+	 * active configuration */
+	struct usb_interface_cache *intf_cache[USB_MAXINTERFACES];
+
 	unsigned char *extra;   /* Extra descriptors */
 	int extralen;
 };