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?
Ioana
> + /* The interface changed, e.g. 1000base-X <-> 2500base-X */
> + /* We need to force the link down, then change the interface */
> + if (pl->old_link_state) {
> + phylink_link_down(pl);
> + pl->old_link_state = false;
> + }
> + if (!test_bit(PHYLINK_DISABLE_STOPPED,
> + &pl->phylink_disable_state))
> + phylink_change_interface(pl, false, &config);
> + pl->link_config.interface = config.interface;
> + linkmode_copy(pl->link_config.advertising, config.advertising);
> + } else if (!linkmode_equal(pl->link_config.advertising,
> + config.advertising)) {
> + linkmode_copy(pl->link_config.advertising, config.advertising);
> + phylink_change_inband_advert(pl);
> }
> mutex_unlock(&pl->state_mutex);
>
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index d9913d8e6b91..2f1315f32113 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -76,7 +76,9 @@ struct phylink_config {
> * struct phylink_mac_ops - MAC operations structure.
> * @validate: Validate and update the link configuration.
> * @mac_pcs_get_state: Read the current link state from the hardware.
> + * @mac_prepare: prepare for a major reconfiguration of the interface.
> * @mac_config: configure the MAC for the selected mode and state.
> + * @mac_finish: finish a major reconfiguration of the interface.
> * @mac_an_restart: restart 802.3z BaseX autonegotiation.
> * @mac_link_down: take the link down.
> * @mac_link_up: allow the link to come up.
> @@ -89,8 +91,12 @@ struct phylink_mac_ops {
> struct phylink_link_state *state);
> void (*mac_pcs_get_state)(struct phylink_config *config,
> struct phylink_link_state *state);
> + int (*mac_prepare)(struct phylink_config *config, unsigned int mode,
> + phy_interface_t iface);
> void (*mac_config)(struct phylink_config *config, unsigned int mode,
> const struct phylink_link_state *state);
> + int (*mac_finish)(struct phylink_config *config, unsigned int mode,
> + phy_interface_t iface);
> void (*mac_an_restart)(struct phylink_config *config);
> void (*mac_link_down)(struct phylink_config *config, unsigned int mode,
> phy_interface_t interface);
> @@ -145,6 +151,31 @@ void validate(struct phylink_config *config, unsigned
> long *supported,
> void mac_pcs_get_state(struct phylink_config *config,
> struct phylink_link_state *state);
>
> +/**
> + * mac_prepare() - prepare to change the PHY interface mode
> + * @config: a pointer to a &struct phylink_config.
> + * @mode: one of %MLO_AN_FIXED, %MLO_AN_PHY, %MLO_AN_INBAND.
> + * @iface: interface mode to switch to
> + *
> + * phylink will call this method at the beginning of a full initialisation
> + * of the link, which includes changing the interface mode or at initial
> + * startup time. It may be called for the current mode. The MAC driver
> + * should perform whatever actions are required, e.g. disabling the
> + * Serdes PHY.
> + *
> + * This will be the first call in the sequence:
> + * - mac_prepare()
> + * - mac_config()
> + * - pcs_config()
> + * - possible pcs_an_restart()
> + * - mac_finish()
> + *
> + * Returns zero on success, or negative errno on failure which will be
> + * reported to the kernel log.
> + */
> +int mac_prepare(struct phylink_config *config, unsigned int mode,
> + phy_interface_t iface);
> +
> /**
> * mac_config() - configure the MAC for the selected mode and state
> * @config: a pointer to a &struct phylink_config.
> @@ -220,6 +251,23 @@ void mac_pcs_get_state(struct phylink_config *config,
> void mac_config(struct phylink_config *config, unsigned int mode,
> const struct phylink_link_state *state);
>
> +/**
> + * mac_finish() - finish a to change the PHY interface mode
> + * @config: a pointer to a &struct phylink_config.
> + * @mode: one of %MLO_AN_FIXED, %MLO_AN_PHY, %MLO_AN_INBAND.
> + * @iface: interface mode to switch to
> + *
> + * phylink will call this if it called mac_prepare() to allow the MAC to
> + * complete any necessary steps after the MAC and PCS have been configured
> + * for the @mode and @iface. E.g. a MAC driver may wish to re-enable the
> + * Serdes PHY here if it was previously disabled by mac_prepare().
> + *
> + * Returns zero on success, or negative errno on failure which will be
> + * reported to the kernel log.
> + */
> +int mac_finish(struct phylink_config *config, unsigned int mode,
> + phy_interface_t iface);
> +
> /**
> * mac_an_restart() - restart 802.3z BaseX autonegotiation
> * @config: a pointer to a &struct phylink_config.
>
