On Tue, Jan 10, 2023 at 12:48 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > Cc: Hongling Zeng <zenghongling@xxxxxxxxxx>
> > Signed-off-by: Juhyung Park <qkrwngud825@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx> */
> > -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@xxxxxxxxxx> */
> > UNUSUAL_DEV(0x13fd, 0x3940, 0x0000, 0x9999,
> > "Initio Corporation",
> > --
> > 2.39.0
> >
