Re: [PATCH RFC net-next 5/5] net: dsa: always use phylink for CPU and DSA ports

[Vladimir Oltean <olteanv@gmail.com>]

On Thu, Jul 07, 2022 at 05:32:57PM +0100, Russell King (Oracle) wrote:
> Great, so I'll mark ocelot is safe.

Yes, please.

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

> > 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".
> 
> > 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).