On Wed, Jul 03, 2024 at 09:09:53PM +0200, Andrew Lunn wrote:

quoted

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.

quoted

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)