Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 4/25/21 5:50 PM, Rene Rebe wrote:
> Hey
> 
> From: Greg KH <gregkh@linuxfoundation.org>:
> 
>>>>> 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@rowland.harvard.edu> 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@exactcode.com>
>>>>>>>>>>>>
>>>>>>>>>>>> --- 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@exactcode.com>
>>>>>>>>
>>>>>>>> --- 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?
>>
>> Because the down-side of this is if we guess wrong, we break things.
>>
>>>> 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,
> 
> Ok, so I went there and wanted to quickly add the requested 1X_OK
> unusual flag, buuuutt, apparently all 32-bits of the US_FLAG enum in
> ./include/linux/usb_usual.h are already exhausted, ...

Ah yes, well that was bound to happen sooner or later.

> What should we do now? Make it 64-bit or other workaround suggestions?
> Maybe reverting the original 9 blacklist removals after all?

This is not part of any userspace API, so we can safely bump it to
a 64 bit type, say "u64", thanks.

Regards,

Hans