Date:   Sat, 30 Jul 2022 00:49:27 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Marcin Wojtas <mw@...ihalf.com>
CC:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>, 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>,
        Vivien Didelot <vivien.didelot@...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>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Tomasz Nowicki <tn@...ihalf.com>,
        Grzegorz Jaszczyk <jaz@...ihalf.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:33:30PM +0200, Marcin Wojtas wrote:
> Initially, I thought that the idea is a probe failure (hence the camps
> to prevent that) - but it was clarified later, it's not the case.

The idea _is_ a probe failure, but not for whom you seem to believe
(not for the 'expected bad' drivers but for the 'expected good' ones).
The probe failure is for drivers using OF whose compatibles are outside
of dsa_switches_dont_enforce_validation, and all ACPI drivers (that's
going to be your part), effective immediately.

For example the sja1105 doesn't have any parasitic users, I'm absolutely
sure about that because it already has validation, although only coincidentally.
For the ocelot driver I'm also relatively certain practically speaking,
because we're talking about an embedded switch where the description is
in an SoC dtsi. But I can't totally exclude the possibility that somebody
won't be crazy enough to /delete-property/ the fixed-link from a board
device tree, or to redefine everything from scratch without including
the SoC dtsi, and then claim that hey, this was actually working before
this or that refactoring, due to some marginal condition which I never
designed for.

And that's the point of this patch. The DSA core is flawed because it
doesn't let phylink fail when it should. It tries to save the day by
making some odd decisions which make sense considering the old drivers,
but simply don't, when you consider what the code would have looked like,
were it not for the pre-phylink baggage.

So this change essentially lets happen for new drivers what should have
happened in a normal world where DSA would not interfere at all -
nondescriptive bindings: fail to probe, bye.

Rob is right, the DSA core shouldn't validate the OF node, it should
just let phylink fail if that's what it will, and go about its day.
Individual DSA drivers shouldn't have to validate their OF node either,
but they'd have to, if they wanted to circumvent DSA's logic. And why
put the burden on them?

> I totally agree and I am all against breaking the backward
> compatibility (this is why I work on ACPI support that much :) ). The
> question is whether for existing deployments with 'broken' DT
> description we would be ok to introduce a dev_warn/WARN_ON message
> after a kernel update.

Andrew suggested it. In v1, the array was for skipping DT validation
(this implies skipping printing warnings) for drivers where we knew it's
going to be violated anyway. But we thought, why not also tell users
that what's going on isn't quite ideal.

https://patchwork.kernel.org/project/netdevbpf/patch/20220723164635.1621911-1-vladimir.oltean@nxp.com/#24949229

This is a detail to me and not the purpose of the change. It would be
nothing short of a miracle if people would suddenly see the light and
update all DT blobs tomorrow. But no one here is planning based on that.
Quite far from it - Russell King and Marek Behún were (are?) working on
a patch set that actually pushes those incomplete DT blobs to the next
level, and fakes what the proper OF node should have looked like, based
on driver level knowledge.

> That would be a case if the check is performed unconditionally - this
> way we can keep compat strings out of net/dsa.  What do you think?

So you're saying keep the validation and the warnings, but don't fail
the probing of anyone? Why? Being strict about validation allows me as a
driver maintainer to be formally correct when I say that there are no
users that expect to not register with phylink, rather than just guess
or hope, or even waste time checking. And if I was someone new coming to
the DSA framework, I'd want to have that guarantee. I didn't design for
that possibility and I don't want to have it, just because I use the
same framework as someone who put workarounds in place so that it works
for him. We could argue about who of existing but otherwise newish
drivers should be part of dsa_switches_dont_enforce_validation, and
that's where the camps come in.

Being warm and fuzzy about this, and just print, doesn't really describe
anything except for a preference. After all, we're not abandoning the
broken DT blobs.

What's your concrete problem with compatible strings inside net/dsa anyway,
and do you have a competent alternative? Is it the problem that you'll
have to convert them to fwnode? You do realize that this array is not
planned to expand at all after the patch is merged, including not to
ACPI IDs, right?

I've explained in the commit message why:

| Because there is a pattern where newly added switches reuse existing
| drivers more often than introducing new ones, I've opted for deciding
| who gets to skip validation based on an OF compatible match table in the
| DSA core. The alternative would have been to add another boolean
| property to struct dsa_switch, like configure_vlan_while_not_filtering.
| But this avoids situations where sometimes driver maintainers obfuscate
| what goes on by sharing a common probing function, and therefore
| making new switches inherit old quirks.

The latter has actually happened no farther than a few weeks ago,
refactoring was done to the microchip ksz driver, and a newly added
switch gained support for an old one's quirks, by refactoring the code
to share common logic (which I was otherwise very happy about).
Now good luck with adding a compatible string to
dsa_switches_dont_enforce_validation without anyone noticing.

Powered by blists - more mailing lists