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

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

[Rene Rebe]

Hi there,

for some time I already wondered why my external USB Seagate Seven
drive does not report any SMART status. Only recently did I take a
look and it turns out ATA pass-through was explicitly disabed for all
Seagate drives with 7fee72 "uas: Always apply US_FL_NO_ATA_1X quirk to
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);
 

-- 
  René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
  https://exactcode.com | https://t2sde.org | https://rene.rebe.de

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

[Alan Stern]

On Sat, Apr 24, 2021 at 10:03:16PM +0200, Rene Rebe wrote:
> Hi there,
> 
> for some time I already wondered why my external USB Seagate Seven
> drive does not report any SMART status. Only recently did I take a
> look and it turns out ATA pass-through was explicitly disabed for all
> Seagate drives with 7fee72 "uas: Always apply US_FL_NO_ATA_1X quirk to

The kernel now uses >= 12-digit SHA1 values.  7fee72 is ambiguous.

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

Alan Stern

PS: As a matter of good form, you should have CC'ed the person whose 
commit you are proposing to revert.

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

[René Rebe]

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?

> Alan Stern
> 
> PS: As a matter of good form, you should have CC'ed the person whose 
> commit you are proposing to revert.

Thanks, forgot to paste them ;-)

	René

-- 
 ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin, https://exactcode.com
 https://exactscan.com | https://ocrkit.com | https://t2sde.org | https://rene.rebe.de

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

[Greg KH]

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 version of the device, right?

thanks,

greg k-h

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

[Rene Rebe]

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;
 

-- 
  René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
  https://exactcode.com | https://t2sde.org | https://rene.rebe.de

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

[Hans de Goede]

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.

Regards,

Hans

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

[Hans de Goede]

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.

Regards,

Hans

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

[Rene Rebe]

From: Hans de Goede <hdegoede@redhat.com>
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?

	René

-- 
  René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
  https://exactcode.com | https://t2sde.org | https://rene.rebe.de

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

[Greg KH]

On Sun, Apr 25, 2021 at 01:50:48PM +0200, Rene Rebe wrote:
> From: Hans de Goede <hdegoede@redhat.com>
> 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.

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.

thanks,

greg k-h

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

[Rene Rebe]

From: Greg KH <gregkh@linuxfoundation.org>
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@redhat.com>
> > 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?

> 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

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

[Greg KH]

On Sun, Apr 25, 2021 at 02:15:36PM +0200, Rene Rebe wrote:
> From: Greg KH <gregkh@linuxfoundation.org>
> 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@redhat.com>
> > > 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,
> 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.

No, let's explicitly add the devices that you know will work properly.
That way we default to a working system overall and do not cause
problems.

"first cause no harm" is a good mantra to follow...

thanks,

greg k-h

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

[Rene Rebe]

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

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

     René

-- 
  René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
  https://exactcode.com | https://t2sde.org | https://rene.rebe.de

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

[Hans de Goede]

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

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

[Alan Stern]

On Sun, Apr 25, 2021 at 02:15:36PM +0200, Rene Rebe wrote:
> From: Greg KH <gregkh@linuxfoundation.org>
> Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
> Date: Sun, 25 Apr 2021 14:00:26 +0200
> 
> > > 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?

You didn't read commit 92335ad9e895, did you?  It lists a large number 
of Seagate devices that don't work with ATA pass-through, and three of 
them have product IDs that are larger than 0xab03.

Please check the facts before guessing about things that will cause 
problems for other people.

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

That's a possibility, and in the future we may do it.  But probably not 
until the enable list grows to a comparable length.

Alan Stern

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

[Rene Rebe]

Alan Stern <stern@rowland.harvard.edu> wrote:

> On Sun, Apr 25, 2021 at 02:15:36PM +0200, Rene Rebe wrote:
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
> > Date: Sun, 25 Apr 2021 14:00:26 +0200
> > 
> > > > 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?
> 
> You didn't read commit 92335ad9e895, did you?  It lists a large number 
> of Seagate devices that don't work with ATA pass-through, and three of 
> them have product IDs that are larger than 0xab03.
> 
> Please check the facts before guessing about things that will cause 
> problems for other people.

