Re: [PATCH RFC net-next 08/13] net: phylink: simplify phy case for ksettings_set method

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

On 6/30/20 5:29 PM, Russell King wrote:
> When we have a PHY attached, an ethtool ksettings_set() call only
> really needs to call through to the phylib equivalent; phylib will
> call back to us when the link changes so we can update our state.
> Therefore, we can bypass most of our ksettings_set() call for this
> case.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com>

> ---
>   drivers/net/phy/phylink.c | 104 +++++++++++++++++---------------------
>   1 file changed, 47 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 103d2a550415..967c068d16c8 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1312,13 +1312,33 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>   				  const struct ethtool_link_ksettings *kset)
>   {
>   	__ETHTOOL_DECLARE_LINK_MODE_MASK(support);
> -	struct ethtool_link_ksettings our_kset;
>   	struct phylink_link_state config;
>   	const struct phy_setting *s;
> -	int ret;
>   
>   	ASSERT_RTNL();
>   
> +	if (pl->phydev) {
> +		/* We can rely on phylib for this update; we also do not need
> +		 * to update the pl->link_config settings:
> +		 * - the configuration returned via ksettings_get() will come
> +		 *   from phylib whenever a PHY is present.
> +		 * - link_config.interface will be updated by the PHY calling
> +		 *   back via phylink_phy_change() and a subsequent resolve.
> +		 * - initial link configuration for PHY mode comes from the
> +		 *   last phy state updated via phylink_phy_change().
> +		 * - other configuration changes (e.g. pause modes) are
> +		 *   performed directly via phylib.
> +		 * - if in in-band mode with a PHY, the link configuration
> +		 *   is passed on the link from the PHY, and all of
> +		 *   link_config.{speed,duplex,an_enabled,pause} are not used.
> +		 * - the only possible use would be link_config.advertising
> +		 *   pause modes when in 1000base-X mode with a PHY, but in
> +		 *   the presence of a PHY, this should not be changed as that
> +		 *   should be determined from the media side advertisement.
> +		 */
> +		return phy_ethtool_ksettings_set(pl->phydev, kset);
> +	}
> +
Also tested the PHY use case, no issue encountered with changing the 
advertisements, autoneg etc

>   	linkmode_copy(support, pl->supported);
>   	config = pl->link_config;
>   	config.an_enabled = kset->base.autoneg == AUTONEG_ENABLE;
> @@ -1365,65 +1385,35 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>   		return -EINVAL;
>   	}
>   
> -	if (pl->phydev) {
> -		/* If we have a PHY, we process the kset change via phylib.
> -		 * phylib will call our link state function if the PHY
> -		 * parameters have changed, which will trigger a resolve
> -		 * and update the MAC configuration.
> -		 */
> -		our_kset = *kset;
> -		linkmode_copy(our_kset.link_modes.advertising,
> -			      config.advertising);
> -		our_kset.base.speed = config.speed;
> -		our_kset.base.duplex = config.duplex;
> +	/* For a fixed link, this isn't able to change any parameters,
> +	 * which just leaves inband mode.
> +	 */
> +	if (phylink_validate(pl, support, &config))
> +		return -EINVAL;
>   
> -		ret = phy_ethtool_ksettings_set(pl->phydev, &our_kset);
> -		if (ret)
> -			return ret;
> +	/* If autonegotiation is enabled, we must have an advertisement */
> +	if (config.an_enabled && phylink_is_empty_linkmode(config.advertising))
> +		return -EINVAL;
>   
> -		mutex_lock(&pl->state_mutex);
> -		/* Save the new configuration */
> -		linkmode_copy(pl->link_config.advertising,
> -			      our_kset.link_modes.advertising);
> -		pl->link_config.interface = config.interface;
> -		pl->link_config.speed = our_kset.base.speed;
> -		pl->link_config.duplex = our_kset.base.duplex;
> -		pl->link_config.an_enabled = our_kset.base.autoneg !=
> -					     AUTONEG_DISABLE;
> -		mutex_unlock(&pl->state_mutex);
> -	} else {
> -		/* For a fixed link, this isn't able to change any parameters,
> -		 * which just leaves inband mode.
> +	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;

Is there a specific reason why this is not just using config.an_enabled 
to sync back to pl->link_config?

> +
> +	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.
>   		 */
> -		if (phylink_validate(pl, support, &config))
> -			return -EINVAL;
> -
> -		/* If autonegotiation is enabled, we must have an advertisement */
> -		if (config.an_enabled &&
> -		    phylink_is_empty_linkmode(config.advertising))
> -			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);
> -		}
> -		mutex_unlock(&pl->state_mutex);
> +		phylink_pcs_config(pl, true, &pl->link_config);
>   	}
> +	mutex_unlock(&pl->state_mutex);
>   
>   	return 0;
>   }
>