unbreak all modern Seagate ATA pass-through for SMART

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 <[email protected]>

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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.

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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?

Quoted 28 lines · René Rebe

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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.

Quoted 5 lines

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

Quoted 35 lines · Greg KH

Quoted 28 lines · René Rebe

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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.

Quoted 5 lines

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?

Quoted 3 lines

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 <[email protected]>

Hi,

Quoted 51 lines · Rene Rebe

Quoted 35 lines · Greg KH

Quoted 28 lines · René Rebe

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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.

Quoted 5 lines

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?

Quoted 3 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h line 118

Quoted 7 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +2 -1

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

Hi,

Quoted 70 lines · Hans de Goede

Hi,

Quoted 51 lines · Rene Rebe

Quoted 35 lines · Greg KH

Quoted 28 lines · René Rebe

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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.

Quoted 5 lines

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?

Quoted 3 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h line 118

Quoted 7 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +2 -1

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

Quoted 7 lines

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.

From: Hans de Goede <[email protected]>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 12:58:40 +0200

Quoted 95 lines

Hi,

Quoted 70 lines · Hans de Goede

Hi,

Quoted 51 lines · Rene Rebe

Quoted 35 lines · Greg KH

Quoted 28 lines · René Rebe

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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.

Quoted 5 lines

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?

Quoted 3 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h line 118

Quoted 7 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +2 -1

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

Quoted 7 lines

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.

Quoted 2 lines

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?

Quoted 99 lines

From: Hans de Goede <[email protected]>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 12:58:40 +0200

Quoted 95 lines

Hi,

Quoted 70 lines · Hans de Goede

Hi,

Quoted 51 lines · Rene Rebe

Quoted 35 lines · Greg KH

Quoted 28 lines · René Rebe

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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.

Quoted 5 lines

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?

Quoted 3 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h line 118

Quoted 7 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +2 -1

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

Quoted 7 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

From: Greg KH <[email protected]>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 14:00:26 +0200

Quoted 108 lines · Rene Rebe

Quoted 106 lines

From: Hans de Goede <[email protected]>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 12:58:40 +0200

Quoted 89 lines

Hi,

Quoted 70 lines · Hans de Goede

Hi,

Quoted 51 lines · Rene Rebe

Quoted 35 lines · Greg KH

Quoted 28 lines · René Rebe

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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.

Quoted 5 lines

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?

Quoted 3 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h line 118

Quoted 7 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +2 -1

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

Quoted 7 lines

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.

Quoted 8 lines

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?

Quoted 4 lines

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?

Quoted 2 lines

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.

Quoted 111 lines

Quoted 110 lines

Quoted 98 lines

Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 12:58:40 +0200

Quoted 95 lines

Hi,

Quoted 70 lines · Hans de Goede

Hi,

Quoted 51 lines · Rene Rebe

Quoted 35 lines · Greg KH

Quoted 28 lines · René Rebe

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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.

Quoted 5 lines

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?

Quoted 3 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h line 118

Quoted 7 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +2 -1

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

Quoted 7 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

Quoted 2 lines

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,

Hey

From: Greg KH <[email protected]>:

Quoted 120 lines

Quoted 106 lines

Quoted 106 lines

Quoted 105 lines

Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 12:58:40 +0200

Quoted 89 lines

Hi,

Quoted 70 lines · Hans de Goede

Hi,

Quoted 51 lines · Rene Rebe

Quoted 35 lines · Greg KH

Quoted 28 lines · René Rebe

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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.

Quoted 5 lines

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?

Quoted 3 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h line 118

Quoted 7 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +2 -1

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

Quoted 7 lines

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.

Quoted 8 lines

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 3 lines

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?

From: Greg KH <[email protected]>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 14:00:26 +0200

Quoted 5 lines

Quoted 4 lines

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?

Quoted 4 lines

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?

Quoted 2 lines

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.

Quoted 9 lines · Rene Rebe

From: Greg KH <[email protected]>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 14:00:26 +0200

Quoted 5 lines

Quoted 4 lines

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 12 lines

Quoted 2 lines

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.

Quoted 23 lines · Alan Stern

Quoted 9 lines · Rene Rebe

From: Greg KH <[email protected]>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 14:00:26 +0200

Quoted 5 lines

Quoted 4 lines

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Hi Rene,

Quoted 25 lines · Rene Rebe

Quoted 23 lines · Alan Stern

Quoted 9 lines · Rene Rebe

From: Greg KH <[email protected]>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 14:00:26 +0200

Quoted 5 lines

Quoted 4 lines

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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>

Quoted 13 lines

Quoted 13 lines

Quoted 12 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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 ?

Quoted 4 lines

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.

Hi,

Quoted 70 lines · Hans de Goede

Hi Rene,

Quoted 25 lines · Rene Rebe

Quoted 23 lines · Alan Stern

Quoted 9 lines · Rene Rebe

From: Greg KH <[email protected]>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 14:00:26 +0200

Quoted 5 lines

Quoted 4 lines

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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>

Quoted 16 lines

Quoted 15 lines

Quoted 12 lines

Quoted 2 lines

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.

Quoted 4 lines

Quoted 1 line

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.

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

Quoted 1 line

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.

Hi,

Quoted 72 lines · René Rebe

Hi,

Quoted 70 lines · Hans de Goede

Hi Rene,

Quoted 25 lines · Rene Rebe

Quoted 23 lines · Alan Stern

Quoted 9 lines · Rene Rebe

From: Greg KH <[email protected]>
Subject: Re: [PATCH] unbreak all modern Seagate ATA pass-through for SMART
Date: Sun, 25 Apr 2021 14:00:26 +0200

Quoted 5 lines

Quoted 4 lines

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?

Quoted 8 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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>

Quoted 16 lines

Quoted 15 lines

Quoted 12 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

From: Hans de Goede <[email protected]>
Date: Mon, 26 Apr 2021 10:16:12 +0200

Quoted 3 lines

Hi,

On 4/25/21 10:52 PM, René Rebe wrote:

Quoted 3 lines

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 ;-)

Quoted 10 lines

Quoted 4 lines

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.

Quoted 3 lines

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>

Quoted 5 lines

Quoted 5 lines

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:

Quoted 5 lines

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 9 lines

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 ;-)

Hey,

Quoted 16 lines · Alan Stern

Quoted 15 lines

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 <[email protected]>

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -0

@@ -113,8 +113,4 @@
 	}
 

Quoted 9 lines

drivers/usb/storage/uas-detect.h line 115

Quoted 5 lines

drivers/usb/storage/uas-detect.h renamed from drivers/usb/storage/uas-detect.h.backup +0 -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?