[PATCH net-next 0/7] Allow controlling PHY loopback and isolate modes

[PATCH net-next 0/7] Allow controlling PHY loopback and isolate modes

[Maxime Chevallier]

Hello everyone,

This series brings support for controlling the isolation and loopback
modes for PHY devices, through a netlink interface.

The isolation support work is made in preparation for the support of
interfaces that posesses multiple PHYs attached to the same MAC, on the
same MII bus. In this configuration, the isolation mode for the PHY is
used to avoid interferences on the MII bus, which doesn't support
multidrop topologies.

This mode is part of the 802.3 spec, however rarely used. It was
discovered that some PHYs don't implement that mode correctly, and will
continue being active on the MII interface when isolated. This series
supports that case, and flags the LXT973 as having such a broken
isolation mode. The Marvell 88x3310/3340 PHYs also don't support this
mdoe, and are also flagged accordingly.

The main part needed for the upcomping multi-PHY support really is the
internal kernel API to support this.

The second part of the series (patches 5, 6 and 7) focus on allowing
userspace to control that mode. The only real benefit of controlling this
from userspace is to make it easier to find out if this mode really
works or not on the PHY being used.

This relies on a new set of ethtool_phy_ops, set_config and get_config,
to toggle these modes.

The loopback control from that API is added as it fits the API
well, and having the ability to easily set the PHY in MII-loopback
mode is a helpful tool to have when bringing-up a new device and
troubleshooting the link setup.

The netlink API is an extension of the existing PHY_GET, reporting 2 new
attributes (one for isolate, one for loopback). A PHY_SET command is
introduced as well, allowing to configure the loopback and isolation.

All in all, the userspace part is a bonus here, let me know if you think
is just doesn't make sense, although the loopback feature can be useful
and sent as a standalone series. In such case, I'll include the kernel-only
support for isolation (so, patches 1 to 5) as part of the multi-PHY
support series when it comes out.

Thanks,

Maxime

Maxime Chevallier (7):
  net: phy: allow isolating PHY devices
  net: phy: Allow flagging PHY devices that can't isolate their MII
  net: phy: lxt: Mark LXT973 PHYs as having a broken isolate mode
  net: phy: marvell10g: 88x3310 and 88x3340 don't support isolate mode
  net: phy: introduce ethtool_phy_ops to get and set phy configuration
  net: ethtool: phy: allow reporting and setting the phy isolate status
  netlink: specs: introduce phy-set command along with configurable
    attributes

 Documentation/netlink/specs/ethtool.yaml     | 20 +++++
 Documentation/networking/ethtool-netlink.rst |  2 +
 drivers/net/phy/lxt.c                        |  2 +
 drivers/net/phy/marvell10g.c                 |  2 +
 drivers/net/phy/phy.c                        | 59 +++++++++++++
 drivers/net/phy/phy_device.c                 | 89 ++++++++++++++++++--
 include/linux/ethtool.h                      |  8 ++
 include/linux/phy.h                          | 26 ++++++
 include/uapi/linux/ethtool_netlink.h         |  3 +
 net/ethtool/netlink.c                        |  8 ++
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/phy.c                            | 75 +++++++++++++++++
 12 files changed, 289 insertions(+), 6 deletions(-)

-- 
2.46.0

[PATCH net-next 1/7] net: phy: allow isolating PHY devices

[Maxime Chevallier]

The 802.3 specifications describes the isolation mode as setting the
PHY's MII interface in high-impedance mode, thus isolating the PHY from
that bus. This effectively breaks the link between the MAC and the PHY,
but without necessarily disrupting the link between the PHY and the LP.

This mode can be useful for testing purposes, but also when there are
multiple PHYs on the same MII bus (a case that the 802.3 specification
refers to).

In Isolation mode, the PHY will still continue to respond to MDIO
commands.

Introduce a helper to set the phy in an isolated mode.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 76 +++++++++++++++++++++++++++++++++---
 include/linux/phy.h          |  4 ++
 2 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 560e338b307a..c468e72bef4b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2127,6 +2127,38 @@ int phy_loopback(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL(phy_loopback);
 
+int phy_isolate(struct phy_device *phydev, bool enable)
+{
+	int ret = 0;
+
+	if (!phydev->drv)
+		return -EIO;
+
+	mutex_lock(&phydev->lock);
+
+	if (enable && phydev->isolated) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	if (!enable && !phydev->isolated) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = genphy_isolate(phydev, enable);
+
+	if (ret)
+		goto out;
+
+	phydev->isolated = enable;
+
+out:
+	mutex_unlock(&phydev->lock);
+	return ret;
+}
+EXPORT_SYMBOL(phy_isolate);
+
 /**
  * phy_reset_after_clk_enable - perform a PHY reset if needed
  * @phydev: target phy_device struct
@@ -2280,7 +2312,7 @@ int genphy_setup_forced(struct phy_device *phydev)
 	ctl = mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
 
 	return phy_modify(phydev, MII_BMCR,
-			  ~(BMCR_LOOPBACK | BMCR_ISOLATE | BMCR_PDOWN), ctl);
+			  ~(BMCR_LOOPBACK | BMCR_PDOWN), ctl);
 }
 EXPORT_SYMBOL(genphy_setup_forced);
 
@@ -2369,8 +2401,11 @@ EXPORT_SYMBOL(genphy_read_master_slave);
  */
 int genphy_restart_aneg(struct phy_device *phydev)
 {
-	/* Don't isolate the PHY if we're negotiating */
-	return phy_modify(phydev, MII_BMCR, BMCR_ISOLATE,
+	u16 mask = phydev->isolated ? 0 : BMCR_ISOLATE;
+	/* Don't isolate the PHY if we're negotiating, unless the PHY is
+	 * explicitly isolated
+	 */
+	return phy_modify(phydev, MII_BMCR, mask,
 			  BMCR_ANENABLE | BMCR_ANRESTART);
 }
 EXPORT_SYMBOL(genphy_restart_aneg);
@@ -2394,7 +2429,8 @@ int genphy_check_and_restart_aneg(struct phy_device *phydev, bool restart)
 		if (ret < 0)
 			return ret;
 
-		if (!(ret & BMCR_ANENABLE) || (ret & BMCR_ISOLATE))
+		if (!(ret & BMCR_ANENABLE) ||
+		    ((ret & BMCR_ISOLATE) && !phydev->isolated))
 			restart = true;
 	}
 
@@ -2495,7 +2531,8 @@ int genphy_c37_config_aneg(struct phy_device *phydev)
 		if (ctl < 0)
 			return ctl;
 
-		if (!(ctl & BMCR_ANENABLE) || (ctl & BMCR_ISOLATE))
+		if (!(ctl & BMCR_ANENABLE) ||
+		    ((ctl & BMCR_ISOLATE) && !phydev->isolated))
 			changed = 1; /* do restart aneg */
 	}
 