I really don't appreciate the unfriendly tone on the linux kernel
mailing lists (which is why for 20 years I never felt like contributing
here mo), and well so far this change has caused a problem for me.
While I did not read that commit I was speaking from my experience
with USB devices and drivers, like the hundreds of Avision scanenrs my
SANE backend support and Canon, Fujitsu and Panasonic et
al. scanners. While you proof the point that proposed condition would
avoid 6 out of the 9 entires.

> > > 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.
> 
> That's a possibility, and in the future we may do it.  But probably not 
> until the enable list grows to a comparable length.

Yeah, why future proof it now, ... which is exactly what brought us
here from the original regression. The enable list will likely not
grow quickly as most users will just expect a broken device hw/
firmware and not bother looking into it and instead live w/o SMART.

But of course I do rewrite this as requested.

     René

-- 
  René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
  https://exactcode.com | https://t2sde.org | https://rene.rebe.de

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

[Hans de Goede]

Hi Rene,

On 4/25/21 5:02 PM, Rene Rebe wrote:
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
>> On Sun, Apr 25, 2021 at 02:15:36PM +0200, Rene Rebe wrote:
>>> From: Greg KH <gregkh@linuxfoundation.org>
>>> Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
>>> Date: Sun, 25 Apr 2021 14:00:26 +0200
>>>
>>>>> 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?
>>
>> You didn't read commit 92335ad9e895, did you?  It lists a large number 
>> of Seagate devices that don't work with ATA pass-through, and three of 
>> them have product IDs that are larger than 0xab03.
>>
>> Please check the facts before guessing about things that will cause 
>> problems for other people.
> 
> I really don't appreciate the unfriendly tone on the linux kernel
> mailing lists (which is why for 20 years I never felt like contributing
> here mo),

I'm sorry to hear that the admittently sometimes unfriendly tone
on the kernel mailinglists have stopped you from contributing.

Note that I do believe that things have gotten better recently.

As for this email-thread, I don't really see anything wrong with
Alan's reply here. You have been quite argumentative in this entire
thread about how things would be much better if they are done your
way.

