Re: [PATCH v2 net-next 4/4] net: dsa: validate that DT nodes of shared ports have the properties they need

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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.

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?
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?