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@xxxxxxxx> (maintainer:USB ATTACHED SCSI) Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> (maintainer:USB MASS STORAGE DRIVER) Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> (supporter:USB SUBSYSTEM) linux-usb@xxxxxxxxxxxxxxx (open list:USB ATTACHED SCSI) linux-scsi@xxxxxxxxxxxxxxx (open list:USB ATTACHED SCSI) usb-storage@xxxxxxxxxxxxxxxxxxxxxxxx (open list:USB MASS STORAGE DRIVER) linux-kernel@xxxxxxxxxxxxxxx (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@xxxxxxxxxxxxxxx> > */ > > +/* Reported-by: Aaron Dewes <aaron.dewes@xxxxxx */ > +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@xxxxxx */ > +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@xxxxxx */ > +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@xxxxxxxxxxxx> */ > 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@xxxxxx */ > +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@xxxxxx */ > +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@xxxxxxxxxx> */ > 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@xxxxxx */ > +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@xxxxxxxxx> */ > 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@xxxxxx */ > +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@xxxxxx */ > +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@xxxxxxxxxxxx> */ > 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@xxxxxx */ > +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
