Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <a87032fc-3ad5-4286-828e-77735475c2c5@arinc9.com>
Date:   Fri, 22 Sep 2023 08:47:03 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH net-next v2 07/10] dt-bindings: net: enforce phylink
 bindings on certain ethernet controllers
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Rob Herring <robh@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        George McCollister <george.mccollister@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        Kurt Kanzenbach <kurt@linutronix.de>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Woojung Huh <woojung.huh@microchip.com>,
        UNGLinuxDriver@microchip.com,
        Linus Walleij <linus.walleij@linaro.org>,
        =?UTF-8?Q?Alvin_=C5=A0ipraga?= <alsi@bang-olufsen.dk>,
        =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <clement.leger@bootlin.com>,
        Marcin Wojtas <mw@semihalf.com>,
        "Russell King (Oracle)" <linux@armlinux.org.uk>,
        Lars Povlsen <lars.povlsen@microchip.com>,
        Steen Hegelund <Steen.Hegelund@microchip.com>,
        Daniel Machon <daniel.machon@microchip.com>,
        Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>,
        Daniel Golle <daniel@makrotopia.org>,
        Landen Chao <Landen.Chao@mediatek.com>,
        DENG Qingfang <dqfext@gmail.com>,
        Sean Wang <sean.wang@mediatek.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Magnus Damm <magnus.damm@gmail.com>,
        Maxime Chevallier <maxime.chevallier@bootlin.com>,
        Nicolas Ferre <nicolas.ferre@microchip.com>,
        Claudiu Beznea <claudiu.beznea@microchip.com>,
        Marek Vasut <marex@denx.de>,
        Claudiu Manoil <claudiu.manoil@nxp.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        John Crispin <john@phrozen.org>,
        Madalin Bucur <madalin.bucur@nxp.com>,
        Ioana Ciornei <ioana.ciornei@nxp.com>,
        Lorenzo Bianconi <lorenzo@kernel.org>,
        Felix Fietkau <nbd@nbd.name>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        Oleksij Rempel <linux@rempel-privat.de>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Giuseppe Cavallaro <peppe.cavallaro@st.com>,
        Jose Abreu <joabreu@synopsys.com>,
        Grygorii Strashko <grygorii.strashko@ti.com>,
        Sekhar Nori <nsekhar@ti.com>,
        Shyam Pandey <radhey.shyam.pandey@xilinx.com>,
        mithat.guner@xeront.com, erkin.bozoglu@xeront.com,
        netdev@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org,
        linux-renesas-soc@vger.kernel.org
References: <20230916110902.234273-1-arinc.unal@arinc9.com>
 <20230916110902.234273-8-arinc.unal@arinc9.com>
 <20230918181319.GA1445647-robh@kernel.org>
 <16710cf9-8911-4fed-8e2d-b19b581446c1@arinc9.com>
 <a8d49992-4fa8-4a9f-b954-79011a3040a8@lunn.ch>
 <85cc3b27-417e-4cf4-9f77-347a338c9d67@arinc9.com>
 <8935d431-be0c-43e0-a908-f7dff2048f7c@lunn.ch>
Content-Language: en-US
From:   =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>
In-Reply-To: <8935d431-be0c-43e0-a908-f7dff2048f7c@lunn.ch>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 22.09.2023 02:29, Andrew Lunn wrote:
> On Thu, Sep 21, 2023 at 09:21:40PM +0300, Arınç ÜNAL wrote:
>> On 21.09.2023 16:00, Andrew Lunn wrote:
>>>> - Link descriptions must be required on ethernet controllers. We don't care
>>>>     whether some Linux driver can or cannot find the PHY or set up a fixed
>>>>     link without looking at the devicetree.
>>>
>>> That can lead to future surprises, and breakage.
>>>
>>> Something which is not used is not tested, and so sometimes wrong, and
>>> nobody knows. Say the driver is extended to a new device and actually
>>> does need to use this never before used information. You then find it
>>> is wrong, and you get a regression.
>>>
>>> We have had issues like this before. There are four rgmii phy-link
>>> modes. We have had PHY drivers which ignored one of those modes, it
>>> silently accepted it, but did not change the hardware to actually use
>>> that mode. The PHY continues to use its reset defaults or strapping,
>>> and it worked. A lot of device trees ended up using this mode. And it
>>> was not the same as reset defaults/strapping.
>>>
>>> And then somebody needed that fourth mode, and made it actually
>>> work. And all those boards wrongly using that mode broke.
>>>
>>> The lesson i learned from that episode is that anything in device tree
>>> must actually be used and tested.
>>
>> It looks like the root cause here was the lack of dt-bindings to
>> only allow the phy-mode values the hardware supports.
> 
> That would not help. The hardware supported all 4 RGMII modes. So
> listing all four in the dt-binding would be correct. But the driver
> for the hardware had a bug, and so silently ignored one of the
> modes. That then masked the bugs in board DT files.
> 
>> What I see here is the driver change should've been tested on all
>> different hardware the driver controls then the improper describing
>> of hardware on the devicetree source file addressed.
> 
> Which is what did happen. But it took a while to find all those broken
> boards.  For a period of time, we had regressions.
> 
> Bugs happen. It is a fact of life. But we want those bugs to be easy
> to find as possible. If we force DT writers to add properties which
> the driver never uses, they are going to be bugs in those
> properties. And those bugs are not going to be easy to find, and quite
> likely, they will only be found a long time after they are added. We
> should not be adding unused properties and bugs just to keep a yaml
> checker happy.

Understood. Then I will refrain from enforcing link descriptions altogether
as I can't justify enforcing them on ethernet controllers selectively
because certain Linux drivers don't make use of the link properties and may
expose bugs if used.

The link properties are still allowed for all ethernet controllers though,
just not enforced. But I suppose it's better to optionally find the bug
instead of causing a regression then finding the bug.

Arınç