On Sun, Jan 8, 2012 at 2:38 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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
>
Hi Alan,
Updated the patch according to your comments, and following is the new
one, thanks.
-----------------------------------------------------------------------------------------------
diff --git a/drivers/usb/storage/alauda.c b/drivers/usb/storage/alauda.c
index 51af2fe..bab8c8f 100644
--- a/drivers/usb/storage/alauda.c
+++ b/drivers/usb/storage/alauda.c
@@ -1276,6 +1276,7 @@ static struct usb_driver alauda_driver = {
.post_reset = usb_stor_post_reset,
.id_table = alauda_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(alauda_driver);
diff --git a/drivers/usb/storage/cypress_atacb.c
b/drivers/usb/storage/cypress_atacb.c
index 387cbd47..5fe451d 100644
--- a/drivers/usb/storage/cypress_atacb.c
+++ b/drivers/usb/storage/cypress_atacb.c
@@ -272,6 +272,7 @@ static struct usb_driver cypress_driver = {
.post_reset = usb_stor_post_reset,
.id_table = cypress_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(cypress_driver);
diff --git a/drivers/usb/storage/datafab.c b/drivers/usb/storage/datafab.c
index 15d41f2..35e9c51 100644
--- a/drivers/usb/storage/datafab.c
+++ b/drivers/usb/storage/datafab.c
@@ -751,6 +751,7 @@ static struct usb_driver datafab_driver = {
.post_reset = usb_stor_post_reset,
.id_table = datafab_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(datafab_driver);
diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index a6ade40..30532d9 100644
--- a/drivers/usb/storage/ene_ub6250.c
+++ b/drivers/usb/storage/ene_ub6250.c
@@ -2407,6 +2407,7 @@ static struct usb_driver ene_ub6250_driver = {
.post_reset = usb_stor_post_reset,
.id_table = ene_ub6250_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(ene_ub6250_driver);
diff --git a/drivers/usb/storage/freecom.c b/drivers/usb/storage/freecom.c
index fa16157..042cf9e 100644
--- a/drivers/usb/storage/freecom.c
+++ b/drivers/usb/storage/freecom.c
@@ -553,6 +553,7 @@ static struct usb_driver freecom_driver = {
.post_reset = usb_stor_post_reset,
.id_table = freecom_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(freecom_driver);
diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index bd55027..31fa24e 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -1566,6 +1566,7 @@ static struct usb_driver isd200_driver = {
.post_reset = usb_stor_post_reset,
.id_table = isd200_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(isd200_driver);
diff --git a/drivers/usb/storage/jumpshot.c b/drivers/usb/storage/jumpshot.c
index a19211b..e3b9738 100644
--- a/drivers/usb/storage/jumpshot.c
+++ b/drivers/usb/storage/jumpshot.c
@@ -677,6 +677,7 @@ static struct usb_driver jumpshot_driver = {
.post_reset = usb_stor_post_reset,
.id_table = jumpshot_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(jumpshot_driver);
diff --git a/drivers/usb/storage/karma.c b/drivers/usb/storage/karma.c
index e720f8e..a8708ea 100644
--- a/drivers/usb/storage/karma.c
+++ b/drivers/usb/storage/karma.c
@@ -230,6 +230,7 @@ static struct usb_driver karma_driver = {
.post_reset = usb_stor_post_reset,
.id_table = karma_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(karma_driver);
diff --git a/drivers/usb/storage/onetouch.c b/drivers/usb/storage/onetouch.c
index d75155c..886567a 100644
--- a/drivers/usb/storage/onetouch.c
+++ b/drivers/usb/storage/onetouch.c
@@ -312,6 +312,7 @@ static struct usb_driver onetouch_driver = {
.post_reset = usb_stor_post_reset,
.id_table = onetouch_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(onetouch_driver);
diff --git a/drivers/usb/storage/realtek_cr.c b/drivers/usb/storage/realtek_cr.c
index 1f62723..ccf271d 100644
--- a/drivers/usb/storage/realtek_cr.c
+++ b/drivers/usb/storage/realtek_cr.c
@@ -1100,6 +1100,7 @@ static struct usb_driver realtek_cr_driver = {
.id_table = realtek_cr_ids,
.soft_unbind = 1,
.supports_autosuspend = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(realtek_cr_driver);
diff --git a/drivers/usb/storage/sddr09.c b/drivers/usb/storage/sddr09.c
index 425df7d..3252a62 100644
--- a/drivers/usb/storage/sddr09.c
+++ b/drivers/usb/storage/sddr09.c
@@ -1787,6 +1787,7 @@ static struct usb_driver sddr09_driver = {
.post_reset = usb_stor_post_reset,
.id_table = sddr09_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(sddr09_driver);
diff --git a/drivers/usb/storage/sddr55.c b/drivers/usb/storage/sddr55.c
index e4ca5fc..c144078 100644
--- a/drivers/usb/storage/sddr55.c
+++ b/drivers/usb/storage/sddr55.c
@@ -1006,6 +1006,7 @@ static struct usb_driver sddr55_driver = {
.post_reset = usb_stor_post_reset,
.id_table = sddr55_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(sddr55_driver);
diff --git a/drivers/usb/storage/shuttle_usbat.c
b/drivers/usb/storage/shuttle_usbat.c
index 1369d25..fa1ceeb 100644
--- a/drivers/usb/storage/shuttle_usbat.c
+++ b/drivers/usb/storage/shuttle_usbat.c
@@ -1863,6 +1863,7 @@ static struct usb_driver usbat_driver = {
.post_reset = usb_stor_post_reset,
.id_table = usbat_usb_ids,
.soft_unbind = 1,
+ .no_dynamic_id = 1,
};
module_usb_driver(usbat_driver);
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 3dd7da9..fc2dae1 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -125,6 +125,9 @@ 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);
+
#undef UNUSUAL_DEV
#undef COMPLIANT_DEV
#undef USUAL_DEV
@@ -1027,8 +1030,11 @@ EXPORT_SYMBOL_GPL(usb_stor_disconnect);
static int storage_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
+ struct us_unusual_dev *unusual_dev;
struct us_data *us;
int result;
+ int index;
+ int size;
/*
* If libusual is configured, let it decide whether a standard
@@ -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) {
+ 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;
+
+ result = usb_stor_probe1(&us, intf, id, unusual_dev);
if (result)
return result;
@@ -1074,7 +1091,6 @@ static struct usb_driver usb_storage_driver = {
.id_table = usb_storage_usb_ids,
.supports_autosuspend = 1,
.soft_unbind = 1,
- .no_dynamic_id = 1,
};
static int __init usb_stor_init(void)
--
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
