Date:   Fri, 29 Jul 2022 12:39:06 -0600
From:   Rob Herring <robh+dt@...nel.org>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Oleksij Rempel <linux@...pel-privat.de>,
        Christian Marangi <ansuelsmth@...il.com>,
        John Crispin <john@...ozen.org>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Mans Rullgard <mans@...sr.com>,
        Arun Ramadoss <arun.ramadoss@...rochip.com>,
        Woojung Huh <woojung.huh@...rochip.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        George McCollister <george.mccollister@...il.com>,
        DENG Qingfang <dqfext@...il.com>,
        Sean Wang <sean.wang@...iatek.com>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Aleksander Jan Bajkowski <olek2@...pl>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Luiz Angelo Daros de Luca <luizluca@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Pawel Dembicki <paweldembicki@...il.com>,
        Clément Léger <clement.leger@...tlin.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Marek Behún <kabel@...nel.org>,
        Marcin Wojtas <mw@...ihalf.com>,
        Frank Rowand <frowand.list@...il.com>
Subject: Re: [PATCH v2 net-next 4/4] net: dsa: validate that DT nodes of
 shared ports have the properties they need

On Fri, Jul 29, 2022 at 11:01 AM Vladimir Oltean
<vladimir.oltean@....com> wrote:
>
> On Fri, Jul 29, 2022 at 10:22:49AM -0600, Rob Herring wrote:
> > It's not the kernel's job to validate the DT. If it was, it does a
> > horrible job.
>
> I'm surprised by you saying this.

I've said it many times...

> The situation is as follows: phylink parses the fwnode it's given, and
> errors out if it can't find everything it needs. See phylink_parse_mode()
> and phylink_parse_fixedlink(). This is a matter of fact - if you start
> parsing stuff, you'll eventually need to treat the case where what
> you're searching for isn't there, or isn't realistic.
>
> DSA is a common framework used by multiple drivers, and it wasn't always
> integrated with phylink. The DT nodes of some ports will lack what
> phylink needs, but these ports don't really need phylink to work, it's
> optional, they work without it too. However if we begin the process of
> registering them with phylink and we let phylink fail, this process is
> irreversible and the ports don't work anymore.
>
> So what DSA currently does (even before this patch set) is it
> pre-validates that phylink has what it needs, and skips phylink if it
> doesn't. It's only that it doesn't name it this way, and it doesn't
> print anything.
>
> Being a common framework, new drivers opt into this behavior willy-nilly.
> I am adding a table of compatible strings of old drivers where the
> behavior is retained. For new drivers, we fail them in DSA rather than
> in phylink, this is true. Maybe this is what you disagree with?

No, I haven't looked at it that closely.

> We do this as a matter of practicality - we already need to predetermine
> whether phylink has a chance of working, and if we find something missing,
> we know it won't. Seems illogical to let phylink go through the same
> parsing again.
>
> As for the lousy job, I can't contradict you...
>
> > Is the schema providing this validation? If not, you need to add it.
>
> No, it's not. I can also look into providing a patch that statically
> validates this. But I'm afraid, with all due respect, that not many
> people take the YAML validator too seriously? With the volume of output
> it even throws, it would be even hard to detect something new, you'd
> need to know to search for it. Most of the DSA drivers aren't even
> converted to YAML, and it is precisely the biggest offenders that
> aren't. And even if the schema says a property is required but the code
> begs to differ, and a future DT blob gets to enter production based on
> undocumented behavior, who's right?

All valid points. At least for the sea of warnings, you can limit
checking to only a subset of schemas you care about. Setting
'DT_SCHEMA_FILES=net/' will only check networking schemas for example.
Just need folks to care about those subsets.

I'm not saying don't put warnings in the kernel for this. Just don't
make it the only source of warnings. Given you are tightening the
requirements, it makes sense to have a warning. If it was something
entirely new, then I'd be more inclined to say only the schema should
check.

Rob

Powered by blists - more mailing lists