Re: [PATCH] USB IAD Support (kernel2.6.21.3)

On Mon, 11 Jun 2007, Craig W. Nadler wrote:

> diff -uprN a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> --- a/drivers/usb/core/config.c	2007-05-24 17:22:47.000000000 -0400
> +++ b/drivers/usb/core/config.c	2007-06-09 03:20:51.000000000 -0400
> @@ -232,6 +232,7 @@ static int usb_parse_configuration(struc
>  	struct usb_descriptor_header *header;
>  	int len, retval;
>  	u8 inums[USB_MAXINTERFACES], nalts[USB_MAXINTERFACES];
> +	u8 iad_num = 0;

Remember what I said about smaller-than-word-size local variables?
About the only reason ever to use a short or char is if you really need
it to occupy exactly that much space (if it is part of an array or a
structure, for example).  Make iad_num plain old unsigned.

> @@ -309,6 +310,17 @@ static int usb_parse_configuration(struc
>  				++n;
>  			}
>  
> +		} else if (header->bDescriptorType ==
> +				USB_DT_INTERFACE_ASSOCIATION) {
> +			if (iad_num == USB_MAXIADS) {
> +				dev_err(ddev, "found more Interface "
> +					       "Association Descriptors "
> +					       "than allocated for\n");

This should be a warning, not an error.  Also you should include cfgno
in the error message so that people will know which configuration has
too many IADs.

> diff -uprN a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
> --- a/drivers/usb/core/devices.c	2007-05-24 17:22:47.000000000 -0400
> +++ b/drivers/usb/core/devices.c	2007-06-09 03:20:51.000000000 -0400
> @@ -324,6 +344,11 @@ static char *usb_dump_config (
>  	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 < USB_MAXIADS; i++) {
> +		if (config->intf_assoc[i] == NULL) break;

Style: the break statement should be on a separate line from the if
statement.

> diff -uprN a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> --- a/drivers/usb/core/message.c	2007-05-24 17:22:47.000000000 -0400
> +++ b/drivers/usb/core/message.c	2007-06-09 03:30:02.000000000 -0400
> @@ -1315,6 +1315,39 @@ static void release_interface(struct dev
...
> +		intf_assoc = config->intf_assoc[i];
> +		if (intf_assoc->bInterfaceCount == 0) continue;

Same style violation.

> +
> +		first_intf = intf_assoc->bFirstInterface;
> +		last_intf = first_intf + (intf_assoc->bInterfaceCount - 1);
> +		if (inum >= first_intf && inum <= last_intf) {
> +			if (!retval) {
> +				retval = intf_assoc;
> +			} else {
> +				dev_err (&dev->dev, "Interface #%d referenced"
> +						" by multiple IADs\n", inum);
> +			}

Are you really sure this needs to be an error and not a warning?

Remember also what I said earlier about putting extraneous space
characters between a function name and the following open paren.

> diff -uprN a/include/linux/usb.h b/include/linux/usb.h
> --- a/include/linux/usb.h	2007-05-24 17:22:47.000000000 -0400
> +++ b/include/linux/usb.h	2007-06-09 03:20:51.000000000 -0400
> @@ -245,6 +250,11 @@ struct usb_host_config {
>  	struct usb_config_descriptor	desc;
>  
>  	char *string;		/* iConfiguration string, if present */
> +	
> +	/* List of any Interface Association Descriptors in this
> +	 * configuration. */
> +	struct usb_interface_assoc_descriptor *intf_assoc[USB_MAXIADS];

This may be just my own confusion, but is it necessary to list
interface association descriptors in both the host_interface and the
host_config structures?

Alan Stern


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel