linux-kernel - Re: [PATCH net-next v3 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zv5hrvOspTcYuQYs@shell.armlinux.org.uk>
Date: Thu, 3 Oct 2024 10:19:42 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Qingtao Cao <qingtao.cao.au@...il.com>
Cc: Qingtao Cao <qingtao.cao@...i.com>, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v3 1/1] net: phy: marvell: avoid bringing down
 fibre link when autoneg is bypassed

On Thu, Oct 03, 2024 at 05:10:50PM +1000, Qingtao Cao wrote:
> On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it is
> activated, the device assumes a link-up status with existing configuration
> in BMCR, avoid bringing down the fibre link in this case
> 
> Test case:
> 1. Two 88E151x connected with SFP, both enable autoneg, link is up with
>    speed 1000M
> 2. Disable autoneg on one device and explicitly set its speed to 1000M
> 3. The fibre link can still up with this change, otherwise not.

As you're clearly using fibre, there's this chunk of code in the same
function that is (IMHO) wrong:

                if (phydev->duplex == DUPLEX_FULL) {
                        if (!(lpa & LPA_PAUSE_FIBER)) {
                                phydev->pause = 0;
                                phydev->asym_pause = 0;
                        } else if ((lpa & LPA_PAUSE_ASYM_FIBER)) {
                                phydev->pause = 1;
                                phydev->asym_pause = 1;
                        } else {
                                phydev->pause = 1;
                                phydev->asym_pause = 0;
                        }
                }

as ->pause and ->asym_pause are supposed to be the _resolved_
state, and this is only looking at the link partner's state.

fiber_lpa_mod_linkmode_lpa_t() also uses the baseT linkmodes for
Fibre.

IMHO, this should be:

		mii_lpa_mod_linkmode_x(phydev->lp_advertising, lpa,
				ETHTOOL_LINK_MODE_1000baseX_Full_BIT);
		phy_resolve_aneg_pause(phydev);

Please can you test whether that works correctly in addition to
your other fix? Thanks.

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