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

Hans de Goede
2021-04-25T18:25:59Z

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