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