Date:   Wed, 22 Sep 2021 21:14:42 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     Antoine Tenart <atenart@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Bjarni Jonasson <bjarni.jonasson@...rochip.com>,
        Steen Hegelund <steen.hegelund@...rochip.com>,
        UNGLinuxDriver@...rochip.com,
        bcm-kernel-feedback-list@...adcom.com,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>
Subject: [RFC PATCH v3 net-next 2/6] net: phylink: introduce a generic method for querying PHY in-band autoneg capability

Phylink parses the firmware node for 'managed = "in-band-status"' and
populates the initial pl->cfg_link_an_mode to MLO_AN_PHY or MLO_AN_INBAND
accordingly, but sometimes things do not really work out at runtime, and
the pl->cur_link_an_mode may change.

The most notable case is when an SFP module with a PHY that has broken
in-band autoneg is attached. Phylink currently open-codes a check for
the BCM84881 PHY ID and updates pl->cur_link_an_mode from MLO_AN_INBAND
to MLO_AN_PHY.

There is an additional degree of freedom I would like to add. This has
to do with the on-board PHY case (not on SFP). Sometimes, a PHY can only
operate with in-band autoneg enabled, but the MAC driver does not
declare 'managed = "in-band-status"' in the firmware node (say it was
recently converted from phylib to phylink). If the MAC driver is strict
in its phylink ops implementation, it will disable in-band autoneg and
thus the connection to the PHY will be broken.

The firmware can (and should) be updated, but if the PHY driver is
patched to report that it only supports in-band autoneg, then the
pl->cur_link_an_mode can be fixed up to request in-band autoneg from the
MAC driver, even if the firmware node does not. While I do not expect
production systems to rely on this feature, it seems sensible to have it
as long as it is not difficult to implement (the PHY driver should be
updated with a small .validate_inband_aneg method), and it can even ease
the transition from phylib to phylink.

There is also the reverse case: the firmware node reports MLO_AN_INBAND
but the on-board PHY doesn't support that. That sounds like a serious
bug, so while we still do attempt to fix it up (it seems within our
reach to handle it, and worth it), we print to the kernel log on a more
severe tone and not just at the debug level.

So if the 3 code paths:
- phylink_sfp_config
- phylink_connect_phy
- phylink_fwnode_phy_connect

do more or less the same thing (adapt pl->cur_link_an_mode based on the
capability reported by the PHY), the intention is different. With SFP
modules this behavior is absolutely to be expected, and pl->cfg_link_an_mode
only denotes the initial operating mode. On the other hand, when the PHY
is on-board, the initial link AN mode should ideally also be the final
one. So the implementations for the three are different.

Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/phy/phy.c     | 13 ++++++++
 drivers/net/phy/phylink.c | 63 +++++++++++++++++++++++++++++++++++++--
 include/linux/phy.h       | 16 ++++++++++
 3 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index f124a8a58bd4..975ae3595f8f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -750,6 +750,19 @@ static int phy_check_link_status(struct phy_device *phydev)
 	return 0;
 }
 