@@ -2782,12 +2819,18 @@ EXPORT_SYMBOL(genphy_c37_read_status);
 int genphy_soft_reset(struct phy_device *phydev)
 {
 	u16 res = BMCR_RESET;
+	u16 mask = 0;
 	int ret;
 
 	if (phydev->autoneg == AUTONEG_ENABLE)
 		res |= BMCR_ANRESTART;
 
-	ret = phy_modify(phydev, MII_BMCR, BMCR_ISOLATE, res);
+	if (phydev->isolated)
+		res |= BMCR_ISOLATE;
+	else
+		mask |= BMCR_ISOLATE;
+
+	ret = phy_modify(phydev, MII_BMCR, mask, res);
 	if (ret < 0)
 		return ret;
 
@@ -2912,6 +2955,12 @@ int genphy_loopback(struct phy_device *phydev, bool enable)
 		u16 ctl = BMCR_LOOPBACK;
 		int ret, val;
 
+		/* Isolating and looping-back the MII interface doesn't really
+		 * make sense
+		 */
+		if (phydev->isolated)
+			return -EINVAL;
+
 		ctl |= mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);
 
 		phy_modify(phydev, MII_BMCR, ~0, ctl);
@@ -2924,6 +2973,8 @@ int genphy_loopback(struct phy_device *phydev, bool enable)
 	} else {
 		phy_modify(phydev, MII_BMCR, BMCR_LOOPBACK, 0);
 
+		genphy_isolate(phydev, phydev->isolated);
+
 		phy_config_aneg(phydev);
 	}
 
@@ -2931,6 +2982,19 @@ int genphy_loopback(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL(genphy_loopback);
 
+int genphy_isolate(struct phy_device *phydev, bool enable)
+{
+	u16 val = 0;
+
+	if (enable)
+		val = BMCR_ISOLATE;
+
+	phy_modify(phydev, MII_BMCR, BMCR_ISOLATE, val);
+
+	return 0;
+}
+EXPORT_SYMBOL(genphy_isolate);
+
 /**
  * phy_remove_link_mode - Remove a supported link mode
  * @phydev: phy_device structure to remove link mode from
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a98bc91a0cde..ae33919aa0f5 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -573,6 +573,7 @@ struct macsec_ops;
  * @mac_managed_pm: Set true if MAC driver takes of suspending/resuming PHY
  * @wol_enabled: Set to true if the PHY or the attached MAC have Wake-on-LAN
  * 		 enabled.
+ * @isolated: Set to true if the PHY's MII has been isolated.
  * @state: State of the PHY for management purposes
  * @dev_flags: Device-specific flags used by the PHY driver.
  *
@@ -676,6 +677,7 @@ struct phy_device {
 	unsigned is_on_sfp_module:1;
 	unsigned mac_managed_pm:1;
 	unsigned wol_enabled:1;
+	unsigned isolated:1;
 
 	unsigned autoneg:1;
 	/* The most recently read link state */
@@ -1781,6 +1783,7 @@ int phy_suspend(struct phy_device *phydev);
 int phy_resume(struct phy_device *phydev);
 int __phy_resume(struct phy_device *phydev);
 int phy_loopback(struct phy_device *phydev, bool enable);
+int phy_isolate(struct phy_device *phydev, bool enable);
 int phy_sfp_connect_phy(void *upstream, struct phy_device *phy);
 void phy_sfp_disconnect_phy(void *upstream, struct phy_device *phy);
 void phy_sfp_attach(void *upstream, struct sfp_bus *bus);
@@ -1894,6 +1897,7 @@ int genphy_read_master_slave(struct phy_device *phydev);
 int genphy_suspend(struct phy_device *phydev);
 int genphy_resume(struct phy_device *phydev);
 int genphy_loopback(struct phy_device *phydev, bool enable);
+int genphy_isolate(struct phy_device *phydev, bool enable);
 int genphy_soft_reset(struct phy_device *phydev);
 irqreturn_t genphy_handle_interrupt_no_ack(struct phy_device *phydev);
 
-- 
2.46.0

[PATCH net-next 2/7] net: phy: Allow flagging PHY devices that can't isolate their MII

[Maxime Chevallier]

Some PHYs have malfunctionning isolation modes, where the MII lines
aren't correctly set in high-impedance, potentially interfering with the
MII bus in unexpected ways. Some other PHYs simply don't support it.

Allow flagging these PHYs, and prevent isolating them altogether.

Assume the PHY can isolate by default.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy_device.c | 11 +++++++++++
 include/linux/phy.h          |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c468e72bef4b..2a3db1043626 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2127,6 +2127,14 @@ int phy_loopback(struct phy_device *phydev, bool enable)
 }
 EXPORT_SYMBOL(phy_loopback);
 
