| Message ID | E1sbdxI-0024Eo-HE@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phylib: do not disable autoneg for fixed speeds >= 1G | expand |
Hi Russell, kernel test robot noticed the following build errors: [auto build test ERROR on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Russell-King-Oracle/net-phylib-do-not-disable-autoneg-for-fixed-speeds-1G/20240807-185909 base: net-next/main patch link: https://lore.kernel.org/r/E1sbdxI-0024Eo-HE%40rmk-PC.armlinux.org.uk patch subject: [PATCH net-next] net: phylib: do not disable autoneg for fixed speeds >= 1G config: i386-buildonly-randconfig-004-20240808 (https://download.01.org/0day-ci/archive/20240808/202408080419.a2PXcqh8-lkp@intel.com/config) compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408080419.a2PXcqh8-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408080419.a2PXcqh8-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/net/phy/phy_device.c:2124:34: error: passing 'const unsigned long *' to parameter of type 'unsigned long *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 2124 | adv = linkmode_adv_to_mii_adv_t(advert); | ^~~~~~ include/linux/mii.h:143:60: note: passing argument to parameter 'advertising' here 143 | static inline u32 linkmode_adv_to_mii_adv_t(unsigned long *advertising) | ^ drivers/net/phy/phy_device.c:2147:39: error: passing 'const unsigned long *' to parameter of type 'unsigned long *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] 2147 | adv = linkmode_adv_to_mii_ctrl1000_t(advert); | ^~~~~~ include/linux/mii.h:218:65: note: passing argument to parameter 'advertising' here 218 | static inline u32 linkmode_adv_to_mii_ctrl1000_t(unsigned long *advertising) | ^ >> drivers/net/phy/phy_device.c:2401:4: error: call to undeclared function 'linkmode_set'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] 2401 | linkmode_set(set->bit, fixed_advert); | ^ 3 errors generated. vim +2124 drivers/net/phy/phy_device.c 2107 2108 /** 2109 * genphy_config_advert - sanitize and advertise auto-negotiation parameters 2110 * @phydev: target phy_device struct 2111 * @advert: auto-negotiation parameters to advertise 2112 * 2113 * Description: Writes MII_ADVERTISE with the appropriate values, 2114 * after sanitizing the values to make sure we only advertise 2115 * what is supported. Returns < 0 on error, 0 if the PHY's advertisement 2116 * hasn't changed, and > 0 if it has changed. 2117 */ 2118 static int genphy_config_advert(struct phy_device *phydev, 2119 const unsigned long *advert) 2120 { 2121 int err, bmsr, changed = 0; 2122 u32 adv; 2123 > 2124 adv = linkmode_adv_to_mii_adv_t(advert); 2125 2126 /* Setup standard advertisement */ 2127 err = phy_modify_changed(phydev, MII_ADVERTISE, 2128 ADVERTISE_ALL | ADVERTISE_100BASE4 | 2129 ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM, 2130 adv); 2131 if (err < 0) 2132 return err; 2133 if (err > 0) 2134 changed = 1; 2135 2136 bmsr = phy_read(phydev, MII_BMSR); 2137 if (bmsr < 0) 2138 return bmsr; 2139 2140 /* Per 802.3-2008, Section 22.2.4.2.16 Extended status all 2141 * 1000Mbits/sec capable PHYs shall have the BMSR_ESTATEN bit set to a 2142 * logical 1. 2143 */ 2144 if (!(bmsr & BMSR_ESTATEN)) 2145 return changed; 2146 2147 adv = linkmode_adv_to_mii_ctrl1000_t(advert); 2148 2149 err = phy_modify_changed(phydev, MII_CTRL1000, 2150 ADVERTISE_1000FULL | ADVERTISE_1000HALF, 2151 adv); 2152 if (err < 0) 2153 return err; 2154 if (err > 0) 2155 changed = 1; 2156 2157 return changed; 2158 } 2159
Hi Russell, kernel test robot noticed the following build errors: [auto build test ERROR on net-next/main] url: https://github.com/intel-lab-lkp/linux/commits/Russell-King-Oracle/net-phylib-do-not-disable-autoneg-for-fixed-speeds-1G/20240807-185909 base: net-next/main patch link: https://lore.kernel.org/r/E1sbdxI-0024Eo-HE%40rmk-PC.armlinux.org.uk patch subject: [PATCH net-next] net: phylib: do not disable autoneg for fixed speeds >= 1G config: arc-randconfig-001-20240808 (https://download.01.org/0day-ci/archive/20240808/202408080457.vhF74DGf-lkp@intel.com/config) compiler: arceb-elf-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240808/202408080457.vhF74DGf-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408080457.vhF74DGf-lkp@intel.com/ All error/warnings (new ones prefixed by >>): drivers/net/phy/phy_device.c: In function 'genphy_config_advert': >> drivers/net/phy/phy_device.c:2124:41: warning: passing argument 1 of 'linkmode_adv_to_mii_adv_t' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] 2124 | adv = linkmode_adv_to_mii_adv_t(advert); | ^~~~~~ In file included from include/uapi/linux/mdio.h:15, from include/linux/mdio.h:9, from drivers/net/phy/phy_device.c:23: include/linux/mii.h:143:60: note: expected 'long unsigned int *' but argument is of type 'const long unsigned int *' 143 | static inline u32 linkmode_adv_to_mii_adv_t(unsigned long *advertising) | ~~~~~~~~~~~~~~~^~~~~~~~~~~ >> drivers/net/phy/phy_device.c:2147:46: warning: passing argument 1 of 'linkmode_adv_to_mii_ctrl1000_t' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] 2147 | adv = linkmode_adv_to_mii_ctrl1000_t(advert); | ^~~~~~ include/linux/mii.h:218:65: note: expected 'long unsigned int *' but argument is of type 'const long unsigned int *' 218 | static inline u32 linkmode_adv_to_mii_ctrl1000_t(unsigned long *advertising) | ~~~~~~~~~~~~~~~^~~~~~~~~~~ drivers/net/phy/phy_device.c: In function '__genphy_config_aneg': >> drivers/net/phy/phy_device.c:2401:25: error: implicit declaration of function 'linkmode_set'; did you mean 'linkmode_subset'? [-Werror=implicit-function-declaration] 2401 | linkmode_set(set->bit, fixed_advert); | ^~~~~~~~~~~~ | linkmode_subset cc1: some warnings being treated as errors vim +2401 drivers/net/phy/phy_device.c 2359 2360 /** 2361 * __genphy_config_aneg - restart auto-negotiation or write BMCR 2362 * @phydev: target phy_device struct 2363 * @changed: whether autoneg is requested 2364 * 2365 * Description: If auto-negotiation is enabled, we configure the 2366 * advertising, and then restart auto-negotiation. If it is not 2367 * enabled, then we write the BMCR. 2368 */ 2369 int __genphy_config_aneg(struct phy_device *phydev, bool changed) 2370 { 2371 __ETHTOOL_DECLARE_LINK_MODE_MASK(fixed_advert); 2372 const struct phy_setting *set; 2373 unsigned long *advert; 2374 int err; 2375 2376 err = genphy_c45_an_config_eee_aneg(phydev); 2377 if (err < 0) 2378 return err; 2379 else if (err) 2380 changed = true; 2381 2382 err = genphy_setup_master_slave(phydev); 2383 if (err < 0) 2384 return err; 2385 else if (err) 2386 changed = true; 2387 2388 if (phydev->autoneg == AUTONEG_ENABLE) { 2389 /* Only allow advertising what this PHY supports */ 2390 linkmode_and(phydev->advertising, phydev->advertising, 2391 phydev->supported); 2392 advert = phydev->advertising; 2393 } else if (phydev->speed < SPEED_1000) { 2394 return genphy_setup_forced(phydev); 2395 } else { 2396 linkmode_zero(fixed_advert); 2397 2398 set = phy_lookup_setting(phydev->speed, phydev->duplex, 2399 phydev->supported, true); 2400 if (set) > 2401 linkmode_set(set->bit, fixed_advert); 2402 2403 advert = fixed_advert; 2404 } 2405 2406 err = genphy_config_advert(phydev, advert); 2407 if (err < 0) /* error */ 2408 return err; 2409 else if (err) 2410 changed = true; 2411 2412 return genphy_check_and_restart_aneg(phydev, changed); 2413 } 2414 EXPORT_SYMBOL(__genphy_config_aneg); 2415
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 29a4225cb712..76312591dcb8 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -2103,22 +2103,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, @@ -2141,7 +2139,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, @@ -2365,6 +2363,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); @@ -2379,10 +2380,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)
We have an increasing number of drivers that are forcing auto-negotiation to be enabled for speeds of 1G or faster. It would appear that auto-negotiation is mandatory for speeds above 100M. In 802.3, Annex 40C's state diagrams seems to imply that mr_autoneg_enable (BMCR AN ENABLE) doesn't affect whether or not the AN state machines work for 1000base-T, and some PHY datasheets (e.g. Marvell Alaska) state that disabling mr_autoneg_enable leaves AN enabled but forced to 1G full duplex. Other PHY datasheets imply that BMCR AN ENABLE should not be cleared for >= 1G. Thus, this should be handled in phylib rather than in each driver. Rather than erroring out, arrange to implement the Marvell Alaska solution but in software for all PHYs: generate an appropriate single-speed advertisement for the requested speed, and keep AN enabled to the PHY driver. However, to avoid userspace API breakage, continue to report to userspace that we have AN disabled. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phy_device.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)