[PATCH] Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS"

[PATCH] Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS"

[Juhyung Park]

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(-)

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

Re: [PATCH] Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS"

[Oliver Neukum]

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

Re: [PATCH] Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS"

[Juhyung Park]

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

Re: [PATCH] Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS"

[Alan Stern]

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
> 

Re: [PATCH] Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS"

[Juhyung Park]

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
> >

Re: [PATCH] Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS"

[Oliver Neukum]

On 17.01.23 07:16, Juhyung Park wrote:
> Posted a new patch:
> https://lore.kernel.org/all/20230117061046.114145-1-qkrwngud825@gmail.com/T/#u
> 
> This issue has been stalled for way too long. If string-based
> comparisons are too ugly, we can improve it later when Hongling is
> more active towards this issue.

Hi,

very well, you really see no other solution?
If so, could you please enhance the commit log to literally state
that there is no other way to tell them apart? And then resubmit?

	Regards
		Oliver

Re: [PATCH] Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS"

[Juhyung Park]

Hi Oliver,

On Tue, Jan 17, 2023 at 5:09 PM Oliver Neukum <oneukum@suse.com> wrote:
>
>
>
> On 17.01.23 07:16, Juhyung Park wrote:
> > Posted a new patch:
> > https://lore.kernel.org/all/20230117061046.114145-1-qkrwngud825@gmail.com/T/#u
> >
> > This issue has been stalled for way too long. If string-based
> > comparisons are too ugly, we can improve it later when Hongling is
> > more active towards this issue.
>
> Hi,
>
> very well, you really see no other solution?
> If so, could you please enhance the commit log to literally state
> that there is no other way to tell them apart? And then resubmit?

My unit is fine and only Hongling's isn't.

I'm fairly certain that this is not the best way to deal with that
specific defective unit, but unless Hongling actively participates in
debugging and providing more info, the affected users (who have their
completely fine RTL9210 enclosure suddenly have UAS nuked, including
me) suffer more and more.

String-based method is the only way *I* can do to keep Hongling's
working with usb-storage while restoring UAS back to the rest of the
RTL9210 enclosures with limited info Hongling's supplied thus far.

I'm not sure if this is worthy enough to write in the commit message,
let me know what you think.
Maybe link the relevant lore.kernel.org discussions directly into the
commit message?

Thanks, regards

>
>         Regards
>                 Oliver

Re: [PATCH] Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS"

[Oliver Neukum]

On 17.01.23 09:18, Juhyung Park wrote:

Hi,

> I'm not sure if this is worthy enough to write in the commit message,
> let me know what you think.
> Maybe link the relevant lore.kernel.org discussions directly into the
> commit message?

By any means. The patch is the best you can do. I see and appreciate it.
It is still ugly. Yet, if there is no better way, so be it. But this
needs an extensive justification in the change log.
Please rather be wordier than you think necessary than too short.

I'll ack it.

	Regards
		Oliver

Re: [PATCH] Revert "usb-storage: Add Hiksemi USB3-FW to IGNORE_UAS"

[Juhyung Park]

Hi Oliver,

On Tue, Jan 17, 2023 at 5:34 PM Oliver Neukum <oneukum@suse.com> wrote:
>
>
>
> On 17.01.23 09:18, Juhyung Park wrote:
>
> Hi,
>
> > I'm not sure if this is worthy enough to write in the commit message,
> > let me know what you think.
> > Maybe link the relevant lore.kernel.org discussions directly into the
> > commit message?
>
> By any means. The patch is the best you can do. I see and appreciate it.
> It is still ugly. Yet, if there is no better way, so be it. But this
> needs an extensive justification in the change log.
> Please rather be wordier than you think necessary than too short.
>
> I'll ack it.

Thanks again for your input.

I've posted v2 here:
https://lore.kernel.org/all/20230117085154.123301-1-qkrwngud825@gmail.com/

I've added cc to stable as well, forgot that for v1.

Thanks, regards.

>
>         Regards
>                 Oliver
>