netdev - Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band capability check where it belongs

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: <Y34NK9h86cgYmcoM@shell.armlinux.org.uk>
Date:   Wed, 23 Nov 2022 12:08:11 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        UNGLinuxDriver@...rochip.com,
        bcm-kernel-feedback-list@...adcom.com,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Camelia Groza <camelia.groza@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Sean Anderson <sean.anderson@...o.com>,
        Antoine Tenart <atenart@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Raag Jadav <raagjadav@...il.com>,
        Siddharth Vadapalli <s-vadapalli@...com>,
        Ong Boon Leong <boon.leong.ong@...el.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Marek Behun <marek.behun@....cz>
Subject: Re: [PATCH v4 net-next 3/8] net: phy: bcm84881: move the in-band
 capability check where it belongs

On Tue, Nov 22, 2022 at 09:36:59PM +0200, Vladimir Oltean wrote:
> I think we're in agreement, but please let's wait until tomorrow, I need
> to take a break for today.

I think we do have a sort of agreement... but lets give this a go. The
following should be sufficient for copper SFP modules using the 88E1111
PHY. However, I haven't build-tested this patch yet.

Reading through the documentation has brought up some worms in this
area. :(

It may be worth printing the fiber page BMCR and extsr at various
strategic points in this driver and reporting back if things don't
seem to be working right for your modules. In the mean time, I'll try
to see how the modules in the Honeycomb appear to be setup at power-up
and after the driver has configured the PHY... assuming I left both
MicroUSBs connected and the board has a network connection via the
main ethernet jack.

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 3c54d7d0f17f..9d537a2bccda 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -669,6 +669,54 @@ static int marvell_config_aneg_fiber(struct phy_device *phydev)
 	return genphy_check_and_restart_aneg(phydev, changed);
 }
 
+static int m88e1111_validate_an_inband(struct phy_device *phydev,
+				       phy_interface_t interface)
+{
+	int hwcfg_mode, extsr, bmcr;
+
+	if (interface != PHY_INTERFACE_MODE_1000BASEX &&
+	    interface != PHY_INTERFACE_MODE_SGMII)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
+	if (extsr < 0)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	hwcfg_mode = extsr & MII_M1111_HWCFG_MODE_MASK;
+
+	/* If we are in 1000base-X no-AN hwcfg_mode,
+	 * m88e1111_config_init_1000basex() will allegedly enable AN and
+	 * allow AN bypass - but there is a question over whether that is
+	 * actually the case. Let's follow what the comment says, and assume
+	 * that it was actually tested.
+	 */
+	if (interface == PHY_INTERFACE_MODE_1000BASEX &&
+	    hwcfg_mode == MII_M1111_HWCFG_MODE_COPPER_1000X_NOAN)
+		return PHY_AN_INBAND_ON_TIMEOUT;
+
+	/* Otherwise, we leave the AN enable bit and the AN bypass bit
+	 * alone, so we need to read the registers to determine how the
+	 * MAC facing side of the PHY has been setup by firmware and/or
+	 * hardware reset.
+	 *
+	 * If the AN enable bit is clear, then all in-band signalling
+	 * on the SGMII/1000base-X side is disabled. Otherwise, AN is
+	 * enabled. If the bypass bit is set, AN can complete without
+	 * a response from the partner (MAC).
+	 */
+	bmcr = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
+	if (bmcr < 0)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	if (!(bmcr & BMCR_ANENABLE))
+		return PHY_AN_INBAND_OFF;
+
+	if (extsr & MII_M1111_HWCFG_SERIAL_AN_BYPASS)
+		return PHY_AN_INBAND_ON_TIMEOUT;
+
+	return PHY_AN_INBAND_ON;
+}
+
 static int m88e1111_config_aneg(struct phy_device *phydev)
 {
 	int extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
@@ -915,7 +963,10 @@ static int m88e1111_config_init_1000basex(struct phy_device *phydev)
 	if (extsr < 0)
 		return extsr;
 
-	/* If using copper mode, ensure 1000BaseX auto-negotiation is enabled */
+	/* If using copper mode, ensure 1000BaseX auto-negotiation is enabled.
+	 * FIXME: does this actually enable 1000BaseX auto-negotiation if it
+	 * was previously disabled in the Fiber BMCR? 2.3.1.6 suggests not!
+	 */
 	mode = extsr & MII_M1111_HWCFG_MODE_MASK;
 	if (mode == MII_M1111_HWCFG_MODE_COPPER_1000X_NOAN) {
 		err = phy_modify(phydev, MII_M1111_PHY_EXT_SR,
@@ -2997,6 +3048,7 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1111_get_tunable,
 		.set_tunable = m88e1111_set_tunable,
+		.validate_an_inband = m88e1111_validate_an_inband,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1111_FINISAR,

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