[PATCH fixes for 3.17 0/2] uas: Disable uas on ASM1051 devices

[PATCH fixes for 3.17 0/2] uas: Disable uas on ASM1051 devices

[Hans de Goede]

Hi Greg,

I've received a number of bug-reports from users related to uas on ASM1051
chipset using devices. After some searching around I've managed to get myself
an ASM1051 device.

As a result I've spend the last 4 days trying to get the ASM1051 chipset to
work. After some initial success which makes it work with most disks I've
laying around (patch 1), I hit problems with one disk where it just would
not work.

To makes matters worse the ASM1051 chipset shares its usb-id with older
versions of the ASM1053, which works fine with all disks.

So I've come up with a patch which disables uas on all ASM1051 devices and on
some ASM1053 devices. Both the need to blacklist and the actual blacklisting
code are less then satisfactory, esp. after 4 days of work. But this is the
best I can come up with :|

It seems that older uas-bridge chips and xhci controllers streams support are
suffering from quite a few hw bugs, and that blacklisting is the best we can
do.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] uas: Set no_report_opcodes

[Hans de Goede]

asm1051e usb <-> sata bridges hang when receiving a report opcodes scsi cmnd.
Take a page out of the usb-storage book, and simple disable no_report_opcodes
outright.

Cc: stable@vger.kernel.org # 3.16
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/storage/uas.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 3f42785..4e2f576 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -944,6 +944,8 @@ static int uas_slave_alloc(struct scsi_device *sdev)
 	 */
 	blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
 
+	sdev->no_report_opcodes = 1;
+
 	return 0;
 }
 
-- 
2.1.0

Re: [PATCH 1/2] uas: Set no_report_opcodes

[Christoph Hellwig]

On Tue, Sep 09, 2014 at 04:59:59PM +0200, Hans de Goede wrote:
> asm1051e usb <-> sata bridges hang when receiving a report opcodes scsi cmnd.
> Take a page out of the usb-storage book, and simple disable no_report_opcodes
> outright.

Given that this device also seems broken in other ways can we wait a bit
before using the big hammer?  I'm still hoping UAS might give us some
better SCSI implementation so that we don't have to disable any kind of
advanced feature.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] uas: Set no_report_opcodes

[Hans de Goede]

Hi,

On 09/09/2014 06:01 PM, Christoph Hellwig wrote:
> On Tue, Sep 09, 2014 at 04:59:59PM +0200, Hans de Goede wrote:
>> asm1051e usb <-> sata bridges hang when receiving a report opcodes scsi cmnd.
>> Take a page out of the usb-storage book, and simple disable no_report_opcodes
>> outright.
>
> Given that this device also seems broken in other ways can we wait a bit
> before using the big hammer?

Actually the big hammer I would like to avoid is disabling uas all together
on these devices, as they work fine with 2 out of 3 of the 3 disks I've
tested with, as long as no report opcodes is not used.

Which is why my other patch also includes a log message to explain how
to enable uas despite the blacklist, but that will only work if we don't
send report opcodes.

 > I'm still hoping UAS might give us some
> better SCSI implementation so that we don't have to disable any kind of
> advanced feature.

I understand, so an alternative would be to make this a quirk and only
set it for ASM1051/ASM1053 bridges.

Even better would be to combine this with figuring out why the bridge is
becoming unhappy when paired with a crucial m500 ssd, and fix that +
a report opcodes quirk, so that we don't have the blacklist it at all.

Any ideas how to fix the unhappiness ? I've already tried setting all
the sdev flags the usb-storage driver sets, but that does not help.

Regards,

Hans



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] uas: Set no_report_opcodes

[Hans de Goede]

Hi,

