From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 14:00:26 +0200
> On Sun, Apr 25, 2021 at 01:50:48PM +0200, Rene Rebe wrote:
> > 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?
>
> Vendors do not allocate device ids that way at all, as there is no
> requirement to do so. I know of many vendors that seemingly use random
> values from their product id space, so there is no guarantee that this
> will work, sorry.
I did not say it is a requirement, just that they usually do speaking
of just this Seagate case. What is wrong with using that to
potentially significantly cut down the quirk list?
> What is wrong with just allowing specific devices that you have tested
> will work, to the list instead? That's the safest way to handle this.
The problem is that out of the box it does not work for users, and
normal users do not dive into the kernel code to find out and simply
think their devices sucks. Even I for years thought the drive sucks,
before I took a closer look. If you long term want more new devices in
an allow list than the previous quirk list included (9 or 10 does not
sound that many to me), ... whatever you prefer ,-) I would rather
have 10 quirk disable list than potential many more white listed the
next years to come. Maybe we shoudl simply restore the prevoius
quirks.
René
--
René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
https://exactcode.com | https://t2sde.org | https://rene.rebe.de
