Greg:
This patch implements what I described earlier: allocating interfaces
dynamically to avoid the problems involved in reusing them. It's a
sizeable patch and careful testing is called for.
Several issues of varying importance came up while I was working on this.
(1) The locking in devices.c needs to be fixed. The USB subsystem
rwsem should be held over a much larger part of the code. That should be
pretty easy to do. I don't think it's necessary to use usb_get_dev or to
lock usbdev->serialize, but someone should verify this.
(2) The exact relationships among usbdev->state, usbdev->actconfig,
and the actual state of the physical device aren't clear. Various error
scenarios can leave ->state disagreeing with the device or ->state equal
to USB_STATE_CONFIGURED even though ->actconfig is NULL. This needs more
thought.
(3) I decided that it wasn't really necessary to make a deep copy of
the altsetting and endpoint structures for the dynamic interfaces (those
structures are read-only), so this patch just copies some pointers.
Unfortunately this opens up a hole: A user program can pin an interface
attribute file and use it to try reading an altsetting structure even
after the device has been disconnected and the structure deallocated. I
added a test to the attribute read method to prevent dereferencing a NULL
pointer, but there remained the possibility of a race between using the
pointer and setting it to NULL. So I added another semaphore to handle
_that_. All this makes those attribute methods a lot bigger than they
used to be.
Alternative approaches include: making a deep copy after all, or adding a
kref to the altsetting array (a little tricky to do since it is a
variable-length array allocated as a single unit, so there's no logical
place to store the kref). None of these possibilities is elegant, and I
would welcome suggestions for improvements.
Alan Stern
===== include/linux/usb.h 1.181 vs edited =====
--- 1.181/include/linux/usb.h Wed Mar 31 17:13:05 2004
+++ edited/include/linux/usb.h Fri Apr 9 12:00:22 2004
@@ -147,11 +147,35 @@
#define USB_MAXINTERFACES 32
/**
+ * struct usb_interface_cache - long-term representation of a device interface
+ * @altsetting: 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.
+ * @num_altsetting: number of altsettings defined.
+ *
+ * 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 {
+ /* array of alternate settings for this interface,
+ * stored in no particular order */
+ struct usb_host_interface *altsetting;
+ unsigned num_altsetting; /* number of alternate settings */
+};
+
+/**
* 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: pointer to array of 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).
@@ -184,6 +208,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;
unsigned char *extra; /* Extra descriptors */
int extralen;
===== drivers/usb/core/config.c 1.33 vs edited =====
--- 1.33/drivers/usb/core/config.c Mon Mar 29 11:17:38 2004
+++ edited/drivers/usb/core/config.c Mon Apr 12 09:44:18 2004
@@ -96,19 +96,18 @@
return buffer - buffer0 + i;
}
-static void usb_free_intf(struct usb_interface *intf)
+static void usb_free_intf_cache(struct usb_interface_cache *intfc)
{
int j;
- if (intf->altsetting) {
- for (j = 0; j < intf->num_altsetting; j++) {
- struct usb_host_interface *alt = &intf->altsetting[j];
+ if (intfc->altsetting) {
+ for (j = 0; j < intfc->num_altsetting; j++) {
+ struct usb_host_interface *alt = &intfc->altsetting[j];
kfree(alt->endpoint);
}
- kfree(intf->altsetting);
+ kfree(intfc->altsetting);
}
- kfree(intf);
}
static int usb_parse_interface(struct device *ddev, int cfgno,
@@ -117,7 +116,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 +136,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,7 +209,7 @@
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;
@@ -238,13 +237,11 @@
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));
- }
+ len = sizeof(*intfc) * nintf;
+ config->intf_cache = intfc = kmalloc(len, GFP_KERNEL);
+ if (!intfc)
+ return -ENOMEM;
+ memset(intfc, 0, len);
/* Go through the descriptors, checking their length and counting the
* number of altsettings for each interface */
@@ -278,7 +275,7 @@
return -EINVAL;
}
if (i < nintf)
- ++config->interface[i]->num_altsetting;
+ ++intfc[i].num_altsetting;
} else if (header->bDescriptorType == USB_DT_DEVICE ||
header->bDescriptorType == USB_DT_CONFIG) {
@@ -291,28 +288,26 @@
} /* for ((buffer2 = buffer, size2 = size); ...) */
/* Allocate the altsetting arrays */
- for (i = 0; i < nintf; ++i) {
- interface = config->interface[i];
- if (interface->num_altsetting > USB_MAXALTSETTING) {
+ for (i = 0; i < nintf; (++i, ++intfc)) {
+ if (intfc->num_altsetting > USB_MAXALTSETTING) {
dev_err(ddev, "too many alternate settings for "
"config %d interface %d: %d, "
"maximum allowed: %d\n",
- cfgno, i, interface->num_altsetting,
+ cfgno, i, intfc->num_altsetting,
USB_MAXALTSETTING);
return -EINVAL;
}
- if (interface->num_altsetting == 0) {
+ if (intfc->num_altsetting == 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->altsetting) * intfc->num_altsetting;
+ intfc->altsetting = kmalloc(len, GFP_KERNEL);
+ if (!intfc->altsetting)
return -ENOMEM;
- memset(interface->altsetting, 0, len);
+ memset(intfc->altsetting, 0, len);
}
/* Skip over any Class Specific or Vendor Specific descriptors;
@@ -339,10 +334,10 @@
}
/* 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;
+ for (i = 0; i < nintf; (++i, ++intfc)) {
+ 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;
@@ -373,11 +368,11 @@
for (c = 0; c < dev->descriptor.bNumConfigurations; c++) {
struct usb_host_config *cf = &dev->config[c];
- for (i = 0; i < cf->desc.bNumInterfaces; i++) {
- struct usb_interface *ifp = cf->interface[i];
+ if (cf->intf_cache) {
+ for (i = 0; i < cf->desc.bNumInterfaces; i++)
+ usb_free_intf_cache(&cf->intf_cache[i]);
- if (ifp)
- usb_free_intf(ifp);
+ kfree(cf->intf_cache);
}
}
kfree(dev->config);
===== drivers/usb/core/devices.c 1.29 vs edited =====
--- 1.29/drivers/usb/core/devices.c Tue Mar 30 11:50:40 2004
+++ edited/drivers/usb/core/devices.c Mon Apr 12 09:44:18 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)
@@ -310,15 +318,14 @@
if (!config) /* getting these some in 2.3.7; none in 2.3.6 */
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;
+ for (i = 0; i < config->desc.bNumInterfaces; (i++, intfc++)) {
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;
===== drivers/usb/core/driverfs.c 1.19 vs edited =====
--- 1.19/drivers/usb/core/driverfs.c Wed Apr 7 19:49:16 2004
+++ edited/drivers/usb/core/driverfs.c Mon Apr 12 11:13:51 2004
@@ -22,6 +22,9 @@
#include "usb.h"
+DECLARE_MUTEX(interface_attribute_sem);
+
+
/* Active configuration fields */
#define usb_actconfig_show(field, multiplier, format_string) \
static ssize_t show_##field (struct device *dev, char *buf) \
@@ -204,8 +207,15 @@
show_##field (struct device *dev, char *buf) \
{ \
struct usb_interface *intf = to_usb_interface (dev); \
+ int rc; \
\
- return sprintf (buf, format_string, intf->cur_altsetting->desc.field); \
+ down (&interface_attribute_sem); \
+ rc = (intf->cur_altsetting \
+ ? sprintf (buf, format_string, \
+ intf->cur_altsetting->desc.field) \
+ : -ENXIO); \
+ up (&interface_attribute_sem); \
+ return rc; \
} \
static DEVICE_ATTR(field, S_IRUGO, show_##field, NULL);
===== drivers/usb/core/message.c 1.80 vs edited =====
--- 1.80/drivers/usb/core/message.c Tue Apr 6 05:52:25 2004
+++ edited/drivers/usb/core/message.c Mon Apr 12 11:14:34 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
@@ -834,7 +830,15 @@
interface = dev->actconfig->interface[i];
dev_dbg (&dev->dev, "unregistering interface %s\n",
interface->dev.bus_id);
- device_unregister (&interface->dev);
+ device_del (&interface->dev);
+
+ /* Nobody should use these pointers any more */
+ down (&interface_attribute_sem);
+ interface->altsetting = interface->cur_altsetting =
+ NULL;
+ up (&interface_attribute_sem);
+ dev->actconfig->interface[i] = NULL;
+ put_device (&interface->dev);
}
dev->actconfig = 0;
if (dev->state == USB_STATE_CONFIGURED)
@@ -1071,6 +1075,13 @@
return 0;
}
+static void release_interface(struct device *dev)
+{
+ struct usb_interface *intf = to_usb_interface(dev);
+
+ kfree(intf);
+}
+
/*
* usb_set_configuration - Makes a particular device setting be current
* @dev: the device whose configuration is being updated
@@ -1109,19 +1120,18 @@
{
int i, ret;
struct usb_host_config *cp = NULL;
-
+ struct usb_interface_cache *intfc;
+
/* 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,
@@ -1139,72 +1149,84 @@
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;
+ return ret;
dev->actconfig = cp;
- if (!cp)
+ if (!cp) {
dev->state = USB_STATE_ADDRESS;
- else {
- dev->state = USB_STATE_CONFIGURED;
+ return 0;
+ }
- /* 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_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->cur_altsetting = alt;
- 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;
- intf->dev.release = release_interface;
- device_initialize (&intf->dev);
- sprintf (&intf->dev.bus_id[0], "%d-%s:%d.%d",
- dev->bus->busnum, dev->devpath,
- configuration,
- alt->desc.bInterfaceNumber);
+ dev->state = USB_STATE_CONFIGURED;
+
+ /* re-initialize hc/hcd/usbcore interface/endpoint state.
+ * this triggers binding of drivers to interfaces; and
+ * maybe probe() calls will choose different altsettings.
+ */
+ intfc = cp->intf_cache;
+ for (i = 0; i < cp->desc.bNumInterfaces; (++i, ++intfc)) {
+ struct usb_interface *intf;
+ struct usb_host_interface *alt;
+
+ cp->interface[i] = intf = kmalloc(sizeof(*intf), GFP_KERNEL);
+ if (!intf) {
+ dev_err(&dev->dev, "Out of memory");
+ for (; i >= 0; --i) {
+ kfree(cp->interface[i]);
+ cp->interface[i] = NULL;
+ }
+ dev->actconfig = NULL;
+ return -ENOMEM;
}
- /* Now that all interfaces are setup, probe() calls
- * may claim() any interface that's not yet bound.
- * Many class drivers need that: CDC, audio, video, etc.
+ memset(intf, 0, sizeof(*intf));
+ intf->altsetting = intfc->altsetting;
+ intf->num_altsetting = intfc->num_altsetting;
+
+ 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.
*/
- for (i = 0; i < cp->desc.bNumInterfaces; ++i) {
- struct usb_interface *intf = cp->interface[i];
- struct usb_interface_descriptor *desc;
-
- desc = &intf->altsetting [0].desc;
- dev_dbg (&dev->dev,
- "adding %s (config #%d, interface %d)\n",
- intf->dev.bus_id, configuration,
- desc->bInterfaceNumber);
- ret = device_add (&intf->dev);
- if (ret != 0) {
- dev_err(&dev->dev,
- "device_add(%s) --> %d\n",
- intf->dev.bus_id,
- ret);
- continue;
- }
- usb_create_driverfs_intf_files (intf);
+ if (!alt)
+ alt = &intf->altsetting[0];
+
+ intf->cur_altsetting = alt;
+ 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;
+ intf->dev.release = release_interface;
+ device_initialize (&intf->dev);
+ sprintf (&intf->dev.bus_id[0], "%d-%s:%d.%d",
+ dev->bus->busnum, dev->devpath,
+ configuration, alt->desc.bInterfaceNumber);
+ }
+
+ /* Now that all interfaces are setup, probe() calls
+ * may claim() any interface that's not yet bound.
+ * Many class drivers need that: CDC, audio, video, etc.
+ */
+ for (i = 0; i < cp->desc.bNumInterfaces; ++i) {
+ struct usb_interface *intf = cp->interface[i];
+ struct usb_interface_descriptor *desc;
+
+ desc = &intf->altsetting [0].desc;
+ dev_dbg (&dev->dev, "adding %s (config #%d, interface %d)\n",
+ intf->dev.bus_id, configuration,
+ desc->bInterfaceNumber);
+ ret = device_add (&intf->dev);
+ if (ret != 0) {
+ dev_err(&dev->dev, "device_add(%s) --> %d\n",
+ intf->dev.bus_id, ret);
+ continue;
}
+ usb_create_driverfs_intf_files (intf);
}
-out:
return ret;
}
===== drivers/usb/core/usb.c 1.260 vs edited =====
--- 1.260/drivers/usb/core/usb.c Wed Mar 31 17:13:05 2004
+++ edited/drivers/usb/core/usb.c Mon Apr 12 09:44:19 2004
@@ -1193,8 +1193,7 @@
/* heuristic: Linux is more likely to have class
* drivers, so avoid vendor-specific interfaces.
*/
- desc = &dev->config[i].interface[0]
- ->altsetting->desc;
+ desc = &dev->config[i].intf_cache[0].altsetting->desc;
if (desc->bInterfaceClass == USB_CLASS_VENDOR_SPEC)
continue;
/* COMM/2/all is CDC ACM, except 0xff is MSFT RNDIS */
===== drivers/usb/core/usb.h 1.8 vs edited =====
--- 1.8/drivers/usb/core/usb.h Tue Mar 30 04:06:12 2004
+++ edited/drivers/usb/core/usb.h Mon Apr 12 09:48:56 2004
@@ -21,3 +21,6 @@
/* for labeling diagnostics */
extern const char *usbcore_name;
+
+/* protect interface->cur_altsetting for sysfs attributes */
+extern struct semaphore interface_attribute_sem;
-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