I cannot speak for the others but I certainly have gotten annoyed by
the tone of your emails so far, I apologize if any of that annoyance
has bleed through in the tone of my emails. I do strive to always
stay professional (but as all of us I'm only human).

<snip>

>>>> 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.
>>
>> That's a possibility, and in the future we may do it.  But probably not 
>> until the enable list grows to a comparable length.
> 
> Yeah, why future proof it now, ...

Perhaps look in the mirror and start with improving the tone of your
own emails ?

> which is exactly what brought us
> here from the original regression. The enable list will likely not
> grow quickly as most users will just expect a broken device hw/
> firmware and not bother looking into it and instead live w/o SMART.

Right because people can happily live without SMART most users won't
even know they're missing some optional functionality. Non working disks
OTOH will lead to bug reports, bug reports of which Alan, Greg and I
will be on the receiving end.

Moreover the kernel has a very strong no regressions policy, and what
you suggest would almost certainly break stuff for some of our users,
so we can simply not do as you suggest.

I notice that you call the lack of SMART support on your own disk
a regression, but that has never worked with any recent kernel
(due to the discussed code which has been present since 2017) so
from the no-regressions kernel policy pov that is not a regression.

Regards,

Hans

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

[René Rebe]

Hi,

> On 25. Apr 2021, at 20:25, Hans de Goede <hdegoede@redhat.com> wrote:
> 
> Hi Rene,
> 
> On 4/25/21 5:02 PM, Rene Rebe wrote:
>> Alan Stern <stern@rowland.harvard.edu> wrote:
>> 
>>> On Sun, Apr 25, 2021 at 02:15:36PM +0200, Rene Rebe wrote:
>>>> From: Greg KH <gregkh@linuxfoundation.org>
>>>> Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
>>>> Date: Sun, 25 Apr 2021 14:00:26 +0200
>>>> 
>>>>>> 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?
>>> 
>>> You didn't read commit 92335ad9e895, did you?  It lists a large number 
>>> of Seagate devices that don't work with ATA pass-through, and three of 
>>> them have product IDs that are larger than 0xab03.
>>> 
>>> Please check the facts before guessing about things that will cause 
>>> problems for other people.
>> 
>> I really don't appreciate the unfriendly tone on the linux kernel
>> mailing lists (which is why for 20 years I never felt like contributing
>> here mo),
> 
> I'm sorry to hear that the admittently sometimes unfriendly tone
> on the kernel mailinglists have stopped you from contributing.
> 
> Note that I do believe that things have gotten better recently.
> 
> As for this email-thread, I don't really see anything wrong with
> Alan's reply here. You have been quite argumentative in this entire
> thread about how things would be much better if they are done your
> way.
> 
> I cannot speak for the others but I certainly have gotten annoyed by
> the tone of your emails so far, I apologize if any of that annoyance
> has bleed through in the tone of my emails. I do strive to always
> stay professional (but as all of us I'm only human).
> 
> <snip>
> 
>>>>> 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.
>>> 
>>> That's a possibility, and in the future we may do it.  But probably not 
>>> until the enable list grows to a comparable length.
>> 
>> Yeah, why future proof it now, ...
> 
> Perhaps look in the mirror and start with improving the tone of your
> own emails ?

Thanks, I re-read them and find them pretty ok.

>> which is exactly what brought us
>> here from the original regression. The enable list will likely not
>> grow quickly as most users will just expect a broken device hw/
>> firmware and not bother looking into it and instead live w/o SMART.
> 
> Right because people can happily live without SMART most users won't
> even know they're missing some optional functionality. Non working disks
> OTOH will lead to bug reports, bug reports of which Alan, Greg and I
> will be on the receiving end.
> 
> Moreover the kernel has a very strong no regressions policy, and what
> you suggest would almost certainly break stuff for some of our users,
> so we can simply not do as you suggest.
> 
> I notice that you call the lack of SMART support on your own disk
> a regression, but that has never worked with any recent kernel
> (due to the discussed code which has been present since 2017) so
> from the no-regressions kernel policy pov that is not a regression.

Well, it was working before, and does not since the quoted commit.
I call that a regression. Weather that was recently or some years
ago does not change the definition of regression IMHO.

On further internet searching that there are at least 4 more drivers
listed on the smartmontools page that should work:

	https://www.smartmontools.org/wiki/Supported_USB-Devices

Plus a dedicated page about this change and devices stopped working:

	https://www.smartmontools.org/wiki/SAT-with-UAS-Linux

Given this, I will not continue spending my time on implementing
what you suggested and instead simply reverted this for our Linux
SDE as I believe results in the best out of the box experience
for our users:

	https://svn.exactcode.de/t2/trunk/package/base/linux/uas-seagate.patch

Have a great day,
	René Rebe

-- 
 ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin, https://exactcode.com
 https://exactscan.com | https://ocrkit.com | https://t2sde.org | https://rene.rebe.de

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

[Hans de Goede]

Hi,

On 4/25/21 10:52 PM, René Rebe wrote:
> Hi,
> 
>> On 25. Apr 2021, at 20:25, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Rene,
>>
>> On 4/25/21 5:02 PM, Rene Rebe wrote:
>>> Alan Stern <stern@rowland.harvard.edu> wrote:
>>>
>>>> On Sun, Apr 25, 2021 at 02:15:36PM +0200, Rene Rebe wrote:
>>>>> From: Greg KH <gregkh@linuxfoundation.org>
>>>>> Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
>>>>> Date: Sun, 25 Apr 2021 14:00:26 +0200
>>>>>
>>>>>>> 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?
>>>>
>>>> You didn't read commit 92335ad9e895, did you?  It lists a large number 
>>>> of Seagate devices that don't work with ATA pass-through, and three of 
>>>> them have product IDs that are larger than 0xab03.
>>>>
>>>> Please check the facts before guessing about things that will cause 
>>>> problems for other people.
>>>
>>> I really don't appreciate the unfriendly tone on the linux kernel
>>> mailing lists (which is why for 20 years I never felt like contributing
>>> here mo),
>>
>> I'm sorry to hear that the admittently sometimes unfriendly tone
>> on the kernel mailinglists have stopped you from contributing.
>>
>> Note that I do believe that things have gotten better recently.
>>
>> As for this email-thread, I don't really see anything wrong with
>> Alan's reply here. You have been quite argumentative in this entire
>> thread about how things would be much better if they are done your
>> way.
>>
>> I cannot speak for the others but I certainly have gotten annoyed by
>> the tone of your emails so far, I apologize if any of that annoyance
>> has bleed through in the tone of my emails. I do strive to always
>> stay professional (but as all of us I'm only human).
>>
>> <snip>
>>
>>>>>> 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.
>>>>
>>>> That's a possibility, and in the future we may do it.  But probably not 
>>>> until the enable list grows to a comparable length.
>>>
>>> Yeah, why future proof it now, ...
>>
>> Perhaps look in the mirror and start with improving the tone of your
>> own emails ?
> 
> Thanks, I re-read them and find them pretty ok.

I was directly referring to your "Yeah, why future proof it now, ..."
remark here. But more in general I personally find the tone of your
emails in this thread somewhat "combative".

I believe that in the end we all want the same thing which is to help
Linux users to get the best experience possible. It just seems that we
disagree on how to reach that goal.

Assuming that I have that correct, then I believe that there is a lot
of common ground between us; and I wish that we could approach this
in a way where we try to find a solution which we can all agree on.

Ideally we could just wave a magic wand and make this all work,
but unfortunately reality is not cooperating, so we need to come up
with some pragmatic solution here.

<snip>

> On further internet searching that there are at least 4 more drivers
> listed on the smartmontools page that should work:
> 
> 	https://www.smartmontools.org/wiki/Supported_USB-Devices

That is a very interesting link thank you. That certainly advocates
for a generic approach introducing a new US_FL_ATA_1X_OK and then
adding quirks setting that for both your model and the 4 models
listed there.

I would really appreciate it if you could submit a patch series
for this. But if you don't want to do that then I'll put this on
my own TODO list.

> Plus a dedicated page about this change and devices stopped working:
> 
> 	https://www.smartmontools.org/wiki/SAT-with-UAS-Linux

Thank you, that is another interesting link. Note that the page does
acknowledge that the problem with Seagate enclosures is real and
that in many cases the choice is falling back to usb-storage support
with working ATA pass-through, or UAS without ATA pass-through.

Given the huge performance advantages of UAS, especially with SSDs,
IMHO it is better to go with UAS in this case. But I guess in some
scenarios SMART support may be more important then UAS support.

I'll contact the author of that wiki page to discuss this further
with him and see if he has any good ideas for this problem.

> Given this, I will not continue spending my time on implementing
> what you suggested and instead simply reverted this for our Linux
> SDE as I believe results in the best out of the box experience
> for our users:
> 
> 	https://svn.exactcode.de/t2/trunk/package/base/linux/uas-seagate.patch

I've taken a quick peek at this and I see that you've also restored
the old per model quirks to disable ATA pass-through on known to be
broken models, good.

Note that the list of broken models which you've added it missing the
0xab25 and 0xab38 product-ids which according to:
https://www.smartmontools.org/wiki/Supported_USB-Devices
have broken ATA passthrough support with UAS.

If I assume that these behave as some of the other Seagate drivers and
the bridge-chip crashes when trying to use ATA pass-through, then once
these changes hit your users and customers you have just broken usage
of those disks together with your product. This nicely illustrates
why we don't want to make this change in the mainline kernel.

Note depending on how important disk performance is for you
an alternative approach might be to modify the Seagate product-id check
to simply disable UAS on Seagate devices, that would be a lot safer.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 4/26/21 10:16 AM, Hans de Goede wrote:

<snip>

>> 	https://www.smartmontools.org/wiki/SAT-with-UAS-Linux
> 
> Thank you, that is another interesting link. Note that the page does
> acknowledge that the problem with Seagate enclosures is real and
> that in many cases the choice is falling back to usb-storage support
> with working ATA pass-through, or UAS without ATA pass-through.
> 
> Given the huge performance advantages of UAS, especially with SSDs,
> IMHO it is better to go with UAS in this case. But I guess in some
> scenarios SMART support may be more important then UAS support.
> 
> I'll contact the author of that wiki page to discuss this further
> with him and see if he has any good ideas for this problem.

Done:

https://listi.jpberlin.de/pipermail/smartmontools-support/2021-April/000670.html

Note I wasn't sure if I should add any of the people here to the Cc.
I've chosen not to do that. Let me know if you want me to add you
to the Cc in my next email in that thread.

Alternatively you can temporarily subscribe to the list:
https://listi.jpberlin.de/mailman/listinfo/smartmontools-support
it is very low traffic.

Regards,

Hans

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

[Rene Rebe]

From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 26 Apr 2021 10:16:12 +0200

> Hi,
> 
> On 4/25/21 10:52 PM, René Rebe wrote:
> > Hi,
> > 
> >> On 25. Apr 2021, at 20:25, Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi Rene,
> >>
> >> On 4/25/21 5:02 PM, Rene Rebe wrote:
> >>> Alan Stern <stern@rowland.harvard.edu> wrote:
> >>>
> >>>> On Sun, Apr 25, 2021 at 02:15:36PM +0200, Rene Rebe wrote:
> >>>>> From: Greg KH <gregkh@linuxfoundation.org>
> >>>>> Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
> >>>>> Date: Sun, 25 Apr 2021 14:00:26 +0200
> >>>>>
> >>>>>>> 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?
> >>>>
> >>>> You didn't read commit 92335ad9e895, did you?  It lists a large number 
> >>>> of Seagate devices that don't work with ATA pass-through, and three of 
> >>>> them have product IDs that are larger than 0xab03.
> >>>>
> >>>> Please check the facts before guessing about things that will cause 
> >>>> problems for other people.
> >>>
> >>> I really don't appreciate the unfriendly tone on the linux kernel
> >>> mailing lists (which is why for 20 years I never felt like contributing
> >>> here mo),
> >>
> >> I'm sorry to hear that the admittently sometimes unfriendly tone
> >> on the kernel mailinglists have stopped you from contributing.
> >>
> >> Note that I do believe that things have gotten better recently.
> >>
> >> As for this email-thread, I don't really see anything wrong with
> >> Alan's reply here. You have been quite argumentative in this entire
> >> thread about how things would be much better if they are done your
> >> way.
> >>
> >> I cannot speak for the others but I certainly have gotten annoyed by
> >> the tone of your emails so far, I apologize if any of that annoyance
> >> has bleed through in the tone of my emails. I do strive to always
> >> stay professional (but as all of us I'm only human).
> >>
> >> <snip>
> >>
> >>>>>> 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.
> >>>>
> >>>> That's a possibility, and in the future we may do it.  But probably not 
> >>>> until the enable list grows to a comparable length.
> >>>
> >>> Yeah, why future proof it now, ...
> >>
> >> Perhaps look in the mirror and start with improving the tone of your
> >> own emails ?
> > 
> > Thanks, I re-read them and find them pretty ok.
> 
> I was directly referring to your "Yeah, why future proof it now, ..."
> remark here. But more in general I personally find the tone of your
> emails in this thread somewhat "combative".
> 
> I believe that in the end we all want the same thing which is to help
> Linux users to get the best experience possible. It just seems that we
> disagree on how to reach that goal.
> 
> Assuming that I have that correct, then I believe that there is a lot
> of common ground between us; and I wish that we could approach this
> in a way where we try to find a solution which we can all agree on.
> 
> Ideally we could just wave a magic wand and make this all work,
> but unfortunately reality is not cooperating, so we need to come up
> with some pragmatic solution here.

I did not mean to be compative, however, as usual in real life we just
do not agree with all the reasoning ;-)

> > On further internet searching that there are at least 4 more drivers
> > listed on the smartmontools page that should work:
> > 
> > 	https://www.smartmontools.org/wiki/Supported_USB-Devices
> 
> That is a very interesting link thank you. That certainly advocates
> for a generic approach introducing a new US_FL_ATA_1X_OK and then
> adding quirks setting that for both your model and the 4 models
> listed there.
> 
> I would really appreciate it if you could submit a patch series
> for this. But if you don't want to do that then I'll put this on
> my own TODO list.

Maybe another week, however, as this is not the semantic I prefer that
would only cause more work for me with a bigger reverting patch in our
tree at the end, ...

<snip>

> > Given this, I will not continue spending my time on implementing
> > what you suggested and instead simply reverted this for our Linux
> > SDE as I believe results in the best out of the box experience
> > for our users:
> > 
> > 	https://svn.exactcode.de/t2/trunk/package/base/linux/uas-seagate.patch
> 
> I've taken a quick peek at this and I see that you've also restored
> the old per model quirks to disable ATA pass-through on known to be
> broken models, good.

Yes, I reverted that, and added two more I found from the old email
thread that probably triggered the code change back in the day.

> Note that the list of broken models which you've added it missing the
> 0xab25 and 0xab38 product-ids which according to:
> https://www.smartmontools.org/wiki/Supported_USB-Devices
> have broken ATA passthrough support with UAS.

Thanks, I added those two now as well.

> If I assume that these behave as some of the other Seagate drivers and
> the bridge-chip crashes when trying to use ATA pass-through, then once
> these changes hit your users and customers you have just broken usage
> of those disks together with your product. This nicely illustrates
> why we don't want to make this change in the mainline kernel.
> 
> Note depending on how important disk performance is for you
> an alternative approach might be to modify the Seagate product-id check
> to simply disable UAS on Seagate devices, that would be a lot safer.

We do not run a smartd by default, and I actually prefer a driver that
deaults to behave by the standard book, and get notified when
something goes wrong, instead of globally disallow listing a whole
vendor.

Maybe it is still an option to restore the updated unusual quirk list
entries, that way users with newer devices get their SMART back sooner
than later and this also encourages Seagate to continue producing
fully working devices, without hiding any ATA pass-through bugs by
default ;-) Given the hidden nature of this, it probably would take
more than a decade to ok-list working devices by interested users,
which hopefully going forward should hopefully be all new ones.

	René

-- 
  René Rebe, ExactCODE GmbH, Lietzenburger Str. 42, DE-10789 Berlin
  https://exactcode.com | https://t2sde.org | https://rene.rebe.de

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

[Hans de Goede]

Hi,

On 4/26/21 11:40 AM, Rene Rebe wrote:
> From: Hans de Goede <hdegoede@redhat.com>
> Date: Mon, 26 Apr 2021 10:16:12 +0200
> 
>> Hi,
>>
>> On 4/25/21 10:52 PM, René Rebe wrote:

<snip>

>> Ideally we could just wave a magic wand and make this all work,
>> but unfortunately reality is not cooperating, so we need to come up
>> with some pragmatic solution here.
> 
> I did not mean to be compative, however, as usual in real life we just
> do not agree with all the reasoning ;-)

Yes, thank you for understanding.

>>> On further internet searching that there are at least 4 more drivers
>>> listed on the smartmontools page that should work:
>>>
>>> 	https://www.smartmontools.org/wiki/Supported_USB-Devices
>>
>> That is a very interesting link thank you. That certainly advocates
>> for a generic approach introducing a new US_FL_ATA_1X_OK and then
>> adding quirks setting that for both your model and the 4 models
>> listed there.
>>
>> I would really appreciate it if you could submit a patch series
>> for this. But if you don't want to do that then I'll put this on
>> my own TODO list.
> 
> Maybe another week, however, as this is not the semantic I prefer that
> would only cause more work for me with a bigger reverting patch in our
> tree at the end, ...

Thank you for considering working on this. If you decide not to do
this in the end, please let me know then I'll add this to my
(way too long) TODO list.

> <snip>
> 
>>> Given this, I will not continue spending my time on implementing
>>> what you suggested and instead simply reverted this for our Linux
>>> SDE as I believe results in the best out of the box experience
>>> for our users:
>>>
>>> 	https://svn.exactcode.de/t2/trunk/package/base/linux/uas-seagate.patch
>>
>> I've taken a quick peek at this and I see that you've also restored
>> the old per model quirks to disable ATA pass-through on known to be
>> broken models, good.
> 
> Yes, I reverted that, and added two more I found from the old email
> thread that probably triggered the code change back in the day.

Ah I was already why there were more entries then I expected,
good job.


>> Note that the list of broken models which you've added it missing the
>> 0xab25 and 0xab38 product-ids which according to:
>> https://www.smartmontools.org/wiki/Supported_USB-Devices
>> have broken ATA passthrough support with UAS.
> 
> Thanks, I added those two now as well.

Great, that means your patch will be a good starting point for
the broken devices list if we do ever decide to flip the
default for Seagate devices back to allowing ATA pass-through by
default.

>> If I assume that these behave as some of the other Seagate drivers and
>> the bridge-chip crashes when trying to use ATA pass-through, then once
>> these changes hit your users and customers you have just broken usage
>> of those disks together with your product. This nicely illustrates
>> why we don't want to make this change in the mainline kernel.
>>
>> Note depending on how important disk performance is for you
>> an alternative approach might be to modify the Seagate product-id check
>> to simply disable UAS on Seagate devices, that would be a lot safer.
> 
> We do not run a smartd by default>

Ah yes that helps, unfortunately in the mainline kernel we cannot
assume that that is the case.

> and I actually prefer a driver that
> deaults to behave by the standard book, and get notified when
> something goes wrong, instead of globally disallow listing a whole
> vendor.
> 
> Maybe it is still an option to restore the updated unusual quirk list
> entries, that way users with newer devices get their SMART back sooner
> than later and this also encourages Seagate to continue producing
> fully working devices, without hiding any ATA pass-through bugs by
> default ;-)

