ChangeSet 1.1673.8.9, 2004/03/25 15:17:16-08:00, stern@rowland.harvard.edu

[PATCH] USB: Code improvements for core/config.c

This patch makes some improvements to the code in config.c.

	Create a subroutine to handle the repeated task of skipping
	forward to the next descriptor of a certain type.

	Remove some fairly useless debugging messages (they could
	never even have been enabled in the pre-as221 code).

	Verify that endpoint descriptors don't have an address
	equal to 0 (as well as not being above 15).

	Rename some local variables so they are a little more
	consistent and meaningful.

Despite all the changes, the functionality should remain the same.
Please apply.


 drivers/usb/core/config.c |  235 +++++++++++++++++++---------------------------
 1 files changed, 101 insertions(+), 134 deletions(-)


diff -Nru a/drivers/usb/core/config.c b/drivers/usb/core/config.c
--- a/drivers/usb/core/config.c	Wed Apr 14 14:39:42 2004
+++ b/drivers/usb/core/config.c	Wed Apr 14 14:39:42 2004
@@ -17,14 +17,38 @@
 
 #define USB_MAXCONFIG			8	/* Arbitrary limit */
 
+
+static int find_next_descriptor(unsigned char *buffer, int size,
+    int dt1, int dt2, int *num_skipped)
+{
+	struct usb_descriptor_header *h;
+	int n = 0;
+	unsigned char *buffer0 = buffer;
+
+	/* Find the next descriptor of type dt1 or dt2 */
+	while (size >= sizeof(struct usb_descriptor_header)) {
+		h = (struct usb_descriptor_header *) buffer;
+		if (h->bDescriptorType == dt1 || h->bDescriptorType == dt2)
+			break;
+		buffer += h->bLength;
+		size -= h->bLength;
+		++n;
+	}
+
+	/* Store the number of descriptors skipped and return the
+	 * number of bytes skipped */
+	if (num_skipped)
+		*num_skipped = n;
+	return buffer - buffer0;
+}
+
 static int usb_parse_endpoint(struct device *ddev, int cfgno, int inum,
     int asnum, struct usb_host_endpoint *endpoint,
     unsigned char *buffer, int size)
 {
 	unsigned char *buffer0 = buffer;
 	struct usb_descriptor_header *header;
-	unsigned char *begin;
-	int numskipped;
+	int n, i;
 
 	header = (struct usb_descriptor_header *)buffer;
 	if (header->bDescriptorType != USB_DT_ENDPOINT) {
@@ -47,7 +71,8 @@
 		return -EINVAL;
 	}
 
-	if ((endpoint->desc.bEndpointAddress & ~USB_ENDPOINT_DIR_MASK) >= 16) {
+	i = endpoint->desc.bEndpointAddress & ~USB_ENDPOINT_DIR_MASK;
+	if (i >= 16 || i == 0) {
 		dev_err(ddev, "config %d interface %d altsetting %d has an "
 		    "invalid endpoint with address 0x%X\n",
 		    cfgno, inum, asnum, endpoint->desc.bEndpointAddress);
@@ -59,32 +84,16 @@
 	buffer += header->bLength;
 	size -= header->bLength;
 
-	/* Skip over any Class Specific or Vendor Specific descriptors */
-	begin = buffer;
-	numskipped = 0;
-	while (size >= sizeof(struct usb_descriptor_header)) {
-		header = (struct usb_descriptor_header *)buffer;
-
-		/* If we find another "proper" descriptor then we're done  */
-		if ((header->bDescriptorType == USB_DT_ENDPOINT) ||
-		    (header->bDescriptorType == USB_DT_INTERFACE))
-			break;
-
-		dev_dbg(ddev, "skipping descriptor 0x%X\n",
-		    header->bDescriptorType);
-		numskipped++;
-
-		buffer += header->bLength;
-		size -= header->bLength;
-	}
-	if (numskipped) {
+	/* Skip over any Class Specific or Vendor Specific descriptors;
+	 * find the next endpoint or interface descriptor */
+	endpoint->extra = buffer;
+	i = find_next_descriptor(buffer, size, USB_DT_ENDPOINT,
+	    USB_DT_INTERFACE, &n);
+	endpoint->extralen = i;
+	if (n > 0)
 		dev_dbg(ddev, "skipped %d class/vendor specific endpoint "
-		    "descriptors\n", numskipped);
-		endpoint->extra = begin;
-		endpoint->extralen = buffer - begin;
-	}
-
-	return buffer - buffer0;
+		    "descriptors\n", n);
+	return buffer - buffer0 + i;
 }
 
 static void usb_free_intf(struct usb_interface *intf)
@@ -93,9 +102,9 @@
 
 	if (intf->altsetting) {
 		for (j = 0; j < intf->num_altsetting; j++) {
-			struct usb_host_interface *as = &intf->altsetting[j];
+			struct usb_host_interface *alt = &intf->altsetting[j];
 
-			kfree(as->endpoint);
+			kfree(alt->endpoint);
 		}
 		kfree(intf->altsetting);
 	}
@@ -109,13 +118,14 @@
 	struct usb_interface_descriptor	*d;
 	int inum, asnum;
 	struct usb_interface *interface;
-	struct usb_host_interface *ifp;
-	int len, numskipped;
-	struct usb_descriptor_header *header;
-	unsigned char *begin;
-	int i, retval;
+	struct usb_host_interface *alt;
+	int i, n;
+	int len, retval;
 
 	d = (struct usb_interface_descriptor *) buffer;
+	buffer += d->bLength;
+	size -= d->bLength;
+
 	if (d->bDescriptorType != USB_DT_INTERFACE) {
 		dev_err(ddev, "config %d has an unexpected descriptor of type "
 		    "0x%X, expecting interface type 0x%X\n",
@@ -124,21 +134,8 @@
 	}
 
 	inum = d->bInterfaceNumber;
-	if (inum >= config->desc.bNumInterfaces) {
-
-		/* Skip to the next interface descriptor */
-		buffer += d->bLength;
-		size -= d->bLength;
-		while (size >= sizeof(struct usb_descriptor_header)) {
-			header = (struct usb_descriptor_header *) buffer;
-
-			if (header->bDescriptorType == USB_DT_INTERFACE)
-				break;
-			buffer += header->bLength;
-			size -= header->bLength;
-		}
-		return buffer - buffer0;
-	}
+	if (inum >= config->desc.bNumInterfaces)
+		goto skip_to_next_interface_descriptor;
 
 	interface = config->interface[inum];
 	asnum = d->bAlternateSetting;
@@ -149,57 +146,41 @@
 		return -EINVAL;
 	}
 
-	ifp = &interface->altsetting[asnum];
-	if (ifp->desc.bLength) {
+	alt = &interface->altsetting[asnum];
+	if (alt->desc.bLength) {
 		dev_err(ddev, "Duplicate descriptor for config %d "
 		    "interface %d altsetting %d\n", cfgno, inum, asnum);
 		return -EINVAL;
 	}
-	memcpy(&ifp->desc, buffer, USB_DT_INTERFACE_SIZE);
+	memcpy(&alt->desc, d, USB_DT_INTERFACE_SIZE);
 
-	buffer += d->bLength;
-	size -= d->bLength;
-
-	/* Skip over any Class Specific or Vendor Specific descriptors */
-	begin = buffer;
-	numskipped = 0;
-	while (size >= sizeof(struct usb_descriptor_header)) {
-		header = (struct usb_descriptor_header *)buffer;
-
-		/* If we find another "proper" descriptor then we're done  */
-		if ((header->bDescriptorType == USB_DT_INTERFACE) ||
-		    (header->bDescriptorType == USB_DT_ENDPOINT))
-			break;
-
-		dev_dbg(ddev, "skipping descriptor 0x%X\n",
-		    header->bDescriptorType);
-		numskipped++;
-
-		buffer += header->bLength;
-		size -= header->bLength;
-	}
-	if (numskipped) {
+	/* Skip over any Class Specific or Vendor Specific descriptors;
+	 * find the first endpoint or interface descriptor */
+	alt->extra = buffer;
+	i = find_next_descriptor(buffer, size, USB_DT_ENDPOINT,
+	    USB_DT_INTERFACE, &n);
+	alt->extralen = i;
+	if (n > 0)
 		dev_dbg(ddev, "skipped %d class/vendor specific "
-		    "interface descriptors\n", numskipped);
-		ifp->extra = begin;
-		ifp->extralen = buffer - begin;
-	}
+		    "interface descriptors\n", n);
+	buffer += i;
+	size -= i;
 
-	if (ifp->desc.bNumEndpoints > USB_MAXENDPOINTS) {
+	if (alt->desc.bNumEndpoints > USB_MAXENDPOINTS) {
 		dev_err(ddev, "too many endpoints for config %d interface %d "
 		    "altsetting %d: %d, maximum allowed: %d\n",
-		    cfgno, inum, asnum, ifp->desc.bNumEndpoints,
+		    cfgno, inum, asnum, alt->desc.bNumEndpoints,
 		    USB_MAXENDPOINTS);
 		return -EINVAL;
 	}
 
-	len = ifp->desc.bNumEndpoints * sizeof(struct usb_host_endpoint);
-	ifp->endpoint = kmalloc(len, GFP_KERNEL);
-	if (!ifp->endpoint)
+	len = alt->desc.bNumEndpoints * sizeof(struct usb_host_endpoint);
+	alt->endpoint = kmalloc(len, GFP_KERNEL);
+	if (!alt->endpoint)
 		return -ENOMEM;
-	memset(ifp->endpoint, 0, len);
+	memset(alt->endpoint, 0, len);
 
-	for (i = 0; i < ifp->desc.bNumEndpoints; i++) {
+	for (i = 0; i < alt->desc.bNumEndpoints; i++) {
 		if (size < USB_DT_ENDPOINT_SIZE) {
 			dev_err(ddev, "too few endpoint descriptors for "
 			    "config %d interface %d altsetting %d\n",
@@ -208,15 +189,19 @@
 		}
 
 		retval = usb_parse_endpoint(ddev, cfgno, inum, asnum,
-		    ifp->endpoint + i, buffer, size);
+		    alt->endpoint + i, buffer, size);
 		if (retval < 0)
 			return retval;
 
 		buffer += retval;
 		size -= retval;
 	}
-
 	return buffer - buffer0;
+
+skip_to_next_interface_descriptor:
+	i = find_next_descriptor(buffer, size, USB_DT_INTERFACE,
+	    USB_DT_INTERFACE, NULL);
+	return buffer - buffer0 + i;
 }
 
 int usb_parse_configuration(struct device *ddev, int cfgidx,
@@ -224,14 +209,12 @@
 {
 	int cfgno;
 	int nintf, nintf_orig;
-	int i, j;
+	int i, j, n;
 	struct usb_interface *interface;
 	unsigned char *buffer2;
 	int size2;
 	struct usb_descriptor_header *header;
-	int numskipped, len;
-	unsigned char *begin;
-	int retval;
+	int len, retval;
 
 	memcpy(&config->desc, buffer, USB_DT_CONFIG_SIZE);
 	if (config->desc.bDescriptorType != USB_DT_CONFIG ||
@@ -244,6 +227,9 @@
 	config->desc.wTotalLength = size;
 	cfgno = config->desc.bConfigurationValue;
 
+	buffer += config->desc.bLength;
+	size -= config->desc.bLength;
+
 	nintf = nintf_orig = config->desc.bNumInterfaces;
 	if (nintf > USB_MAXINTERFACES) {
 		dev_warn(ddev, "config %d has too many interfaces: %d, "
@@ -255,7 +241,6 @@
 	for (i = 0; i < nintf; ++i) {
 		interface = config->interface[i] =
 		    kmalloc(sizeof(struct usb_interface), GFP_KERNEL);
-		dev_dbg(ddev, "kmalloc IF %p, numif %i\n", interface, i);
 		if (!interface)
 			return -ENOMEM;
 		memset(interface, 0, sizeof(struct usb_interface));
@@ -263,10 +248,10 @@
 
 	/* Go through the descriptors, checking their length and counting the
 	 * number of altsettings for each interface */
-	buffer2 = buffer;
-	size2 = size;
-	j = 0;
-	while (size2 >= sizeof(struct usb_descriptor_header)) {
+	for ((buffer2 = buffer, size2 = size);
+	      size2 >= sizeof(struct usb_descriptor_header);
+	     (buffer2 += header->bLength, size2 -= header->bLength)) {
+
 		header = (struct usb_descriptor_header *) buffer2;
 		if ((header->bLength > size2) || (header->bLength < 2)) {
 			dev_err(ddev, "config %d has an invalid descriptor "
@@ -277,13 +262,14 @@
 		if (header->bDescriptorType == USB_DT_INTERFACE) {
 			struct usb_interface_descriptor *d;
 
-			if (header->bLength < USB_DT_INTERFACE_SIZE) {
+			d = (struct usb_interface_descriptor *) header;
+			if (d->bLength < USB_DT_INTERFACE_SIZE) {
 				dev_err(ddev, "config %d has an invalid "
 				    "interface descriptor of length %d\n",
-				    cfgno, header->bLength);
+				    cfgno, d->bLength);
 				return -EINVAL;
 			}
-			d = (struct usb_interface_descriptor *) header;
+
 			i = d->bInterfaceNumber;
 			if (i >= nintf_orig) {
 				dev_err(ddev, "config %d has an invalid "
@@ -294,21 +280,18 @@
 			if (i < nintf)
 				++config->interface[i]->num_altsetting;
 
-		} else if ((header->bDescriptorType == USB_DT_DEVICE ||
-		    header->bDescriptorType == USB_DT_CONFIG) && j) {
+		} else if (header->bDescriptorType == USB_DT_DEVICE ||
+			    header->bDescriptorType == USB_DT_CONFIG) {
 			dev_err(ddev, "config %d contains an unexpected "
 			    "descriptor of type 0x%X\n",
 			    cfgno, header->bDescriptorType);
 			return -EINVAL;
 		}
 
-		j = 1;
-		buffer2 += header->bLength;
-		size2 -= header->bLength;
-	}
+	}	/* for ((buffer2 = buffer, size2 = size); ...) */
 
 	/* Allocate the altsetting arrays */
-	for (i = 0; i < config->desc.bNumInterfaces; ++i) {
+	for (i = 0; i < nintf; ++i) {
 		interface = config->interface[i];
 		if (interface->num_altsetting > USB_MAXALTSETTING) {
 			dev_err(ddev, "too many alternate settings for "
@@ -332,33 +315,17 @@
 		memset(interface->altsetting, 0, len);
 	}
 
-	buffer += config->desc.bLength;
-	size -= config->desc.bLength;
-
-	/* Skip over any Class Specific or Vendor Specific descriptors */
-	begin = buffer;
-	numskipped = 0;
-	while (size >= sizeof(struct usb_descriptor_header)) {
-		header = (struct usb_descriptor_header *)buffer;
-
-		/* If we find another "proper" descriptor then we're done  */
-		if ((header->bDescriptorType == USB_DT_ENDPOINT) ||
-		    (header->bDescriptorType == USB_DT_INTERFACE))
-			break;
-
-		dev_dbg(ddev, "skipping descriptor 0x%X\n",
-		    header->bDescriptorType);
-		numskipped++;
-
-		buffer += header->bLength;
-		size -= header->bLength;
-	}
-	if (numskipped) {
-		dev_dbg(ddev, "skipped %d class/vendor specific configuration "
-		    "descriptors\n", numskipped);
-		config->extra = begin;
-		config->extralen = buffer - begin;
-	}
+	/* Skip over any Class Specific or Vendor Specific descriptors;
+	 * find the first interface descriptor */
+	config->extra = buffer;
+	i = find_next_descriptor(buffer, size, USB_DT_INTERFACE,
+	    USB_DT_INTERFACE, &n);
+	config->extralen = i;
+	if (n > 0)
+		dev_dbg(ddev, "skipped %d class/vendor specific "
+		    "configuration descriptors\n", n);
+	buffer += i;
+	size -= i;
 
 	/* Parse all the interface/altsetting descriptors */
 	while (size >= sizeof(struct usb_descriptor_header)) {