On Thu, Jul 07, 2022 at 05:32:57PM +0100, Russell King (Oracle) wrote:

Great, so I'll mark ocelot is safe.

Yes, please.

quoted

As for sja1105, there is DT validation that checks for the presence of
all required properties in sja1105_parse_ports_node().

Looking at those, it requires all of:

- a phy mode to be specified (as determined by of_get_phy_mode())
- a phy-handle or of_phy_is_fixed_link() to return true

otherwise it errors out.

I know. The problem with this ad-hoc validation is that it doesn't cover
the pure MLO_AN_INBAND:

	managed = "in-band-status";

so it is more restrictive than it needs to be. Also it doesn't recognize
the presence of an SFP bus in MLO_AN_PHY mode.

That is part 1 of my problem. I want to have validation that I'm
providing phylink with all the right things it may need, but I don't
want to make the driver code super clunky. By checking just the presence of
either phy-handle or fixed-link I am rejecting valid phylink configurations.
What I need is a validation function that is actually in sync with
phylink, not just ad-hoc.

quoted

There is some DT validation in felix_parse_ports_node() too, but it
doesn't check that all specifiers that phylink might use are there.

Phylink (correction, fwnode_get_phy_node() which is not part of phylink
anymore) will look for phy-handle, phy, or phy-device. This is I don't
see that there's any incompatibility between what the driver is doing
and what phylink does.

If there's a fixed-link property, then sja1105_parse_ports_node() is
happy, and so will phylink. If there's a phy-handle, the same is true.
If there's a "phy" or "phy-device" then sja1105_parse_ports_node()
errors out. That's completely fine.

"phy" and "phy-device" are the backwards compatibility for DT - I
believe one of them is the ePAPR specified property that we in Linux
have decided to only fall back on if there's not our more modern
"phy-handle" property.

It seems We have a lot of users of "phy" in DT today, so we can't drop
that from generic code such as phylink, but I haven't found any users
of "phy-device".

quoted

I'd really like to add some validation before I gain any involuntary
users, but all open-coded constructs I can come up with are clumsy.
What would you suggest, if I explicitly don't want to rely on
context-specific phylink interpretation of empty OF nodes, and rather
error out?

So I also don't see a problem - sja1105 rejects DTs that fail to
describe a port using at least one of a phy-handle, a fixed-link, or
a managed in-band link, and I don't think it needs to do further
validation, certainly not for the phy describing properties that
the kernel has chosen to deprecate for new implementations.

And this is part 2 of my problem, ocelot/felix doesn't have validation
at all except for phy-mode, because if it were to simply copy the
phy-handle/fixed-link either/or logic from sja1105, it would break some
customer boards with SFP cages. But without that validation, I am
exposing this driver to configurations I don't want it to support (CPU
ports with empty OF nodes, i.o.w. what this patch set is about).