Add unusal uas devices reported by Umbrel users

---
 drivers/usb/storage/unusual_uas.h | 70 +++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

--
2.30.1

On Sun, Mar 07, 2021 at 04:18:30PM +0100, Aaron Dewes wrote:
> ---
>  drivers/usb/storage/unusual_uas.h | 70 +++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch does not have a Signed-off-by: line.  Please read the
  kernel file, Documentation/SubmittingPatches and resend it after
  adding that line.  Note, the line needs to be in the body of the
  email, before the patch, not at the bottom of the patch or in the
  email signature.

- You did not specify a description of why the patch is needed, or
  possibly, any description at all, in the email body.  Please read the
  section entitled "The canonical patch format" in the kernel file,
  Documentation/SubmittingPatches for what is needed in order to
  properly describe the change.

- You did not write a descriptive Subject: for the patch, allowing Greg,
  and everyone else, to know what this patch is all about.  Please read
  the section entitled "The canonical patch format" in the kernel file,
  Documentation/SubmittingPatches for what a proper Subject: line should
  look like.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Hi,

First of all I'm not the maintainer of this file, to find the
right email address to submit patches to see:


[hans@x1 linux]$ scripts/get_maintainer.pl -f drivers/usb/storage/uas.c 
Oliver Neukum <oneukum@suse.com> (maintainer:USB ATTACHED SCSI)
Alan Stern <stern@rowland.harvard.edu> (maintainer:USB MASS STORAGE DRIVER)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM)
linux-usb@vger.kernel.org (open list:USB ATTACHED SCSI)
linux-scsi@vger.kernel.org (open list:USB ATTACHED SCSI)
usb-storage@lists.one-eyed-alien.net (open list:USB MASS STORAGE DRIVER)
linux-kernel@vger.kernel.org (open list)


On 3/7/21 4:18 PM, Aaron Dewes wrote:

No commit message ? No explanation, just disable UAS on a long list
of devices, including several very popular devices such as various
Samsung models, without any explanation at all ?

Sorry, but in that case all I can do is NACK this patch.

Please split this into a series with one patch per model, while
explaining for each mode what the problem is, including links to
bug-reports or forum discussions.

Disabling UAS is a big hammer and severely limits performance,
as it also disables all forms of NCQ which is a must have
feature to get decent performance from SSDs.

And often UAS is not the problem, disabling UAS simply papers
over the problem, e.g.:

1. Disabling it either hides issue with the XHCI bulk-streams
handling (could be a bug in the XHCI controller, or in the kernel's
XHCI code).

2. Disabling it makes the SSD use much less power since it now
cannot multi-task, so often it is hiding problems with the
power-supplied to the disk.

I see at https://github.com/getumbrel/umbrel that the recommended
platform for Umbrel is a Raspberry Pi 4. Which certainly explains
the long list of UAS problems. Pi-s are famous for their USB
problems. I don't know if the Pi 4 suffers from 1. above, but I
would not be surprised if does; and the Pi certainly suffers from 2.
https://www.raspberrypi.org/forums/viewtopic.php?p=1507278 is an
interesting read here. The USB-3 spec says that USB-3 ports must
be capable of 0.9 A per port, the R-Pi 4 maxes out at 1.2A
for all 4 ports combined.. And then only if its own power-supply
is capable enough; and nothing is said there about how low the
voltage drops when actually drawing 1.2A and any voltage drop
itself might also be an issue.

Also see below for some inline comments.



> ---
>  drivers/usb/storage/unusual_uas.h | 70 +++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h
> index f9677a5ec31b..a67ed2b527fa 100644
> --- a/drivers/usb/storage/unusual_uas.h
> +++ b/drivers/usb/storage/unusual_uas.h
> @@ -28,6 +28,27 @@
>   * and don't forget to CC: the USB development list <linux-usb@vger.kernel.org>
>   */
> 
> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
> +UNUSUAL_DEV(0x04e8, 0x4001, 0x0000, 0x9999,
> +		"Samsung",
> +		"SSD",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),

There is no need to specify US_FL_NO_REPORT_OPCODES if you are
disabling UAS, same for all the other entries.