+static bool phy_can_isolate(struct phy_device *phydev)
+{
+	if (phydev->drv)
+		return !(phydev->drv->flags & PHY_NO_ISOLATE);
+
+	return true;
+}
+
 int phy_isolate(struct phy_device *phydev, bool enable)
 {
 	int ret = 0;
@@ -2134,6 +2142,9 @@ int phy_isolate(struct phy_device *phydev, bool enable)
 	if (!phydev->drv)
 		return -EIO;
 
+	if (!phy_can_isolate(phydev))
+		return -EOPNOTSUPP;
+
 	mutex_lock(&phydev->lock);
 
 	if (enable && phydev->isolated) {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ae33919aa0f5..f0a8a5459fbe 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -90,6 +90,7 @@ extern const int phy_10gbit_features_array[1];
 #define PHY_RST_AFTER_CLK_EN	0x00000002
 #define PHY_POLL_CABLE_TEST	0x00000004
 #define PHY_ALWAYS_CALL_SUSPEND	0x00000008
+#define PHY_NO_ISOLATE		0x00000010
 #define MDIO_DEVICE_IS_PHY	0x80000000
 
 /**
-- 
2.46.0

[PATCH net-next 3/7] net: phy: lxt: Mark LXT973 PHYs as having a broken isolate mode

[Maxime Chevallier]

Testing showed that PHYs from the LXT973 family have a non-working
isolate mode, where the MII lines aren't set in high-impedance as would
be expected. Prevent isolating these PHYs.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/lxt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index e3bf827b7959..55cf67391533 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -334,6 +334,7 @@ static struct phy_driver lxt97x_driver[] = {
 	.read_status	= lxt973a2_read_status,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
+	.flags		= PHY_NO_ISOLATE,
 }, {
 	.phy_id		= 0x00137a10,
 	.name		= "LXT973",
@@ -344,6 +345,7 @@ static struct phy_driver lxt97x_driver[] = {
 	.config_aneg	= lxt973_config_aneg,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
+	.flags		= PHY_NO_ISOLATE,
 } };
 
 module_phy_driver(lxt97x_driver);
-- 
2.46.0

[PATCH net-next 4/7] net: phy: marvell10g: 88x3310 and 88x3340 don't support isolate mode

[Maxime Chevallier]

The 88x3310 and 88x3340 PHYs don't support the isolation mode,
regardless of the MII mode being used
(SGMII/1000BaseX/2500BaseX/5GBaseR/10GBaseR). Flag this in the driver.

This was confirmed on the 88x3310 on a MacchiatoBin.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/marvell10g.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 6642eb642d4b..867fd1451b3f 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -1422,6 +1422,7 @@ static struct phy_driver mv3310_drivers[] = {
 		.set_loopback	= genphy_c45_loopback,
 		.get_wol	= mv3110_get_wol,
 		.set_wol	= mv3110_set_wol,
+		.flags		= PHY_NO_ISOLATE,
 	},
 	{
 		.phy_id		= MARVELL_PHY_ID_88X3310,
@@ -1441,6 +1442,7 @@ static struct phy_driver mv3310_drivers[] = {
 		.set_tunable	= mv3310_set_tunable,
 		.remove		= mv3310_remove,
 		.set_loopback	= genphy_c45_loopback,
+		.flags		= PHY_NO_ISOLATE,
 	},
 	{
 		.phy_id		= MARVELL_PHY_ID_88E2110,
-- 
2.46.0

[PATCH net-next 5/7] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier]

Expose phy-specific configuration hooks to get and set the state of an
ethernet PHY's internal configuration.

So far, these parameters include the loopback state of the PHY
(host-side loopback) and the isolation state of the PHY.

The .get_config() ethtool_phy_ops gets these status information from the
phy_device's internal flags, while the .set_config() operation allows
changing these configuration parameters.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/phy.c        | 59 ++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c |  2 ++
 include/linux/ethtool.h      |  8 +++++
 include/linux/phy.h          | 21 +++++++++++++
 4 files changed, 90 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 4f3e742907cb..0cdb0fc30727 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1811,3 +1811,62 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
 	return ret;
 }
 EXPORT_SYMBOL(phy_ethtool_nway_reset);
+
+/**
+ * phy_get_config - Get PHY configuration parameters
+ * @phydev: the PHY device to act upon
+ * @phy_cfg:  The configuration to apply
+ */
+
+int phy_get_config(struct phy_device *phydev,
+		   struct phy_device_config *phy_cfg)
+{
+	mutex_lock(&phydev->lock);
+	phy_cfg->isolate = phydev->isolated;
+	phy_cfg->loopback = phydev->loopback_enabled;
+	mutex_unlock(&phydev->lock);
+
+	return 0;
+}
+
+/**
+ * phy_set_config - Set PHY configuration parameters
+ * @phydev: the PHY device to act upon
+ * @phy_cfg: the configuration to apply
+ * @extack: a netlink extack for useful error reporting
+ */
+
+int phy_set_config(struct phy_device *phydev,
+		   const struct phy_device_config *phy_cfg,
+		   struct netlink_ext_ack *extack)
+{
+	bool loopback_change, isolate_change;
+	int ret;
+
+	/* As the phydev's loopback and isolation state are protected by the
+	 * phy lock, check first if we'll need to perform the action,
+	 * then do them as a second step.
+	 */
+	mutex_lock(&phydev->lock);
+	isolate_change = (phy_cfg->isolate != phydev->isolated);
+	loopback_change = (phy_cfg->loopback != phydev->loopback_enabled);
+	mutex_unlock(&phydev->lock);
+
+	if (isolate_change) {
+		ret = phy_isolate(phydev, phy_cfg->isolate);
+		if (ret) {
+			NL_SET_ERR_MSG(extack, "Error while configuring PHY isolation");
+			return ret;
+		}
+	}
+
+	if (loopback_change) {
+		ret = phy_loopback(phydev, phy_cfg->loopback);
+		if (ret) {
+			NL_SET_ERR_MSG(extack, "Error while configuring PHY loopback");
+			return ret;
+		}
+	}
+
+	return 0;
+}
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 2a3db1043626..0714a2b83d18 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3845,6 +3845,8 @@ static const struct ethtool_phy_ops phy_ethtool_phy_ops = {
 	.get_plca_status	= phy_ethtool_get_plca_status,
 	.start_cable_test	= phy_start_cable_test,
 	.start_cable_test_tdr	= phy_start_cable_test_tdr,
+	.get_config		= phy_get_config,
+	.set_config		= phy_set_config,
 };
 
 static const struct phylib_stubs __phylib_stubs = {
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 12f6dc567598..480ee99a69a5 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1140,6 +1140,7 @@ struct phy_device;
 struct phy_tdr_config;
 struct phy_plca_cfg;
 struct phy_plca_status;
+struct phy_device_config;
 
 /**
  * struct ethtool_phy_ops - Optional PHY device options
@@ -1151,6 +1152,8 @@ struct phy_plca_status;
  * @get_plca_status: Get PLCA configuration.
  * @start_cable_test: Start a cable test
  * @start_cable_test_tdr: Start a Time Domain Reflectometry cable test
+ * @get_config: Retrieve phy device configuration parameters
+ * @set_config: Set phy device configuration parameters
  *
  * All operations are optional (i.e. the function pointer may be set to %NULL)
  * and callers must take this into account. Callers must hold the RTNL lock.
@@ -1172,6 +1175,11 @@ struct ethtool_phy_ops {
 	int (*start_cable_test_tdr)(struct phy_device *phydev,
 				    struct netlink_ext_ack *extack,
 				    const struct phy_tdr_config *config);
+	int (*get_config)(struct phy_device *phydev,
+			  struct phy_device_config *phy_cfg);
+	int (*set_config)(struct phy_device *phydev,
+			  const struct phy_device_config *phy_cfg,
+			  struct netlink_ext_ack *extack);
 };
 
 /**
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f0a8a5459fbe..ee0364d2afc3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -887,6 +887,22 @@ enum phy_led_modes {
 	__PHY_LED_MODES_NUM,
 };
 
+/**
+ * struct phy_device_config - General PHY device configuration parameters for
+ * status reporting and bulk configuration
+ *
+ * A structure containing generic PHY device information, allowing to expose
+ * internal status to userspace, and perform PHY configuration in a controlled
+ * manner.
+ *
+ * @isolate: The MII-side isolation status of the PHY
+ * @loopback: The loopback state of the PHY
+ */
+struct phy_device_config {
+	bool isolate;
+	bool loopback;
+};
+
 /**
  * struct phy_led: An LED driven by the PHY
  *
@@ -2067,6 +2083,11 @@ int phy_ethtool_set_plca_cfg(struct phy_device *phydev,
 			     struct netlink_ext_ack *extack);
 int phy_ethtool_get_plca_status(struct phy_device *phydev,
 				struct phy_plca_status *plca_st);
+int phy_get_config(struct phy_device *phydev,
+		   struct phy_device_config *phy_cfg);
+int phy_set_config(struct phy_device *phydev,
+		   const struct phy_device_config *phy_cfg,
+		   struct netlink_ext_ack *extack);
 
 int __phy_hwtstamp_get(struct phy_device *phydev,
 		       struct kernel_hwtstamp_config *config);
-- 
2.46.0

[PATCH net-next 6/7] net: ethtool: phy: allow reporting and setting the phy isolate status

[Maxime Chevallier]

Add the isolate and loopback status information to the ETHTOOL_PHY_GET
netlink command attributes, and allow changing these parameters from a
newly-introduced ETHTOOL_PHY_SET command.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/networking/ethtool-netlink.rst |  2 +
 include/uapi/linux/ethtool_netlink.h         |  3 +
 net/ethtool/netlink.c                        |  8 +++
 net/ethtool/netlink.h                        |  1 +
 net/ethtool/phy.c                            | 75 ++++++++++++++++++++
 5 files changed, 89 insertions(+)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index ba90457b8b2d..bbd4ca8b9dbd 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -2234,6 +2234,8 @@ Kernel response contents:
                                                 bus, the name of this sfp bus
   ``ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME`` string  if the phy controls an sfp bus,
                                                 the name of the sfp bus
+  ``ETHTOOL_A_PHY_ISOLATE``             u8      The PHY Isolate status
+  ``ETHTOOL_A_PHY_LOOPBACK``            u8      The PHY Loopback status
   ===================================== ======  ===============================
 
 When ``ETHTOOL_A_PHY_UPSTREAM_TYPE`` is PHY_UPSTREAM_PHY, the PHY's parent is
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 283305f6b063..070565dcf497 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -59,6 +59,7 @@ enum {
 	ETHTOOL_MSG_MM_SET,
 	ETHTOOL_MSG_MODULE_FW_FLASH_ACT,
 	ETHTOOL_MSG_PHY_GET,
+	ETHTOOL_MSG_PHY_SET,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -1079,6 +1080,8 @@ enum {
 	ETHTOOL_A_PHY_UPSTREAM_INDEX,		/* u32 */
 	ETHTOOL_A_PHY_UPSTREAM_SFP_NAME,	/* string */
 	ETHTOOL_A_PHY_DOWNSTREAM_SFP_NAME,	/* string */
+	ETHTOOL_A_PHY_ISOLATE,			/* u8 */
+	ETHTOOL_A_PHY_LOOPBACK,			/* u8 */
 
 	/* add new constants above here */
 	__ETHTOOL_A_PHY_CNT,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index e3f0ef6b851b..26982f47a934 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -394,6 +394,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_PLCA_GET_STATUS]	= &ethnl_plca_status_request_ops,
 	[ETHTOOL_MSG_MM_GET]		= &ethnl_mm_request_ops,
 	[ETHTOOL_MSG_MM_SET]		= &ethnl_mm_request_ops,
+	[ETHTOOL_MSG_PHY_SET]		= &ethnl_phy_request_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -1243,6 +1244,13 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.policy = ethnl_phy_get_policy,
 		.maxattr = ARRAY_SIZE(ethnl_phy_get_policy) - 1,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_PHY_SET,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_default_set_doit,
+		.policy = ethnl_phy_set_policy,
+		.maxattr = ARRAY_SIZE(ethnl_phy_set_policy) - 1,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 203b08eb6c6f..7ae73e2eab32 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -485,6 +485,7 @@ extern const struct nla_policy ethnl_mm_get_policy[ETHTOOL_A_MM_HEADER + 1];
 extern const struct nla_policy ethnl_mm_set_policy[ETHTOOL_A_MM_MAX + 1];
 extern const struct nla_policy ethnl_module_fw_flash_act_policy[ETHTOOL_A_MODULE_FW_FLASH_PASSWORD + 1];
 extern const struct nla_policy ethnl_phy_get_policy[ETHTOOL_A_PHY_HEADER + 1];
+extern const struct nla_policy ethnl_phy_set_policy[ETHTOOL_A_PHY_MAX + 1];
 
 int ethnl_set_features(struct sk_buff *skb, struct genl_info *info);
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
index 99d2a8b6144c..cc1dc45a6264 100644
--- a/net/ethtool/phy.c
+++ b/net/ethtool/phy.c
@@ -30,10 +30,13 @@ ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
 	struct phy_req_info *req_info = PHY_REQINFO(req_base);
 	struct phy_device_node *pdn = req_info->pdn;
 	struct phy_device *phydev = pdn->phy;
+	const struct ethtool_phy_ops *ops;
 	size_t size = 0;
 
 	ASSERT_RTNL();
 
+	ops = ethtool_phy_ops;
+
 	/* ETHTOOL_A_PHY_INDEX */
 	size += nla_total_size(sizeof(u32));
 
@@ -66,6 +69,14 @@ ethnl_phy_reply_size(const struct ethnl_req_info *req_base,
 			size += nla_total_size(strlen(sfp_name) + 1);
 	}
 
+	if (ops && ops->get_config) {
+		/* ETHTOOL_A_PHY_ISOLATE */
+		size += nla_total_size(sizeof(u8));
+
+		/* ETHTOOL_A_PHY_LOOPBACK */
+		size += nla_total_size(sizeof(u8));
+	}
+
 	return size;
 }
 
@@ -75,10 +86,20 @@ ethnl_phy_fill_reply(const struct ethnl_req_info *req_base, struct sk_buff *skb)
 	struct phy_req_info *req_info = PHY_REQINFO(req_base);
 	struct phy_device_node *pdn = req_info->pdn;
 	struct phy_device *phydev = pdn->phy;
+	const struct ethtool_phy_ops *ops;
+	struct phy_device_config cfg;
 	enum phy_upstream ptype;
+	int ret;
 
 	ptype = pdn->upstream_type;
 
+	ops = ethtool_phy_ops;
+	if (ops && ops->get_config) {
+		ret = ops->get_config(phydev, &cfg);
+		if (ret)
+			return ret;
+	}
+
 	if (nla_put_u32(skb, ETHTOOL_A_PHY_INDEX, phydev->phyindex) ||
 	    nla_put_string(skb, ETHTOOL_A_PHY_NAME, dev_name(&phydev->mdio.dev)) ||
 	    nla_put_u32(skb, ETHTOOL_A_PHY_UPSTREAM_TYPE, ptype))
@@ -114,6 +135,14 @@ ethnl_phy_fill_reply(const struct ethnl_req_info *req_base, struct sk_buff *skb)
 			return -EMSGSIZE;
 	}
 
