On Sat, 7 Jan 2012, Huajun Li wrote:
> Hi Greg,
> I worked out a patch based on the idea. But put 'index' validation
> codes to usb_stor_probe1() rather than individual subdrivers' probing
> routines, and add a global variable 'for_dynamic_ids' , which defines
> Bulk-Only transport with the Transparent SCSI protocol. When storage
> drivers receive dynamic id request, they can use the transport and
> protocol specified in the variable.
> Moreover, just as Alan said, subdriver need lots of work but little
> gain, and I find most of them need pre-initialization, this may not
> suitable for using dynamic id, so disable the feature for them.
That's a strange way of doing things. The only reason for putting the
validation code in usb_stor_probe1 is so that the subdrivers can use
it. But at the same time, you prevent the subdrivers from using it by
setting their no_dynamic_id flag.
You should do one or the other but not both.
> Following proposal is against usb-next, please check it, thanks.
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -125,6 +125,10 @@ static struct us_unusual_dev us_unusual_dev_list[] = {
> { } /* Terminating entry */
> };
>
> +static struct us_unusual_dev for_dynamic_ids = USUAL_DEV(USB_SC_SCSI,
> + USB_PR_BULK, 0);
Okay, but the style I prefer is:
static struct us_unusual_dev for_dynamic_ids =
USUAL_DEV(USB_SC_SCSI, USB_PR_BULK, 0);
> +
> +
> #undef UNUSUAL_DEV
> #undef COMPLIANT_DEV
> #undef USUAL_DEV
> @@ -521,6 +525,10 @@ static int get_device_info(struct us_data *us,
> const struct usb_device_id *id,
> &us->pusb_intf->cur_altsetting->desc;
> struct device *pdev = &us->pusb_intf->dev;
>
> + /* fill_inquiry_response() may need the 2 info, initialize them here */
> + unusual_dev->vendorName = dev->manufacturer;
> + unusual_dev->productName = dev->product;
You just erased a large part of the information in the unusual_dev
entry. That's not acceptable.
This should be done _only_ when using a dynamic ID -- and since devices
with dynamic IDs can't ever use fill_inquiry_response, there's no
reason to do this at all.
> @@ -885,10 +893,12 @@ static unsigned int usb_stor_sg_tablesize(struct
> usb_interface *intf)
> int usb_stor_probe1(struct us_data **pus,
> struct usb_interface *intf,
> const struct usb_device_id *id,
> + const unsigned long index,
The const is totally unnecessary here. And using unsigned long is the
result of a bad design choice -- passing the entry's array index (which
is not well defined when the entry isn't actually in the array) instead
of passing a pointer to the start of the array.
Besides, I still think that the interface to usb_stor_probe1 should
remain unchanged and the new validation code should be put in
storage_probe.
> struct us_unusual_dev *unusual_dev)
> {
> struct Scsi_Host *host;
> struct us_data *us;
> + unsigned int size;
> int result;
>
> US_DEBUGP("USB Mass Storage device detected\n");
> @@ -922,7 +932,25 @@ int usb_stor_probe1(struct us_data **pus,
> if (result)
> goto BadDevice;
>
> + /* FIXME: any easy way to get unusual_dev's size? */
> + for (size = 0; ; size++)
> + if (unusual_dev[size].vendorName == NULL &&
> + unusual_dev[size].productName == NULL &&
> + unusual_dev[size].useProtocol == 0 &&
> + unusual_dev[size].useTransport == 0 &&
> + unusual_dev[size].initFunction == NULL)
> + break;
> +
Then this loop wouldn't be needed; you could use
ARRAY_SIZE(us_unusual_dev_list) directly.
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
