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

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