+	/* Append PHY configuration, if possible */
+	if (!ops || !ops->get_config)
+		return 0;
+
+	if (nla_put_u8(skb, ETHTOOL_A_PHY_ISOLATE, cfg.isolate) ||
+	    nla_put_u8(skb, ETHTOOL_A_PHY_LOOPBACK, cfg.loopback))
+		return -EMSGSIZE;
+
 	return 0;
 }
 
@@ -311,3 +340,49 @@ int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 
 	return ret;
 }
+
+const struct nla_policy ethnl_phy_set_policy[] = {
+	[ETHTOOL_A_PHY_HEADER]		=
+		NLA_POLICY_NESTED(ethnl_header_policy_phy),
+	[ETHTOOL_A_PHY_ISOLATE]		= NLA_POLICY_MAX(NLA_U8, 1),
+	[ETHTOOL_A_PHY_LOOPBACK]	= NLA_POLICY_MAX(NLA_U8, 1),
+};
+
+static int ethnl_set_phy(struct ethnl_req_info *req_info, struct genl_info *info)
+{
+	struct netlink_ext_ack *extack = info->extack;
+	const struct ethtool_phy_ops *ops;
+	struct nlattr **tb = info->attrs;
+	struct phy_device_config cfg;
+	struct phy_device *phydev;
+	bool mod = false;
+	int ret;
+
+	ops = ethtool_phy_ops;
+	if (!ops || !ops->set_config || !ops->get_config)
+		return -EOPNOTSUPP;
+
+	/* We're running under rtnl */
+	phydev = ethnl_req_get_phydev(req_info, tb[ETHTOOL_A_PHY_HEADER],
+				      extack);
+	if (IS_ERR_OR_NULL(phydev))
+		return -ENODEV;
+
+	ret = ops->get_config(phydev, &cfg);
+	if (ret)
+		return ret;
+
+	ethnl_update_bool(&cfg.isolate, tb[ETHTOOL_A_PHY_ISOLATE], &mod);
+	ethnl_update_bool(&cfg.loopback, tb[ETHTOOL_A_PHY_LOOPBACK], &mod);
+
+	if (!mod)
+		return 0;
+
+	/* Returning 0 is fine as we don't have a notification */
+	return ops->set_config(phydev, &cfg, extack);
+}
+
+const struct ethnl_request_ops ethnl_phy_request_ops = {
+	.hdr_attr		= ETHTOOL_A_PHY_HEADER,
+	.set			= ethnl_set_phy,
+};
-- 
2.46.0

