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

[Ioana Ciornei <ioana.ciornei@nxp.com>]

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?

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