On 09/09/2014 09:21 PM, Hans de Goede wrote:
> Hi,
> 
> On 09/09/2014 06:01 PM, Christoph Hellwig wrote:
>> On Tue, Sep 09, 2014 at 04:59:59PM +0200, Hans de Goede wrote:
>>> asm1051e usb <-> sata bridges hang when receiving a report opcodes scsi cmnd.
>>> Take a page out of the usb-storage book, and simple disable no_report_opcodes
>>> outright.
>>
>> Given that this device also seems broken in other ways can we wait a bit
>> before using the big hammer?
> 
> Actually the big hammer I would like to avoid is disabling uas all together
> on these devices, as they work fine with 2 out of 3 of the 3 disks I've
> tested with, as long as no report opcodes is not used.
> 
> Which is why my other patch also includes a log message to explain how
> to enable uas despite the blacklist, but that will only work if we don't
> send report opcodes.
> 
>> I'm still hoping UAS might give us some
>> better SCSI implementation so that we don't have to disable any kind of
>> advanced feature.
> 
> I understand, so an alternative would be to make this a quirk and only
> set it for ASM1051/ASM1053 bridges.

So further (stress) testing of ASM1051 bridges with disks with which they
do work under normal conditions have shown that even with those disks
they can get stuck / hang in a state which can only be recovered by an
unplug + replug (power cycle).

So basically ASM1051 bridges should just never be used together with uas,
which means that this patch can be dropped.

So: self-nak.

I'll respin the second patch in this set to change the log messages a
bit to reflect this new insight.

Regards,

Hans

[PATCH 2/2] uas: Disable uas on ASM1051 devices

[Hans de Goede]

Even with REPORT SUPPORTED OPERATION CODES blacklisted the ASM1051 chipset
still does not work when combined with some disks, e.g. a Crucial M500 ssd.

When used with a troublesome disk, the chipset throws all kinds of USB errors,
and eventually hangs, where as in BOT mode it works fine.

To make matters worse the ASM1051 chipset and older versions of the ASM1053
chipset use the same usb-id.

When connected over USB-3 the 2 can be told apart by the number of streams
they support. So this patch adds some less then pretty code to disable uas for
the ASM1051. When connected over USB-2, simply disable uas alltogether for
devices with the shared usb-id.

