| Message ID | 20230109115550.71688-1-qkrwngud825@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS" | expand |
On 09.01.23 12:55, Juhyung Park wrote: > This reverts commit e00b488e813f0f1ad9f778e771b7cd2fe2877023. > > The commit e00b488e813f ("usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS") > blacklists UAS for the entire RTL9210 enclosures. Realtek's VendorId is 0x0bda > and RTL9210 enclosures reports 0x9210 for its ProductId. > > The RTL9210 controller was advertised with UAS since its release back in 2019 > and was shipped with a lot of enclosure products with different firmware > combinations. > > If UAS blacklisting is really required said product (Hiksemi USB3-FW), it > should be done without blacklisting the entire RTL9210 products. Hi, I see this the issue. Do you have an idea how to limit the scope. Hongling Zeng, do you have an idea, respectively if not, could you provide "lsusb -v" for the defective device? Regards Oliver
Hi Oliver, On Mon, Jan 9, 2023 at 9:02 PM Oliver Neukum <oneukum@suse.com> wrote: > > > > On 09.01.23 12:55, Juhyung Park wrote: > > This reverts commit e00b488e813f0f1ad9f778e771b7cd2fe2877023. > > > > The commit e00b488e813f ("usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS") > > blacklists UAS for the entire RTL9210 enclosures. Realtek's VendorId is 0x0bda > > and RTL9210 enclosures reports 0x9210 for its ProductId. > > > > The RTL9210 controller was advertised with UAS since its release back in 2019 > > and was shipped with a lot of enclosure products with different firmware > > combinations. > > > > If UAS blacklisting is really required said product (Hiksemi USB3-FW), it > > should be done without blacklisting the entire RTL9210 products. > > Hi, > > I see this the issue. Do you have an idea how to limit the scope. Unfortunately, no. This might be the ugly case where, if a proper workaround could be found (if the original report is valid at all), it may change the code logic itself with some if branch rather than just unusual_uas.h. With my RTL9210 enclosure, using multiple different firmware versions still reports the same bcdDevice. Note that, despite Hongling reporting that Windows doesn't use UAS in https://lore.kernel.org/all/fbeffee7-3ac5-4798-14b0-724e0ed01947@126.com/ , Windows uses it on mine and respectively trim works. > > Hongling Zeng, do you have an idea, respectively if not, could > you provide "lsusb -v" for the defective device? > Hongling didn't respond to Greg when he asked the same question back in November: https://lore.kernel.org/all/Y29RtXGcey6V9iTY@kroah.com/ Anyways, here's my lsusb -v output. Hope it helps: Bus 004 Device 002: ID 0bda:9210 Realtek Semiconductor Corp. RTL9210 M.2 NVME Adapter Device Descriptor: bLength 18 bDescriptorType 1 bcdUSB 3.20 bDeviceClass 0 bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize0 9 idVendor 0x0bda Realtek Semiconductor Corp. idProduct 0x9210 RTL9210 M.2 NVME Adapter bcdDevice 20.01 iManufacturer 1 Realtek iProduct 2 RTL9210 iSerial 3 012345678906 bNumConfigurations 1 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 0x0079 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 bmAttributes 0x80 (Bus Powered) MaxPower 896mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 0 bNumEndpoints 2 bInterfaceClass 8 Mass Storage bInterfaceSubClass 6 SCSI bInterfaceProtocol 80 Bulk-Only iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 15 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x02 EP 2 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 15 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber 0 bAlternateSetting 1 bNumEndpoints 4 bInterfaceClass 8 Mass Storage bInterfaceSubClass 6 SCSI bInterfaceProtocol 98 iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 15 MaxStreams 32 Data-in pipe (0x03) Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x02 EP 2 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 15 MaxStreams 32 Data-out pipe (0x04) Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 15 MaxStreams 32 Status pipe (0x02) Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x04 EP 4 OUT bmAttributes 2 Transfer Type Bulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 0 Command pipe (0x01) Binary Object Store Descriptor: bLength 5 bDescriptorType 15 wTotalLength 0x003e bNumDeviceCaps 4 USB 2.0 Extension Device Capability: bLength 7 bDescriptorType 16 bDevCapabilityType 2 bmAttributes 0x00000006 BESL Link Power Management (LPM) Supported SuperSpeed USB Device Capability: bLength 10 bDescriptorType 16 bDevCapabilityType 3 bmAttributes 0x00 wSpeedsSupported 0x000e Device can operate at Full Speed (12Mbps) Device can operate at High Speed (480Mbps) Device can operate at SuperSpeed (5Gbps) bFunctionalitySupport 1 Lowest fully-functional device speed is Full Speed (12Mbps) bU1DevExitLat 10 micro seconds bU2DevExitLat 2047 micro seconds SuperSpeedPlus USB Device Capability: bLength 20 bDescriptorType 16 bDevCapabilityType 10 bmAttributes 0x00000001 Sublink Speed Attribute count 1 Sublink Speed ID count 0 wFunctionalitySupport 0x1100 bmSublinkSpeedAttr[0] 0x000a4030 Speed Attribute ID: 0 10Gb/s Symmetric RX SuperSpeedPlus bmSublinkSpeedAttr[1] 0x000a40b0 Speed Attribute ID: 0 10Gb/s Symmetric TX SuperSpeedPlus Container ID Device Capability: bLength 20 bDescriptorType 16 bDevCapabilityType 4 bReserved 0 ContainerID {03020100-0504-0706-0002-020200020202} Device Status: 0x0000 (Bus Powered) > Regards > Oliver
On Mon, Jan 09, 2023 at 08:55:50PM +0900, Juhyung Park wrote: > This reverts commit e00b488e813f0f1ad9f778e771b7cd2fe2877023. > > The commit e00b488e813f ("usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS") > blacklists UAS for the entire RTL9210 enclosures. Realtek's VendorId is 0x0bda > and RTL9210 enclosures reports 0x9210 for its ProductId. > > The RTL9210 controller was advertised with UAS since its release back in 2019 > and was shipped with a lot of enclosure products with different firmware > combinations. > > If UAS blacklisting is really required said product (Hiksemi USB3-FW), it > should be done without blacklisting the entire RTL9210 products. We cannot simply revert a patch if it fixes a problem for some devices. The devices would then stop working and that would be a regression, which is not allowed. It will be necessary to find some other way of solving this problem. For example, a small piece of test code which can safely determine whether the firmware can handle UAS. Alan Stern > Fixes: e00b488e813f ("usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS") > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Hongling Zeng <zenghongling@kylinos.cn> > Signed-off-by: Juhyung Park <qkrwngud825@gmail.com> > --- > drivers/usb/storage/unusual_uas.h | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h > index 251778d14e2d..c7b763d6d102 100644 > --- a/drivers/usb/storage/unusual_uas.h > +++ b/drivers/usb/storage/unusual_uas.h > @@ -83,13 +83,6 @@ UNUSUAL_DEV(0x0bc2, 0x331a, 0x0000, 0x9999, > USB_SC_DEVICE, USB_PR_DEVICE, NULL, > US_FL_NO_REPORT_LUNS), > > -/* Reported-by: Hongling Zeng <zenghongling@kylinos.cn> */ > -UNUSUAL_DEV(0x0bda, 0x9210, 0x0000, 0x9999, > - "Hiksemi", > - "External HDD", > - USB_SC_DEVICE, USB_PR_DEVICE, NULL, > - US_FL_IGNORE_UAS), > - > /* Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> */ > UNUSUAL_DEV(0x13fd, 0x3940, 0x0000, 0x9999, > "Initio Corporation", > -- > 2.39.0 >
On Tue, Jan 10, 2023 at 12:48 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, Jan 09, 2023 at 08:55:50PM +0900, Juhyung Park wrote: > > This reverts commit e00b488e813f0f1ad9f778e771b7cd2fe2877023. > > > > The commit e00b488e813f ("usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS") > > blacklists UAS for the entire RTL9210 enclosures. Realtek's VendorId is 0x0bda > > and RTL9210 enclosures reports 0x9210 for its ProductId. > > > > The RTL9210 controller was advertised with UAS since its release back in 2019 > > and was shipped with a lot of enclosure products with different firmware > > combinations. > > > > If UAS blacklisting is really required said product (Hiksemi USB3-FW), it > > should be done without blacklisting the entire RTL9210 products. > > We cannot simply revert a patch if it fixes a problem for some devices. > The devices would then stop working and that would be a regression, > which is not allowed. This to me, sounds equivalent to "disable trim on all NVMe SSDs on 'some' vendor because it fixes issues on one reported case, 3 years after release". More thorough reviews should have taken place to begin with. Reading through previous threads for all 7 patch-sets(!), there apparently was no effort spent in minimizing the affected products, especially when 0x0bda is blatantly a VendorId for Realtek, or to avoid using US_FL_IGNORE_UAS at all and try other workarounds, not to mention Hongling's questionable method of determining whether Windows uses UAS on it too. His product name in the commit is also questionable. RTL9210 is a NVMe-to-USB bridge, but his commit names it "Hiksemi External HDD". I was unable to find any product online that matches "Hiksemi External HDD" that could be using a NVMe-to-USB bridge. Disabling UAS can even workaround a broken hardware, I've seen it personally happen with a JMS567 controller: the device originally worked just fine with UAS enabled on both Linux and Windows, later it started to not work on both platforms and it started working again under Linux when UAS was disabled. I'd be not surprised if Hongling's unit is defective. Unlike US_FL_BROKEN_FUA or US_FL_NO_REPORT_OPCODES, US_FL_IGNORE_UAS is far more detrimental to both performance and functionality. For users like me, the original patch is a regression itself as I need trim to work (which is my topmost concern, rather than just raw performance). RTL9210 is an extremely popular NVMe-to-USB bridge controller and the original patch-set was merged with no real deep thought and reviews spent into evaluating its effect. With Hongling not responding to Greg's question for nearly 2 months, I'm afraid this original patch does more harm than good left long-term. Just my two cents, apologies in advance for my strong words. Thanks, regards > > It will be necessary to find some other way of solving this problem. > For example, a small piece of test code which can safely determine > whether the firmware can handle UAS. > > Alan Stern > > > Fixes: e00b488e813f ("usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS") > > Cc: Alan Stern <stern@rowland.harvard.edu> > > Cc: Hongling Zeng <zenghongling@kylinos.cn> > > Signed-off-by: Juhyung Park <qkrwngud825@gmail.com> > > --- > > drivers/usb/storage/unusual_uas.h | 7 ------- > > 1 file changed, 7 deletions(-) > > > > diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h > > index 251778d14e2d..c7b763d6d102 100644 > > --- a/drivers/usb/storage/unusual_uas.h > > +++ b/drivers/usb/storage/unusual_uas.h > > @@ -83,13 +83,6 @@ UNUSUAL_DEV(0x0bc2, 0x331a, 0x0000, 0x9999, > > USB_SC_DEVICE, USB_PR_DEVICE, NULL, > > US_FL_NO_REPORT_LUNS), > > > > -/* Reported-by: Hongling Zeng <zenghongling@kylinos.cn> */ > > -UNUSUAL_DEV(0x0bda, 0x9210, 0x0000, 0x9999, > > - "Hiksemi", > > - "External HDD", > > - USB_SC_DEVICE, USB_PR_DEVICE, NULL, > > - US_FL_IGNORE_UAS), > > - > > /* Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> */ > > UNUSUAL_DEV(0x13fd, 0x3940, 0x0000, 0x9999, > > "Initio Corporation", > > -- > > 2.39.0 > >
diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h index 251778d14e2d..c7b763d6d102 100644 --- a/drivers/usb/storage/unusual_uas.h +++ b/drivers/usb/storage/unusual_uas.h @@ -83,13 +83,6 @@ UNUSUAL_DEV(0x0bc2, 0x331a, 0x0000, 0x9999, USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NO_REPORT_LUNS), -/* Reported-by: Hongling Zeng <zenghongling@kylinos.cn> */ -UNUSUAL_DEV(0x0bda, 0x9210, 0x0000, 0x9999, - "Hiksemi", - "External HDD", - USB_SC_DEVICE, USB_PR_DEVICE, NULL, - US_FL_IGNORE_UAS), - /* Reported-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> */ UNUSUAL_DEV(0x13fd, 0x3940, 0x0000, 0x9999, "Initio Corporation",
This reverts commit e00b488e813f0f1ad9f778e771b7cd2fe2877023. The commit e00b488e813f ("usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS") blacklists UAS for the entire RTL9210 enclosures. Realtek's VendorId is 0x0bda and RTL9210 enclosures reports 0x9210 for its ProductId. The RTL9210 controller was advertised with UAS since its release back in 2019 and was shipped with a lot of enclosure products with different firmware combinations. If UAS blacklisting is really required said product (Hiksemi USB3-FW), it should be done without blacklisting the entire RTL9210 products. Fixes: e00b488e813f ("usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS") Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Hongling Zeng <zenghongling@kylinos.cn> Signed-off-by: Juhyung Park <qkrwngud825@gmail.com> --- drivers/usb/storage/unusual_uas.h | 7 ------- 1 file changed, 7 deletions(-)