Re: [PATCH RFC net-next 11/13] net: phylink: re-implement interface configuration with PCS

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Mon, Jul 20, 2020 at 01:02:21PM +0000, Ioana Ciornei wrote:
> On 7/20/20 3:44 PM, Russell King - ARM Linux admin wrote:
> > On Mon, Jul 20, 2020 at 11:40:44AM +0000, Ioana Ciornei wrote:
> >> On 6/30/20 5:29 PM, Russell King wrote:
> >>> With PCS support, how we implement interface reconfiguration is not up
> >>> to the job; we end up reconfiguring the PCS for an interface change
> >>> while the link could potentially be up.  In order to solve this, add
> >>> two additional MAC methods for interface configuration, one to prepare
> >>> for the change, and one to finish the change.
> >>>
> >>> This allows mvneta and mvpp2 to shutdown what they require prior to the
> >>> MAC and PCS configuration calls, and then restart as appropriate.
> >>>
> >>> This impacts ksettings_set(), which now needs to identify whether the
> >>> change is a minor tweak to the advertisement masks or whether the
> >>> interface mode has changed, and call the appropriate function for that
> >>> update.
> >>>
> >>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> >>> ---
> >>>    drivers/net/phy/phylink.c | 80 ++++++++++++++++++++++++++-------------
> >>>    include/linux/phylink.h   | 48 +++++++++++++++++++++++
> >>>    2 files changed, 102 insertions(+), 26 deletions(-)
> >>>
> >>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> >>> index 09f4aeef15c7..a31a00fb4974 100644
> >>> --- a/drivers/net/phy/phylink.c
> >>> +++ b/drivers/net/phy/phylink.c
> >>> @@ -433,23 +433,47 @@ static void phylink_mac_pcs_an_restart(struct phylink *pl)
> >>>    	}
> >>>    }
> >>>    
> >>> -static void phylink_pcs_config(struct phylink *pl, bool force_restart,
> >>> -			       const struct phylink_link_state *state)
> >>> +static void phylink_change_interface(struct phylink *pl, bool restart,
> >>> +				     const struct phylink_link_state *state)
> >>>    {
> >>> -	bool restart = force_restart;
> >>> +	int err;
> >>> +
> >>> +	phylink_dbg(pl, "change interface %s\n", phy_modes(state->interface));
> >>>    
> >>> -	if (pl->pcs_ops && pl->pcs_ops->pcs_config(pl->config,
> >>> -						   pl->cur_link_an_mode,
> >>> -						   state->interface,
> >>> -						   state->advertising,
> >>> -						   !!(pl->link_config.pause &
> >>> -						      MLO_PAUSE_AN)))
> >>> -		restart = true;
> >>> +	if (pl->mac_ops->mac_prepare) {
> >>> +		err = pl->mac_ops->mac_prepare(pl->config, pl->cur_link_an_mode,
> >>> +					       state->interface);
> >>> +		if (err < 0) {
> >>> +			phylink_err(pl, "mac_prepare failed: %pe\n",
> >>> +				    ERR_PTR(err));
> >>> +			return;
> >>> +		}
> >>> +	}
> >>>    
> >>>    	phylink_mac_config(pl, state);
> >>>    
> >>> +	if (pl->pcs_ops) {
> >>> +		err = pl->pcs_ops->pcs_config(pl->config, pl->cur_link_an_mode,
> >>> +					      state->interface,
> >>> +					      state->advertising,
> >>> +					      !!(pl->link_config.pause &
> >>> +						 MLO_PAUSE_AN));
> >>> +		if (err < 0)
> >>> +			phylink_err(pl, "pcs_config failed: %pe\n",
> >>> +				    ERR_PTR(err));
> >>> +		if (err > 0)
> >>> +			restart = true;
> >>> +	}
> >>>    	if (restart)
> >>>    		phylink_mac_pcs_an_restart(pl);
> >>> +
> >>> +	if (pl->mac_ops->mac_finish) {
> >>> +		err = pl->mac_ops->mac_finish(pl->config, pl->cur_link_an_mode,
> >>> +					      state->interface);
> >>> +		if (err < 0)
> >>> +			phylink_err(pl, "mac_prepare failed: %pe\n",
> >>> +				    ERR_PTR(err));
> >>> +	}
> >>>    }
> >>>    
> >>>    /*
> >>> @@ -555,7 +579,7 @@ static void phylink_mac_initial_config(struct phylink *pl, bool force_restart)
> >>>    	link_state.link = false;
> >>>    
> >>>    	phylink_apply_manual_flow(pl, &link_state);
> >>> -	phylink_pcs_config(pl, force_restart, &link_state);
> >>> +	phylink_change_interface(pl, force_restart, &link_state);
> >>>    }
> >>>    
> >>>    static const char *phylink_pause_to_str(int pause)
> >>> @@ -674,7 +698,7 @@ static void phylink_resolve(struct work_struct *w)
> >>>    				phylink_link_down(pl);
> >>>    				cur_link_state = false;
> >>>    			}
> >>> -			phylink_pcs_config(pl, false, &link_state);
> >>> +			phylink_change_interface(pl, false, &link_state);
> >>>    			pl->link_config.interface = link_state.interface;
> >>>    		} else if (!pl->pcs_ops) {
> >>>    			/* The interface remains unchanged, only the speed,
> >>> @@ -1450,22 +1474,26 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
> >>>    		return -EINVAL;
> >>>    
> >>>    	mutex_lock(&pl->state_mutex);
> >>> -	linkmode_copy(pl->link_config.advertising, config.advertising);
> >>> -	pl->link_config.interface = config.interface;
> >>>    	pl->link_config.speed = config.speed;
> >>>    	pl->link_config.duplex = config.duplex;
> >>> -	pl->link_config.an_enabled = kset->base.autoneg !=
> >>> -				     AUTONEG_DISABLE;
> >>> -
> >>> -	if (pl->cur_link_an_mode == MLO_AN_INBAND &&
> >>> -	    !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state)) {
> >>> -		/* If in 802.3z mode, this updates the advertisement.
> >>> -		 *
> >>> -		 * If we are in SGMII mode without a PHY, there is no
> >>> -		 * advertisement; the only thing we have is the pause
> >>> -		 * modes which can only come from a PHY.
> >>> -		 */
> >>> -		phylink_pcs_config(pl, true, &pl->link_config);
> >>> +	pl->link_config.an_enabled = kset->base.autoneg != AUTONEG_DISABLE;
> >>> +
> >>> +	if (pl->link_config.interface != config.interface) {
> >>
> >>
> >> I cannot seem to understand where in this function config.interface
> >> could become different from pl->link_config.interface.
> >>
> >> Is there something obvious that I am missing?
> > 
> > The validate() method is free to change the interface if required -
> > there's a helper that MACs can use to achieve that for switching
> > between 1000base-X and 2500base-X.  Useful if you have a FC SFP
> > plugged in capable of 2500base-X, but want to communicate with a
> > 1000base-X link partner.
> > 
> 
> Ok, I was not aware of this possibility. Now that I looked, it's even 
> documented in phylink's header file.
> 
> My confusion stems from the fact that I expected this to come from the 
> SFP itself requesting an interface type or the other. So the usage here 
> is to know what is at the other end of the line and configure the 
> appropriate advertisement with ethtool?

SFPs do not specify their host interface - they specify a range of
parameters which describe the media side.  Even then, it is not
unusual for various parameters in the EEPROM to be incorrect.

Also, 2500BASE-X at one end and 1000BASE-X at the other end is not
compatible - you won't even get synchronisation between the two ends,
so there's no possibility for passing any data about capabilities.

The idea is that if you have a module that can support 2500BASE-X and
1000BASE-X (such as a fibrechannel SFP) then, rather than being solidly
tied by the kernel to one or other of the speeds, you can request the
kernel use the other speed via ethtool - and switching between the two
requires changing the phy_interface_t setting.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!