From: Hans de Goede <hdegoede@xxxxxxxxxx>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 12:58:40 +0200
> Hi,
>
> On 4/25/21 12:47 PM, Hans de Goede wrote:
> > Hi,
> >
> > On 4/25/21 12:41 PM, Rene Rebe wrote:
> >> Greg KH wrote:
> >>
> >>> On Sun, Apr 25, 2021 at 09:20:59AM +0200, René Rebe wrote:
> >>>> Hey,
> >>>>
> >>>>> On 25. Apr 2021, at 04:31, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> >>>>>> Seagate devices" in 2017. Apparently some early ones where buggy, ...
> >>>>>>
> >>>>>> However, fast forward a couple of years and this is no longer true,
> >>>>>> this Segate Seven even is already from 2016, and apparently first
> >>>>>> available in 2015. I suggest removing this rather drastic global
> >>>>>> measure, and instead only add very old broken ones with individual
> >>>>>> quirks, should any of them still be alive ;-)
> >>>>>>
> >>>>>> Signed-off-by: René Rebe <rene@xxxxxxxxxxxxx>
> >>>>>>
> >>>>>> --- linux-5.11/drivers/usb/storage/uas-detect.h.backup 2021-03-05 11:36:00.517423726 +0100
> >>>>>> +++ linux-5.11/drivers/usb/storage/uas-detect.h 2021-03-05 11:36:16.373424544 +0100
> >>>>>> @@ -113,8 +113,4 @@
> >>>>>> }
> >>>>>>
> >>>>>> - /* All Seagate disk enclosures have broken ATA pass-through support */
> >>>>>> - if (le16_to_cpu(udev->descriptor.idVendor) == 0x0bc2)
> >>>>>> - flags |= US_FL_NO_ATA_1X;
> >>>>>> -
> >>>>>> usb_stor_adjust_quirks(udev, &flags);
> >>>>>
> >>>>> I don't want to do this unless you can suggest an approach that won't
> >>>>> suddenly break all those old buggy drives. Just because they are now
> >>>>> five years old or more doesn't mean they are no longer in use.
> >>>>
> >>>> Well, what do you propose then? A allow quirk for all new devices going forward?
> >>>> Given that the user usually needs to actively run something like smartctl
> >>>> manually on the drive I don’t see that this should cause too many issues.
> >>>> I don’t have any non-supporting device - can we not just add them to the
> >>>> quirk list when someone reports one?
> >>>
> >>> How about since you know your device works, you make the check detect
> >>> your specific device and not apply the flag to it? You should be able
> >>> to do so based on the
> >>
> >> Sure, while that does not really solve this for all the other newer
> >> Seagate drives other users might have at home, here is a patch
> >> checking for this one USB product ID. I hope that is what you meant:
> >>
> >> Signed-off-by: René Rebe <rene@xxxxxxxxxxxxx>
> >>
> >> --- linux-5.11/drivers/usb/storage/uas-detect.h.backup 2021-03-05 11:36:00.517423726 +0100
> >> +++ linux-5.11/drivers/usb/storage/uas-detect.h 2021-03-05 11:36:16.373424544 +0100
> >> @@ -113,5 +113,6 @@
> >>
> >> /* All Seagate disk enclosures have broken ATA pass-through support */
> >> - if (le16_to_cpu(udev->descriptor.idVendor) == 0x0bc2)
> >> + if ((le16_to_cpu(udev->descriptor.idVendor) == 0x0bc2) &&
> >> + (le16_to_cpu(udev->descriptor.idProduct) != 0xab03))
> >> flags |= US_FL_NO_ATA_1X;
> >>
> >>
> >
> > As I indicated in my other email which crossed with this one, please make this
> > more generic, add a new US_FL_ATA_1X_OK flag and make the above code check that +
> > add a new unusual_uas.h entry for your device setting the new flag.
> >
> > Note there is no need to add support for the new flag to usb_stor_adjust_quirks()
> > if a user overrides quirks for a device on the kernel commandline without specifying
> > the "t" flag then the US_FL_NO_ATA_1X flag will already get cleared.
> >
> > I deliberately put the:
> >
> > if (le16_to_cpu(udev->descriptor.idVendor) == 0x0bc2)
> > flags |= US_FL_NO_ATA_1X;
> >
> > code before the usb_stor_adjust_quirks() call to allow users to override this
> > from the kernel commandline.
>
> p.s.
>
> A "git log drivers/usb/storage/unusual_uas.h" quickly finds the commit which removed the
> quirks which the generic Seagate check replaces. At that time there were US_FL_NO_ATA_1X
> quirks for *9* different Seagate models present in unusual_uas.h and I assume someone
> reporting a 10th model is what made me go for the just disable this for all Seagate
> driver option.
>
> See commit 92335ad9e895 ("uas: Remove US_FL_NO_ATA_1X unusual device entries for Seagate devices")
>
> Also I did a quick websearch for the "Seagate Seven" and rather then the usual re-usable
> drive-enclosure with a standard 2.5" or 3.5" drive in there, this seems to be a custom
> model where the enclosure is actually integrated into the drive to make it smaller.
>
> So I would not be surprised if this is using another chipset then their usual enclosures,
> which would explain why it does have working ATA1x passthrough.
I would expect that more modern devices to work. Vendors usually
linearly allocate their product ids for new devices, and we could
allow list product ids higher than this Seven to unbreak more modern
devices by default and limit the amount of device quirks needed?
René
--
René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
https://exactcode.com | https://t2sde.org | https://rene.rebe.de
