Re: [PATCH net-next v13 12/15] net: stmmac: Fixed failure to set network speed to 1000.
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
Date: 2024-07-03 20:33:19
Subsystem:
ethernet phy library, networking drivers, the rest · Maintainers:
Andrew Lunn, Heiner Kallweit, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On Wed, Jul 03, 2024 at 09:09:53PM +0200, Andrew Lunn wrote:
quoted
Rather than erroring out, I think it may be better to just adopt the Marvell solution to this problem to give consistent behaviour across all PHYs.Yes, expand phy_config_aneg() to look for this condition and enable autoneg. But should we disable it when the condition is reverted? The user swaps to 100Mbps forced?
I think we should "lie" to userspace rather than report how the hardware was actually programmed - again, because that's what would happen with Marvell Alaska.
What about other speeds? Is this limited to 1G? Since we have devices without auto-neg for 2500BaseX i assume it is not an issue there.
1000base-X can have AN disabled - that's not an issue. Yes, there's the ongoing issues with 2500base-X. 10Gbase-T wording is similar to 1000base-T, so we probably need to do similar there. Likely also the case for 2500base-T and 5000base-T as well. So I'm thinking of something like this (untested):
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6c6ec9475709..197c4d5ab55b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c@@ -2094,22 +2094,20 @@ EXPORT_SYMBOL(phy_reset_after_clk_enable); /** * genphy_config_advert - sanitize and advertise auto-negotiation parameters * @phydev: target phy_device struct + * @advert: auto-negotiation parameters to advertise * * Description: Writes MII_ADVERTISE with the appropriate values, * after sanitizing the values to make sure we only advertise * what is supported. Returns < 0 on error, 0 if the PHY's advertisement * hasn't changed, and > 0 if it has changed. */ -static int genphy_config_advert(struct phy_device *phydev) +static int genphy_config_advert(struct phy_device *phydev, + const unsigned long *advert) { int err, bmsr, changed = 0; u32 adv; - /* Only allow advertising what this PHY supports */ - linkmode_and(phydev->advertising, phydev->advertising, - phydev->supported); - - adv = linkmode_adv_to_mii_adv_t(phydev->advertising); + adv = linkmode_adv_to_mii_adv_t(advert); /* Setup standard advertisement */ err = phy_modify_changed(phydev, MII_ADVERTISE,
@@ -2132,7 +2130,7 @@ static int genphy_config_advert(struct phy_device *phydev) if (!(bmsr & BMSR_ESTATEN)) return changed; - adv = linkmode_adv_to_mii_ctrl1000_t(phydev->advertising); + adv = linkmode_adv_to_mii_ctrl1000_t(advert); err = phy_modify_changed(phydev, MII_CTRL1000, ADVERTISE_1000FULL | ADVERTISE_1000HALF,
@@ -2356,6 +2354,9 @@ EXPORT_SYMBOL(genphy_check_and_restart_aneg); */ int __genphy_config_aneg(struct phy_device *phydev, bool changed) { + __ETHTOOL_DECLARE_LINK_MODE_MASK(fixed_advert); + const struct phy_setting *set; + unsigned long *advert; int err; err = genphy_c45_an_config_eee_aneg(phydev);
@@ -2370,10 +2371,25 @@ int __genphy_config_aneg(struct phy_device *phydev, bool changed) else if (err) changed = true; - if (AUTONEG_ENABLE != phydev->autoneg) + if (phydev->autoneg == AUTONEG_ENABLE) { + /* Only allow advertising what this PHY supports */ + linkmode_and(phydev->advertising, phydev->advertising, + phydev->supported); + advert = phydev->advertising; + } else if (phydev->speed < SPEED_1000) { return genphy_setup_forced(phydev); + } else { + linkmode_zero(fixed_advert); + + set = phy_lookup_setting(phydev->speed, phydev->duplex, + phydev->supported, true); + if (set) + linkmode_set(set->bit, fixed_advert); + + advert = fixed_advert; + } - err = genphy_config_advert(phydev); + err = genphy_config_advert(phydev, advert); if (err < 0) /* error */ return err; else if (err)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!