[PATCH net-next 7/7] netlink: specs: introduce phy-set command along with configurable attributes

[Maxime Chevallier]

Update the ethnl specification to include the newly introduced loopback
and isolated attributes, and describe the newly introduced
ETHTOOL_PHY_SET command.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 Documentation/netlink/specs/ethtool.yaml | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/netlink/specs/ethtool.yaml b/Documentation/netlink/specs/ethtool.yaml
index 6a050d755b9c..f17ddd8783f7 100644
--- a/Documentation/netlink/specs/ethtool.yaml
+++ b/Documentation/netlink/specs/ethtool.yaml
@@ -1132,6 +1132,12 @@ attribute-sets:
       -
         name: downstream-sfp-name
         type: string
+      -
+        name: isolate
+        type: u8
+      -
+        name: loopback
+        type: u8
 
 operations:
   enum-model: directional
@@ -1950,4 +1956,18 @@ operations:
             - upstream-index
             - upstream-sfp-name
             - downstream-sfp-name
+            - isolate
+            - loopback
       dump: *phy-get-op
+    -
+      name: phy-set
+      doc: Set configuration attributes for attached PHY devices
+
+      attribute-set: phy
+
+      do:
+        request:
+          attributes:
+            - header
+            - isolate
+            - loopback
-- 
2.46.0

Re: [PATCH net-next 5/7] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Oleksij Rempel]

Hi Maxime,

On Wed, Sep 11, 2024 at 11:27:09PM +0200, Maxime Chevallier wrote:
...
>  
> +/**
> + * struct phy_device_config - General PHY device configuration parameters for
> + * status reporting and bulk configuration
> + *
> + * A structure containing generic PHY device information, allowing to expose
> + * internal status to userspace, and perform PHY configuration in a controlled
> + * manner.
> + *
> + * @isolate: The MII-side isolation status of the PHY
> + * @loopback: The loopback state of the PHY
> + */
> +struct phy_device_config {
> +	bool isolate;
> +	bool loopback;
> +};
 
I would recommend to have loopback enum. There are different levels of
loopback:
https://www.ti.com/document-viewer/DP83TD510E/datasheet#GUID-50834313-DEF1-42FB-BA00-9B0902B2D7E4/TITLE-SNLS656SNLS5055224

I imagine something like this:

/*
 * enum phy_loopback_mode - PHY loopback modes
 * These modes represent different loopback configurations to
 * facilitate in-circuit testing of the PHY's digital and analog paths.
 */
enum phy_loopback_mode {
	PHY_LOOPBACK_NONE = 0,		/* No loopback mode enabled */
	PHY_LOOPBACK_MII,		/* MII Loopback: MAC to PHY internal loopback */
	PHY_LOOPBACK_PCS,		/* PCS Loopback: PCS layer loopback, no signal processing */
	PHY_LOOPBACK_DIGITAL,		/* Digital Loopback: Loops back entire digital TX/RX path */
	PHY_LOOPBACK_ANALOG,		/* Analog Loopback: Loops back after analog front-end */
	PHY_LOOPBACK_FAR_END		/* Far-End (Reverse) Loopback: Receiver to MAC interface loopback */
};

At same time, one interface will have multiple loopback providers, except of
multiple PHYs, MAC will provide it too.

I assume, we need a bit field per component to reflect supported loopback modes.

If you have time, please take a look at net/core/selftests.c this will be
one of consumers which should walk over different loopback levels to find the
location of potential problem.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next 0/7] Allow controlling PHY loopback and isolate modes

[Maxime Chevallier]

On Wed, 11 Sep 2024 23:27:04 +0200
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> Hello everyone,
> 
> This series brings support for controlling the isolation and loopback
> modes for PHY devices, through a netlink interface.
> 
> The isolation support work is made in preparation for the support of
> interfaces that posesses multiple PHYs attached to the same MAC, on the
> same MII bus. In this configuration, the isolation mode for the PHY is
> used to avoid interferences on the MII bus, which doesn't support
> multidrop topologies.
> 
> This mode is part of the 802.3 spec, however rarely used. It was
> discovered that some PHYs don't implement that mode correctly, and will
> continue being active on the MII interface when isolated. This series
> supports that case, and flags the LXT973 as having such a broken
> isolation mode. The Marvell 88x3310/3340 PHYs also don't support this
> mdoe, and are also flagged accordingly.
> 
> The main part needed for the upcomping multi-PHY support really is the
> internal kernel API to support this.
> 
> The second part of the series (patches 5, 6 and 7) focus on allowing
> userspace to control that mode. The only real benefit of controlling this
> from userspace is to make it easier to find out if this mode really
> works or not on the PHY being used.
> 
> This relies on a new set of ethtool_phy_ops, set_config and get_config,
> to toggle these modes.
> 
> The loopback control from that API is added as it fits the API
> well, and having the ability to easily set the PHY in MII-loopback
> mode is a helpful tool to have when bringing-up a new device and
> troubleshooting the link setup.
> 
> The netlink API is an extension of the existing PHY_GET, reporting 2 new
> attributes (one for isolate, one for loopback). A PHY_SET command is
> introduced as well, allowing to configure the loopback and isolation.

