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
