On Wed, 11 Jan 2012, Huajun Li wrote:
> Hi Alan,
> Updated the patch according to your comments, and following is the new
> one, thanks.
This really needs to be broken up into two patches. One to modify
usb.c, adding support for dynamic IDs, and the other to modify all the
other subdrivers.
The subdriver changes are all simple, so I'll skip over them.
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -1047,8 +1053,19 @@ static int storage_probe(struct usb_interface *intf,
> * table, so we use the index of the id entry to find the
> * corresponding unusual_devs entry.
> */
> - result = usb_stor_probe1(&us, intf, id,
> - (id - usb_storage_usb_ids) + us_unusual_dev_list);
> +
> + index = id - usb_storage_usb_ids;
> + size = sizeof(us_unusual_dev_list) / sizeof(struct us_unusual_dev);
> + if (index < 0 || index >= size - 1) {
No, no! This is totally bogus. According to the C language
specification, if id doesn't point to a member of the
usb_storage_usb_ids array then the difference id - usb_storage_usb_ids
is meaningless. You can't depend on it having any particular value at
all.
The right way to do the test is like this:
size = ARRAY_SIZE(us_unusual_dev_list);
if (id < usb_storage_usb_ids || id >= usb_storage_usb_ids + size) {
Actually, the code would be a little cleaner if you reversed the test.
Look for the normal case, and put the dynamic ID case in the "else"
clause.
> + unusual_dev = &for_dynamic_ids;
> +
> + US_DEBUGP("%s %s 0x%04x 0x%04x\n", "Use Bulk-Only transport",
> + "with the Transparent SCSI protocol for dynamic id:",
> + id->idVendor, id->idProduct);
> + } else
> + unusual_dev += index;
According to section 3 of Documentation/CodingStyle, there should be
braces around the "else" part of this "if" statement.
Also, didn't the compiler complain about the unusual_dev variable being
uninitialized here when you tested your changes? This line should say
unusual_dev = &usb_unusual_dev_list[index];
Or if you prefer,
unusual_dev = (id - usb_storage_usb_ids) + us_unusual_dev_list;
in which case the "index" variable wouldn't be needed at all.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