+int phy_validate_inband_aneg(struct phy_device *phydev,
+			     phy_interface_t interface)
+{
+	if (!phydev->drv)
+		return -EIO;
+
+	if (!phydev->drv->validate_inband_aneg)
+		return PHY_INBAND_ANEG_UNKNOWN;
+
+	return phydev->drv->validate_inband_aneg(phydev, interface);
+}
+EXPORT_SYMBOL_GPL(phy_validate_inband_aneg);
+
 /**
  * phy_start_aneg - start auto-negotiation for this PHY device
  * @phydev: the phy_device struct
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index fd02ec265a39..f9a7c999821b 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1043,6 +1043,39 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 	return phy_attach_direct(pl->netdev, phy, 0, interface);
 }
 
+static unsigned int phylink_fixup_inband_aneg(struct phylink *pl,
+					      struct phy_device *phy,
+					      unsigned int mode)
+{
+	int ret;
+
+	ret = phy_validate_inband_aneg(phy, pl->link_interface);
+	if (ret == PHY_INBAND_ANEG_UNKNOWN) {
+		phylink_dbg(pl,
+			    "PHY driver does not report in-band autoneg capability, assuming %s\n",
+			    phylink_autoneg_inband(mode) ? "true" : "false");
+
+		return mode;
+	}
+
+	if (phylink_autoneg_inband(mode) && !(ret & PHY_INBAND_ANEG_ON)) {
+		phylink_err(pl,
+			    "Requested in-band autoneg but driver does not support this, disabling it.\n");
+
+		return MLO_AN_PHY;
+	}
+
+	if (!phylink_autoneg_inband(mode) && !(ret & PHY_INBAND_ANEG_OFF)) {
+		phylink_dbg(pl,
+			    "PHY driver requests in-band autoneg, force-enabling it.\n");
+
+		mode = MLO_AN_INBAND;
+	}
+
+	/* Peaceful agreement, isn't it great? */
+	return mode;
+}
+
 /**
  * phylink_connect_phy() - connect a PHY to the phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()
@@ -1062,6 +1095,9 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
 {
 	int ret;
 
+	pl->cur_link_an_mode = phylink_fixup_inband_aneg(pl, phy,
+							 pl->cfg_link_an_mode);
+
 	/* Use PHY device/driver interface */
 	if (pl->link_interface == PHY_INTERFACE_MODE_NA) {
 		pl->link_interface = phy->interface;
@@ -1137,6 +1173,9 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 	if (!phy_dev)
 		return -ENODEV;
 
+	pl->cur_link_an_mode = phylink_fixup_inband_aneg(pl, phy_dev,
+							 pl->cfg_link_an_mode);
+
 	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
 				pl->link_interface);
 	if (ret) {
@@ -2207,10 +2246,28 @@ static int phylink_sfp_config(struct phylink *pl, struct phy_device *phy,
 		return -EINVAL;
 	}
 
-	if (phy && phylink_phy_no_inband(phy))
-		mode = MLO_AN_PHY;
-	else
+	/* Select whether to operate in in-band mode or not, based on the
+	 * presence and capability of the PHY in the current link mode.
+	 */
+	if (phy) {
+		ret = phy_validate_inband_aneg(phy, iface);
+		if (ret == PHY_INBAND_ANEG_UNKNOWN) {
+			if (phylink_phy_no_inband(phy))
+				mode = MLO_AN_PHY;
+			else
+				mode = MLO_AN_INBAND;
+
+			phylink_dbg(pl,
+				    "PHY driver does not report in-band autoneg capability, assuming %s\n",
+				    phylink_autoneg_inband(mode) ? "true" : "false");
+		} else if (ret & PHY_INBAND_ANEG_ON) {
+			mode = MLO_AN_INBAND;
+		} else {
+			mode = MLO_AN_PHY;
+		}
+	} else {
 		mode = MLO_AN_INBAND;
+	}
 
 	config.interface = iface;
 	linkmode_copy(support1, support);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 736e1d1a47c4..4ac876f988ca 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -698,6 +698,12 @@ struct phy_tdr_config {
 };
 #define PHY_PAIR_ALL -1
 
+enum phy_inband_aneg {
+	PHY_INBAND_ANEG_UNKNOWN		= BIT(0),
+	PHY_INBAND_ANEG_OFF		= BIT(1),
+	PHY_INBAND_ANEG_ON		= BIT(2),
+};
+
 /**
  * struct phy_driver - Driver structure for a particular PHY type
  *
@@ -767,6 +773,14 @@ struct phy_driver {
 	 */
 	int (*config_aneg)(struct phy_device *phydev);
 
+	/**
+	 * @validate_inband_aneg: Report what types of in-band auto-negotiation
+	 * are available for the given PHY interface type. Returns a bit mask
+	 * of type enum phy_inband_aneg.
+	 */
+	int (*validate_inband_aneg)(struct phy_device *phydev,
+				    phy_interface_t interface);
+
 	/** @aneg_done: Determines the auto negotiation result */
 	int (*aneg_done)(struct phy_device *phydev);
 
@@ -1458,6 +1472,8 @@ void phy_start(struct phy_device *phydev);
 void phy_stop(struct phy_device *phydev);
 int phy_config_aneg(struct phy_device *phydev);
 int phy_start_aneg(struct phy_device *phydev);
+int phy_validate_inband_aneg(struct phy_device *phydev,
+			     phy_interface_t interface);
 int phy_aneg_done(struct phy_device *phydev);
 int phy_speed_down(struct phy_device *phydev, bool sync);
 int phy_speed_up(struct phy_device *phydev);
-- 
2.25.1

Powered by blists - more mailing lists