On Thu, Jan 12, 2012 at 12:12 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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.
>
Yes, I will, and thanks for your reminder. The code I pasted here was
just a diff file, and will send out a formal patch soon.
> 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) {
>
Will change it although it could work per my test.
However, there is a terminating entry in usb_storage_usb_ids, so
maybe it should be changed to below one, right ?
if (id < usb_storage_usb_ids || id >= usb_storage_usb_ids + size - 1) {
> 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
>
I did not get any warning or other info while compiling the codes, and
I did not change any special configurations, my gcc is: gcc version
4.6.1
> 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.
>
The later keeps original code style, it's good.
--
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
