Message-ID: <af561268-9793-4b5d-aa0f-d09698fd6fb0@arinc9.com>
Date: Mon, 17 Jun 2024 14:08:19 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Linux regressions mailing list <regressions@...ts.linux.dev>,
 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,
 Frank Wunderlich <linux@...web.de>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH] arm64: dts: mt7622: fix switch probe on bananapi-r64

On 17/06/2024 11:33, Linux regression tracking (Thorsten Leemhuis) wrote:
> 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.

I've submitted a patch series that fixes the regression. Angelo argued
against the way the regression is fixed. I've very clearly argued back why
I find Angelo's approach wrong. There's been no response back. I don't
understand why reverting the patch is the likely outcome whilst the
standing argument points towards applying the said patch series. If a
revert happens before this discussion with Angelo finalises, this will set
a precedent that will tell maintainers that they can have their way by just
not replying to the ongoing discussions.

That said, the decision of resolving the regression by either reverting the
patch or applying the patch series shall not depend on whether or not
Angelo is pleased but rather there're no counter-arguments left on the
points brought, meaning the decision shall be made depending on the
argument that stands.

Therefore, I suggest that unless Angelo responds back with a
counter-argument in the window of a week or two, as you've described, my
patch series shall be applied.

Arınç

Powered by blists - more mailing lists