Re: [PATCH] net: phy: fix auto-negotiation in case of 'down-shift'

From: Antonio Borneo <antonio.borneo@st.com>
To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, <netdev@vger.kernel.org>,
	Yonglong Liu <liuyonglong@huawei.com>, <stable@vger.kernel.org>,
	<linuxarm@huawei.com>, Salil Mehta <salil.mehta@huawei.com>,
	<linux-stm32@st-md-mailman.stormreply.com>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] net: phy: fix auto-negotiation in case of 'down-shift'
Date: Tue, 24 Nov 2020 16:17:42 +0100	[thread overview]
Message-ID: <bd83b9c15f6cfed5df90da4f6b50d1a3f479b831.camel@st.com> (raw)
In-Reply-To: <20201124145647.GF1551@shell.armlinux.org.uk>

On Tue, 2020-11-24 at 14:56 +0000, Russell King - ARM Linux admin wrote:
> On Tue, Nov 24, 2020 at 03:38:48PM +0100, Antonio Borneo wrote:
> > If the auto-negotiation fails to establish a gigabit link, the phy
> > can try to 'down-shift': it resets the bits in MII_CTRL1000 to
> > stop advertising 1Gbps and retries the negotiation at 100Mbps.
> > 
> > From commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode
> > in genphy_read_status") the content of MII_CTRL1000 is not checked
> > anymore at the end of the negotiation, preventing the detection of
> > phy 'down-shift'.
> > In case of 'down-shift' phydev->advertising gets out-of-sync wrt
> > MII_CTRL1000 and still includes modes that the phy have already
> > dropped. The link partner could still advertise higher speeds,
> > while the link is established at one of the common lower speeds.
> > The logic 'and' in phy_resolve_aneg_linkmode() between
> > phydev->advertising and phydev->lp_advertising will report an
> > incorrect mode.
> > 
> > Issue detected with a local phy rtl8211f connected with a gigabit
> > capable router through a two-pairs network cable.
> > 
> > After auto-negotiation, read back MII_CTRL1000 and mask-out from
> > phydev->advertising the modes that have been eventually discarded
> > due to the 'down-shift'.
> 
> Sorry, but no. While your solution will appear to work, in
> introduces unexpected changes to the user visible APIs.
> 
> >  	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
> > +		if (phydev->is_gigabit_capable) {
> > +			adv = phy_read(phydev, MII_CTRL1000);
> > +			if (adv < 0)
> > +				return adv;
> > +			/* update advertising in case of 'down-shift' */
> > +			mii_ctrl1000_mod_linkmode_adv_t(phydev->advertising,
> > +							adv);
> 
> If a down-shift occurs, this will cause the configured advertising
> mask to lose the 1G speed, which will be visible to userspace.

You are right, it gets propagated to user that 1Gbps is not advertised

> Userspace doesn't expect the advertising mask to change beneath it.
> Since updates from userspace are done using a read-modify-write of
> the ksettings, this can have the undesired effect of removing 1G
> from the configured advertising mask.
> 
> We've had other PHYs have this behaviour; the correct solution is for
> the PHY driver to implement reading the resolution from the PHY rather
> than relying on the generic implementation if it can down-shift

If it's already upstream, could you please point to one of the phy driver
that already implements this properly?

Thanks
Antonio