Message-ID: <8935d431-be0c-43e0-a908-f7dff2048f7c@lunn.ch>
Date: Fri, 22 Sep 2023 01:29:55 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Arınç ÜNAL <arinc.unal@...nc9.com>
Cc: Rob Herring <robh@...nel.org>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	George McCollister <george.mccollister@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>,
	Kurt Kanzenbach <kurt@...utronix.de>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Woojung Huh <woojung.huh@...rochip.com>,
	UNGLinuxDriver@...rochip.com,
	Linus Walleij <linus.walleij@...aro.org>,
	Alvin Šipraga <alsi@...g-olufsen.dk>,
	Clément Léger <clement.leger@...tlin.com>,
	Marcin Wojtas <mw@...ihalf.com>,
	"Russell King (Oracle)" <linux@...linux.org.uk>,
	Lars Povlsen <lars.povlsen@...rochip.com>,
	Steen Hegelund <Steen.Hegelund@...rochip.com>,
	Daniel Machon <daniel.machon@...rochip.com>,
	Radhey Shyam Pandey <radhey.shyam.pandey@....com>,
	Daniel Golle <daniel@...rotopia.org>,
	Landen Chao <Landen.Chao@...iatek.com>,
	DENG Qingfang <dqfext@...il.com>,
	Sean Wang <sean.wang@...iatek.com>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Magnus Damm <magnus.damm@...il.com>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	Nicolas Ferre <nicolas.ferre@...rochip.com>,
	Claudiu Beznea <claudiu.beznea@...rochip.com>,
	Marek Vasut <marex@...x.de>,
	Claudiu Manoil <claudiu.manoil@....com>,
	Alexandre Belloni <alexandre.belloni@...tlin.com>,
	John Crispin <john@...ozen.org>,
	Madalin Bucur <madalin.bucur@....com>,
	Ioana Ciornei <ioana.ciornei@....com>,
	Lorenzo Bianconi <lorenzo@...nel.org>, Felix Fietkau <nbd@....name>,
	Horatiu Vultur <horatiu.vultur@...rochip.com>,
	Oleksij Rempel <linux@...pel-privat.de>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Giuseppe Cavallaro <peppe.cavallaro@...com>,
	Jose Abreu <joabreu@...opsys.com>,
	Grygorii Strashko <grygorii.strashko@...com>,
	Sekhar Nori <nsekhar@...com>,
	Shyam Pandey <radhey.shyam.pandey@...inx.com>,
	mithat.guner@...ont.com, erkin.bozoglu@...ont.com,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org,
	linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH net-next v2 07/10] dt-bindings: net: enforce phylink
 bindings on certain ethernet controllers

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.

	Andrew

Powered by blists - more mailing lists