[U-Boot] [PATCH] usb_storage: skip all unknown devices when probing

[U-Boot] [PATCH] usb_storage: skip all unknown devices when probing

[Soeren Moch]

Not only skip storage devices with DEV_TYPE_UNKNOWN, but also all devices
which are unknown to u-boot (e.g., are not HARDDISK, TAPE, CDROM, OPDISK).

This especially avoids long timeouts when probing for external usb harddisks
which provide "Enclosure Services".

Signed-off-by: Soeren Moch <smoch@web.de>
--
Cc: Marek Vasut <marex@denx.de>
Cc: Tom Rini <trini@ti.com>
---
 common/usb_storage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index eb7706c..0ac7b48 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -1351,7 +1351,7 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
 	perq = usb_stor_buf[0];
 	modi = usb_stor_buf[1];
 
-	if ((perq & 0x1f) == 0x1f) {
+	if ((perq & 0x1f) > DEV_TYPE_OPDISK) {
 		/* skip unknown devices */
 		return 0;
 	}
-- 
1.9.1

[U-Boot] [PATCH] usb_storage: skip all unknown devices when probing

[Marek Vasut]

On Thursday, November 06, 2014 at 01:51:51 PM, Soeren Moch wrote:
> Not only skip storage devices with DEV_TYPE_UNKNOWN, but also all devices
> which are unknown to u-boot (e.g., are not HARDDISK, TAPE, CDROM, OPDISK).
> 
> This especially avoids long timeouts when probing for external usb
> harddisks which provide "Enclosure Services".
> 
> Signed-off-by: Soeren Moch <smoch@web.de>
> --
> Cc: Marek Vasut <marex@denx.de>
> Cc: Tom Rini <trini@ti.com>
> ---
>  common/usb_storage.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index eb7706c..0ac7b48 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -1351,7 +1351,7 @@ int usb_stor_get_info(struct usb_device *dev, struct
> us_data *ss, perq = usb_stor_buf[0];
>  	modi = usb_stor_buf[1];
> 
> -	if ((perq & 0x1f) == 0x1f) {
> +	if ((perq & 0x1f) > DEV_TYPE_OPDISK) {

Why can't you just blacklist 0xd instead ? I mean, this patch would do a bulk
blacklisting of all the obscure devices with peripheral ID above 0x7, but might
still work with this layer (like 0xc ... the RAID controller ; or 0xe ... the
reduced block device).

Won't it make sense to just selectively blacklist the 0xd ?

>  		/* skip unknown devices */
>  		return 0;
>  	}

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb_storage: skip all unknown devices when probing

[Soeren Moch]

On 11/06/14 21:41, Marek Vasut wrote:
> On Thursday, November 06, 2014 at 01:51:51 PM, Soeren Moch wrote:
>> Not only skip storage devices with DEV_TYPE_UNKNOWN, but also all devices
>> which are unknown to u-boot (e.g., are not HARDDISK, TAPE, CDROM, OPDISK).
>>
>> This especially avoids long timeouts when probing for external usb
>> harddisks which provide "Enclosure Services".
>>
>> Signed-off-by: Soeren Moch <smoch@web.de>
>> --
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Tom Rini <trini@ti.com>
>> ---
>>   common/usb_storage.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>> index eb7706c..0ac7b48 100644
>> --- a/common/usb_storage.c
>> +++ b/common/usb_storage.c
>> @@ -1351,7 +1351,7 @@ int usb_stor_get_info(struct usb_device *dev, struct
>> us_data *ss, perq = usb_stor_buf[0];
>>   	modi = usb_stor_buf[1];
>>
>> -	if ((perq & 0x1f) == 0x1f) {
>> +	if ((perq & 0x1f) > DEV_TYPE_OPDISK) {
>
> Why can't you just blacklist 0xd instead ? I mean, this patch would do a bulk
> blacklisting of all the obscure devices with peripheral ID above 0x7, but might
> still work with this layer (like 0xc ... the RAID controller ; or 0xe ... the
> reduced block device).
>
> Won't it make sense to just selectively blacklist the 0xd ?

If you think it makes more sense to blacklist 0xd only, that is fine with me.
I will prepare a new patch for this.

Best regards,
Soeren Moch

>
>>   		/* skip unknown devices */
>>   		return 0;
>>   	}
>
> Best regards,
> Marek Vasut
>

[U-Boot] [PATCH] usb_storage: skip all unknown devices when probing

[Marek Vasut]

On Friday, November 07, 2014 at 10:28:18 AM, Soeren Moch wrote:
> On 11/06/14 21:41, Marek Vasut wrote:
> > On Thursday, November 06, 2014 at 01:51:51 PM, Soeren Moch wrote:
> >> Not only skip storage devices with DEV_TYPE_UNKNOWN, but also all
> >> devices which are unknown to u-boot (e.g., are not HARDDISK, TAPE,
> >> CDROM, OPDISK).
> >> 
> >> This especially avoids long timeouts when probing for external usb
> >> harddisks which provide "Enclosure Services".
> >> 
> >> Signed-off-by: Soeren Moch <smoch@web.de>
> >> --
> >> Cc: Marek Vasut <marex@denx.de>
> >> Cc: Tom Rini <trini@ti.com>
> >> ---
> >> 
> >>   common/usb_storage.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/common/usb_storage.c b/common/usb_storage.c
> >> index eb7706c..0ac7b48 100644
> >> --- a/common/usb_storage.c
> >> +++ b/common/usb_storage.c
> >> @@ -1351,7 +1351,7 @@ int usb_stor_get_info(struct usb_device *dev,
> >> struct us_data *ss, perq = usb_stor_buf[0];
> >> 
> >>   	modi = usb_stor_buf[1];
> >> 
> >> -	if ((perq & 0x1f) == 0x1f) {
> >> +	if ((perq & 0x1f) > DEV_TYPE_OPDISK) {
> > 
> > Why can't you just blacklist 0xd instead ? I mean, this patch would do a
> > bulk blacklisting of all the obscure devices with peripheral ID above
> > 0x7, but might still work with this layer (like 0xc ... the RAID
> > controller ; or 0xe ... the reduced block device).
> > 
> > Won't it make sense to just selectively blacklist the 0xd ?
> 
> If you think it makes more sense to blacklist 0xd only, that is fine with
> me. I will prepare a new patch for this.

Please also make sure to document why this change is in place. (because the
Enclosure Services cause trouble). A simple comment in the code would be nice.

Thank you!

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb_storage: blacklist Enclosure Service Devices

[Soeren Moch]

Skip enclosure service devices when probing for usb storage devices.

This avoids long timeouts when probing for external usb harddisks
which provide "Enclosure Services".

Signed-off-by: Soeren Moch <smoch@web.de>
--

This is a new version of the patch
"usb_storage: skip all unknown devices when probing"
http://http://lists.denx.de/pipermail/u-boot/2014-November/194622.html

Cc: Marek Vasut <marex@denx.de>
Cc: Tom Rini <trini@ti.com>
---
 common/usb_storage.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c
index eb7706c..9198f73 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -1351,8 +1351,9 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
 	perq = usb_stor_buf[0];
 	modi = usb_stor_buf[1];
 
-	if ((perq & 0x1f) == 0x1f) {
-		/* skip unknown devices */
+	if (((perq & 0x1f) == 0x1f) || ((perq & 0x1f) == 0x0d)) {
+		/* skip unknown devices and enclosure service devices, */
+		/* they would not respond to test_unit_ready           */
 		return 0;
 	}
 	if ((modi&0x80) == 0x80) {
-- 
1.9.1

[U-Boot] [PATCH] usb_storage: blacklist Enclosure Service Devices

[Nikolay Dimitrov]

Hi Soeren,

On 11/08/2014 08:02 AM, Soeren Moch wrote:
> Skip enclosure service devices when probing for usb storage devices.
>
> This avoids long timeouts when probing for external usb harddisks
> which provide "Enclosure Services".
>
> Signed-off-by: Soeren Moch <smoch@web.de>
> --
>
> This is a new version of the patch
> "usb_storage: skip all unknown devices when probing"
> http://http://lists.denx.de/pipermail/u-boot/2014-November/194622.html
>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Tom Rini <trini@ti.com>
> ---
>   common/usb_storage.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index eb7706c..9198f73 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -1351,8 +1351,9 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss,
>   	perq = usb_stor_buf[0];
>   	modi = usb_stor_buf[1];
>
> -	if ((perq & 0x1f) == 0x1f) {
> -		/* skip unknown devices */
> +	if (((perq & 0x1f) == 0x1f) || ((perq & 0x1f) == 0x0d)) {
> +		/* skip unknown devices and enclosure service devices, */
> +		/* they would not respond to test_unit_ready           */
>   		return 0;
>   	}
>   	if ((modi&0x80) == 0x80) {
>

Is it possible to provide a configuration option and some default 
behavior, instead of just disabling it?

Regards,
Nikolay

[U-Boot] [PATCH] usb_storage: blacklist Enclosure Service Devices

[Marek Vasut]

On Saturday, November 08, 2014 at 07:02:14 AM, Soeren Moch wrote:
> Skip enclosure service devices when probing for usb storage devices.
> 
> This avoids long timeouts when probing for external usb harddisks
> which provide "Enclosure Services".
> 
> Signed-off-by: Soeren Moch <smoch@web.de>

I tweaked the comment so it matches the patch guidelines [1] and applied. 
Thanks!

[1] http://www.denx.de/wiki/U-Boot/Patches

Best regards,
Marek Vasut

[U-Boot] [PATCH] usb_storage: blacklist Enclosure Service Devices

[Soeren Moch]

>> > Skip enclosure service devices when probing for usb storage devices.
>> >
>> > This avoids long timeouts when probing for external usb harddisks
>> > which provide "Enclosure Services".
>> >
>> > Signed-off-by: Soeren Moch <smoch@web.de>

> Is it possible to provide a configuration option and some default 
> behavior, instead of just disabling it?
> 

Nikolay,

I don't understand your concern, why do you need some other default
behavior? Since the "block device" part of the harddisk keeps active,
why not skipping the "enclosure service" part? Is there any command to
access enclosure services from u-boot?

Please keep me (and maintainers) on cc when answering the mail. But
since the patch is already applied (thanks Marek!), it may be to late
for discussions anyway...

Regards,
Soeren