[RFC PATCH v3 net-next 0/6] Let phylink manage in-band AN for the PHY

This small series creates a configuration knob for PHY drivers which use
serial MII-side interfaces and support clause 37 in-band auto-negotiation
there. With this knob, phylink can control both the MAC side and the PHY
side, and keep them in sync, which is needed for a functional link. This
fixes the VSC8514 QSGMII PHY on the NXP T1040RDB and LS1028ARDB which is
described in the device tree as having in-band autoneg enabled, but
sometimes it has and sometimes not, depending on boot loader version.

Changes in v2:
Incorporated feedback from Russell, which was to consider PHYs on SFP
modules too, and unify phylink's detection of PHYs with broken in-band
autoneg with the newly introduced PHY driver methods.
https://patchwork.kernel.org/project/netdevbpf/cover/20210212172341.3489046-1-olteanv@gmail.com/

Changes in v3:
Added patch for the Atheros PHY family.

Vladimir Oltean (6):
  net: phylink: pass the phy argument to phylink_sfp_config
  net: phylink: introduce a generic method for querying PHY in-band
    autoneg capability
  net: phy: bcm84881: move the in-band capability check where it belongs
  net: phylink: explicitly configure in-band autoneg for PHYs that
    support it
  net: phy: mscc: configure in-band auto-negotiation for VSC8514
  net: phy: at803x: configure in-band auto-negotiation for AR8031/AR8033

 drivers/net/phy/at803x.c         | 72 ++++++++++++++++++++++++-
 drivers/net/phy/bcm84881.c       | 10 ++++
 drivers/net/phy/mscc/mscc.h      |  2 +
 drivers/net/phy/mscc/mscc_main.c | 20 +++++++
 drivers/net/phy/phy.c            | 25 +++++++++
 drivers/net/phy/phylink.c        | 93 +++++++++++++++++++++++++-------
 include/linux/phy.h              | 25 +++++++++
 7 files changed, 226 insertions(+), 21 deletions(-)

-- 
2.25.1

Problem statement: I would like to move the phy_no_inband() check inside
phylink_sfp_config(), right _after_ the PHY mode was determined by
sfp_select_interface(). But phylink_sfp_config() does not take the "phy"
as argument, only one of its callers (phylink_sfp_connect_phy) does.

phylink_sfp_config is called from:

- phylink_sfp_module_insert, if we know that the SFP module may not have
  a PHY
- phylink_sfp_module_start, if the SFP module may have a PHY but it is
  not available here (otherwise the "if (pl->phydev)" check right above
  would have triggered)
- phylink_sfp_connect_phy, which by definition has a PHY

So of all 3 callers, 2 are certain there is no PHY at that particular
moment, and 1 is certain there is one.

After further analysis, the "mode" is assumed to be MLO_AN_INBAND unless
there is a PHY, and that PHY has broken inband capabilities. So if we
pass the PHY pointer (be it NULL), we can drop the "mode" argument and
deduce it locally.

To avoid a forward-declaration, this change also moves phylink_phy_no_inband
above phylink_sfp_config.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/phylink.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 5a58c77d0002..fd02ec265a39 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -2146,6 +2146,15 @@ int phylink_speed_up(struct phylink *pl)
 }
 EXPORT_SYMBOL_GPL(phylink_speed_up);
 
+/* The Broadcom BCM84881 in the Methode DM7052 is unable to provide a SGMII
+ * or 802.3z control word, so inband will not work.
+ */
+static bool phylink_phy_no_inband(struct phy_device *phy)
+{
+	return phy->is_c45 &&
+		(phy->c45_ids.device_ids[1] & 0xfffffff0) == 0xae025150;
+}
+
 static void phylink_sfp_attach(void *upstream, struct sfp_bus *bus)
 {
 	struct phylink *pl = upstream;

@@ -2160,7 +2169,7 @@ static void phylink_sfp_detach(void *upstream, struct sfp_bus *bus)
 	pl->netdev->sfp_bus = NULL;
 }
 
