Re: [PATCH net-next v3 3/5] net: phy: update in-band AN mode when changing interface by PHY driver

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Sep 21, 2023 at 08:19:44PM +0800, Choong Yong Liang wrote:
> As there is a mechanism in PHY drivers to switch the PHY interface
> between SGMII and 2500BaseX according to link speed. In this case,
> the in-band AN mode should be switching based on the PHY interface
> as well, if the PHY interface has been changed/updated by PHY driver.
> 
> For e.g., disable in-band AN in 2500BaseX mode, or enable in-band AN
> back for SGMII mode (10/100/1000Mbps).
> 
> Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>

This approach is *going* to break existing setups, sorry.

> +/**
> + * phylink_interface_change() - update both cfg_link_an_mode and
> + * cur_link_an_mode when there is a change in the interface.
> + * @phydev: pointer to &struct phy_device
> + *
> + * When the PHY interface switches between SGMII and 2500BaseX in
> + * accordance with the link speed, the in-band AN mode should also switch
> + * based on the PHY interface
> + */
> +static void phylink_interface_change(struct phy_device *phydev)
> +{
> +	struct phylink *pl = phydev->phylink;
> +
> +	if (pl->phy_state.interface != phydev->interface) {
> +		/* Fallback to the correct AN mode. */
> +		if (phy_interface_mode_is_8023z(phydev->interface) &&
> +		    pl->cfg_link_an_mode == MLO_AN_INBAND) {
> +			pl->cfg_link_an_mode = MLO_AN_PHY;
> +			pl->cur_link_an_mode = MLO_AN_PHY;

1. Why are you changing both cfg_link_an_mode (configured link AN mode)
and cur_link_an_mode (current link AN mode) ?

The "configured" link AN mode is supposed to be whatever was configured
at phylink creation time, and it's never supposed to change. The
"current" link AN mode can change, but changing that must be followed
by a major reconfiguration to ensure everything is correctly setup.
That will happen only because the change to the current link AN mode
can only happen when pl->phy_state.interface has changed, and the
change of pl->phy_state.interface triggers the reconfiguration.

2. You force this behaviour on everyone, so now everyone with a SFP
module that operates in 802.3z mode will be switched out of inband mode
whether they want that or not. This is likely to cause some breakage.

> +		} else if (pl->config->ovr_an_inband) {
> +			pl->cfg_link_an_mode = MLO_AN_INBAND;
> +			pl->cur_link_an_mode = MLO_AN_INBAND;

Here you force inband when not 802.3z mode and ovr_an_inband is set.
There are SFP modules that do *not* support in-band at all, and this
will break these modules when combined with a driver that sets
ovr_an_inband. So more breakage.

Please enumerate the PHY interface modes that you are trying to support
with this patch set, and indicate whether you want in-band for that
mode or not, and where the restriction for whether in-band can be used
comes from (PHY, PCS or MAC) so that it's possible to better understand
what you're trying to achieve.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!