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

[Choong Yong Liang]

On 21/9/2023 10:04 pm, Russell King (Oracle) wrote:
> 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@xxxxxxxxxxxxxxx>
>
> 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.
During the phylink_create, the cfg_link_an_mode will be set to MLO_AN_INBAND.

Then we switch from the SGMII interface to 2500BASEX interface. When we 
perform 'ifconfig eth0 down' and 'ifconfig eth0 up' then we will not able 
to bring up the PHY due to the phylink_attach_phy function. It is not 
expect to have MLO_AN_INBAND with PHY_INTERFACE_MODE_2500BASEX interface.

static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
			      phy_interface_t interface)
{
	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
		     phy_interface_mode_is_8023z(interface) && !pl->sfp_bus)))
		return -EINVAL;

	if (pl->phydev)
		return -EBUSY;

	return phy_attach_direct(pl->netdev, phy, 0, interface);
}

I did change different ways to handle it in the new patch series.
So it should not impact much on the existing phylink framework.

> 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.
Thank you for pointing out the bug that will impact everyone. Actually 
cur_link_an_mode is just required for PCS, I did handle it that only intel 
platforms will get the PCS negotiation mode for the PCS.