-static int phylink_sfp_config(struct phylink *pl, u8 mode,
+static int phylink_sfp_config(struct phylink *pl, struct phy_device *phy,
 			      const unsigned long *supported,
 			      const unsigned long *advertising)
 {

@@ -2168,6 +2177,7 @@ static int phylink_sfp_config(struct phylink *pl, u8 mode,
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(support);
 	struct phylink_link_state config;
 	phy_interface_t iface;
+	unsigned int mode;
 	bool changed;
 	int ret;
 

@@ -2197,6 +2207,11 @@ static int phylink_sfp_config(struct phylink *pl, u8 mode,
 		return -EINVAL;
 	}
 
+	if (phy && phylink_phy_no_inband(phy))
+		mode = MLO_AN_PHY;
+	else
+		mode = MLO_AN_INBAND;
+
 	config.interface = iface;
 	linkmode_copy(support1, support);
 	ret = phylink_validate(pl, support1, &config);

@@ -2261,7 +2276,7 @@ static int phylink_sfp_module_insert(void *upstream,
 	if (pl->sfp_may_have_phy)
 		return 0;
 
-	return phylink_sfp_config(pl, MLO_AN_INBAND, support, support);
+	return phylink_sfp_config(pl, NULL, support, support);
 }
 
 static int phylink_sfp_module_start(void *upstream)

@@ -2280,8 +2295,7 @@ static int phylink_sfp_module_start(void *upstream)
 	if (!pl->sfp_may_have_phy)
 		return 0;
 
-	return phylink_sfp_config(pl, MLO_AN_INBAND,
-				  pl->sfp_support, pl->sfp_support);
+	return phylink_sfp_config(pl, NULL, pl->sfp_support, pl->sfp_support);
 }
 
 static void phylink_sfp_module_stop(void *upstream)

@@ -2312,20 +2326,10 @@ static void phylink_sfp_link_up(void *upstream)
 	phylink_run_resolve(pl);
 }
 
-/* The Broadcom BCM84881 in the Methode DM7052 is unable to provide a SGMII
- * or 802.3z control word, so inband will not work.
- */
-static bool phylink_phy_no_inband(struct phy_device *phy)
-{
-	return phy->is_c45 &&
-		(phy->c45_ids.device_ids[1] & 0xfffffff0) == 0xae025150;
-}
-
 static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 {
 	struct phylink *pl = upstream;
 	phy_interface_t interface;
-	u8 mode;
 	int ret;
 
 	/*

@@ -2337,13 +2341,8 @@ static int phylink_sfp_connect_phy(void *upstream, struct phy_device *phy)
 	 */
 	phy_support_asym_pause(phy);
 
-	if (phylink_phy_no_inband(phy))
-		mode = MLO_AN_PHY;
-	else
-		mode = MLO_AN_INBAND;
-
 	/* Do the initial configuration */
-	ret = phylink_sfp_config(pl, mode, phy->supported, phy->advertising);
+	ret = phylink_sfp_config(pl, phy, phy->supported, phy->advertising);
 	if (ret < 0)
 		return ret;
 

-- 
2.25.1

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@nxp.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

Now that there is a generic interface through which phylink can query
PHY drivers whether they support various forms of in-band autoneg, use
that and delete the special case from phylink.c.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/bcm84881.c | 10 ++++++++++
 drivers/net/phy/phylink.c  | 17 ++---------------
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
index 9717a1626f3f..5c0e4f85fc4e 100644
--- a/drivers/net/phy/bcm84881.c
+++ b/drivers/net/phy/bcm84881.c

@@ -223,6 +223,15 @@ static int bcm84881_read_status(struct phy_device *phydev)
 	return genphy_c45_read_mdix(phydev);
 }
 
+/* The Broadcom BCM84881 in the Methode DM7052 is unable to provide a SGMII
+ * or 802.3z control word, so inband will not work.
+ */
+static int bcm84881_validate_inband_aneg(struct phy_device *phydev,
+					 phy_interface_t interface)
+{
+	return PHY_INBAND_ANEG_OFF;
+}
+
 static struct phy_driver bcm84881_drivers[] = {
 	{
 		.phy_id		= 0xae025150,

@@ -234,6 +243,7 @@ static struct phy_driver bcm84881_drivers[] = {
 		.config_aneg	= bcm84881_config_aneg,
 		.aneg_done	= bcm84881_aneg_done,
 		.read_status	= bcm84881_read_status,
+		.validate_inband_aneg = bcm84881_validate_inband_aneg,
 	},
 };
 

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index f9a7c999821b..358246775ad1 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -2185,15 +2185,6 @@ int phylink_speed_up(struct phylink *pl)
 }
 EXPORT_SYMBOL_GPL(phylink_speed_up);
 
-/* The Broadcom BCM84881 in the Methode DM7052 is unable to provide a SGMII
- * or 802.3z control word, so inband will not work.
- */
-static bool phylink_phy_no_inband(struct phy_device *phy)
-{
-	return phy->is_c45 &&
-		(phy->c45_ids.device_ids[1] & 0xfffffff0) == 0xae025150;
-}
-
 static void phylink_sfp_attach(void *upstream, struct sfp_bus *bus)
 {
 	struct phylink *pl = upstream;

@@ -2252,14 +2243,10 @@ static int phylink_sfp_config(struct phylink *pl, struct phy_device *phy,
 	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;
+			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");
+				    "PHY driver does not report in-band autoneg capability, assuming true\n");
 		} else if (ret & PHY_INBAND_ANEG_ON) {
 			mode = MLO_AN_INBAND;
 		} else {

-- 
2.25.1

Currently Linux has no control over whether a MAC-to-PHY interface uses
in-band signaling or not, even though phylink has the
	managed = "in-band-status";
property which denotes that the MAC expects in-band signaling to be used.

The problem is really that if the in-band signaling is configurable in
both the PHY and the MAC, there is a risk that they are out of sync
unless phylink manages them both. Most if not all in-band autoneg state
machines follow IEEE 802.3 clause 37, which means that they will not
change the operating mode of the SERDES lane from control to data mode
unless in-band AN completed successfully. Therefore traffic will not
work.

It is particularly unpleasant that currently, we assume that PHYs which
have configurable in-band AN come pre-configured from a prior boot stage
such as U-Boot, because once the bootloader changes, all bets are off.

Let's introduce a new PHY driver method for configuring in-band autoneg,
and make phylink be its first user. The main PHY library does not call
phy_config_inband_autoneg, because it does not know what to configure it
to. Presumably, non-phylink drivers can also call phy_config_inband_autoneg
individually.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/phy.c     | 12 ++++++++++++
 drivers/net/phy/phylink.c | 10 ++++++++++
 include/linux/phy.h       |  8 ++++++++
 3 files changed, 30 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 975ae3595f8f..3adc818db30d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c

@@ -763,6 +763,18 @@ int phy_validate_inband_aneg(struct phy_device *phydev,
 }
 EXPORT_SYMBOL_GPL(phy_validate_inband_aneg);
 
+int phy_config_inband_aneg(struct phy_device *phydev, bool enabled)
+{
+	if (!phydev->drv)
+		return -EIO;
+
+	if (!phydev->drv->config_inband_aneg)
+		return -EOPNOTSUPP;
+
+	return phydev->drv->config_inband_aneg(phydev, enabled);
+}
+EXPORT_SYMBOL_GPL(phy_config_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 358246775ad1..b86e8f7b6c40 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -955,6 +955,7 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 {
 	struct phylink_link_state config;
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
+	bool use_inband;
 	char *irq_str;
 	int ret;
 

@@ -994,6 +995,15 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 		return ret;
 	}
 
+	use_inband = phylink_autoneg_inband(pl->cur_link_an_mode);
+
+	ret = phy_config_inband_aneg(phy, use_inband);
+	if (ret && ret != -EOPNOTSUPP) {
+		phylink_warn(pl, "failed to configure PHY in-band autoneg: %pe\n",
+			     ERR_PTR(ret));
+		return ret;
+	}
+
 	phy->phylink = pl;
 	phy->phy_link_change = phylink_phy_change;
 

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 4ac876f988ca..c81c6554d564 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -781,6 +781,13 @@ struct phy_driver {
 	int (*validate_inband_aneg)(struct phy_device *phydev,
 				    phy_interface_t interface);
 
+	/**
+	 * @config_inband_aneg: Enable or disable in-band auto-negotiation for
+	 * the system-side interface if the PHY operates in a mode that
+	 * requires it: (Q)SGMII, USXGMII, 1000Base-X, etc.
+	 */
+	int (*config_inband_aneg)(struct phy_device *phydev, bool enabled);
+
 	/** @aneg_done: Determines the auto negotiation result */
 	int (*aneg_done)(struct phy_device *phydev);
 

@@ -1474,6 +1481,7 @@ 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_config_inband_aneg(struct phy_device *phydev, bool enabled);
 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

Add the in-band configuration knob for the VSC8514 quad PHY. Tested with
QSGMII in-band AN both on and off on NXP LS1028A-RDB and T1040-RDB.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/mscc/mscc.h      |  2 ++
 drivers/net/phy/mscc/mscc_main.c | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index a50235fdf7d9..366db1425561 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h

@@ -195,6 +195,8 @@ enum rgmii_clock_delay {
 #define MSCC_PHY_EXTENDED_INT_MS_EGR	  BIT(9)
 
 /* Extended Page 3 Registers */
+#define MSCC_PHY_SERDES_PCS_CTRL	  16
+#define MSCC_PHY_SERDES_ANEG		  BIT(7)
 #define MSCC_PHY_SERDES_TX_VALID_CNT	  21
 #define MSCC_PHY_SERDES_TX_CRC_ERR_CNT	  22
 #define MSCC_PHY_SERDES_RX_VALID_CNT	  28

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 6e32da28e138..ca537204bc27 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c

@@ -2189,6 +2189,24 @@ static int vsc85xx_read_status(struct phy_device *phydev)
 	return genphy_read_status(phydev);
 }
 
+static int vsc8514_validate_inband_aneg(struct phy_device *phydev,
+					phy_interface_t interface)
+{
+	return PHY_INBAND_ANEG_OFF | PHY_INBAND_ANEG_ON;
+}
+
+static int vsc8514_config_inband_aneg(struct phy_device *phydev, bool enabled)
+{
+	int reg_val = 0;
+
+	if (enabled)
+		reg_val = MSCC_PHY_SERDES_ANEG;
+
+	return phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_3,
+				MSCC_PHY_SERDES_PCS_CTRL, MSCC_PHY_SERDES_ANEG,
+				reg_val);
+}
+
 static int vsc8514_probe(struct phy_device *phydev)
 {
 	struct vsc8531_private *vsc8531;

@@ -2395,6 +2413,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_sset_count = &vsc85xx_get_sset_count,
 	.get_strings    = &vsc85xx_get_strings,
 	.get_stats      = &vsc85xx_get_stats,
+	.validate_inband_aneg = vsc8514_validate_inband_aneg,
+	.config_inband_aneg = vsc8514_config_inband_aneg,
 },
 {
 	.phy_id		= PHY_ID_VSC8530,

-- 
2.25.1

The Atheros PHY driver supports quite a wide variety of hardware, but
the validation function for the in-band autoneg setting was deliberately
made common to all revisions.

The reasoning went as follows:

- In-band autonegotiation is only of concern for protocols which use
  the IEEE 802.3 clause 37 state machines. In the case of Atheros PHYs,
  that is only SGMII. The only PHYs that support SGMII are AR8031 and
  AR8033.

- The other PHYs either use MII/RMII (AR8030/AR8032), RGMII (AR8035,
  AR8031/AR8033 in certain configurations), or are straight out internal
  to a switch, and in that case, in-band autoneg makes no sense.

- In any case it is buggy to request in-band autoneg for an
  MII/RMII/RGMII/internal PHY, so the common method also validates that.

In any case, for AR8031/AR8033, the original intention was to only
declare support for PHY_INBAND_ANEG_ON. The idea is that even that is
valuable in its own right. For example, this avoids future breakages
caused by conversions to phylink such as the one fixed by commit
df392aefe96b ("arm64: dts: fsl-ls1028a-kontron-sl28: specify in-band
mode for ENETC"), by pulling the MAC side of phylink into using
MLO_AN_INBAND.

Nonetheless, after playing around a bit, I managed to get my AR8033 to
work fine with all of 10/100/1000 link speeds even with in-band autoneg
disabled. The strategy to keep the fiber and copper page speeds in sync
was based on the comments made by Michael Walle here:
https://patchwork.kernel.org/project/netdevbpf/patch/20210212172341.3489046-2-olteanv@gmail.com/

I wanted to see if it works at all, more than anything else, but now I'm
in a bit of a dilemma whether to make this PHY driver support both
cases, but risk regressions with MAC drivers that don't disable inband
autoneg in MLO_AN_PHY mode, or just force PHY_INBAND_ANEG_ON and hence
MLO_AN_INBAND, and continue to work with those. The thing is, I'm pretty
sure that there isn't any in-tree user of Atheros PHYs in SGMII mode
with inband autoneg off, because that requires manually keeping the
speeds in sync, and since the code did not do that, that would have been
a pretty broken link, working just at 1Gbps. So the risk is definitely
larger to actually do what the PHY has been requested, but it also
requires the MAC driver to put its money where its mouth is. I've
audited the tree and macb_mac_config() looks suspicious, but I don't
have all the details to understand whether there is any system that
would be affected by this change.

Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Claudiu Beznea <redacted>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/at803x.c | 72 +++++++++++++++++++++++++++++++++++++++-
 include/linux/phy.h      |  1 +
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 3feee4d59030..7262ce509762 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c

@@ -196,6 +196,7 @@ struct at803x_priv {
 	struct regulator_dev *vddh_rdev;
 	struct regulator *vddio;
 	u64 stats[ARRAY_SIZE(at803x_hw_stats)];
+	bool inband_an;
 };
 
 struct at803x_context {

@@ -784,8 +785,17 @@ static int at8031_pll_config(struct phy_device *phydev)
 
 static int at803x_config_init(struct phy_device *phydev)
 {
+	struct at803x_priv *priv = phydev->priv;
 	int ret;
 
+	if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+		ret = phy_read_paged(phydev, AT803X_PAGE_FIBER, MII_BMCR);
+		if (ret < 0)
+			return ret;
+
+		priv->inband_an = !!(ret & BMCR_ANENABLE);
+	}
+
 	/* The RX and TX delay default is:
 	 *   after HW reset: RX delay enabled and TX delay disabled
 	 *   after SW reset: RX delay enabled, while TX delay retains the

@@ -922,8 +932,26 @@ static void at803x_link_change_notify(struct phy_device *phydev)
 	}
 }
 
+/* When in-band autoneg is turned off, this hardware has a split-brain problem,
+ * it requires the SGMII-side link speed needs to be kept in sync with the
+ * media-side link speed by the driver, so do that.
+ */
+static int at803x_sync_fiber_page_speed(struct phy_device *phydev)
+{
+	int mask = BMCR_SPEED1000 | BMCR_SPEED100;
+	int val = 0;
+
+	if (phydev->speed == SPEED_1000)
+		val = BMCR_SPEED1000;
+	else if (phydev->speed == SPEED_100)
+		val = BMCR_SPEED100;
+
+	return phy_modify_paged(phydev, AT803X_PAGE_FIBER, MII_BMCR, mask, val);
+}
+
 static int at803x_read_status(struct phy_device *phydev)
 {
+	struct at803x_priv *priv = phydev->priv;
 	int ss, err, old_link = phydev->link;
 
 	/* Update the link, but return if there was an error */

@@ -996,7 +1024,10 @@ static int at803x_read_status(struct phy_device *phydev)
 	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete)
 		phy_resolve_aneg_pause(phydev);
 
-	return 0;
+	if (priv->inband_an)
+		return 0;
+
+	return at803x_sync_fiber_page_speed(phydev);
 }
 
 static int at803x_config_mdix(struct phy_device *phydev, u8 ctrl)

@@ -1043,6 +1074,36 @@ static int at803x_config_aneg(struct phy_device *phydev)
 	return genphy_config_aneg(phydev);
 }
 
+static int at803x_config_inband_aneg(struct phy_device *phydev, bool enabled)
+{
+	struct at803x_priv *priv = phydev->priv;
+	int err, val = 0;
+
+	if (phydev->interface != PHY_INTERFACE_MODE_SGMII)
+		return 0;
+
+	if (enabled)
+		val = BMCR_ANENABLE;
+
+	err = phy_modify_paged(phydev, AT803X_PAGE_FIBER, MII_BMCR,
+			       BMCR_ANENABLE, val);
+	if (err)
+		return err;
+
+	priv->inband_an = enabled;
+
+	return at803x_sync_fiber_page_speed(phydev);
+}
+
+static int at803x_validate_inband_aneg(struct phy_device *phydev,
+				       phy_interface_t interface)
+{
+	if (interface == PHY_INTERFACE_MODE_SGMII)
+		return PHY_INBAND_ANEG_ON | PHY_INBAND_ANEG_OFF;
+
+	return PHY_INBAND_ANEG_OFF;
+}
+
 static int at803x_get_downshift(struct phy_device *phydev, u8 *d)
 {
 	int val;

@@ -1335,6 +1396,7 @@ static struct phy_driver at803x_driver[] = {
 	.set_tunable		= at803x_set_tunable,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* Qualcomm Atheros AR8030 */
 	.phy_id			= ATH8030_PHY_ID,

@@ -1351,6 +1413,7 @@ static struct phy_driver at803x_driver[] = {
 	/* PHY_BASIC_FEATURES */
 	.config_intr		= at803x_config_intr,
 	.handle_interrupt	= at803x_handle_interrupt,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* Qualcomm Atheros AR8031/AR8033 */
 	PHY_ID_MATCH_EXACT(ATH8031_PHY_ID),

@@ -1375,6 +1438,8 @@ static struct phy_driver at803x_driver[] = {
 	.set_tunable		= at803x_set_tunable,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
+	.config_inband_aneg	= at803x_config_inband_aneg,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* Qualcomm Atheros AR8032 */
 	PHY_ID_MATCH_EXACT(ATH8032_PHY_ID),

@@ -1393,6 +1458,7 @@ static struct phy_driver at803x_driver[] = {
 	.handle_interrupt	= at803x_handle_interrupt,
 	.cable_test_start	= at803x_cable_test_start,
 	.cable_test_get_status	= at803x_cable_test_get_status,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* ATHEROS AR9331 */
 	PHY_ID_MATCH_EXACT(ATH9331_PHY_ID),

@@ -1408,6 +1474,7 @@ static struct phy_driver at803x_driver[] = {
 	.read_status		= at803x_read_status,
 	.soft_reset		= genphy_soft_reset,
 	.config_aneg		= at803x_config_aneg,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* QCA8337 */
 	.phy_id			= QCA8337_PHY_ID,

@@ -1423,6 +1490,7 @@ static struct phy_driver at803x_driver[] = {
 	.get_stats		= at803x_get_stats,
 	.suspend		= genphy_suspend,
 	.resume			= genphy_resume,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* QCA8327-A from switch QCA8327-AL1A */
 	.phy_id			= QCA8327_A_PHY_ID,

@@ -1438,6 +1506,7 @@ static struct phy_driver at803x_driver[] = {
 	.get_stats		= at803x_get_stats,
 	.suspend		= genphy_suspend,
 	.resume			= genphy_resume,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, {
 	/* QCA8327-B from switch QCA8327-BL1A */
 	.phy_id			= QCA8327_B_PHY_ID,

@@ -1453,6 +1522,7 @@ static struct phy_driver at803x_driver[] = {
 	.get_stats		= at803x_get_stats,
 	.suspend		= genphy_suspend,
 	.resume			= genphy_resume,
+	.validate_inband_aneg	= at803x_validate_inband_aneg,
 }, };
 
 module_phy_driver(at803x_driver);

diff --git a/include/linux/phy.h b/include/linux/phy.h
index c81c6554d564..de90b22f014d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h

@@ -584,6 +584,7 @@ struct phy_device {
 	unsigned mac_managed_pm:1;
 
 	unsigned autoneg:1;
+	unsigned inband_an:1;
 	/* The most recently read link state */
 	unsigned link:1;
 	unsigned autoneg_complete:1;

-- 
2.25.1

On Wed, Sep 22, 2021 at 09:14:42PM +0300, Vladimir Oltean wrote:

If we add support to the BCM84881 driver to work with
phy_validate_inband_aneg(), then this will always return
PHY_INBAND_ANEG_OFF and never PHY_INBAND_ANEG_ON. Consequently,
this will always produce this "error". It is not an error in the
SFP case, but it is if firmware is misconfigured.

So, this needs better handling - we should not be issuing an error-
level kernel message for something that is "normal".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

On Wed, Sep 22, 2021 at 10:22:19PM +0100, Russell King (Oracle) wrote:

On Wed, Sep 22, 2021 at 09:14:42PM +0300, Vladimir Oltean wrote:

quoted

+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");

If we add support to the BCM84881 driver to work with
phy_validate_inband_aneg(), then this will always return
PHY_INBAND_ANEG_OFF and never PHY_INBAND_ANEG_ON. Consequently,
this will always produce this "error". It is not an error in the
SFP case, but it is if firmware is misconfigured.

So, this needs better handling - we should not be issuing an error-
level kernel message for something that is "normal".

Is this better?

		phylink_printk(phy_on_sfp(phy) ? KERN_DEBUG : KERN_ERR, pl,
			       "Requested in-band autoneg but driver does not support this, disabling it.\n");

On Thu, Sep 23, 2021 at 12:31:16AM +0300, Vladimir Oltean wrote:

On Wed, Sep 22, 2021 at 10:22:19PM +0100, Russell King (Oracle) wrote:

quoted

On Wed, Sep 22, 2021 at 09:14:42PM +0300, Vladimir Oltean wrote:

quoted

+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");

If we add support to the BCM84881 driver to work with
phy_validate_inband_aneg(), then this will always return
PHY_INBAND_ANEG_OFF and never PHY_INBAND_ANEG_ON. Consequently,
this will always produce this "error". It is not an error in the
SFP case, but it is if firmware is misconfigured.

So, this needs better handling - we should not be issuing an error-
level kernel message for something that is "normal".

Is this better?

		phylink_printk(phy_on_sfp(phy) ? KERN_DEBUG : KERN_ERR, pl,
			       "Requested in-band autoneg but driver does not support this, disabling it.\n");

Ah, not sure whether that was a trick question or not, but
phylink_fixup_inband_aneg function does not get called for the SFP code
path, I even noted this in the commit message but forgot:

|   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.

That's why phy_validate_inband_aneg is called twice, once in
phylink_sfp_config and once for the on-board case.

On Wed, Sep 22, 2021 at 09:48:28PM +0000, Vladimir Oltean wrote:

On Thu, Sep 23, 2021 at 12:31:16AM +0300, Vladimir Oltean wrote:

quoted

On Wed, Sep 22, 2021 at 10:22:19PM +0100, Russell King (Oracle) wrote:

quoted

On Wed, Sep 22, 2021 at 09:14:42PM +0300, Vladimir Oltean wrote:

quoted

+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");

If we add support to the BCM84881 driver to work with
phy_validate_inband_aneg(), then this will always return
PHY_INBAND_ANEG_OFF and never PHY_INBAND_ANEG_ON. Consequently,
this will always produce this "error". It is not an error in the
SFP case, but it is if firmware is misconfigured.

So, this needs better handling - we should not be issuing an error-
level kernel message for something that is "normal".

Is this better?

		phylink_printk(phy_on_sfp(phy) ? KERN_DEBUG : KERN_ERR, pl,
			       "Requested in-band autoneg but driver does not support this, disabling it.\n");

Ah, not sure whether that was a trick question or not, but
phylink_fixup_inband_aneg function does not get called for the SFP code
path, I even noted this in the commit message but forgot:

No it wasn't a trick question. I thought you were calling
phylink_fixup_inband_aneg() from phylink_sfp_config(), but I see now
that you don't. That's what happens when you try and rush to review.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

On Thu, Sep 23, 2021 at 12:03:22AM +0100, Russell King (Oracle) wrote:

On Wed, Sep 22, 2021 at 09:48:28PM +0000, Vladimir Oltean wrote:

quoted

On Thu, Sep 23, 2021 at 12:31:16AM +0300, Vladimir Oltean wrote:

quoted

On Wed, Sep 22, 2021 at 10:22:19PM +0100, Russell King (Oracle) wrote:

quoted

On Wed, Sep 22, 2021 at 09:14:42PM +0300, Vladimir Oltean wrote:

quoted

+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");

If we add support to the BCM84881 driver to work with
phy_validate_inband_aneg(), then this will always return
PHY_INBAND_ANEG_OFF and never PHY_INBAND_ANEG_ON. Consequently,
this will always produce this "error". It is not an error in the
SFP case, but it is if firmware is misconfigured.

So, this needs better handling - we should not be issuing an error-
level kernel message for something that is "normal".

Is this better?

		phylink_printk(phy_on_sfp(phy) ? KERN_DEBUG : KERN_ERR, pl,
			       "Requested in-band autoneg but driver does not support this, disabling it.\n");

Ah, not sure whether that was a trick question or not, but
phylink_fixup_inband_aneg function does not get called for the SFP code
path, I even noted this in the commit message but forgot:

No it wasn't a trick question. I thought you were calling
phylink_fixup_inband_aneg() from phylink_sfp_config(), but I see now
that you don't. That's what happens when you try and rush to review.

How did I "rush to review" exactly? I waited for 24 days since the v2
for even a single review comment, with even a ping in between, before
resending the series largely unaltered, just with an extra patch appended.

Not complaining that you haven't reviewed this in 24 days, we all have
other things to do, but, "rush"? I have genuinely forgotten the
implementation details of the patches already, and I don't have any medical
issues with the memory that I know of (or I may have forgotten about them too).

So let's say I want to strike a balance between not rushing reviewers
and being able to usefully respond to questions. What is a good waiting time?
It is generally accepted on netdev that if there is no reaction within 3
days, then "out of sight" becomes "out of mind".

Also, "that's what happens" => what? Some confusion, which gets clarified
over two email exchanges in a few tens of minutes? Is that so bad?

On Wed, Sep 22, 2021 at 11:50:34PM +0000, Vladimir Oltean wrote:

On Thu, Sep 23, 2021 at 12:03:22AM +0100, Russell King (Oracle) wrote:

quoted

On Wed, Sep 22, 2021 at 09:48:28PM +0000, Vladimir Oltean wrote:

quoted

On Thu, Sep 23, 2021 at 12:31:16AM +0300, Vladimir Oltean wrote:

quoted

On Wed, Sep 22, 2021 at 10:22:19PM +0100, Russell King (Oracle) wrote:

quoted

On Wed, Sep 22, 2021 at 09:14:42PM +0300, Vladimir Oltean wrote:

quoted

+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");

If we add support to the BCM84881 driver to work with
phy_validate_inband_aneg(), then this will always return
PHY_INBAND_ANEG_OFF and never PHY_INBAND_ANEG_ON. Consequently,
this will always produce this "error". It is not an error in the
SFP case, but it is if firmware is misconfigured.

So, this needs better handling - we should not be issuing an error-
level kernel message for something that is "normal".

Is this better?

		phylink_printk(phy_on_sfp(phy) ? KERN_DEBUG : KERN_ERR, pl,
			       "Requested in-band autoneg but driver does not support this, disabling it.\n");

Ah, not sure whether that was a trick question or not, but
phylink_fixup_inband_aneg function does not get called for the SFP code
path, I even noted this in the commit message but forgot:

No it wasn't a trick question. I thought you were calling
phylink_fixup_inband_aneg() from phylink_sfp_config(), but I see now
that you don't. That's what happens when you try and rush to review.

How did I "rush to review" exactly? I waited for 24 days since the v2
for even a single review comment, with even a ping in between, before
resending the series largely unaltered, just with an extra patch appended.

FFS. Are you intentionally trying to misinterpret everything I say?
Who here is doing a review? You or me?

"That's what happens when you try and rush to review." is a form of
speech - clearly the "you" is not aimed at you Vladimir, but me.
Let's put this a different way.

I am blaming myself for rushing to review this last night.

Is that more clear for you?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

On Thu, Sep 23, 2021 at 09:19:21AM +0100, Russell King (Oracle) wrote:

On Wed, Sep 22, 2021 at 11:50:34PM +0000, Vladimir Oltean wrote:

quoted

On Thu, Sep 23, 2021 at 12:03:22AM +0100, Russell King (Oracle) wrote:

quoted

On Wed, Sep 22, 2021 at 09:48:28PM +0000, Vladimir Oltean wrote:

quoted

On Thu, Sep 23, 2021 at 12:31:16AM +0300, Vladimir Oltean wrote:

quoted

On Wed, Sep 22, 2021 at 10:22:19PM +0100, Russell King (Oracle) wrote:

quoted

On Wed, Sep 22, 2021 at 09:14:42PM +0300, Vladimir Oltean wrote:

quoted

+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");

If we add support to the BCM84881 driver to work with
phy_validate_inband_aneg(), then this will always return
PHY_INBAND_ANEG_OFF and never PHY_INBAND_ANEG_ON. Consequently,
this will always produce this "error". It is not an error in the
SFP case, but it is if firmware is misconfigured.

So, this needs better handling - we should not be issuing an error-
level kernel message for something that is "normal".

Is this better?

		phylink_printk(phy_on_sfp(phy) ? KERN_DEBUG : KERN_ERR, pl,
			       "Requested in-band autoneg but driver does not support this, disabling it.\n");

Ah, not sure whether that was a trick question or not, but
phylink_fixup_inband_aneg function does not get called for the SFP code
path, I even noted this in the commit message but forgot:

No it wasn't a trick question. I thought you were calling
phylink_fixup_inband_aneg() from phylink_sfp_config(), but I see now
that you don't. That's what happens when you try and rush to review.

How did I "rush to review" exactly? I waited for 24 days since the v2
for even a single review comment, with even a ping in between, before
resending the series largely unaltered, just with an extra patch appended.

FFS. Are you intentionally trying to misinterpret everything I say?
Who here is doing a review? You or me?

"That's what happens when you try and rush to review." is a form of
speech - clearly the "you" is not aimed at you Vladimir, but me.
Let's put this a different way.

I am blaming myself for rushing to review this last night.

Is that more clear for you?

Apologies for misinterpreting, even though that was still the only
interpretation I could give that would make logical sense. Why would you
rush to review an RFC in the middle of the night if it wasn't me who was
rushing you, and pinging earlier? And why mention it in the first place?

Anyway... I will keep posting this as an RFC until you feel that all
corner cases are covered reasonably enough, including in-band autoneg
handling in MAC drivers. So there is no risk of it getting applied,
there is no need to rush to review.

On Thu, Sep 23, 2021 at 09:58:18AM +0000, Vladimir Oltean wrote:

On Thu, Sep 23, 2021 at 09:19:21AM +0100, Russell King (Oracle) wrote:

quoted

On Wed, Sep 22, 2021 at 11:50:34PM +0000, Vladimir Oltean wrote:

quoted

On Thu, Sep 23, 2021 at 12:03:22AM +0100, Russell King (Oracle) wrote:

quoted

On Wed, Sep 22, 2021 at 09:48:28PM +0000, Vladimir Oltean wrote:

quoted

On Thu, Sep 23, 2021 at 12:31:16AM +0300, Vladimir Oltean wrote:

quoted

On Wed, Sep 22, 2021 at 10:22:19PM +0100, Russell King (Oracle) wrote:

quoted

On Wed, Sep 22, 2021 at 09:14:42PM +0300, Vladimir Oltean wrote:

quoted

+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");

If we add support to the BCM84881 driver to work with
phy_validate_inband_aneg(), then this will always return
PHY_INBAND_ANEG_OFF and never PHY_INBAND_ANEG_ON. Consequently,
this will always produce this "error". It is not an error in the
SFP case, but it is if firmware is misconfigured.

So, this needs better handling - we should not be issuing an error-
level kernel message for something that is "normal".

Is this better?

		phylink_printk(phy_on_sfp(phy) ? KERN_DEBUG : KERN_ERR, pl,
			       "Requested in-band autoneg but driver does not support this, disabling it.\n");

Ah, not sure whether that was a trick question or not, but
phylink_fixup_inband_aneg function does not get called for the SFP code
path, I even noted this in the commit message but forgot:

No it wasn't a trick question. I thought you were calling
phylink_fixup_inband_aneg() from phylink_sfp_config(), but I see now
that you don't. That's what happens when you try and rush to review.

How did I "rush to review" exactly? I waited for 24 days since the v2
for even a single review comment, with even a ping in between, before
resending the series largely unaltered, just with an extra patch appended.

FFS. Are you intentionally trying to misinterpret everything I say?
Who here is doing a review? You or me?

"That's what happens when you try and rush to review." is a form of
speech - clearly the "you" is not aimed at you Vladimir, but me.
Let's put this a different way.

I am blaming myself for rushing to review this last night.

Is that more clear for you?

Apologies for misinterpreting, even though that was still the only
interpretation I could give that would make logical sense.

I would encourage you to read up on "second-person self reference".
It's a thing in English since at least the 16th century through to
today, and also exists in other languages.

Why would you
rush to review an RFC in the middle of the night if it wasn't me who was
rushing you, and pinging earlier? And why mention it in the first place?

I think at this point I'm just going to give up for the rest of the
week looking at netdev patches. I really don't want this stress.
And it _IS_ extremely stressful dealing with netdev stuff.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!