Date: Mon, 17 Jun 2024 10:33:50 +0200
From: "Linux regression tracking (Thorsten Leemhuis)"
 <regressions@...mhuis.info>
To: Arınç ÜNAL <arinc.unal@...nc9.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Matthias Brugger <matthias.bgg@...il.com>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
 Daniel Golle <daniel@...rotopia.org>, frank-w@...lic-files.de,
 Linux regressions mailing list <regressions@...ts.linux.dev>,
 Frank Wunderlich <linux@...web.de>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH] arm64: dts: mt7622: fix switch probe on bananapi-r64

This thread/fixing the regressions looks stalled again, let me jump in
here with a further comment below.

On 11.06.24 20:15, Arınç ÜNAL wrote:
> On 11/06/2024 16:03, AngeloGioacchino Del Regno wrote:
>> Il 11/06/24 14:56, Arınç ÜNAL ha scritto:
>>> On 11/06/2024 15:28, AngeloGioacchino Del Regno wrote:
>>>> Il 11/06/24 13:38, Arınç ÜNAL ha scritto:
>>>>> On 11/06/2024 14:30, Thorsten Leemhuis wrote:
>>>>>> On 07.06.24 16:15, Thorsten Leemhuis wrote:
>>>>>>> On 07.06.24 16:03, Paolo Abeni wrote:
>>>>>>>> On Thu, 2024-06-06 at 10:26 +0200, Thorsten Leemhuis wrote:
>>>>>>>>> On 31.05.24 08:10, Arınç ÜNAL wrote:
>>>>>>>>>> On 31/05/2024 08.40, Thorsten Leemhuis wrote:
>>>>>>>>>>> [adding Paolo, who committed the culprit]
>>>>>>>>>
>>>>>>>>> /me slowly wonders if the culprit should be reverted for now
>>>>>>>>> (see below)
>>>>>>>>> and should be reapplied later together with the matching
>>>>>>>>> changes from
>>>>>>>>> Arınç ÜNAL.
>>>>>>>>
>>>>>>>> FWIS I think a revert should be avoided, given that a fix is
>>>>>>>> available
>>>>>>>> and nicely small.
>>>>>>>
>>>>>>> Yeah, on one hand I agree; but on the other it seems that the
>>>>>>> maintainers that would have to take care of the dt changes to fix
>>>>>>> this
>>>>>>> until now remained silent in this thread, apart from Rob who sent
>>>>>>> the
>>>>>>> mail regarding the warnings.
>>>>>>>
>>>>>>> I put those maintainers in the To: field of this mail, maybe that
>>>>>>> might
>>>>>>> lead to some reaction.
>>>>>>
>>>>>> Still no reply from the DRS folks or any other progress I noticed.
>>>>>> Guess
>>>>>> that means I will soon have no other choice than to get Linus
>>>>>> involved,
>>>>>> as this looks stuck. :-( #sigh
>>>>>
>>>>> Does it have to be Linus that needs to apply "[PATCH 0/2] Set PHY
>>>>> address
>>>>> of MT7531 switch to 0x1f on MediaTek arm64 boards"? Aren't there
>>>>> any other
>>>>> ARM maintainers that can apply the fix to their tree?
>>>>>
>>>>> Arınç
>>>>
>>>> You have feedback from two people on the series that you mentioned,
>>>> and noone
>>>> is going to apply something that needs to be fixed.
>>>>
>>>> I'm giving you the possibility of addressing the comments in your
>>>> patch, but
>>>> I don't want to see any mention of the driver previously ignoring
>>>> this or that
>>>> as this is irrelevant for a hardware description. Devicetree only
>>>> describes HW.
>>>>
>>>> Adding up, in commit 868ff5f4944a ("net: dsa: mt7530-mdio: read PHY
>>>> address of switch from device tree"),
>>>> you have created a regression.
>>>>
>>>> Regressions should be fixed - as in - if the driver did work before
>>>> with the old
>>>> devicetrees, it shall still work. You can't break ABI. Any changes
>>>> that you do
>>>> to your driver must not break functionality with old devicetrees.
>>>>
>>>> So...
>>>>
>>>> ------> Fix the driver that you broke <------
>>>
>>> The device tree ABI before the change on the driver:
>>>
>>> The reg value represents the PHY address of the switch.
>>>
>>> The device tree ABI after the change on the driver:
>>>
>>> The reg value represents the PHY address of the switch.
>>>
>>> I see no device tree ABI breakage. What I see instead is the driver
>>> starting enforcing the device tree ABI. No change had been made on the
>>> device tree ABI so any non-Linux driver that controls this switch
>>> continues
>>> to work.
>>>
>>> These old device tree source files in question did not abide by the
>>> device
>>> tree ABI in the first place, which is why they don't work anymore as the
>>> Linux driver now enforces the ABI. Device tree source files not
>>> conforming
>>> to the ABI is not something to maintain but to fix. The patch series
>>> that
>>> fixes them are already submitted.
>>
>> As I said, the devicetree MUST describe the hardware correctly, and on
>> that I do
>> agree, and I, again, said that I want to take the devicetree fix.
>>
>> However, the driver regressed, and this broke functionality with old
>> device trees.
>> Old device trees might have been wrong (and they are, yes), but
>> functionality was
>> there and the switch was working.
>>
>> I repeat, driver changes MUST be retro-compatible with older device
>> trees, and your
>> driver changes ARE NOT; otherwise, this wouldn't be called *regression*.
> 
> I'm going to argue that what caused the regression is the broken device
> tree. The recent change on the driver only worked towards exposing the
> broken device tree.

Well, for the kernel that does not matter much: due to our "no
regressions" rule and how Linus handles it the author of that "recent
change" (e.g. you) is responsible to fix regressions a change exposed --
or that change is reverted (I might be wrong, but I think there are
quotes from Linus in
https://docs.kernel.org/process/handling-regressions.html to back this
up). So in the end a revert in a week or two is likely the outcome,
unless you or someone else fixes it in a way that pleases
AngeloGioacchino et. at.

Ciao, Thorsten

Powered by blists - more mailing lists