netdev - Re: [PATCH v4 net-next 0/8] Let phylink manage in-band AN for the PHY

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: <20221121194444.ran2bec6fhfk72lt@skbuf>
Date:   Mon, 21 Nov 2022 21:44:44 +0200
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Sean Anderson <sean.anderson@...o.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>,
        Russell King <linux@...linux.org.uk>,
        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>,
        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 0/8] Let phylink manage in-band AN for the PHY

Hi Sean,

On Mon, Nov 21, 2022 at 01:38:31PM -0500, Sean Anderson wrote:
> On 11/17/22 19:01, Vladimir Oltean wrote:
> > Compared to other solutions
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > Sean Anderson, in commit 5d93cfcf7360 ("net: dpaa: Convert to phylink"),
> > sets phylink_config :: ovr_an_inband to true. This doesn't quite solve
> > all problems, because we don't *know* that the PHY is set for in-band
> > autoneg. For example with the VSC8514, it all depends on what the
> > bootloader has/has not done. This solution eliminates the bootloader
> > dependency by actually programming in-band autoneg in the VSC8514 PHY.
> 
> I tested this on an LS1046ARDB. Unfortunately, although the links came
> up, the SGMII interfaces could not transfer data:
> 
> # dmesg | grep net6
> [    3.846249] fsl_dpaa_mac 1aea000.ethernet net6: renamed from eth3
> [    5.047334] fsl_dpaa_mac 1aea000.ethernet net6: PHY driver does not report in-band autoneg capability, assuming false
> [    5.073739] fsl_dpaa_mac 1aea000.ethernet net6: PHY [0x0000000001afc000:04] driver [RTL8211F Gigabit Ethernet] (irq=POLL)
> [    5.073749] fsl_dpaa_mac 1aea000.ethernet net6: phy: sgmii setting supported 0,00000000,00000000,000062ea advertising 0,00000000,00000000,000062ea
> [    5.073798] fsl_dpaa_mac 1aea000.ethernet net6: configuring for phy/sgmii link mode
> [    5.073803] fsl_dpaa_mac 1aea000.ethernet net6: major config sgmii
> [    5.075369] fsl_dpaa_mac 1aea000.ethernet net6: phylink_mac_config: mode=phy/sgmii/Unknown/Unknown/none adv=0,00000000,00000000,00000000 pause=00 link=0 an=0
> [    5.102308] fsl_dpaa_mac 1aea000.ethernet net6: phy link down sgmii/Unknown/Unknown/none/off
> [    9.186216] fsl_dpaa_mac 1aea000.ethernet net6: phy link up sgmii/1Gbps/Full/none/rx/tx
> [    9.186261] fsl_dpaa_mac 1aea000.ethernet net6: Link is Up - 1Gbps/Full - flow control rx/tx
> 
> I believe this is the same issue I ran into before. This is why I
> defaulted to in-band.

Thanks for testing. Somehow it did not come to me that this kind of
issue might happen when converting a driver that used to use ovr_an_inband
such as dpaa1, but ok, here we are.

The problem, of course, is that the Realtek PHY driver does not report
what the hardware supports, and we're back to trusting the device tree.

I don't think there were that many more PHYs used on NXP evaluation
boards than the Realteks, but of course there are also customer boards
to consider. Considering past history, it might be safer in terms of
regressions to use ovr_an_inband, but eventually, getting regression
reports in is going to make more PHY drivers report their capabilities,
which will improve the situation.

Anyways, we can still keep dpaa1 unconverted for now, and maybe convert
it for the next release cycle.

I also thought of a way of logically combining ovr_an_inband with
sync_an_inband (like say that ovr_an_inband is a "soft" override, and it
only takes place if syncing is not possible), but I'm not sure if that
isn't in fact overkill.

Could you please test the patch below? I only compile-tested it:

-----------------------------[ cut here ]-----------------------------
>From 025f8dedf10defa6d5fd10b4e3dd2a505fdbd313 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@....com>
Date: Mon, 21 Nov 2022 21:34:20 +0200
Subject: [PATCH] net: phy: realtek: validate SGMII in-band autoneg for
 RTL8211FS

Sean Anderson reports that the RTL8211FS on the NXP LS1046A-RDB has
in-band autoneg enabled, and this needs to be detectable by phylink if
the dpaa1 driver is going to use the sync_an_inband mechanism rather
than forcing in-band on via ovr_an_inband.

Reading through the datasheet, it seems like the SGMII Auto-Negotiation
Advertising Register bit 11 (En_Select Link Info) might be responsible
with this.

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/phy/realtek.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 3d99fd6664d7..53e7c1a10ab4 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -24,6 +24,10 @@
 
 #define RTL821x_INSR				0x13
 
+#define RTL8211FS_SGMII_ANARSEL			0x14
+
+#define RTL8211FS_SGMII_ANARSEL_EN		BIT(11)
+
 #define RTL821x_EXT_PAGE_SELECT			0x1e
 #define RTL821x_PAGE_SELECT			0x1f
 
@@ -849,6 +853,30 @@ static irqreturn_t rtl9000a_handle_interrupt(struct phy_device *phydev)
 	return IRQ_HANDLED;
 }
 
+/* RTL8211F and RTL8211FS seem to have the same PHY ID. We really only mean to
+ * run this for the S model which supports SGMII, so report unknown for
+ * everything else.
+ */
+static int rtl8211fs_validate_an_inband(struct phy_device *phydev,
+					phy_interface_t interface)
+{
+	int ret;
+
+	if (interface != PHY_INTERFACE_MODE_SGMII)
+		return PHY_AN_INBAND_UNKNOWN;
+
+	ret = phy_read_paged(phydev, 0xd08, RTL8211FS_SGMII_ANARSEL);
+	if (ret < 0)
+		return ret;
+
+	phydev_err(phydev, "%s: SGMII_ANARSEL 0x%x\n", __func__, ret);
+
+	if (ret & RTL8211FS_SGMII_ANARSEL_EN)
+		return PHY_AN_INBAND_ON;
+
+	return PHY_AN_INBAND_OFF;
+}
+
 static struct phy_driver realtek_drvs[] = {
 	{
 		PHY_ID_MATCH_EXACT(0x00008201),
@@ -931,6 +959,7 @@ static struct phy_driver realtek_drvs[] = {
 		.resume		= rtl821x_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
+		.validate_an_inband = rtl8211fs_validate_an_inband,
 	}, {
 		PHY_ID_MATCH_EXACT(RTL_8211FVD_PHYID),
 		.name		= "RTL8211F-VD Gigabit Ethernet",
-- 
2.34.1

-----------------------------[ cut here ]-----------------------------