One thing I forgot to mention is that the phy-tunable API could also
possibly be a place to set these parameters instead of this new command.

Maybe this would be the preferred way ?

Thanks,

Maxime

Re: [PATCH net-next 5/7] net: phy: introduce ethtool_phy_ops to get and set phy configuration

[Maxime Chevallier]

Hello Oleksij,

On Thu, 12 Sep 2024 06:46:29 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Hi Maxime,
> 
> On Wed, Sep 11, 2024 at 11:27:09PM +0200, Maxime Chevallier wrote:
> ...
> >  
> > +/**
> > + * struct phy_device_config - General PHY device configuration parameters for
> > + * status reporting and bulk configuration
> > + *
> > + * A structure containing generic PHY device information, allowing to expose
> > + * internal status to userspace, and perform PHY configuration in a controlled
> > + * manner.
> > + *
> > + * @isolate: The MII-side isolation status of the PHY
> > + * @loopback: The loopback state of the PHY
> > + */
> > +struct phy_device_config {
> > +	bool isolate;
> > +	bool loopback;
> > +};  
>  
> I would recommend to have loopback enum. There are different levels of
> loopback:
> https://www.ti.com/document-viewer/DP83TD510E/datasheet#GUID-50834313-DEF1-42FB-BA00-9B0902B2D7E4/TITLE-SNLS656SNLS5055224
> 
> I imagine something like this:
> 
> /*
>  * enum phy_loopback_mode - PHY loopback modes
>  * These modes represent different loopback configurations to
>  * facilitate in-circuit testing of the PHY's digital and analog paths.
>  */
> enum phy_loopback_mode {
> 	PHY_LOOPBACK_NONE = 0,		/* No loopback mode enabled */
> 	PHY_LOOPBACK_MII,		/* MII Loopback: MAC to PHY internal loopback */
> 	PHY_LOOPBACK_PCS,		/* PCS Loopback: PCS layer loopback, no signal processing */
> 	PHY_LOOPBACK_DIGITAL,		/* Digital Loopback: Loops back entire digital TX/RX path */
> 	PHY_LOOPBACK_ANALOG,		/* Analog Loopback: Loops back after analog front-end */
> 	PHY_LOOPBACK_FAR_END		/* Far-End (Reverse) Loopback: Receiver to MAC interface loopback */
> };

I agree with you on that, having the ability to fine-tune where the
loopback happens is really useful for debug. The main problem I would
see is to come-up with a set of modes that are somewhat generic, as
vendors implement a wide variety of loopback modes.