> +
> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
> +UNUSUAL_DEV(0x04e8, 0x61b6, 0x0000, 0x9999,
> +		"Samsung",
> +		"M3 Portable Hard Drive",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
> +
> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
> +UNUSUAL_DEV(0x04e8, 0x61f5, 0x0000, 0x9999,
> +		"Samsung",
> +		"Portable SSD T5",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
> +

These are very popular drives if these really had broken
UAS support then we would already have a ton of reports
about these.

>  /* Reported-by: Till Dörges <doerges@pre-sense.de> */
>  UNUSUAL_DEV(0x054c, 0x087d, 0x0000, 0x9999,
>  		"Sony",
> @@ -62,6 +83,20 @@ UNUSUAL_DEV(0x0bc2, 0x331a, 0x0000, 0x9999,
>  		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>  		US_FL_NO_REPORT_LUNS),
> 
> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
> +UNUSUAL_DEV(0x1058, 0x082a, 0x0000, 0x9999,
> +		"Western Digital",
> +		"SSD",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),

Idem.

> +
> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
> +UNUSUAL_DEV(0x125f, 0xa76a, 0x0000, 0x9999,
> +		"ADATA",
> +		"ED600 enclosure",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
> +
>  /* Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> */
>  UNUSUAL_DEV(0x13fd, 0x3940, 0x0000, 0x9999,
>  		"Initio Corporation",
> @@ -76,6 +111,13 @@ UNUSUAL_DEV(0x152d, 0x0539, 0x0000, 0x9999,
>  		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>  		US_FL_NO_REPORT_OPCODES),
> 
> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
> +UNUSUAL_DEV(0x152d, 0x0562, 0x0000, 0x9999,
> +		"JMicron",
> +		"JMS567",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
> +

These are very popular chipsets if these really had broken
UAS support then we would already have a ton of reports
about these.

>  /* Reported-by: Claudio Bizzarri <claudio.bizzarri@gmail.com> */
>  UNUSUAL_DEV(0x152d, 0x0567, 0x0000, 0x9999,
>  		"JMicron",
> @@ -90,6 +132,20 @@ UNUSUAL_DEV(0x152d, 0x0578, 0x0000, 0x9999,
>  		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>  		US_FL_BROKEN_FUA),
> 
> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
> +UNUSUAL_DEV(0x152d, 0x1561, 0x0000, 0x9999,
> +		"JMicron",
> +		"JMS561U",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
> +
> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
> +UNUSUAL_DEV(0x152d, 0x1561, 0x0000, 0x9999,
> +		"JMicron",
> +		"External Disk connector",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
> +

Idem.

>  /* Reported-by: Thinh Nguyen <thinhn@synopsys.com> */
>  UNUSUAL_DEV(0x154b, 0xf00b, 0x0000, 0x9999,
>  		"PNY",
> @@ -104,6 +160,13 @@ UNUSUAL_DEV(0x154b, 0xf00d, 0x0000, 0x9999,
>  		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>  		US_FL_NO_ATA_1X),
> 
> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
> +UNUSUAL_DEV(0x174c, 0x55aa, 0x0000, 0x9999,
> +		"ASMedia",
> +		"ASM1051E and ASM1053E",
> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
> +

Another very popular chipsets, some older versions of these
indeed have some issues, we have special code handling these
with chipset version specific behavior, see:

drivers/usb/storage/uas-detect.h

Which also means that UAS is actually know to work fine
on the newer models and it should not be outright disabled
on them as you are doing here!

Together with the samsung drivers + jmicron bridge chipsets you
are disabling UAS on what is probably 50% of all capable UAS
drives out there (if not 70% or more).

Did I already say: NACK ?


At the top I indicated that you should split this in per
model patches, but that was before I figured out most of
these problems are beeing seen on Raspberry Pi-s (and likely
on Raspberry Pi-s only).

What might be worth considering is disabling bulk-stream support
on the Pi-s XHCI controller by setting the  XHCI_BROKEN_STREAMS
flag in xhci->quirks for that controller.  This will disable UAS
at least when the drivers are plugged into the USB3 / superspeed
ports of the Pi.  This might still be a too big hammer though.
since with a powered-hub, or with a proper power-supply feeding
the Pi 4 (and not using the other ports) UAS might still work
and you are now taking away those options from users.

But setting the XHCI_BROKEN_STREAMS flag on the Pi 4 and only
on the Pi 4 would definitely be closer to acceptable then disabling
UAS for, well everyone, because it is causing issues on the Pi.

Regards,

Hans

Am 07.03.21 um 16:53 schrieb Hans de Goede:
> Hi,
>
> First of all I'm not the maintainer of this file, to find the
> right email address to submit patches to see:
>
>
> [hans@x1 linux]$ scripts/get_maintainer.pl -f drivers/usb/storage/uas.c
> Oliver Neukum <oneukum@suse.com> (maintainer:USB ATTACHED SCSI)
> Alan Stern <stern@rowland.harvard.edu> (maintainer:USB MASS STORAGE DRIVER)
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM)
> linux-usb@vger.kernel.org (open list:USB ATTACHED SCSI)
> linux-scsi@vger.kernel.org (open list:USB ATTACHED SCSI)
> usb-storage@lists.one-eyed-alien.net (open list:USB MASS STORAGE DRIVER)
> linux-kernel@vger.kernel.org (open list)

Then the beginning of that file needs to be updated, because a comment
at the beginning said:


/*
  * If you edit this file, please try to keep it sorted first by VendorID,
  * then by ProductID.
  *
  * If you want to add an entry for this file, be sure to include the
  * following information:
  *      - a patch that adds the entry for your device, including your
  *        email address right above the entry (plus maybe a brief
  *        explanation of the reason for the entry),
  *      - lsusb -v output for the device
  * Send your submission to Hans de Goede <hdegoede@redhat.com>
  * and don't forget to CC: the USB development list
<linux-usb@vger.kernel.org>
  */


>
> No commit message ? No explanation, just disable UAS on a long list
> of devices, including several very popular devices such as various
> Samsung models, without any explanation at all ?
Okay, I'll make sure to get more information and improve the commit.
> Sorry, but in that case all I can do is NACK this patch.
>
> Please split this into a series with one patch per model, while
> explaining for each mode what the problem is, including links to
> bug-reports or forum discussions.
>
> Disabling UAS is a big hammer and severely limits performance,
> as it also disables all forms of NCQ which is a must have
> feature to get decent performance from SSDs.
>
> And often UAS is not the problem, disabling UAS simply papers
> over the problem, e.g.:
>
> 1. Disabling it either hides issue with the XHCI bulk-streams
> handling (could be a bug in the XHCI controller, or in the kernel's
> XHCI code).
>
> 2. Disabling it makes the SSD use much less power since it now
> cannot multi-task, so often it is hiding problems with the
> power-supplied to the disk.
>
> I see at https://github.com/getumbrel/umbrel that the recommended
> platform for Umbrel is a Raspberry Pi 4. Which certainly explains
> the long list of UAS problems. Pi-s are famous for their USB
> problems. I don't know if the Pi 4 suffers from 1. above, but I
> would not be surprised if does; and the Pi certainly suffers from 2.
> https://www.raspberrypi.org/forums/viewtopic.php?p=1507278 is an
> interesting read here. The USB-3 spec says that USB-3 ports must
> be capable of 0.9 A per port, the R-Pi 4 maxes out at 1.2A
> for all 4 ports combined.. And then only if its own power-supply
> is capable enough; and nothing is said there about how low the
> voltage drops when actually drawing 1.2A and any voltage drop
> itself might also be an issue.
>
> Also see below for some inline comments.
>
>
>
>> ---
>>   drivers/usb/storage/unusual_uas.h | 70 +++++++++++++++++++++++++++++++
>>   1 file changed, 70 insertions(+)
>>
>> diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h
>> index f9677a5ec31b..a67ed2b527fa 100644
>> --- a/drivers/usb/storage/unusual_uas.h
>> +++ b/drivers/usb/storage/unusual_uas.h
>> @@ -28,6 +28,27 @@
>>    * and don't forget to CC: the USB development list <linux-usb@vger.kernel.org>
>>    */
>>
>> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
>> +UNUSUAL_DEV(0x04e8, 0x4001, 0x0000, 0x9999,
>> +		"Samsung",
>> +		"SSD",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
> There is no need to specify US_FL_NO_REPORT_OPCODES if you are
> disabling UAS, same for all the other entries.
>
>> +
>> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
>> +UNUSUAL_DEV(0x04e8, 0x61b6, 0x0000, 0x9999,
>> +		"Samsung",
>> +		"M3 Portable Hard Drive",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
>> +
>> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
>> +UNUSUAL_DEV(0x04e8, 0x61f5, 0x0000, 0x9999,
>> +		"Samsung",
>> +		"Portable SSD T5",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
>> +
> These are very popular drives if these really had broken
> UAS support then we would already have a ton of reports
> about these.
>
>>   /* Reported-by: Till Dörges <doerges@pre-sense.de> */
>>   UNUSUAL_DEV(0x054c, 0x087d, 0x0000, 0x9999,
>>   		"Sony",
>> @@ -62,6 +83,20 @@ UNUSUAL_DEV(0x0bc2, 0x331a, 0x0000, 0x9999,
>>   		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>>   		US_FL_NO_REPORT_LUNS),
>>
>> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
>> +UNUSUAL_DEV(0x1058, 0x082a, 0x0000, 0x9999,
>> +		"Western Digital",
>> +		"SSD",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
> Idem.
>
>> +
>> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
>> +UNUSUAL_DEV(0x125f, 0xa76a, 0x0000, 0x9999,
>> +		"ADATA",
>> +		"ED600 enclosure",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
>> +
>>   /* Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> */
>>   UNUSUAL_DEV(0x13fd, 0x3940, 0x0000, 0x9999,
>>   		"Initio Corporation",
>> @@ -76,6 +111,13 @@ UNUSUAL_DEV(0x152d, 0x0539, 0x0000, 0x9999,
>>   		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>>   		US_FL_NO_REPORT_OPCODES),
>>
>> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
>> +UNUSUAL_DEV(0x152d, 0x0562, 0x0000, 0x9999,
>> +		"JMicron",
>> +		"JMS567",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
>> +
> These are very popular chipsets if these really had broken
> UAS support then we would already have a ton of reports
> about these.
>
>>   /* Reported-by: Claudio Bizzarri <claudio.bizzarri@gmail.com> */
>>   UNUSUAL_DEV(0x152d, 0x0567, 0x0000, 0x9999,
>>   		"JMicron",
>> @@ -90,6 +132,20 @@ UNUSUAL_DEV(0x152d, 0x0578, 0x0000, 0x9999,
>>   		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>>   		US_FL_BROKEN_FUA),
>>
>> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
>> +UNUSUAL_DEV(0x152d, 0x1561, 0x0000, 0x9999,
>> +		"JMicron",
>> +		"JMS561U",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
>> +
>> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
>> +UNUSUAL_DEV(0x152d, 0x1561, 0x0000, 0x9999,
>> +		"JMicron",
>> +		"External Disk connector",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
>> +
> Idem.
>
>>   /* Reported-by: Thinh Nguyen <thinhn@synopsys.com> */
>>   UNUSUAL_DEV(0x154b, 0xf00b, 0x0000, 0x9999,
>>   		"PNY",
>> @@ -104,6 +160,13 @@ UNUSUAL_DEV(0x154b, 0xf00d, 0x0000, 0x9999,
>>   		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>>   		US_FL_NO_ATA_1X),
>>
>> +/* Reported-by: Aaron Dewes <aaron.dewes@web.de */
>> +UNUSUAL_DEV(0x174c, 0x55aa, 0x0000, 0x9999,
>> +		"ASMedia",
>> +		"ASM1051E and ASM1053E",
>> +		USB_SC_DEVICE, USB_PR_DEVICE, NULL,
>> +		US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS),
>> +
> Another very popular chipsets, some older versions of these
> indeed have some issues, we have special code handling these
> with chipset version specific behavior, see:
>
> drivers/usb/storage/uas-detect.h
>
> Which also means that UAS is actually know to work fine
> on the newer models and it should not be outright disabled
> on them as you are doing here!
>
> Together with the samsung drivers + jmicron bridge chipsets you
> are disabling UAS on what is probably 50% of all capable UAS
> drives out there (if not 70% or more).
>
> Did I already say: NACK ?
>
>
> At the top I indicated that you should split this in per
> model patches, but that was before I figured out most of
> these problems are beeing seen on Raspberry Pi-s (and likely
> on Raspberry Pi-s only).
>
> What might be worth considering is disabling bulk-stream support
> on the Pi-s XHCI controller by setting the  XHCI_BROKEN_STREAMS
> flag in xhci->quirks for that controller.  This will disable UAS
> at least when the drivers are plugged into the USB3 / superspeed
> ports of the Pi.  This might still be a too big hammer though.
> since with a powered-hub, or with a proper power-supply feeding
> the Pi 4 (and not using the other ports) UAS might still work
> and you are now taking away those options from users.
>
> But setting the XHCI_BROKEN_STREAMS flag on the Pi 4 and only
> on the Pi 4 would definitely be closer to acceptable then disabling
> UAS for, well everyone, because it is causing issues on the Pi.
>
> Regards,
>
> Hans

Okay, thanks for your feedback! I'll try to check if some of these
drives have issues on PCs too and then only use that for a future patch.

 > There is no need to specify US_FL_NO_REPORT_OPCODES if you are
disabling UAS, same for all the other entries.

I'm sorry about that, I actually just copied one of the definitions
which had both from the file and edited it.

Aaron