ChangeSet 1.1371.759.37, 2004/04/27 16:02:32-07:00, stern@rowland.harvard.edu [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 | 75 ++++++++++++++++++--------------------------- drivers/usb/core/devices.c | 31 +++++++++++------- drivers/usb/core/message.c | 59 +++++++++++++++++++++++++++-------- drivers/usb/core/usb.c | 2 - include/linux/usb.h | 41 ++++++++++++++++++++++-- 5 files changed, 135 insertions(+), 73 deletions(-) diff -Nru a/drivers/usb/core/config.c b/drivers/usb/core/config.c --- a/drivers/usb/core/config.c Fri May 14 15:31:52 2004 +++ b/drivers/usb/core/config.c Fri May 14 15:31:52 2004 @@ -96,19 +96,14 @@ 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 @@ 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 @@ 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 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 @@ 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 @@ 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 @@ } /* 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 @@ /* 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 @@ 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); diff -Nru a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c --- a/drivers/usb/core/devices.c Fri May 14 15:31:52 2004 +++ b/drivers/usb/core/devices.c Fri May 14 15:31:52 2004 @@ -232,13 +232,21 @@ 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 @@ 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 @@ 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 @@ ) { int i, j; + struct usb_interface_cache *intfc; struct usb_interface *interface; if (start > end) @@ -311,14 +319,13 @@ 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; @@ -395,7 +402,7 @@ return start; start = usb_dump_device_strings (start, end, dev); - + for (i = 0; i < dev->descriptor.bNumConfigurations; i++) { if (start > end) return start; diff -Nru a/drivers/usb/core/message.c b/drivers/usb/core/message.c --- a/drivers/usb/core/message.c Fri May 14 15:31:52 2004 +++ b/drivers/usb/core/message.c Fri May 14 15:31:52 2004 @@ -796,10 +796,6 @@ } } -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 @@ 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 @@ 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 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; idescriptor.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 @@ 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 @@ 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 @@ * 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 @@ } } -out: return ret; } diff -Nru a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c --- a/drivers/usb/core/usb.c Fri May 14 15:31:52 2004 +++ b/drivers/usb/core/usb.c Fri May 14 15:31:52 2004 @@ -1127,7 +1127,7 @@ /* 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; diff -Nru a/include/linux/usb.h b/include/linux/usb.h --- a/include/linux/usb.h Fri May 14 15:31:52 2004 +++ b/include/linux/usb.h Fri May 14 15:31:52 2004 @@ -148,11 +148,42 @@ #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). @@ -185,6 +216,10 @@ /* the interfaces associated with this configuration, * 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;