For example, on BaseT4 PHYs the Analog loopback doesn't exist and is
more akin to using a loopback stub, whereas the Digital loopback would
be a loopback at the PMD level (I don't know if that even exists).

That being said, the list of possible places to setup the loopback
within a PHY is finite and it's conceivable to come-up with a set of
loopback modes.

Thanks a lot for the feedback,

Maxime

Re: [PATCH net-next 3/7] net: phy: lxt: Mark LXT973 PHYs as having a broken isolate mode

[Simon Horman]

On Wed, Sep 11, 2024 at 11:27:07PM +0200, Maxime Chevallier wrote:
> Testing showed that PHYs from the LXT973 family have a non-working
> isolate mode, where the MII lines aren't set in high-impedance as would
> be expected. Prevent isolating these PHYs.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/net/phy/lxt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
> index e3bf827b7959..55cf67391533 100644
> --- a/drivers/net/phy/lxt.c
> +++ b/drivers/net/phy/lxt.c
> @@ -334,6 +334,7 @@ static struct phy_driver lxt97x_driver[] = {
>  	.read_status	= lxt973a2_read_status,
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
> +	.flags		= PHY_NO_ISOLATE,
>  }, {
>  	.phy_id		= 0x00137a10,
>  	.name		= "LXT973",
> @@ -344,6 +345,7 @@ static struct phy_driver lxt97x_driver[] = {
>  	.config_aneg	= lxt973_config_aneg,
>  	.suspend	= genphy_suspend,
>  	.resume		= genphy_resume,
> +	.flags		= PHY_NO_ISOLATE,
>  } };

Hi Maxime,

This duplicates setting .flags for each array member
updated by this patch.

>  
>  module_phy_driver(lxt97x_driver);

-- 
pw-bot: cr

Re: [PATCH net-next 3/7] net: phy: lxt: Mark LXT973 PHYs as having a broken isolate mode

[Maxime Chevallier]

Hello Simon,

On Thu, 12 Sep 2024 13:24:51 +0100
Simon Horman <horms@kernel.org> wrote:

> On Wed, Sep 11, 2024 at 11:27:07PM +0200, Maxime Chevallier wrote:
> > Testing showed that PHYs from the LXT973 family have a non-working
> > isolate mode, where the MII lines aren't set in high-impedance as would
> > be expected. Prevent isolating these PHYs.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > ---
> >  drivers/net/phy/lxt.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
> > index e3bf827b7959..55cf67391533 100644
> > --- a/drivers/net/phy/lxt.c
> > +++ b/drivers/net/phy/lxt.c
> > @@ -334,6 +334,7 @@ static struct phy_driver lxt97x_driver[] = {
> >  	.read_status	= lxt973a2_read_status,
> >  	.suspend	= genphy_suspend,
> >  	.resume		= genphy_resume,
> > +	.flags		= PHY_NO_ISOLATE,
> >  }, {
> >  	.phy_id		= 0x00137a10,
> >  	.name		= "LXT973",
> > @@ -344,6 +345,7 @@ static struct phy_driver lxt97x_driver[] = {
> >  	.config_aneg	= lxt973_config_aneg,
> >  	.suspend	= genphy_suspend,
> >  	.resume		= genphy_resume,
> > +	.flags		= PHY_NO_ISOLATE,
> >  } };  
> 
> Hi Maxime,
> 
> This duplicates setting .flags for each array member
> updated by this patch.

Arg yes you're correct... Don't know how I missed that...

Thanks !

Maxime

Re: [PATCH net-next 0/7] Allow controlling PHY loopback and isolate modes

[Andrew Lunn]

> The loopback control from that API is added as it fits the API
> well, and having the ability to easily set the PHY in MII-loopback
> mode is a helpful tool to have when bringing-up a new device and
> troubleshooting the link setup.

We might want to take a step back and think about loopback some more.

Loopback can be done at a number of points in the device(s). Some
Marvell PHYs can do loopback in the PHY PCS layer. Some devices also
support loopback in the PHY SERDES layer. I've not seen it for Marvell
devices, but maybe some PHYs allow loopback much closer to the line?
And i expect some MAC PCS allow loopback.

So when talking about loopback, we might also want to include the
concept of where the loopback occurs, and maybe it needs to be a NIC
wide concept, not a PHY concept?

	Andrew

Re: [PATCH net-next 0/7] Allow controlling PHY loopback and isolate modes

[Florian Fainelli]

On 9/12/24 11:21, Andrew Lunn wrote:
>> The loopback control from that API is added as it fits the API
>> well, and having the ability to easily set the PHY in MII-loopback
>> mode is a helpful tool to have when bringing-up a new device and
>> troubleshooting the link setup.
> 
> We might want to take a step back and think about loopback some more.
> 
> Loopback can be done at a number of points in the device(s). Some
> Marvell PHYs can do loopback in the PHY PCS layer. Some devices also
> support loopback in the PHY SERDES layer. I've not seen it for Marvell
> devices, but maybe some PHYs allow loopback much closer to the line?
> And i expect some MAC PCS allow loopback.
> 
> So when talking about loopback, we might also want to include the
> concept of where the loopback occurs, and maybe it needs to be a NIC
> wide concept, not a PHY concept?

Agreed, you can loop pretty much anywhere in the data path, assuming the 
hardware allows it. For the hardware I maintain, we can loop back within 
the MAC as close as possible from the interface to DRAM, or as "far" as 
possible, within the MII signals, but without actually involving the PHY.

Similarly, the PHY can loop as close as possible from the electrical 
data lines, or as far as possible by looping the *MII pins, before 
hitting the MAC.

So if nothing else, we have at least 4 kinds of loopbacks that could be 
supported, it is not clear whether we want to define all of those as 
standardized "modes" within Linux, and let drivers implement the ones 
they can, or if we just let drivers implement the mode they have, and 
advertise those. Meaning your user experience could vary.
-- 
Florian

Re: [PATCH net-next 1/7] net: phy: allow isolating PHY devices

[Florian Fainelli]

On 9/11/24 14:27, Maxime Chevallier wrote:
> The 802.3 specifications describes the isolation mode as setting the
> PHY's MII interface in high-impedance mode, thus isolating the PHY from
> that bus. This effectively breaks the link between the MAC and the PHY,
> but without necessarily disrupting the link between the PHY and the LP.
> 
> This mode can be useful for testing purposes, but also when there are
> multiple PHYs on the same MII bus (a case that the 802.3 specification
> refers to).
> 
> In Isolation mode, the PHY will still continue to respond to MDIO
> commands.
> 
> Introduce a helper to set the phy in an isolated mode.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Not sure where that comment belongs so I will put it here, one thing 
that concerns me is if you have hardware that is not strapped to be 
isolated by default, and the PHY retains the state configured by Linux, 
such that the PHY is in isolation mode. A boot loader that is not 
properly taking the PHY out of isolation mode would be unavailable to 
use it and that would be a bug that Linux would likely be on the hook to 
fix.

Would recommend adding a phy_shutdown() method which is called upon 
reboot/kexec and which, based upon a quirk/flag can ensure that the 
isolation bit is cleared.
-- 
Florian

Re: [PATCH net-next 3/7] net: phy: lxt: Mark LXT973 PHYs as having a broken isolate mode

[kernel test robot]

Hi Maxime,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Maxime-Chevallier/net-phy-allow-isolating-PHY-devices/20240912-053106
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240911212713.2178943-4-maxime.chevallier%40bootlin.com
patch subject: [PATCH net-next 3/7] net: phy: lxt: Mark LXT973 PHYs as having a broken isolate mode
config: x86_64-randconfig-121-20240913 (https://download.01.org/0day-ci/archive/20240913/202409131315.SEzdGTvD-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240913/202409131315.SEzdGTvD-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/202409131315.SEzdGTvD-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/phy/lxt.c:331:10: sparse: sparse: Initializer entry defined twice
   drivers/net/phy/lxt.c:337:10: sparse:   also defined here
   drivers/net/phy/lxt.c:343:10: sparse: sparse: Initializer entry defined twice
   drivers/net/phy/lxt.c:348:10: sparse:   also defined here

vim +331 drivers/net/phy/lxt.c

e13647c158307f Richard Cochran      2010-06-07  307  
d5bf9071e71a4d Christian Hohnstaedt 2012-07-04  308  static struct phy_driver lxt97x_driver[] = {
d5bf9071e71a4d Christian Hohnstaedt 2012-07-04  309  {
600991b003039b Uwe Zeisberger       2006-06-25  310  	.phy_id		= 0x78100000,
00db8189d984d6 Andy Fleming         2005-07-30  311  	.name		= "LXT970",
600991b003039b Uwe Zeisberger       2006-06-25  312  	.phy_id_mask	= 0xfffffff0,
dcdecdcfe1fc39 Heiner Kallweit      2019-04-12  313  	/* PHY_BASIC_FEATURES */
00db8189d984d6 Andy Fleming         2005-07-30  314  	.config_init	= lxt970_config_init,
00db8189d984d6 Andy Fleming         2005-07-30  315  	.config_intr	= lxt970_config_intr,
01c4a00bf34797 Ioana Ciornei        2020-11-13  316  	.handle_interrupt = lxt970_handle_interrupt,
d5bf9071e71a4d Christian Hohnstaedt 2012-07-04  317  }, {
600991b003039b Uwe Zeisberger       2006-06-25  318  	.phy_id		= 0x001378e0,
00db8189d984d6 Andy Fleming         2005-07-30  319  	.name		= "LXT971",
600991b003039b Uwe Zeisberger       2006-06-25  320  	.phy_id_mask	= 0xfffffff0,
dcdecdcfe1fc39 Heiner Kallweit      2019-04-12  321  	/* PHY_BASIC_FEATURES */
00db8189d984d6 Andy Fleming         2005-07-30  322  	.config_intr	= lxt971_config_intr,
01c4a00bf34797 Ioana Ciornei        2020-11-13  323  	.handle_interrupt = lxt971_handle_interrupt,
5556fdb0c2ea3a Christophe Leroy     2019-05-23  324  	.suspend	= genphy_suspend,
5556fdb0c2ea3a Christophe Leroy     2019-05-23  325  	.resume		= genphy_resume,
871d1d6b59802a LEROY Christophe     2012-09-24  326  }, {
871d1d6b59802a LEROY Christophe     2012-09-24  327  	.phy_id		= 0x00137a10,
871d1d6b59802a LEROY Christophe     2012-09-24  328  	.name		= "LXT973-A2",
871d1d6b59802a LEROY Christophe     2012-09-24  329  	.phy_id_mask	= 0xffffffff,
dcdecdcfe1fc39 Heiner Kallweit      2019-04-12  330  	/* PHY_BASIC_FEATURES */
871d1d6b59802a LEROY Christophe     2012-09-24 @331  	.flags		= 0,
871d1d6b59802a LEROY Christophe     2012-09-24  332  	.probe		= lxt973_probe,
871d1d6b59802a LEROY Christophe     2012-09-24  333  	.config_aneg	= lxt973_config_aneg,
871d1d6b59802a LEROY Christophe     2012-09-24  334  	.read_status	= lxt973a2_read_status,
5556fdb0c2ea3a Christophe Leroy     2019-05-23  335  	.suspend	= genphy_suspend,
5556fdb0c2ea3a Christophe Leroy     2019-05-23  336  	.resume		= genphy_resume,
c9dd02714c2f91 Maxime Chevallier    2024-09-11  337  	.flags		= PHY_NO_ISOLATE,
d5bf9071e71a4d Christian Hohnstaedt 2012-07-04  338  }, {
e13647c158307f Richard Cochran      2010-06-07  339  	.phy_id		= 0x00137a10,
e13647c158307f Richard Cochran      2010-06-07  340  	.name		= "LXT973",
e13647c158307f Richard Cochran      2010-06-07  341  	.phy_id_mask	= 0xfffffff0,
dcdecdcfe1fc39 Heiner Kallweit      2019-04-12  342  	/* PHY_BASIC_FEATURES */
e13647c158307f Richard Cochran      2010-06-07  343  	.flags		= 0,
e13647c158307f Richard Cochran      2010-06-07  344  	.probe		= lxt973_probe,
e13647c158307f Richard Cochran      2010-06-07  345  	.config_aneg	= lxt973_config_aneg,
5556fdb0c2ea3a Christophe Leroy     2019-05-23  346  	.suspend	= genphy_suspend,
5556fdb0c2ea3a Christophe Leroy     2019-05-23  347  	.resume		= genphy_resume,
c9dd02714c2f91 Maxime Chevallier    2024-09-11  348  	.flags		= PHY_NO_ISOLATE,
d5bf9071e71a4d Christian Hohnstaedt 2012-07-04  349  } };
e13647c158307f Richard Cochran      2010-06-07  350  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 0/7] Allow controlling PHY loopback and isolate modes

[Maxime Chevallier]

Hello Andrew, Florian,

On Thu, 12 Sep 2024 11:26:41 -0700
Florian Fainelli <f.fainelli@gmail.com> wrote:

> On 9/12/24 11:21, Andrew Lunn wrote:
> >> The loopback control from that API is added as it fits the API
> >> well, and having the ability to easily set the PHY in MII-loopback
> >> mode is a helpful tool to have when bringing-up a new device and
> >> troubleshooting the link setup.  
> > 
> > We might want to take a step back and think about loopback some more.
> > 
> > Loopback can be done at a number of points in the device(s). Some
> > Marvell PHYs can do loopback in the PHY PCS layer. Some devices also
> > support loopback in the PHY SERDES layer. I've not seen it for Marvell
> > devices, but maybe some PHYs allow loopback much closer to the line?
> > And i expect some MAC PCS allow loopback.
> > 
> > So when talking about loopback, we might also want to include the
> > concept of where the loopback occurs, and maybe it needs to be a NIC
> > wide concept, not a PHY concept?  
> 
> Agreed, you can loop pretty much anywhere in the data path, assuming the 
> hardware allows it. For the hardware I maintain, we can loop back within 
> the MAC as close as possible from the interface to DRAM, or as "far" as 
> possible, within the MII signals, but without actually involving the PHY.
> 
> Similarly, the PHY can loop as close as possible from the electrical 
> data lines, or as far as possible by looping the *MII pins, before 
> hitting the MAC.
> 
> So if nothing else, we have at least 4 kinds of loopbacks that could be 
> supported, it is not clear whether we want to define all of those as 
> standardized "modes" within Linux, and let drivers implement the ones 
> they can, or if we just let drivers implement the mode they have, and 
> advertise those. Meaning your user experience could vary.

Oleksji identified some loopback modes in TI PHYs, the PHYs have access
to have even different sets of loopback modes / locations as well, to me
it's hard to come-up with a list of all the possible loopback locations
indeed.

However, I don't think it's inconceivable to come-up with a list - that
can be extented - of possible loopback spots.

Making the loopback a NIC concept would indeed make sense here, where we
would aggregate all possible loopback points within the NIC and PHY(s)
combined, and having ways for MAC/PHYS to enumerate their loopback
modes through a set of ethtoop ops.

With that being said, is it OK if I split the loopback part out of that
series ? From the comments, it looks like a complex-enough topic to be
covered on its own, and if we consider the loopback as a NIC feature,
then it doesn't really fit into the current work anymore.

I am however happy to continue discussing that topic. Using loopback
has proven to be most helpful several times for me when bringing-up
devices.

Thanks,

Maxime

Re: [PATCH net-next 1/7] net: phy: allow isolating PHY devices

[Maxime Chevallier]

Hello Florian,

On Thu, 12 Sep 2024 11:30:26 -0700
Florian Fainelli <f.fainelli@gmail.com> wrote:

> On 9/11/24 14:27, Maxime Chevallier wrote:
> > The 802.3 specifications describes the isolation mode as setting the
> > PHY's MII interface in high-impedance mode, thus isolating the PHY from
> > that bus. This effectively breaks the link between the MAC and the PHY,
> > but without necessarily disrupting the link between the PHY and the LP.
> > 
> > This mode can be useful for testing purposes, but also when there are
> > multiple PHYs on the same MII bus (a case that the 802.3 specification
> > refers to).
> > 
> > In Isolation mode, the PHY will still continue to respond to MDIO
> > commands.
> > 
> > Introduce a helper to set the phy in an isolated mode.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>  
> 
> Not sure where that comment belongs so I will put it here, one thing 
> that concerns me is if you have hardware that is not strapped to be 
> isolated by default, and the PHY retains the state configured by Linux, 
> such that the PHY is in isolation mode. A boot loader that is not 
> properly taking the PHY out of isolation mode would be unavailable to 
> use it and that would be a bug that Linux would likely be on the hook to 
> fix.
> 
> Would recommend adding a phy_shutdown() method which is called upon 
> reboot/kexec and which, based upon a quirk/flag can ensure that the 
> isolation bit is cleared.

Very good point. I can see the same problem occuring with loopback then
(if we use the 802.3 C22 PHY loopback bit).

I have such a patch ready actually, for the 2-PHYs-on-the-same-MAC
scenario, I will include it in the next iteration.

Thanks for your feedback,

Maxime

Re: [PATCH net-next 0/7] Allow controlling PHY loopback and isolate modes

[Andrew Lunn]

> With that being said, is it OK if I split the loopback part out of that
> series ? From the comments, it looks like a complex-enough topic to be
> covered on its own, and if we consider the loopback as a NIC feature,
> then it doesn't really fit into the current work anymore.
> 
> I am however happy to continue discussing that topic. Using loopback
> has proven to be most helpful several times for me when bringing-up
> devices.

I agree Loopback is a useful facility, and is something we should
support. But i see it as being a topic of its own. So please do split
it out of this patchset.

	Andrew