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 11:09:43AM +0100, Russell King (Oracle) wrote:
> On Wed, Jul 06, 2022 at 05:24:09PM +0100, Russell King (Oracle) wrote:
> > On Wed, Jul 06, 2022 at 01:26:21PM +0300, Vladimir Oltean wrote:
> > > Can we please limit phylink_set_max_link_speed() to just the CPU ports
> > > where a fixed-link property is also missing, not just a phy-handle?
> > > Although to be entirely correct, we can also have MLO_AN_INBAND, which
> > > wouldn't be covered by these 2 checks and would still represent a valid
> > > DT binding.
> > 
> > phylink_set_max_fixed_link() already excludes itself:
> > 
> >         if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus)
                                                      ~~~~~~~~~~

If not NULL, this is an SFP PHY, right? In other words, it's supposed to protect from
phylink_sfp_connect_phy() - code involuntarily triggered by phylink_create() ->
phylink_register_sfp() - and not from calls to phylink_{,fwnode_}connect_phy()
that were initiated by the phylink user between phylink_create() and
phylink_set_max_fixed_link(), correct? Those are specified as invalid in the
kerneldoc and that's about it - that's not what the checking is for, correct?

But if so, why even check for pl->phydev, if you check for pl->sfp_bus?

> >                 return -EBUSY;
> > 
> > intentionally so that if there is anything specified for the port, be
> > that a fixed link or in-band, then phylink_set_max_fixed_link() errors
> > out with -EBUSY.
> > 
> > The only case that it can't detect is if there is a PHY that may be
> > added to phylink at a later time, and that is what the check above
> > is for.

Here by "PHY added at a later time", you do mean calling phylink_{,fwnode_}connect_phy()
after phylink_set_max_fixed_link(), right?

So this is what I don't understand. If we've called phylink_set_max_fixed_link()
we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will
silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA
predicts if it's going to call either of those connect_phy() functions,
and calls phylink_set_max_fixed_link() only if it won't. Right?

You've structured the checks in this "distributed" way because phylink
can't really predict whether phylink_{,fwnode_}connect_phy() will be
called after phylink_set_max_fixed_link(), right? I mean, it can
probably predict the fwnode_ variant, but not phylink_connect_phy, and
this is why it is up to the caller to decide when to call and when not to.

> I've updated the function description to mention this detail:
> 
> +/**
> + * phylink_set_max_fixed_link() - set a fixed link configuration for phylink
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + *
> + * Set a maximum speed fixed-link configuration for the chosen interface mode
> + * and MAC capabilities for the phylink instance if the instance has not
> + * already been configured with a SFP, fixed link, or in-band AN mode. If the
> + * interface mode is PHY_INTERFACE_MODE_NA, then search the supported
> + * interfaces bitmap for the first interface that gives the fastest supported
> + * speed.
> 
> Does this address your concern?
> 
> Thanks.

Not really, no, sorry, it just confuses me more. It should maybe also
say that this function shouldn't be called if phylink_{,fwnode_}connect_phy()
is going to be called later.

Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link()
based on the following?

(1) struct phylink_config gets extended with a bool fallback_max_fixed_link.
(2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register().
(3) phylink_set_max_fixed_link() is hooked into this -ENODEV error
    condition from phylink_fwnode_phy_connect():

	phy_fwnode = fwnode_get_phy_node(fwnode);
	if (IS_ERR(phy_fwnode)) {
		if (pl->cfg_link_an_mode == MLO_AN_PHY)
			return -ENODEV; <- here
		return 0;
	}