This ends up disabling uas in many cases where it actually works fine, which
is unfortunate, but better then leaving some users with completely non working
external disks. This patch adds a log level explaining how perfomance conscious
users can re-enable uas.

Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # 3.16
Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/storage/uas-detect.h | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h
index 503ac5c..3119f3f 100644
--- a/drivers/usb/storage/uas-detect.h
+++ b/drivers/usb/storage/uas-detect.h
@@ -59,10 +59,6 @@ static int uas_use_uas_driver(struct usb_interface *intf,
 	unsigned long flags = id->driver_info;
 	int r, alt;
 
-	usb_stor_adjust_quirks(udev, &flags);
-
-	if (flags & US_FL_IGNORE_UAS)
-		return 0;
 
 	alt = uas_find_uas_alt_setting(intf);
 	if (alt < 0)
@@ -72,6 +68,36 @@ static int uas_use_uas_driver(struct usb_interface *intf,
 	if (r < 0)
 		return 0;
 
+	/*
+	 * ASM1051 and older ASM1053 devices have the same usb-id, and UAS is
+	 * broken with some disks on the ASM1051, use the number of streams to
+	 * differentiate between the 2. Newer ASM1053 devices also support 32
+	 * streams, but have a different product-id.
+	 */
+	if (udev->descriptor.idVendor == 0x174c &&
+			udev->descriptor.idProduct == 0x55aa) {
+		if (udev->speed < USB_SPEED_SUPER) {
+			/* No streams info, assume ASM1051E */
+			dev_warn(&udev->dev, "Assuming ASM1051E chipset\n");
+			flags |= US_FL_IGNORE_UAS;
+		} else if (usb_ss_max_streams(&eps[1]->ss_ep_comp) == 32) {
+			dev_warn(&udev->dev, "Detected ASM1051E chipset\n");
+			flags |= US_FL_IGNORE_UAS;
+		}
+	}
+
+	usb_stor_adjust_quirks(udev, &flags);
+
+	if (flags & US_FL_IGNORE_UAS) {
+		dev_warn(&udev->dev,
+			"UAS does not work with this chipset with some disks, using usb-storage instead\n");
+		dev_warn(&udev->dev,
+			"Add 'usb-storage.quirks=%04x:%04x:' to the kernel commandline to try UAS anyways\n",
+			le16_to_cpu(udev->descriptor.idVendor),
+			le16_to_cpu(udev->descriptor.idProduct));
+		return 0;
+	}
+
 	if (udev->bus->sg_tablesize == 0) {
 		dev_warn(&udev->dev,
 			"The driver for the USB controller %s does not support scatter-gather which is\n",
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] uas: Disable uas on ASM1051 devices

[Alan Stern]

On Tue, 9 Sep 2014, Hans de Goede wrote:

> Even with REPORT SUPPORTED OPERATION CODES blacklisted the ASM1051 chipset
> still does not work when combined with some disks, e.g. a Crucial M500 ssd.
> 
> When used with a troublesome disk, the chipset throws all kinds of USB errors,
> and eventually hangs, where as in BOT mode it works fine.
> 
> To make matters worse the ASM1051 chipset and older versions of the ASM1053
> chipset use the same usb-id.
> 
> When connected over USB-3 the 2 can be told apart by the number of streams
> they support. So this patch adds some less then pretty code to disable uas for
> the ASM1051. When connected over USB-2, simply disable uas alltogether for
> devices with the shared usb-id.
> 
> This ends up disabling uas in many cases where it actually works fine, which
> is unfortunate, but better then leaving some users with completely non working
> external disks. This patch adds a log level explaining how perfomance conscious
> users can re-enable uas.
> 
> Cc: stable@vger.kernel.org # 3.16
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/usb/storage/uas-detect.h | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h
> index 503ac5c..3119f3f 100644
> --- a/drivers/usb/storage/uas-detect.h
> +++ b/drivers/usb/storage/uas-detect.h
> @@ -59,10 +59,6 @@ static int uas_use_uas_driver(struct usb_interface *intf,
>  	unsigned long flags = id->driver_info;
>  	int r, alt;
>  
> -	usb_stor_adjust_quirks(udev, &flags);
> -
> -	if (flags & US_FL_IGNORE_UAS)
> -		return 0;
>  
>  	alt = uas_find_uas_alt_setting(intf);
>  	if (alt < 0)
> @@ -72,6 +68,36 @@ static int uas_use_uas_driver(struct usb_interface *intf,
>  	if (r < 0)
>  		return 0;
>  
> +	/*
> +	 * ASM1051 and older ASM1053 devices have the same usb-id, and UAS is
> +	 * broken with some disks on the ASM1051, use the number of streams to
> +	 * differentiate between the 2. Newer ASM1053 devices also support 32
> +	 * streams, but have a different product-id.
> +	 */
> +	if (udev->descriptor.idVendor == 0x174c &&
> +			udev->descriptor.idProduct == 0x55aa) {
> +		if (udev->speed < USB_SPEED_SUPER) {
> +			/* No streams info, assume ASM1051E */
> +			dev_warn(&udev->dev, "Assuming ASM1051E chipset\n");
> +			flags |= US_FL_IGNORE_UAS;
> +		} else if (usb_ss_max_streams(&eps[1]->ss_ep_comp) == 32) {
> +			dev_warn(&udev->dev, "Detected ASM1051E chipset\n");
> +			flags |= US_FL_IGNORE_UAS;
> +		}
> +	}
> +
> +	usb_stor_adjust_quirks(udev, &flags);

This won't work.  usb_stor_adjust_quirks masks out all the quirks that 
it handles before applying the user-specified quirks.  Therefore it 
will erase your US_FL_IGNORE_UAS flag.

You might as well remove this call and leave the earlier call to
usb_stor_adjust_quirks (in the first hunk of the patch) as is.

Alan Stern

Re: [PATCH 2/2] uas: Disable uas on ASM1051 devices

[Hans de Goede]

Hi,

On 09/09/2014 05:23 PM, Alan Stern wrote:
> On Tue, 9 Sep 2014, Hans de Goede wrote:
>
>> Even with REPORT SUPPORTED OPERATION CODES blacklisted the ASM1051 chipset
>> still does not work when combined with some disks, e.g. a Crucial M500 ssd.
>>
>> When used with a troublesome disk, the chipset throws all kinds of USB errors,
>> and eventually hangs, where as in BOT mode it works fine.
>>
>> To make matters worse the ASM1051 chipset and older versions of the ASM1053
>> chipset use the same usb-id.
>>
>> When connected over USB-3 the 2 can be told apart by the number of streams
>> they support. So this patch adds some less then pretty code to disable uas for
>> the ASM1051. When connected over USB-2, simply disable uas alltogether for
>> devices with the shared usb-id.
>>
>> This ends up disabling uas in many cases where it actually works fine, which
>> is unfortunate, but better then leaving some users with completely non working
>> external disks. This patch adds a log level explaining how perfomance conscious
>> users can re-enable uas.
>>
>> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # 3.16
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/usb/storage/uas-detect.h | 34 ++++++++++++++++++++++++++++++----
>>   1 file changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/storage/uas-detect.h b/drivers/usb/storage/uas-detect.h
>> index 503ac5c..3119f3f 100644
>> --- a/drivers/usb/storage/uas-detect.h
>> +++ b/drivers/usb/storage/uas-detect.h
>> @@ -59,10 +59,6 @@ static int uas_use_uas_driver(struct usb_interface *intf,
>>   	unsigned long flags = id->driver_info;
>>   	int r, alt;
>>
>> -	usb_stor_adjust_quirks(udev, &flags);
>> -
>> -	if (flags & US_FL_IGNORE_UAS)
>> -		return 0;
>>
>>   	alt = uas_find_uas_alt_setting(intf);
>>   	if (alt < 0)
>> @@ -72,6 +68,36 @@ static int uas_use_uas_driver(struct usb_interface *intf,
>>   	if (r < 0)
>>   		return 0;
>>
>> +	/*
>> +	 * ASM1051 and older ASM1053 devices have the same usb-id, and UAS is
>> +	 * broken with some disks on the ASM1051, use the number of streams to
>> +	 * differentiate between the 2. Newer ASM1053 devices also support 32
>> +	 * streams, but have a different product-id.
>> +	 */
>> +	if (udev->descriptor.idVendor == 0x174c &&
>> +			udev->descriptor.idProduct == 0x55aa) {
>> +		if (udev->speed < USB_SPEED_SUPER) {
>> +			/* No streams info, assume ASM1051E */
>> +			dev_warn(&udev->dev, "Assuming ASM1051E chipset\n");
>> +			flags |= US_FL_IGNORE_UAS;
>> +		} else if (usb_ss_max_streams(&eps[1]->ss_ep_comp) == 32) {
>> +			dev_warn(&udev->dev, "Detected ASM1051E chipset\n");
>> +			flags |= US_FL_IGNORE_UAS;
>> +		}
>> +	}
>> +
>> +	usb_stor_adjust_quirks(udev, &flags);
>
> This won't work.  usb_stor_adjust_quirks masks out all the quirks that
> it handles before applying the user-specified quirks.  Therefore it
> will erase your US_FL_IGNORE_UAS flag.

Only if there is a matching quirk entry in the quirks parameter, and then
masking it out is fine, we want to allow this as we're setting the
US_FL_IGNORE_US flag on bridges where it is known to cause issues when
combined with *some* disks, while uas works fine with other disks, so
users may want to override it.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html