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+ker...@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!
