| 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
| ||
|
Message-ID: <aaa87b05-d740-4a59-d47d-6b0b350469c7@seco.com>
Date: Fri, 18 Nov 2022 13:01:35 -0500
From: Sean Anderson <sean.anderson@...o.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
Eric Dumazet <edumazet@...gle.com>,
Tim Harvey <tharvey@...eworks.com>,
"David S . Miller" <davem@...emloft.net>,
linux-kernel@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH] phy: aquantia: Configure SERDES mode by default
On 11/18/22 12:30, Vladimir Oltean wrote:
> On Fri, Nov 18, 2022 at 12:11:30PM -0500, Sean Anderson wrote:
>> >> - We can check all the registers to ensure we are actually going to rate
>> >> adapt. If we aren't, we tell phylink we don't support it. This is the
>> >> least risky, but we can end up not bringing up the link even in
>> >> circumstances where we could if we configured things properly. And we
>> >> generally know the right way to configure things.
>> >
>> > Like when?
>>
>> Well, like whenever the phy says "Please do XFI/2" or some other mode we
>> don't have a phy interface mode for. We will never be able to tell the MAC
>> "Please do XFI/2" (until we add an interface mode for it), so that's
>> obviously wrong.
>
> Add an interface mode for it then...
> But note that I have absolutely no clue what XFI/2 is. Apparently
> Aquantia doesn't want NXP to know....
SERDES Mode [2:0]
0 = XFI
1 = Reserved
2 = Reserved
3 = SGMII
4 = OCSGMII
5 = Low Power
6 = XFI/2 (i.e. XFI 5G)
7 = XFI*2 (i.e. XFI 20G)
This is about it (aside from a mention in the PHY XS System Interface
Connection Status register). I assume it's over/underclocked XFI, much
like how 2500BASE-X is over/underclocked "SGMII."
I got my manual from Marvell's customer portal (okta). My document is
dated March 10, 2022.
>> >> - Add a configuration option (devicetree? ethtool?) on which option
>> >> above to pick. This is probably what we will want to do in the long
>> >> term, but I feel like we have enough information to determine the
>> >> right thing to do most of the time (without needing manual
>> >> intervention).
>> >
>> > Not sure I see the need, when long-term there is no volunteer to make
>> > the Linux driver bring Aquantia PHYs to a known state regardless of
>> > vendor provisioning. Until then, there is just no reason to even attempt
>> > this.
>>
>> I mean a config for option 1 vs 2 above.
>
> How would this interact with Marek's proposal for phy-mode to be an
> array, and some middle entity (phylink?) selects the SERDES protocol and
> rate matching algorithm to use for each medium side link speed?
> https://patchwork.kernel.org/project/netdevbpf/cover/20211123164027.15618-1-kabel@kernel.org/
Yeah, this is what I was referring to in the other thread [1]. In order to
implement this properly, we'd need to know what interfaces are supported,
electrically, by the board.
[1] https://lore.kernel.org/netdev/ea320070-a949-c737-22c4-14fd199fdc23@seco.com/
>> > Until you look at the procedure in the NXP SDK and see that things are a
>> > bit more complicated to get right, like put the PHY in low power mode,
>> > sleep for a while. I think a large part of that was determined experimentally,
>> > out of laziness to change PHY firmware on some riser cards more than anything.
>> > We still expect the production boards to have a good firmware, and Linux
>> > to read what that does and adapt accordingly.
>>
>> Alas, if only Marvell put stuff like this in a manual... All I have is a spec
>> sheet and the register reference, and my company has an NDA...
>
> Can't help with much more than providing this hint, sorry. All I can say
> is that SERDES protocol override from Linux is possible with care, at
> least on some systems. But it may be riddled with landmines.
>
>> We aren't even using this phy on our board, so I am fine disabling rate adaptation
>> for funky firmwares.
>
> Disabling rate adaptation is one thing. But there's also the unresolved
> XFI/2 issue?
I had in mind something like
diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 47a76df36b74..18dfc09e80ef 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -109,6 +109,12 @@
#define VEND1_GLOBAL_CFG_RATE_ADAPT_NONE 0
#define VEND1_GLOBAL_CFG_RATE_ADAPT_USX 1
#define VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE 2
+#define VEND1_GLOBAL_CFG_SERDES_MODE GENMASK(2, 0)
+#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI 0
+#define VEND1_GLOBAL_CFG_SERDES_MODE_SGMII 3
+#define VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII 4
+#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G 6
+#define VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G 7
#define VEND1_GLOBAL_RSVD_STAT1 0xc885
#define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID GENMASK(7, 4)
@@ -675,14 +681,69 @@ static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
return 0;
}
+static int aqr107_global_config_serdes_speed(int global_config)
+{
+ switch (FIELD_GET(VEND1_GLOBAL_CFG_SERDES_MODE, global_config)) {
+ case VEND1_GLOBAL_CFG_SERDES_MODE_XFI:
+ return SPEED_10000;
+ case VEND1_GLOBAL_CFG_SERDES_MODE_SGMII:
+ return SPEED_1000;
+ case VEND1_GLOBAL_CFG_SERDES_MODE_OCSGMII:
+ return SPEED_2500;
+ case VEND1_GLOBAL_CFG_SERDES_MODE_XFI5G:
+ return SPEED_5000;
+ case VEND1_GLOBAL_CFG_SERDES_MODE_XFI20G:
+ return SPEED_20000;
+ default:
+ return SPEED_UNKNOWN;
+ }
+}
+
+static bool aqr107_rate_adapt_ok(struct phy_device *phydev, u16 reg, int speed)
+{
+ int val;
+
+ val = phy_read_mmd(phydev, MDIO_MMD_VEND1, reg);
+ if (val < 0) {
+ phydev_warn(phydev, "could not read register %x:%x (err = %d)\n",
+ MDIO_MMD_VEND1, reg, val);
+ return false;
+ }
+
+ if (FIELD_GET(VEND1_GLOBAL_CFG_RATE_ADAPT, val) !=
+ VEND1_GLOBAL_CFG_RATE_ADAPT_PAUSE)
+ return false;
+
+ if (aqr107_global_config_serdes_speed(val) != speed)
+ return false;
+
+ return true;
+}
+
static int aqr107_get_rate_matching(struct phy_device *phydev,
phy_interface_t iface)
{
- if (iface == PHY_INTERFACE_MODE_10GBASER ||
- iface == PHY_INTERFACE_MODE_2500BASEX ||
- iface == PHY_INTERFACE_MODE_NA)
+ int speed = phy_interface_max_speed(iface);
+
+ switch (speed) {
+ case SPEED_10000:
+ if (!aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_10G, speed) ||
+ !aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_5G, speed))
+ return RATE_MATCH_NONE;
+ fallthrough;
+ case SPEED_2500:
+ if (!aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_2_5G, speed))
+ return RATE_MATCH_NONE;
+ fallthrough;
+ case SPEED_1000:
+ if (!aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_1G, speed) ||
+ !aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_100M, speed) ||
+ !aqr107_rate_adapt_ok(phydev, VEND1_GLOBAL_CFG_10M, speed))
+ return RATE_MATCH_NONE;
return RATE_MATCH_PAUSE;
- return RATE_MATCH_NONE;
+ default:
+ return RATE_MATCH_NONE;
+ };
}
static int aqr107_suspend(struct phy_device *phydev)
--
2.35.1.1320.gc452695387.dirty
Powered by blists - more mailing lists