It is always an option :) I just don't think that this is the right
moment in time to do it. Notice that your email archive digging +
the https://www.smartmontools.org/wiki/Supported_USB-Devices have
turned op 4 new broken devices for which we did not have quirks
before. I'm simply afraid that that is just the tip of the iceberg.
Causing peoples disks to stop working is not just a bug, it is
a very very bad bug, so yeah I'm quite conservative here, sorry.

So for now I believe that the best thing we can do is to agree
that we disagree on the best way to handle this.

(Now if only I had this magic-wand which could give me the complete
list of all broken models, then things would be different.)

Regards,

Hans

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

[Hans de Goede]

Hi,

On 4/25/21 9:20 AM, 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.

At least Fedora ships with smartmontools with a systemd service monitoring the
driver enabled by default; and I would not be surprised if other distros do
the same.

> I don’t have any non-supporting device - can we not just add them to the
> quirk list when someone reports one?

We have a no regressions rule in the kernel, this would cause regressions,
so NACK. Also the code which you are removing replaced a long list of non-working
Seagate devices, it is not just one or 2 models, we started out with model
specific quirks and when the list grew too long we went for this option.

Something else to keep in mind here is that:

1. Every single user out there wants to have a working drive, and removing the
quirk breaks that for older Seagate drive enclosures.

2. Only some advanced users care about SMART monitoring.

So what you are suggesting is breaking older Seagate drives for everyone to
enable a feature which only few users actually care about.

So as Greg said I believe that the best option would be to add a new
US_FL_ATA_1X_OK flag, add a quirk for your drive(s) and make the code setting
US_FL_NO_ATA_1X for all Seagate devices check that flag.

Regards,

Hans