| 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: <20221125153555.uzrl7j2me3lh2aeg@skbuf>
Date: Fri, 25 Nov 2022 17:35:55 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 Fri, Nov 25, 2022 at 01:43:34PM +0000, Russell King (Oracle) wrote:
> Hi Vladimir,
>
> On Fri, Nov 25, 2022 at 02:30:42PM +0200, Vladimir Oltean wrote:
> > Hi Russell,
> >
> > Sorry for the delay. Had to do something else yesterday and the day before.
>
> I think there was some kind of celebration going on in the US for at
> least one of those days...
Yeah, but I don't celebrate that. I had to write some documentation.
> > So the default EXT_SR is being changed by the PHY driver from 0x9088 to
> > 0x9084 (MII_M1111_HWCFG_MODE_COPPER_1000X_AN -> MII_M1111_HWCFG_MODE_SGMII_NO_CLK).
> >
> > I don't know if it's possible to force these modules to operate in
> > 1000BASE-X mode. If you're interested in the results there, please give
> > me some guidance.
>
> The value of "EXT_SR before" is 1000base-X, so if you change sfp-bus.c::
> sfp_select_interface() to use 1000BASEX instead of SGMII then you'll be
> using 1000BASEX instead (and it should work, although at fixed 1G
> speeds). The only reason the module is working in SGMII mode is because,
> as you've noticed above, we switch it to SGMII mode in
> m88e1111_config_init_sgmii().
>
Which is an interesting thing, because m88e1111_config_init_1000basex()
does not change the HWCFG_MODE_MASK to something with 1000X in it.
Anyway, with sfp_select_interface() hacked, I can confirm it works in
1000BASE-X with AN enabled too.
[ 69.746643] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
[ 69.845784] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
[ 69.852764] fsl_dpaa2_eth dpni.1 dpmac7: switched to inband/1000base-x link mode // MLO_AN_INBAND
[ 69.934191] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x1140 after 0x1140
[ 70.015735] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
ping 192.168.100.2
PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
64 bytes from 192.168.100.2: icmp_seq=1 ttl=64 time=0.874 ms
64 bytes from 192.168.100.2: icmp_seq=2 ttl=64 time=0.225 ms
64 bytes from 192.168.100.2: icmp_seq=3 ttl=64 time=0.216 ms
^C
--- 192.168.100.2 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2019ms
rtt min/avg/max/mdev = 0.216/0.438/0.874/0.308 ms
printed with code:
static int m88e1111_config_init_1000basex(struct phy_device *phydev)
{
int extsr = phy_read(phydev, MII_M1111_PHY_EXT_SR);
int fiber_bmcr_before, fiber_bmcr_after;
int ext_sr_after;
int err, mode;
if (extsr < 0)
return extsr;
fiber_bmcr_before = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
if (fiber_bmcr_before < 0)
return fiber_bmcr_before;
/* 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,
MII_M1111_HWCFG_MODE_MASK |
MII_M1111_HWCFG_SERIAL_AN_BYPASS,
MII_M1111_HWCFG_MODE_COPPER_1000X_AN |
MII_M1111_HWCFG_SERIAL_AN_BYPASS);
if (err < 0)
return err;
}
ext_sr_after = phy_read(phydev, MII_M1111_PHY_EXT_SR);
if (ext_sr_after < 0)
return ext_sr_after;
fiber_bmcr_after = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
if (fiber_bmcr_after < 0)
return fiber_bmcr_after;
phydev_err(phydev, "%s: EXT_SR before 0x%x after 0x%x, fiber page BMCR before 0x%x after 0x%x\n",
__func__, extsr, ext_sr_after,
fiber_bmcr_before, fiber_bmcr_after);
return 0;
}
Furthermore, I can confirm that if the fiber page BMCR has BMCR_ANENABLE
disabled, the link with my Lynx PCS in MLO_AN_INBAND is broken (and that
the write to EXT_SR doesn't change the value of the BMCR).
But there's actually a problem (or maybe two problems).
First is that if I make phylink treat the ON_TIMEOUT capability by using
MLO_AN_PHY (basically like this):
phylink_sfp_config_phy():
/* Select whether to operate in in-band mode or not, based on the
* capability of the PHY in the current link mode.
*/
ret = phy_validate_an_inband(phy, iface);
phylink_err(pl, "PHY driver reported AN inband 0x%x\n", ret);
if (ret == PHY_AN_INBAND_UNKNOWN) {
mode = MLO_AN_INBAND;
phylink_dbg(pl,
"PHY driver does not report in-band autoneg capability, assuming true\n");
// } else if (ret & (PHY_AN_INBAND_ON | PHY_AN_INBAND_ON_TIMEOUT)) {
} else if (ret & PHY_AN_INBAND_ON) {
mode = MLO_AN_INBAND;
} else {
mode = MLO_AN_PHY;
}
[ 30.059923] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
[ 30.066867] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/1000base-x link mode // MLO_AN_PHY
[ 30.153350] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x1140 after 0x1140
[ 30.238970] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
then pinging is broken with mismatched in-band AN settings ("TIMEOUT" in
PHY, "OFF" in PCS). I triple-checked this.
ping 192.168.100.2
PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
>From 192.168.100.1 icmp_seq=1 Destination Host Unreachable
>From 192.168.100.1 icmp_seq=2 Destination Host Unreachable
>From 192.168.100.1 icmp_seq=3 Destination Host Unreachable
>From 192.168.100.1 icmp_seq=4 Destination Host Unreachable
>From 192.168.100.1 icmp_seq=5 Destination Host Unreachable
>From 192.168.100.1 icmp_seq=6 Destination Host Unreachable
^C
--- 192.168.100.2 ping statistics ---
9 packets transmitted, 0 received, +6 errors, 100% packet loss, time 8170ms
However, if using the same phylink code (to force a mismatch), I unhack
sfp_select_interface() and use SGMII mode, the timeout feature does
actually work:
[ 30.262979] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // PHY_AN_INBAND_ON_TIMEOUT
[ 30.270349] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/sgmii link mode // MLO_AN_PHY
[ 30.351066] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR before 0x1140 after 0x1140
[ 30.433236] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
this is a functional link despite the mismatched settings.
ping 192.168.100.2
PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
64 bytes from 192.168.100.2: icmp_seq=1 ttl=64 time=0.885 ms
64 bytes from 192.168.100.2: icmp_seq=2 ttl=64 time=0.221 ms
64 bytes from 192.168.100.2: icmp_seq=3 ttl=64 time=0.216 ms
64 bytes from 192.168.100.2: icmp_seq=4 ttl=64 time=0.217 ms
64 bytes from 192.168.100.2: icmp_seq=5 ttl=64 time=0.238 ms
^C
--- 192.168.100.2 ping statistics ---
5 packets transmitted, 5 received, 0% packet loss, time 4062ms
rtt min/avg/max/mdev = 0.216/0.355/0.885/0.264 ms
The second problem is that not even *matched* settings work if I turn
off BMCR_ANENABLE in the PHY fiber page.
[ 30.809869] fsl_dpaa2_eth dpni.1 dpmac7: configuring for inband/sgmii link mode
[ 30.817936] mdio_bus 0x0000000008c1f000:00: MII_BMCR 0x1140 MII_BMSR 0x9 MII_ADVERTISE 0x1 MII_LPA 0x0 IF_MODE 0x3 // PCS registers at the end of lynx_pcs_config_giga()
[ 30.917651] fsl_dpaa2_eth dpni.1 dpmac7: PHY driver reported AN inband 0x4 // ignore; m88e1111_validate_an_inband() is hardcoded for this and does not detect BMCR for BASE-X
[ 30.924571] fsl_dpaa2_eth dpni.1 dpmac7: switched to phy/1000base-x link mode
[ 30.932441] mdio_bus 0x0000000008c1f000:00: MII_BMCR 0x140 MII_BMSR 0xd MII_ADVERTISE 0x1 MII_LPA 0x0 IF_MODE 0x1
[ 31.032547] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_1000basex: EXT_SR before 0x9088 after 0x9088, fiber page BMCR before 0x140 after 0x140
[ 31.117668] fsl_dpaa2_eth dpni.1 dpmac7: PHY [i2c:sfp-0:16] driver [Marvell 88E1111] (irq=POLL)
ping 192.168.100.2
PING 192.168.100.2 (192.168.100.2) 56(84) bytes of data.
^C
--- 192.168.100.2 ping statistics ---
4 packets transmitted, 0 received, 100% packet loss, time 3058ms
What's common is that if in-band autoneg is turned off (either forced
off or via timeout), 1000BASE-X between the Lynx PCS and the 88E1111
simply doesn't work.
> > I was curious if the fiber page BMCR has an effect for in-band autoneg,
> > and at least for SGMII it surely does. If I add this to
> > m88e1111_config_init_sgmii():
> >
> > phy_modify_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR,
> > BMCR_ANENABLE, 0);
> >
> > (and force an intentional mismatch) then I am able to break the link
> > with my Lynx PCS.
>
> Yes, the fiber page is re-used for the host side of the link when
> operating in SGMII and 1000baseX modes, so changes there have the
> expected effect.
>
> > If my hunch is correct that this also holds true for 1000BASE-X, then
> > you are also correct that m88e1111_config_init_1000basex() only allows
> > AN bypass but does not seem to enable in-band AN in itself, if it wasn't
> > enabled.
> >
> > The implication here is that maybe we should test for the fiber page
> > BMCR in both SGMII and 1000BASE-X modes?
>
> I think a more comprehensive test would be to write the fiber page
> BMCR with 0x140 before changing the mode from 1000baseX to SGMII and
> see whether the BMCR changes value. My suspicion is it won't, and
> the hwcfg_mode only has an effect on the settings in the fiber page
> under hardware reset conditions, and mode changes have no effect on
> the fiber page.
Confirmed that changes to the EXT_SR register don't cause changes to the
MII_BMCR register:
[ 28.587838] Marvell 88E1111 i2c:sfp-0:16: m88e1111_config_init_sgmii: EXT_SR before 0x9088 after 0x9084, fiber page BMCR before 0x140 after 0x140
generated by:
static int m88e1111_config_init_sgmii(struct phy_device *phydev)
{
int fiber_bmcr_before, fiber_bmcr_after;
int ext_sr_before, ext_sr_after;
int err;
ext_sr_before = phy_read(phydev, MII_M1111_PHY_EXT_SR);
if (ext_sr_before < 0)
return ext_sr_before;
err = phy_modify_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR,
BMCR_ANENABLE, 0);
if (err < 0)
return err;
fiber_bmcr_before = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
if (fiber_bmcr_before < 0)
return fiber_bmcr_before;
err = m88e1111_config_init_hwcfg_mode(
phydev,
MII_M1111_HWCFG_MODE_SGMII_NO_CLK,
MII_M1111_HWCFG_FIBER_COPPER_AUTO);
if (err < 0)
return err;
ext_sr_after = phy_read(phydev, MII_M1111_PHY_EXT_SR);
if (ext_sr_after < 0)
return ext_sr_after;
fiber_bmcr_after = phy_read_paged(phydev, MII_MARVELL_FIBER_PAGE, MII_BMCR);
if (fiber_bmcr_after < 0)
return fiber_bmcr_after;
phydev_err(phydev, "%s: EXT_SR before 0x%x after 0x%x, fiber page BMCR before 0x%x after 0x%x\n",
__func__, ext_sr_before, ext_sr_after,
fiber_bmcr_before, fiber_bmcr_after);
/* make sure copper is selected */
return marvell_set_page(phydev, MII_MARVELL_COPPER_PAGE);
}
Powered by blists - more mailing lists