Skip to contentSkip to conversation

Support for PHY test modes

RFC In review Florian Fainelli 5 participants 26 messages Apr 28, 2018 – Aug 17, 2023

Subsystems (7)
Networking Network drivers Ethernet PHY Networking DSA +3 more subsystems
lore
Share & tools
lore

Florian Fainelli

author
#0 lore raw

cover Support for PHY test modes

Hi all,

This patch series adds support for specifying PHY test modes through ethtool and paves the ground for adding support for more complex test modes that might require data to be exchanged between user and kernel space.

As an example, patches are included to add support for the IEEE electrical test modes for 100BaseT2 and 1000BaseT. Those do not require data to be passed back and forth.

I believe the infrastructure to be usable enough to add support for other things like:

- cable diagnostics
- pattern generator/waveform generator with specific pattern being indicated for instance

Questions for Andrew, and others:

- there could be room for adding additional ETH_TEST_FL_* values in order to help determine how the test should be running
- some of these tests can be disruptive to connectivity, the minimum we could do is stop the PHY state machine and restart it when "normal" is used to exit those test modes

Comments welcome!

Example:

# ethtool --get-phy-tests gphy
PHY tests gphy:
     normal (Test data: No)
     100baseT2-tx-waveform (Test data: No)
     100baseT2-tx-jitter (Test data: No)
     100baseT2-tx-idle (Test data: No)
     1000baseT-tx-waveform (Test data: No)
     1000baseT-tx-jitter-master (Test data: No)
     1000baseT-tx-jitter-slave (Test data: No)
     1000BaseT-tx-distorsion (Test data: No)
# ethtool --set-phy-test gphy 100baseT2-tx-waveform
# [   65.262513] brcm-sf2 f0b00000.ethernet_switch gphy: Link is Down

Florian Fainelli (5):
net: phy: Pass stringset argument to ethtool operations
net: ethtool: Add UAPI for PHY test modes
net: ethtool: Add plumbing to get/set PHY test modes
net: phy: Add support for IEEE standard test modes
net: phy: broadcom: Add support for PHY test modes

 drivers/net/dsa/b53/b53_common.c |   4 +-
 drivers/net/phy/Kconfig          |   6 ++
 drivers/net/phy/Makefile         |   4 +-
 drivers/net/phy/bcm-phy-lib.c    |  21 ++++--
 drivers/net/phy/bcm-phy-lib.h    |   4 +-
 drivers/net/phy/bcm7xxx.c        |   9 ++-
 drivers/net/phy/broadcom.c       |   6 +-
 drivers/net/phy/marvell.c        |  11 ++-
 drivers/net/phy/micrel.c         |  11 ++-
 drivers/net/phy/phy-tests.c      | 159 +++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/smsc.c           |  10 ++-
 include/linux/phy.h              |  99 +++++++++++++++++++++---
 include/net/dsa.h                |   4 +-
 include/uapi/linux/ethtool.h     |  23 ++++++
 net/core/ethtool.c               |  86 +++++++++++++++++++--
 net/dsa/master.c                 |   9 ++-
 net/dsa/port.c                   |   8 +-
 17 files changed, 427 insertions(+), 47 deletions(-)
 create mode 100644 drivers/net/phy/phy-tests.c
Signature · 1 line
2.14.1
author
#1 lore raw

RFC 1/5 net: phy: Pass stringset argument to ethtool operations

In preparation for returning a different type of strings other than ETH_SS_STATS update the PHY drivers, helpers and consumers of these functions.

Signed-off-by Florian Fainelli <[email protected]>
---
 drivers/net/dsa/b53/b53_common.c |  4 ++--
 drivers/net/phy/bcm-phy-lib.c    | 12 +++++++++---
 drivers/net/phy/bcm-phy-lib.h    |  4 ++--
 drivers/net/phy/bcm7xxx.c        |  6 ++++--
 drivers/net/phy/broadcom.c       |  6 ++++--
 drivers/net/phy/marvell.c        | 11 +++++++++--
 drivers/net/phy/micrel.c         | 11 +++++++++--
 drivers/net/phy/smsc.c           | 10 ++++++++--
 include/linux/phy.h              | 14 ++++++++------
 include/net/dsa.h                |  4 ++--
 net/core/ethtool.c               |  7 ++++---
 net/dsa/master.c                 |  9 +++++----
 net/dsa/port.c                   |  8 ++++----
 13 files changed, 70 insertions(+), 36 deletions(-)
drivers/net/dsa/b53/b53_common.c +2 -2
index 9f561fe505cb..8201e8f5c028 100644
@@ -837,7 +837,7 @@ void b53_get_strings(struct dsa_switch *ds, int port, u32 stringset,
 		if (!phydev)
 			return;
 
-		phy_ethtool_get_strings(phydev, data);
+		phy_ethtool_get_strings(phydev, stringset, data);
 	}
 }
 EXPORT_SYMBOL(b53_get_strings);

@@ -899,7 +899,7 @@ int b53_get_sset_count(struct dsa_switch *ds, int port, int sset)
 		if (!phydev)
 			return 0;
 
-		return phy_ethtool_get_sset_count(phydev);
+		return phy_ethtool_get_sset_count(phydev, sset);
 	}
 
 	return 0;
drivers/net/phy/bcm-phy-lib.c +9 -3
index 0876aec7328c..e797e0863895 100644
@@ -330,16 +330,22 @@ static const struct bcm_phy_hw_stat bcm_phy_hw_stats[] = {
 	{ "phy_remote_rcv_nok", MII_BRCM_CORE_BASE14, 0, 8 },
 };
 
-int bcm_phy_get_sset_count(struct phy_device *phydev)
+int bcm_phy_get_sset_count(struct phy_device *phydev, int sset)
 {
-	return ARRAY_SIZE(bcm_phy_hw_stats);
+	if (sset == ETH_SS_PHY_STATS)
+		return ARRAY_SIZE(bcm_phy_hw_stats);
+
+	return -EOPNOTSUPP;
 }
 EXPORT_SYMBOL_GPL(bcm_phy_get_sset_count);
 
-void bcm_phy_get_strings(struct phy_device *phydev, u8 *data)
+void bcm_phy_get_strings(struct phy_device *phydev, u32 stringset, u8 *data)
 {
 	unsigned int i;
 
+	if (stringset != ETH_SS_PHY_STATS)
+		return;
+
 	for (i = 0; i < ARRAY_SIZE(bcm_phy_hw_stats); i++)
 		strlcpy(data + i * ETH_GSTRING_LEN,
 			bcm_phy_hw_stats[i].string, ETH_GSTRING_LEN);
drivers/net/phy/bcm-phy-lib.h +2 -2
index 7c73808cbbde..bebcfe106283 100644
@@ -42,8 +42,8 @@ int bcm_phy_downshift_get(struct phy_device *phydev, u8 *count);
 
 int bcm_phy_downshift_set(struct phy_device *phydev, u8 count);
 
-int bcm_phy_get_sset_count(struct phy_device *phydev);
-void bcm_phy_get_strings(struct phy_device *phydev, u8 *data);
+int bcm_phy_get_sset_count(struct phy_device *phydev, int sset);
+void bcm_phy_get_strings(struct phy_device *phydev, u32 stringset, u8 *data);
 void bcm_phy_get_stats(struct phy_device *phydev, u64 *shadow,
 		       struct ethtool_stats *stats, u64 *data);
drivers/net/phy/bcm7xxx.c +4 -2
index 29b1c88b55cc..1835af147eea 100644
@@ -587,6 +587,9 @@ static void bcm7xxx_28nm_get_phy_stats(struct phy_device *phydev,
 static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 {
 	struct bcm7xxx_phy_priv *priv;
+	int count;
+
+	count = bcm_phy_get_sset_count(phydev, ETH_SS_PHY_STATS);
 
 	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)

@@ -594,8 +597,7 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
-	priv->stats = devm_kcalloc(&phydev->mdio.dev,
-				   bcm_phy_get_sset_count(phydev), sizeof(u64),
+	priv->stats = devm_kcalloc(&phydev->mdio.dev, count, sizeof(u64),
 				   GFP_KERNEL);
 	if (!priv->stats)
 		return -ENOMEM;
drivers/net/phy/broadcom.c +4 -2
index 3bb6b66dc7bf..dd909799baf0 100644
@@ -547,6 +547,9 @@ struct bcm53xx_phy_priv {
 static int bcm53xx_phy_probe(struct phy_device *phydev)
 {
 	struct bcm53xx_phy_priv *priv;
+	int count;
+
+	count = bcm_phy_get_sset_count(phydev, ETH_SS_PHY_STATS);
 
 	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)

@@ -554,8 +557,7 @@ static int bcm53xx_phy_probe(struct phy_device *phydev)
 
 	phydev->priv = priv;
 
-	priv->stats = devm_kcalloc(&phydev->mdio.dev,
-				   bcm_phy_get_sset_count(phydev), sizeof(u64),
+	priv->stats = devm_kcalloc(&phydev->mdio.dev, count, sizeof(u64),
 				   GFP_KERNEL);
 	if (!priv->stats)
 		return -ENOMEM;
drivers/net/phy/marvell.c +9 -2
index b8f57e9b9379..cf962182297b 100644
@@ -1464,18 +1464,25 @@ static int m88e1318_set_wol(struct phy_device *phydev,
 	return phy_restore_page(phydev, oldpage, err);
 }
 
-static int marvell_get_sset_count(struct phy_device *phydev)
+static int marvell_get_sset_count(struct phy_device *phydev, int sset)
 {
+	if (sset != ETH_SS_PHY_STATS)
+		return -EOPNOTSUPP;
+
 	if (phydev->supported & SUPPORTED_FIBRE)
 		return ARRAY_SIZE(marvell_hw_stats);
 	else
 		return ARRAY_SIZE(marvell_hw_stats) - NB_FIBER_STATS;
 }
 
-static void marvell_get_strings(struct phy_device *phydev, u8 *data)
+static void marvell_get_strings(struct phy_device *phydev, u32 stringset,
+				u8 *data)
 {
 	int i;
 
+	if (stringset != ETH_SS_PHY_STATS)
+		return;
+
 	for (i = 0; i < ARRAY_SIZE(marvell_hw_stats); i++) {
 		strlcpy(data + i * ETH_GSTRING_LEN,
 			marvell_hw_stats[i].string, ETH_GSTRING_LEN);
drivers/net/phy/micrel.c +9 -2
index de31c5170a5b..54f3e400a454 100644
@@ -635,15 +635,22 @@ static int ksz8873mll_config_aneg(struct phy_device *phydev)
 	return 0;
 }
 
-static int kszphy_get_sset_count(struct phy_device *phydev)
+static int kszphy_get_sset_count(struct phy_device *phydev, int sset)
 {
+	if (sset != ETH_SS_PHY_STATS)
+		return -EOPNOTSUPP;
+
 	return ARRAY_SIZE(kszphy_hw_stats);
 }
 
-static void kszphy_get_strings(struct phy_device *phydev, u8 *data)
+static void kszphy_get_strings(struct phy_device *phydev, u32 stringset,
+			       u8 *data)
 {
 	int i;
 
+	if (stringset != ETH_SS_PHY_STATS)
+		return;
+
 	for (i = 0; i < ARRAY_SIZE(kszphy_hw_stats); i++) {
 		strlcpy(data + i * ETH_GSTRING_LEN,
 			kszphy_hw_stats[i].string, ETH_GSTRING_LEN);
drivers/net/phy/smsc.c +8 -2
index c328208388da..c1a4d4d0879d 100644
@@ -153,15 +153,21 @@ static int lan87xx_read_status(struct phy_device *phydev)
 	return err;
 }
 
-static int smsc_get_sset_count(struct phy_device *phydev)
+static int smsc_get_sset_count(struct phy_device *phydev, int sset)
 {
+	if (sset != ETH_SS_PHY_STATS)
+		return -EOPNOTSUPP;
+
 	return ARRAY_SIZE(smsc_hw_stats);
 }
 
-static void smsc_get_strings(struct phy_device *phydev, u8 *data)
+static void smsc_get_strings(struct phy_device *phydev, u32 stringset, u8 *data)
 {
 	int i;
 
+	if (stringset != ETH_SS_PHY_STATS)
+		return;
+
 	for (i = 0; i < ARRAY_SIZE(smsc_hw_stats); i++) {
 		strncpy(data + i * ETH_GSTRING_LEN,
 		       smsc_hw_stats[i].string, ETH_GSTRING_LEN);
include/linux/phy.h +8 -6
index 073235e70442..deba0c11647f 100644
@@ -647,8 +647,8 @@ struct phy_driver {
 			     struct ethtool_eeprom *ee, u8 *data);
 
 	/* Get statistics from the phy using ethtool */
-	int (*get_sset_count)(struct phy_device *dev);
-	void (*get_strings)(struct phy_device *dev, u8 *data);
+	int (*get_sset_count)(struct phy_device *dev, int sset);
+	void (*get_strings)(struct phy_device *dev, u32 stringset, u8 *data);
 	void (*get_stats)(struct phy_device *dev,
 			  struct ethtool_stats *stats, u64 *data);
 

@@ -1069,19 +1069,21 @@ void mdio_bus_exit(void);
 #endif
 
 /* Inline function for use within net/core/ethtool.c (built-in) */
-static inline int phy_ethtool_get_strings(struct phy_device *phydev, u8 *data)
+static inline int phy_ethtool_get_strings(struct phy_device *phydev,
+					  u32 stringset, u8 *data)
 {
 	if (!phydev->drv)
 		return -EIO;
 
 	mutex_lock(&phydev->lock);
-	phydev->drv->get_strings(phydev, data);
+	phydev->drv->get_strings(phydev, stringset, data);
 	mutex_unlock(&phydev->lock);
 
 	return 0;
 }
 
-static inline int phy_ethtool_get_sset_count(struct phy_device *phydev)
+static inline int phy_ethtool_get_sset_count(struct phy_device *phydev,
+					     int sset)
 {
 	int ret;
 

@@ -1092,7 +1094,7 @@ static inline int phy_ethtool_get_sset_count(struct phy_device *phydev)
 	    phydev->drv->get_strings &&
 	    phydev->drv->get_stats) {
 		mutex_lock(&phydev->lock);
-		ret = phydev->drv->get_sset_count(phydev);
+		ret = phydev->drv->get_sset_count(phydev, sset);
 		mutex_unlock(&phydev->lock);
 
 		return ret;
include/net/dsa.h +2 -2
index 462e9741b210..528388cda0a0 100644
@@ -592,8 +592,8 @@ static inline int call_dsa_notifiers(unsigned long val, struct net_device *dev,
 #define BRCM_TAG_GET_QUEUE(v)		((v) & 0xff)
 
 
-int dsa_port_get_phy_strings(struct dsa_port *dp, uint8_t *data);
+int dsa_port_get_phy_strings(struct dsa_port *dp, u32 stringset, uint8_t *data);
 int dsa_port_get_ethtool_phy_stats(struct dsa_port *dp, uint64_t *data);
-int dsa_port_get_phy_sset_count(struct dsa_port *dp);
+int dsa_port_get_phy_sset_count(struct dsa_port *dp, int sset);
 
 #endif
net/core/ethtool.c +4 -3
index b849fdae7e87..0b9e2a44e1d1 100644
@@ -229,7 +229,7 @@ static int __ethtool_get_sset_count(struct net_device *dev, int sset)
 
 	if (sset == ETH_SS_PHY_STATS && dev->phydev &&
 	    !ops->get_ethtool_phy_stats)
-		return phy_ethtool_get_sset_count(dev->phydev);
+		return phy_ethtool_get_sset_count(dev->phydev, sset);
 
 	if (ops->get_sset_count && ops->get_strings)
 		return ops->get_sset_count(dev, sset);

@@ -254,7 +254,7 @@ static void __ethtool_get_strings(struct net_device *dev,
 		memcpy(data, phy_tunable_strings, sizeof(phy_tunable_strings));
 	else if (stringset == ETH_SS_PHY_STATS && dev->phydev &&
 		 !ops->get_ethtool_phy_stats)
-		phy_ethtool_get_strings(dev->phydev, data);
+		phy_ethtool_get_strings(dev->phydev, stringset, data);
 	else
 		/* ops->get_strings is valid because checked earlier */
 		ops->get_strings(dev, stringset, data);

@@ -1977,7 +1977,8 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
 		return -EOPNOTSUPP;
 
 	if (dev->phydev && !ops->get_ethtool_phy_stats)
-		n_stats = phy_ethtool_get_sset_count(dev->phydev);
+		n_stats = phy_ethtool_get_sset_count(dev->phydev,
+						     ETH_SS_PHY_STATS);
 	else
 		n_stats = ops->get_sset_count(dev, ETH_SS_PHY_STATS);
 	if (n_stats < 0)
net/dsa/master.c +5 -4
index c90ee3227dea..4dbbaad2c8aa 100644
@@ -42,7 +42,8 @@ static void dsa_master_get_ethtool_phy_stats(struct net_device *dev,
 	int count = 0;
 
 	if (dev->phydev && !ops->get_ethtool_phy_stats) {
-		count = phy_ethtool_get_sset_count(dev->phydev);
+		count = phy_ethtool_get_sset_count(dev->phydev,
+						   ETH_SS_PHY_STATS);
 		if (count >= 0)
 			phy_ethtool_get_stats(dev->phydev, stats, data);
 	} else if (ops->get_sset_count && ops->get_ethtool_phy_stats) {

@@ -66,7 +67,7 @@ static int dsa_master_get_sset_count(struct net_device *dev, int sset)
 
 	if (sset == ETH_SS_PHY_STATS && dev->phydev &&
 	    !ops->get_ethtool_phy_stats)
-		count = phy_ethtool_get_sset_count(dev->phydev);
+		count = phy_ethtool_get_sset_count(dev->phydev, sset);
 	else if (ops->get_sset_count)
 		count = ops->get_sset_count(dev, sset);
 

@@ -98,11 +99,11 @@ static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
 
 	if (stringset == ETH_SS_PHY_STATS && dev->phydev &&
 	    !ops->get_ethtool_phy_stats) {
-		mcount = phy_ethtool_get_sset_count(dev->phydev);
+		mcount = phy_ethtool_get_sset_count(dev->phydev, stringset);
 		if (mcount < 0)
 			mcount = 0;
 		else
-			phy_ethtool_get_strings(dev->phydev, data);
+			phy_ethtool_get_strings(dev->phydev, stringset, data);
 	} else if (ops->get_sset_count && ops->get_strings) {
 		mcount = ops->get_sset_count(dev, stringset);
 		if (mcount < 0)
net/dsa/port.c +4 -4
index 2413beb995be..4e836da4cdd3 100644
@@ -384,7 +384,7 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
 		dsa_port_setup_phy_of(dp, false);
 }
 
-int dsa_port_get_phy_strings(struct dsa_port *dp, uint8_t *data)
+int dsa_port_get_phy_strings(struct dsa_port *dp, u32 stringset, uint8_t *data)
 {
 	struct phy_device *phydev;
 	int ret = -EOPNOTSUPP;

@@ -396,7 +396,7 @@ int dsa_port_get_phy_strings(struct dsa_port *dp, uint8_t *data)
 	if (IS_ERR_OR_NULL(phydev))
 		return ret;
 
-	ret = phy_ethtool_get_strings(phydev, data);
+	ret = phy_ethtool_get_strings(phydev, stringset, data);
 	put_device(&phydev->mdio.dev);
 
 	return ret;

@@ -422,7 +422,7 @@ int dsa_port_get_ethtool_phy_stats(struct dsa_port *dp, uint64_t *data)
 }
 EXPORT_SYMBOL_GPL(dsa_port_get_ethtool_phy_stats);
 
-int dsa_port_get_phy_sset_count(struct dsa_port *dp)
+int dsa_port_get_phy_sset_count(struct dsa_port *dp, int sset)
 {
 	struct phy_device *phydev;
 	int ret = -EOPNOTSUPP;

@@ -434,7 +434,7 @@ int dsa_port_get_phy_sset_count(struct dsa_port *dp)
 	if (IS_ERR_OR_NULL(phydev))
 		return ret;
 
-	ret = phy_ethtool_get_sset_count(phydev);
+	ret = phy_ethtool_get_sset_count(phydev, sset);
 	put_device(&phydev->mdio.dev);
 
 	return ret;
Signature · 1 line
2.14.1
author
#2 lore raw

RFC 2/5 net: ethtool: Add UAPI for PHY test modes

In reply to Florian Fainelli

Add the necessary UAPI changes to support querying the PHY tests modes implemented and optionally associated test specific data. This will be used as the foundation for supporting:

- IEEE standard electrical test modes
- cable diagnostics
- packet tester

Signed-off-by Florian Fainelli <[email protected]>
---
 include/uapi/linux/ethtool.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
include/uapi/linux/ethtool.h +23 -0
index 4ca65b56084f..a8befecfe853 100644
@@ -567,6 +567,7 @@ struct ethtool_pauseparam {
  * @ETH_SS_RSS_HASH_FUNCS: RSS hush function names
  * @ETH_SS_PHY_STATS: Statistic names, for use with %ETHTOOL_GPHYSTATS
  * @ETH_SS_PHY_TUNABLES: PHY tunable names
+ * @ETH_SS_PHY_TESTS: PHY tests, for use with %ETHTOOL_GPHYTEST
  */
 enum ethtool_stringset {
 	ETH_SS_TEST		= 0,

@@ -578,6 +579,7 @@ enum ethtool_stringset {
 	ETH_SS_TUNABLES,
 	ETH_SS_PHY_STATS,
 	ETH_SS_PHY_TUNABLES,
+	ETH_SS_PHY_TESTS,
 };
 
 /**

@@ -1296,6 +1298,25 @@ enum ethtool_fec_config_bits {
 	ETHTOOL_FEC_BASER_BIT,
 };
 
+/**
+ * struct ethtool_phy_test - Ethernet PHY test mode
+ * @cmd: Command number = %ETHTOOL_GPHYTEST or %ETHTOOL_SPHYTEST
+ * @flags: A bitmask of flags from &enum ethtool_test_flags.  Some
+ *	flags may be set by the user on entry; others may be set by
+ *	the driver on return.
+ * @mode: PHY test mode to enter. The index should be a valid test mode
+ * obtained through ethtool_get_strings with %ETH_SS_PHY_TESTS
+ * @len: The length of the test specific array @data
+ * @data: Array of test specific results to be interpreted with @mode
+ */
+struct ethtool_phy_test {
+	__u32	cmd;
+	__u32	flags;
+	__u32	mode;
+	__u32	len;
+	__u8 	data[0];
+};
+
 #define ETHTOOL_FEC_NONE		(1 << ETHTOOL_FEC_NONE_BIT)
 #define ETHTOOL_FEC_AUTO		(1 << ETHTOOL_FEC_AUTO_BIT)
 #define ETHTOOL_FEC_OFF			(1 << ETHTOOL_FEC_OFF_BIT)

@@ -1396,6 +1417,8 @@ enum ethtool_fec_config_bits {
 #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
 #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
 #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
+#define ETHTOOL_GPHYTEST	0x00000052 /* Get PHY test mode(s) */
+#define ETHTOOL_SPHYTEST	0x00000053 /* Set PHY test mode */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
Signature · 1 line
2.14.1
author
#3 lore raw

RFC 3/5 net: ethtool: Add plumbing to get/set PHY test modes

In reply to Florian Fainelli

Implement the core ethtool changes to get/set PHY test modes, no driver implements that yet, but the internal API is defined and now allows it. We also provide the required helpers in PHYLIB in order to call the appropriate functions within the drivers.

Signed-off-by Florian Fainelli <[email protected]>
---
 include/linux/phy.h | 63 ++++++++++++++++++++++++++++++++++++++++--
 net/core/ethtool.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 135 insertions(+), 7 deletions(-)
include/linux/phy.h +60 -3
index deba0c11647f..449afde7ca7c 100644
@@ -659,6 +659,14 @@ struct phy_driver {
 			    struct ethtool_tunable *tuna,
 			    const void *data);
 	int (*set_loopback)(struct phy_device *dev, bool enable);
+
+	/* Get and Set PHY test modes */
+	int (*get_test_len)(struct phy_device *dev, u32 mode);
+	int (*get_test)(struct phy_device *dev,
+			struct ethtool_phy_test *test, u8 *data);
+	int (*set_test)(struct phy_device *dev,
+			struct ethtool_phy_test *test,
+			const u8 *data);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)

@@ -1090,9 +1098,11 @@ static inline int phy_ethtool_get_sset_count(struct phy_device *phydev,
 	if (!phydev->drv)
 		return -EIO;
 
-	if (phydev->drv->get_sset_count &&
-	    phydev->drv->get_strings &&
-	    phydev->drv->get_stats) {
+	if (!phydev->drv->get_sset_count || !phydev->drv->get_strings)
+		return -EOPNOTSUPP;
+
+	if (phydev->drv->get_stats || phydev->drv->get_test_len ||
+	    phydev->drv->get_test || phydev->drv->set_test) {
 		mutex_lock(&phydev->lock);
 		ret = phydev->drv->get_sset_count(phydev, sset);
 		mutex_unlock(&phydev->lock);

@@ -1116,6 +1126,53 @@ static inline int phy_ethtool_get_stats(struct phy_device *phydev,
 	return 0;
 }
 
+static inline int phy_ethtool_get_test_len(struct phy_device *phydev,
+					   u32 mode)
+{
+	int ret;
+
+	if (!phydev->drv)
+		return -EIO;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->get_test_len(phydev, mode);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+static inline int phy_ethtool_get_test(struct phy_device *phydev,
+				       struct ethtool_phy_test *test,
+				       u8 *data)
+{
+	int ret;
+
+	if (!phydev->drv)
+		return -EIO;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->get_test(phydev, test, data);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
+static inline int phy_ethtool_set_test(struct phy_device *phydev,
+				       struct ethtool_phy_test *test,
+				       const u8 *data)
+{
+	int ret;
+
+	if (!phydev->drv)
+		return -EIO;
+
+	mutex_lock(&phydev->lock);
+	ret = phydev->drv->set_test(phydev, test, data);
+	mutex_unlock(&phydev->lock);
+
+	return ret;
+}
+
 extern struct bus_type mdio_bus_type;
 
 struct mdio_board_info {
net/core/ethtool.c +75 -4

109 lines not on this page · raw message

Signature · 1 line
2.14.1
author
#4 lore raw

RFC 4/5 net: phy: Add support for IEEE standard test modes

In reply to Florian Fainelli

Add support for the 100BaseT2 and 1000BaseT standard test modes as defined by the IEEE 802.3-2012-Section two and three. We provide a set of helper functions for PHY drivers to either punt entirely onto genphy_* functions or if they desire, build additional tests on top of the standard ones available.

Signed-off-by Florian Fainelli <[email protected]>
---
 drivers/net/phy/Kconfig     |   6 ++
 drivers/net/phy/Makefile    |   4 +-
 drivers/net/phy/phy-tests.c | 159 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h         |  22 ++++++
 4 files changed, 190 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/phy/phy-tests.c
drivers/net/phy/Kconfig +6 -0
index edb8b9ab827f..ef3f2f1ae990 100644
@@ -200,6 +200,12 @@ config LED_TRIGGER_PHY
 		<Speed in megabits>Mbps OR <Speed in gigabits>Gbps OR link
 		for any speed known to the PHY.
 
+config CONFIG_PHYLIB_TEST_MODES
+	bool "Support for test modes"
+	---help---
+	  Selecting this option will allow the PHY library to support
+	  test modes: electrical, cable diagnostics, pattern generator etc.
+
 
 comment "MII PHY device drivers"
drivers/net/phy/Makefile +3 -1
index 701ca0b8717e..e9905432e164 100644
@@ -1,7 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Linux PHY drivers and MDIO bus drivers
 
-libphy-y			:= phy.o phy-c45.o phy-core.o phy_device.o
+libphy-y			:= phy.o phy-c45.o phy-core.o phy_device.o \
+				   phy-tests.o
 mdio-bus-y			+= mdio_bus.o mdio_device.o
 
 ifdef CONFIG_MDIO_DEVICE

@@ -18,6 +19,7 @@ obj-$(CONFIG_MDIO_DEVICE)	+= mdio-bus.o
 endif
 libphy-$(CONFIG_SWPHY)		+= swphy.o
 libphy-$(CONFIG_LED_TRIGGER_PHY)	+= phy_led_triggers.o
+libphy-$(CONFIG_PHYLIB_TEST_MODES)	+= phy-tests.o
 
 obj-$(CONFIG_PHYLINK)		+= phylink.o
 obj-$(CONFIG_PHYLIB)		+= libphy.o
drivers/net/phy/phy-tests.c new file +159 -0

159 lines not on this page · raw message

include/linux/phy.h +22 -0
index 449afde7ca7c..7155187cf268 100644
@@ -165,6 +165,20 @@ static inline const char *phy_modes(phy_interface_t interface)
 	}
 }
 
+enum phy_std_test_mode {
+	/* Normal operation - disables test mode */
+	PHY_STD_TEST_MODE_NORMAL = 0,
+	PHY_STD_TEST_MODE_100BASET2_1,
+	PHY_STD_TEST_MODE_100BASET2_2,
+	PHY_STD_TEST_MODE_100BASET2_3,
+	PHY_STD_TEST_MODE_100BASET2_MAX = PHY_STD_TEST_MODE_100BASET2_3,
+	PHY_STD_TEST_MODE_1000BASET_1,
+	PHY_STD_TEST_MODE_1000BASET_2,
+	PHY_STD_TEST_MODE_1000BASET_3,
+	PHY_STD_TEST_MODE_1000BASET_4,
+	PHY_STD_TEST_MODE_MAX,
+	/* PHY drivers can implement their own test modes after that value */
+};
 
 #define PHY_INIT_TIMEOUT	100000
 #define PHY_STATE_TIME		1

@@ -997,6 +1011,14 @@ int genphy_read_mmd_unsupported(struct phy_device *phdev, int devad,
 int genphy_write_mmd_unsupported(struct phy_device *phdev, int devnum,
 				 u16 regnum, u16 val);
 
+int genphy_get_test(struct phy_device *phydev, struct ethtool_phy_test *t,
+		    u8 *data);
+int genphy_set_test(struct phy_device *phydev, struct ethtool_phy_test *t,
+		    const u8 *data);
+int genphy_get_test_count(struct phy_device *phydev);
+void genphy_get_test_strings(struct phy_device *phydev, u8 *data);
+int genphy_get_test_len(struct phy_device *phydev, u32 mode);
+
 /* Clause 45 PHY */
 int genphy_c45_restart_aneg(struct phy_device *phydev);
 int genphy_c45_aneg_done(struct phy_device *phydev);
Signature · 1 line
2.14.1
maintainer
#5 lore raw

Re: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

Quoted 16 lines
drivers/net/phy/phy-tests.c new file +16 -0
+/* genphy_set_test - Make a PHY enter one of the standard IEEE defined
+ * test modes
+ * @phydev: the PHY device instance
+ * @test: the desired test mode
+ * @data: test specific data (none)
+ *
+ * This function makes the designated @phydev enter the desired standard
+ * 100BaseT2 or 1000BaseT test mode as defined in IEEE 802.3-2012 section TWO
+ * and THREE under 32.6.1.2.1 and 40.6.1.1.2 respectively
+ */
+int genphy_set_test(struct phy_device *phydev,
+		    struct ethtool_phy_test *test, const u8 *data)
+{
+	u16 shift, base, bmcr = 0;
+	int ret;
+

drivers/net/phy/phy-tests.c line 47

Quoted 10 lines
drivers/net/phy/phy-tests.c new file +10 -0
+	/* Exit test mode */
+	if (test->mode == PHY_STD_TEST_MODE_NORMAL) {
+		ret = phy_read(phydev, MII_CTRL1000);
+		if (ret < 0)
+			return ret;
+
+		ret &= ~GENMASK(15, 13);
+
+		return phy_write(phydev, MII_CTRL1000, ret);
+	}

Hi Florain

I looked at the Marvell SDK for PHYs. It performs a soft reset after swapping back to normal mode. I assume the broadcom PHY does not need this? But maybe we can add it anyway?

Quoted 26 lines
drivers/net/phy/phy-tests.c new file +26 -0
+
+	switch (test->mode) {
+	case PHY_STD_TEST_MODE_100BASET2_1:
+	case PHY_STD_TEST_MODE_100BASET2_2:
+	case PHY_STD_TEST_MODE_100BASET2_3:
+		if (!(phydev->supported & PHY_100BT_FEATURES))
+			return -EOPNOTSUPP;
+
+		shift = 14;
+		base = test->mode - PHY_STD_TEST_MODE_NORMAL;
+		bmcr = BMCR_SPEED100;
+		break;
+
+	case PHY_STD_TEST_MODE_1000BASET_1:
+	case PHY_STD_TEST_MODE_1000BASET_2:
+	case PHY_STD_TEST_MODE_1000BASET_3:
+	case PHY_STD_TEST_MODE_1000BASET_4:
+		if (!(phydev->supported & PHY_1000BT_FEATURES))
+			return -EOPNOTSUPP;
+
+		shift = 13;
+		base = test->mode - PHY_STD_TEST_MODE_100BASET2_MAX;
+		bmcr = BMCR_SPEED1000;
+		break;
+
+	default:

drivers/net/phy/phy-tests.c line 83

Quoted 10 lines
drivers/net/phy/phy-tests.c new file +10 -0
+		/* Let an upper driver deal with additional modes it may
+		 * support
+		 */
+		return -EOPNOTSUPP;
+	}
+
+	/* Force speed and duplex */
+	ret = phy_write(phydev, MII_BMCR, bmcr | BMCR_FULLDPLX);
+	if (ret < 0)
+		return ret;

Should there be something to undo this when returning to normal mode?

Andrew

author
#6 lore raw

Re: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

Quoted 29 lines · Andrew Lunn
Quoted 16 lines
drivers/net/phy/phy-tests.c new file +16 -0
+/* genphy_set_test - Make a PHY enter one of the standard IEEE defined
+ * test modes
+ * @phydev: the PHY device instance
+ * @test: the desired test mode
+ * @data: test specific data (none)
+ *
+ * This function makes the designated @phydev enter the desired standard
+ * 100BaseT2 or 1000BaseT test mode as defined in IEEE 802.3-2012 section TWO
+ * and THREE under 32.6.1.2.1 and 40.6.1.1.2 respectively
+ */
+int genphy_set_test(struct phy_device *phydev,
+		    struct ethtool_phy_test *test, const u8 *data)
+{
+	u16 shift, base, bmcr = 0;
+	int ret;
+

drivers/net/phy/phy-tests.c line 47

Quoted 10 lines
drivers/net/phy/phy-tests.c new file +10 -0
+	/* Exit test mode */
+	if (test->mode == PHY_STD_TEST_MODE_NORMAL) {
+		ret = phy_read(phydev, MII_CTRL1000);
+		if (ret < 0)
+			return ret;
+
+		ret &= ~GENMASK(15, 13);
+
+		return phy_write(phydev, MII_CTRL1000, ret);
+	}

Hi Florain

Quoted 3 lines

I looked at the Marvell SDK for PHYs. It performs a soft reset after swapping back to normal mode. I assume the broadcom PHY does not need this? But maybe we can add it anyway?

We certainly should reset the PHY, thanks!

Quoted 31 lines
Quoted 31 lines
drivers/net/phy/phy-tests.c new file +30 -0
+
+	switch (test->mode) {
+	case PHY_STD_TEST_MODE_100BASET2_1:
+	case PHY_STD_TEST_MODE_100BASET2_2:
+	case PHY_STD_TEST_MODE_100BASET2_3:
+		if (!(phydev->supported & PHY_100BT_FEATURES))
+			return -EOPNOTSUPP;
+
+		shift = 14;
+		base = test->mode - PHY_STD_TEST_MODE_NORMAL;
+		bmcr = BMCR_SPEED100;
+		break;
+
+	case PHY_STD_TEST_MODE_1000BASET_1:
+	case PHY_STD_TEST_MODE_1000BASET_2:
+	case PHY_STD_TEST_MODE_1000BASET_3:
+	case PHY_STD_TEST_MODE_1000BASET_4:
+		if (!(phydev->supported & PHY_1000BT_FEATURES))
+			return -EOPNOTSUPP;
+
+		shift = 13;
+		base = test->mode - PHY_STD_TEST_MODE_100BASET2_MAX;
+		bmcr = BMCR_SPEED1000;
+		break;
+
+	default:
+		/* Let an upper driver deal with additional modes it may
+		 * support
+		 */
+		return -EOPNOTSUPP;
Quoted 8 lines

drivers/net/phy/phy-tests.c line 83

Quoted 6 lines
drivers/net/phy/phy-tests.c new file +6 -0
+	}
+
+	/* Force speed and duplex */
+	ret = phy_write(phydev, MII_BMCR, bmcr | BMCR_FULLDPLX);
+	if (ret < 0)
+		return ret;

Should there be something to undo this when returning to normal mode?

Yes, resetting the PHY would perform that.

Signature · 1 line
Florian
#7 lore raw

RE: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

In reply to Florian Fainelli

Hi Florian,

Quoted 1 line
drivers/net/phy/phy-tests.c +0 -0

...

Quoted 56 lines
drivers/net/phy/phy-tests.c +56 -0
+/* genphy_set_test - Make a PHY enter one of the standard IEEE defined
+ * test modes
+ * @phydev: the PHY device instance
+ * @test: the desired test mode
+ * @data: test specific data (none)
+ *
+ * This function makes the designated @phydev enter the desired standard
+ * 100BaseT2 or 1000BaseT test mode as defined in IEEE 802.3-2012 section TWO
+ * and THREE under 32.6.1.2.1 and 40.6.1.1.2 respectively
+ */
+int genphy_set_test(struct phy_device *phydev,
+		    struct ethtool_phy_test *test, const u8 *data)
+{
+	u16 shift, base, bmcr = 0;
+	int ret;
+
+	/* Exit test mode */
+	if (test->mode == PHY_STD_TEST_MODE_NORMAL) {
+		ret = phy_read(phydev, MII_CTRL1000);
+		if (ret < 0)
+			return ret;
+
+		ret &= ~GENMASK(15, 13);
+
+		return phy_write(phydev, MII_CTRL1000, ret);
+	}
+
+	switch (test->mode) {
+	case PHY_STD_TEST_MODE_100BASET2_1:
+	case PHY_STD_TEST_MODE_100BASET2_2:
+	case PHY_STD_TEST_MODE_100BASET2_3:
+		if (!(phydev->supported & PHY_100BT_FEATURES))
+			return -EOPNOTSUPP;
+
+		shift = 14;
+		base = test->mode - PHY_STD_TEST_MODE_NORMAL;
+		bmcr = BMCR_SPEED100;
+		break;
+
+	case PHY_STD_TEST_MODE_1000BASET_1:
+	case PHY_STD_TEST_MODE_1000BASET_2:
+	case PHY_STD_TEST_MODE_1000BASET_3:
+	case PHY_STD_TEST_MODE_1000BASET_4:
+		if (!(phydev->supported & PHY_1000BT_FEATURES))
+			return -EOPNOTSUPP;
+
+		shift = 13;
+		base = test->mode - PHY_STD_TEST_MODE_100BASET2_MAX;
+		bmcr = BMCR_SPEED1000;
+		break;
+
+	default:
+		/* Let an upper driver deal with additional modes it may
+		 * support
+		 */
+		return -EOPNOTSUPP;

drivers/net/phy/phy-tests.c line 87

Quoted 10 lines
drivers/net/phy/phy-tests.c +10 -0
+	}
+
+	/* Force speed and duplex */
+	ret = phy_write(phydev, MII_BMCR, bmcr | BMCR_FULLDPLX);
+	if (ret < 0)
+		return ret;
+
+	/* Set the desired test mode bit */
+	return phy_write(phydev, MII_CTRL1000, (test->mode + base) << shift);
+}

For now, these are for 100B-T2 & 1000B-TX.
But, other speeds such as 802.3bw/bq/cq have very similar format,
how about make phy_write() to BMCR & CTRL1000 as another function call per speed?

Thanks.
Woojung

author
#8 lore raw

Re: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

Quoted 65 lines · On 05/01/2018 10:29 AM, [email protected] wrote:

Hi Florian,

Quoted 1 line
drivers/net/phy/phy-tests.c +0 -0

...

Quoted 61 lines
drivers/net/phy/phy-tests.c +61 -0
+/* genphy_set_test - Make a PHY enter one of the standard IEEE defined
+ * test modes
+ * @phydev: the PHY device instance
+ * @test: the desired test mode
+ * @data: test specific data (none)
+ *
+ * This function makes the designated @phydev enter the desired standard
+ * 100BaseT2 or 1000BaseT test mode as defined in IEEE 802.3-2012 section TWO
+ * and THREE under 32.6.1.2.1 and 40.6.1.1.2 respectively
+ */
+int genphy_set_test(struct phy_device *phydev,
+		    struct ethtool_phy_test *test, const u8 *data)
+{
+	u16 shift, base, bmcr = 0;
+	int ret;
+
+	/* Exit test mode */
+	if (test->mode == PHY_STD_TEST_MODE_NORMAL) {
+		ret = phy_read(phydev, MII_CTRL1000);
+		if (ret < 0)
+			return ret;
+
+		ret &= ~GENMASK(15, 13);
+
+		return phy_write(phydev, MII_CTRL1000, ret);
+	}
+
+	switch (test->mode) {
+	case PHY_STD_TEST_MODE_100BASET2_1:
+	case PHY_STD_TEST_MODE_100BASET2_2:
+	case PHY_STD_TEST_MODE_100BASET2_3:
+		if (!(phydev->supported & PHY_100BT_FEATURES))
+			return -EOPNOTSUPP;
+
+		shift = 14;
+		base = test->mode - PHY_STD_TEST_MODE_NORMAL;
+		bmcr = BMCR_SPEED100;
+		break;
+
+	case PHY_STD_TEST_MODE_1000BASET_1:
+	case PHY_STD_TEST_MODE_1000BASET_2:
+	case PHY_STD_TEST_MODE_1000BASET_3:
+	case PHY_STD_TEST_MODE_1000BASET_4:
+		if (!(phydev->supported & PHY_1000BT_FEATURES))
+			return -EOPNOTSUPP;
+
+		shift = 13;
+		base = test->mode - PHY_STD_TEST_MODE_100BASET2_MAX;
+		bmcr = BMCR_SPEED1000;
+		break;
+
+	default:
+		/* Let an upper driver deal with additional modes it may
+		 * support
+		 */
+		return -EOPNOTSUPP;
+	}
+
+	/* Force speed and duplex */
+	ret = phy_write(phydev, MII_BMCR, bmcr | BMCR_FULLDPLX);
+	if (ret < 0)
Quoted 8 lines

drivers/net/phy/phy-tests.c line 87

Quoted 5 lines
drivers/net/phy/phy-tests.c +5 -0
+		return ret;
+
+	/* Set the desired test mode bit */
+	return phy_write(phydev, MII_CTRL1000, (test->mode + base) << shift);
+}

For now, these are for 100B-T2 & 1000B-TX.
But, other speeds such as 802.3bw/bq/cq have very similar format,
how about make phy_write() to BMCR & CTRL1000 as another function call per speed?

Not sure I completely understand your suggestion, do you mean that I should break down the body of that function above such that there are per-speed lower level functions? Something like the pseudo-code below:

genphy_set_test() {
	switch (mode) {
	case PHY_STD_TEST_MODE_100BASET2_1:
	..
	case PHY_STD_TEST_MODE_100BASET2_3:
		return genphy_set_100baset2();

	case PHY_STD_TEST_MODE_1000BASET_1:
	..
	case PHY_STD_TEST_MODE_1000BASET_4:
		return genphy_set_1000baset();

	case PHY_STD_TEST_MODE_8021BWQCQ_1:
		return genphy_set_100baset1();

}

Or did you want to see a different way of mapping a given speed/feature set to a specific test function?

Signature · 1 line
Florian
#9 lore raw

RE: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

Hi Florian,

Quoted 19 lines

Not sure I completely understand your suggestion, do you mean that I should break down the body of that function above such that there are per-speed lower level functions? Something like the pseudo-code below:

genphy_set_test() {
	switch (mode) {
	case PHY_STD_TEST_MODE_100BASET2_1:
	..
	case PHY_STD_TEST_MODE_100BASET2_3:
		return genphy_set_100baset2();

	case PHY_STD_TEST_MODE_1000BASET_1:
	..
	case PHY_STD_TEST_MODE_1000BASET_4:
		return genphy_set_1000baset();

	case PHY_STD_TEST_MODE_8021BWQCQ_1:
		return genphy_set_100baset1();
Quoted 1 line

}

Yes, I should write pseudo code. Sorry about confusion. User can override this function or expand to other modes.

Thanks.
Woojung

author
#10 lore raw

Re: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

Quoted 18 lines · On 05/01/2018 01:07 PM, [email protected] wrote:

Hi Florian,

Quoted 16 lines

Not sure I completely understand your suggestion, do you mean that I should break down the body of that function above such that there are per-speed lower level functions? Something like the pseudo-code below:

genphy_set_test() {
	switch (mode) {
	case PHY_STD_TEST_MODE_100BASET2_1:
	..
	case PHY_STD_TEST_MODE_100BASET2_3:
		return genphy_set_100baset2();

	case PHY_STD_TEST_MODE_1000BASET_1:
	..
	case PHY_STD_TEST_MODE_1000BASET_4:
		return genphy_set_1000baset();
Quoted 6 lines
Quoted 4 lines
	case PHY_STD_TEST_MODE_8021BWQCQ_1:
		return genphy_set_100baset1();

}

Yes, I should write pseudo code. Sorry about confusion. User can override this function or expand to other modes.

Well, the way the code is structure is that if you call that function with a test mode value that is not part of the standard set, it returns -EOPNOTSUPP, so if your particular PHY driver wants to "overlay" standard and non-standard modes, it can by using that hint.

This should work even if we have more standard test modes in the future because the test modes are dynamically fetched by user-space using the ETH_GSTRINGS ioctl().

Does that cover what you had in mind?

Signature · 1 line
Florian
#11 lore raw

RE: [RFC net-next 4/5] net: phy: Add support for IEEE standard test modes

Hi Florian,

Quoted 10 lines

Well, the way the code is structure is that if you call that function with a test mode value that is not part of the standard set, it returns -EOPNOTSUPP, so if your particular PHY driver wants to "overlay" standard and non-standard modes, it can by using that hint.

This should work even if we have more standard test modes in the future because the test modes are dynamically fetched by user-space using the ETH_GSTRINGS ioctl().

Does that cover what you had in mind?

Basically, agree on your explanation.

My idea was making genphy_set_test() more expandable for other test modes because it would be a good place to add more standard test modes later.

No problem to keep current codes.

Thanks.
Woojung

author
#12 lore raw

RFC 5/5 net: phy: broadcom: Add support for PHY test modes

In reply to Florian Fainelli

Re-use the generic PHY library test modes for 100BaseT2 and 1000BaseT and advertise support for those through the newly added ethtool knobs.

Signed-off-by Florian Fainelli <[email protected]>
---
 drivers/net/phy/bcm-phy-lib.c | 15 +++++++++------
 drivers/net/phy/bcm7xxx.c     |  3 +++
 2 files changed, 12 insertions(+), 6 deletions(-)
drivers/net/phy/bcm-phy-lib.c +9 -6
index e797e0863895..cb3081e523a5 100644
@@ -334,6 +334,8 @@ int bcm_phy_get_sset_count(struct phy_device *phydev, int sset)
 {
 	if (sset == ETH_SS_PHY_STATS)
 		return ARRAY_SIZE(bcm_phy_hw_stats);
+	else if (sset == ETH_SS_PHY_TESTS)
+		return genphy_get_test_count(phydev);
 
 	return -EOPNOTSUPP;
 }

@@ -343,12 +345,13 @@ void bcm_phy_get_strings(struct phy_device *phydev, u32 stringset, u8 *data)
 {
 	unsigned int i;
 
-	if (stringset != ETH_SS_PHY_STATS)
-		return;
-
-	for (i = 0; i < ARRAY_SIZE(bcm_phy_hw_stats); i++)
-		strlcpy(data + i * ETH_GSTRING_LEN,
-			bcm_phy_hw_stats[i].string, ETH_GSTRING_LEN);
+	if (stringset == ETH_SS_PHY_STATS) {
+		for (i = 0; i < ARRAY_SIZE(bcm_phy_hw_stats); i++)
+			strlcpy(data + i * ETH_GSTRING_LEN,
+				bcm_phy_hw_stats[i].string, ETH_GSTRING_LEN);
+	} else if (stringset == ETH_SS_PHY_TESTS) {
+		genphy_get_test_strings(phydev, data);
+	}
 }
 EXPORT_SYMBOL_GPL(bcm_phy_get_strings);
drivers/net/phy/bcm7xxx.c +3 -0
index 1835af147eea..1efd287ed320 100644
@@ -619,6 +619,9 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 	.get_sset_count	= bcm_phy_get_sset_count,			\
 	.get_strings	= bcm_phy_get_strings,				\
 	.get_stats	= bcm7xxx_28nm_get_phy_stats,			\
+	.set_test	= genphy_set_test,				\
+	.get_test	= genphy_get_test,				\
+	.get_test_len	= genphy_get_test_len,				\
 	.probe		= bcm7xxx_28nm_probe,				\
 }
Signature · 1 line
2.14.1
author
#13 lore raw

PATCH 1/2 ethtool-copy.h: Sync with net-next

In reply to Florian Fainelli

This brings support for PHY test modes (not accepted yet)

Signed-off-by Florian Fainelli <[email protected]>
---
 ethtool-copy.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
ethtool-copy.h +23 -0
index 8cc61e9ab40b..42fb94129da5 100644
@@ -572,6 +572,7 @@ enum ethtool_stringset {
 	ETH_SS_TUNABLES,
 	ETH_SS_PHY_STATS,
 	ETH_SS_PHY_TUNABLES,
+	ETH_SS_PHY_TESTS,
 };
 
 /**

@@ -1296,6 +1297,25 @@ enum ethtool_fec_config_bits {
 #define ETHTOOL_FEC_RS			(1 << ETHTOOL_FEC_RS_BIT)
 #define ETHTOOL_FEC_BASER		(1 << ETHTOOL_FEC_BASER_BIT)
 
+/**
+ * struct ethtool_phy_test - Ethernet PHY test mode
+ * @cmd: Command number = %ETHTOOL_GPHYTEST or %ETHTOOL_SPHYTEST
+ * @flags: A bitmask of flags from &enum ethtool_test_flags.  Some
+ *      flags may be set by the user on entry; others may be set by
+ *      the driver on return.
+ * @mode: PHY test mode to enter. The index should be a valid test mode
+ * obtained through ethtool_get_strings with %ETH_SS_PHY_TESTS
+ * @len: The length of the test specific array @data
+ * @data: Array of test specific results to be interpreted with @mode
+ */
+struct ethtool_phy_test {
+        __u32   cmd;
+        __u32   flags;
+        __u32   mode;
+        __u32   len;
+        __u8    data[0];
+};
+
 /* CMDs currently supported */
 #define ETHTOOL_GSET		0x00000001 /* DEPRECATED, Get settings.
 					    * Please use ETHTOOL_GLINKSETTINGS

@@ -1391,6 +1411,9 @@ enum ethtool_fec_config_bits {
 #define ETHTOOL_GFECPARAM	0x00000050 /* Get FEC settings */
 #define ETHTOOL_SFECPARAM	0x00000051 /* Set FEC settings */
 
+#define ETHTOOL_GPHYTEST        0x00000052 /* Get PHY test mode(s) */
+#define ETHTOOL_SPHYTEST        0x00000053 /* Set PHY test mode */
+
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
 #define SPARC_ETH_SSET		ETHTOOL_SSET
Signature · 1 line
2.14.1
author
#14 lore raw

PATCH 2/2 ethtool: Add support for PHY test modes

In reply to Florian Fainelli

Add two new commands:

--get-phy-tests which allows fetching supported test modes by a given
network device's PHY interface
--set-phy-test which allows entering one of the modes listed before and
pass an eventual set of test specific data

Signed-off-by Florian Fainelli <[email protected]>
---
 ethtool.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)
ethtool.c +115 -0
index 3289e0f6e8ec..f02cd3560197 100644
@@ -4854,6 +4854,118 @@ static int do_reset(struct cmd_context *ctx)
 	return 0;
 }
 
+static int do_gphytest(struct cmd_context *ctx)
+{
+	struct ethtool_gstrings *strings;
+	struct ethtool_phy_test test;
+	unsigned int i;
+	int max_len = 0, cur_len, rc;
+
+	if (ctx->argc != 0)
+		exit_bad_args();
+
+	strings = get_stringset(ctx, ETH_SS_PHY_TESTS, 0, 1);
+	if (!strings) {
+		perror("Cannot get PHY tests strings");
+		return 1;
+	}
+	if (strings->len == 0) {
+		fprintf(stderr, "No PHY tests defined\n");
+		rc = 1;
+		goto err;
+	}
+
+	/* Find longest string and align all strings accordingly */
+	for (i = 0; i < strings->len; i++) {
+		cur_len = strlen((const char*)strings->data +
+				 i * ETH_GSTRING_LEN);
+		if (cur_len > max_len)
+			max_len = cur_len;
+	}
+
+	printf("PHY tests %s:\n", ctx->devname);
+	for (i = 0; i < strings->len; i++) {
+		memset(&test, 0, sizeof(test));
+		test.cmd = ETHTOOL_GPHYTEST;
+		test.mode = i;
+
+		rc = send_ioctl(ctx, &test);
+		if (rc < 0)
+			continue;
+
+		fprintf(stdout, "     %.*s (Test data: %s)\n",
+		       max_len,
+		       (const char *)strings->data + i * ETH_GSTRING_LEN,
+		       test.len ? "Yes" : "No");
+	}
+
+	rc = 0;
+
+err:
+	free(strings);
+	return rc;
+}
+
+static int do_sphytest(struct cmd_context *ctx)
+{
+	struct ethtool_gstrings *strings;
+	struct ethtool_phy_test gtest;
+	struct ethtool_phy_test *stest;
+	unsigned int i;
+	int rc;
+
+	if (ctx->argc < 1)
+		exit_bad_args();
+
+	strings = get_stringset(ctx, ETH_SS_PHY_TESTS, 0, 1);
+	if (!strings) {
+		perror("Cannot get PHY test modes");
+		return 1;
+	}
+
+	if (strings->len == 0) {
+		fprintf(stderr, "No PHY tests defined\n");
+		rc = 1;
+		goto err;
+	}
+
+	for (i = 0; i < strings->len; i++) {
+		if (!strcmp(ctx->argp[0],
+			    (const char *)strings->data + i * ETH_GSTRING_LEN))
+			break;
+	}
+
+	if (i == strings->len)
+		exit_bad_args();
+
+	memset(&gtest, 0, sizeof(gtest));
+	gtest.cmd = ETHTOOL_GPHYTEST;
+	gtest.mode = i;
+	rc = send_ioctl(ctx, &gtest);
+	if (rc < 0) {
+		rc = 1;
+		goto err;
+	}
+
+	stest = calloc(1, sizeof(*stest) + gtest.len);
+	if (!stest) {
+		perror("Unable to allocate memory");
+		rc = 1;
+		goto err;
+	}
+
+	stest->cmd = ETHTOOL_SPHYTEST;
+	stest->len = gtest.len;
+	stest->mode = i;
+
+	rc = send_ioctl(ctx, stest);
+	free(stest);
+err:
+	free(strings);
+	return rc;
+}
+
+
 static int parse_named_bool(struct cmd_context *ctx, const char *name, u8 *on)
 {
 	if (ctx->argc < 2)

@@ -5223,6 +5335,9 @@ static const struct option {
 	{ "--show-fec", 1, do_gfec, "Show FEC settings"},
 	{ "--set-fec", 1, do_sfec, "Set FEC settings",
 	  "		[ encoding auto|off|rs|baser ]\n"},
+	{ "--get-phy-tests", 1, do_gphytest,"Get PHY test mode(s)" },
+	{ "--set-phy-test", 1, do_sphytest, "Set PHY test mode",
+	  "		[ test options ]\n" },
 	{ "-h|--help", 0, show_usage, "Show this help" },
 	{ "--version", 0, do_version, "Show version number" },
 	{}
Signature · 1 line
2.14.1
#15 lore raw

Re: [PATCH ethtool 2/2] ethtool: Add support for PHY test modes

Hello Florian,

Quoted 8 lines · Florian Fainelli

Add two new commands:

--get-phy-tests which allows fetching supported test modes by a given network device's PHY interface
--set-phy-test which allows entering one of the modes listed before and pass an eventual set of test specific data

Signed-off-by Florian Fainelli <[email protected]>

has this series ever been posted for merging, i.e. not as RFC? It looks quite useful to me and I probably will be able to test the series if you still will be posting it.

Quoted 137 lines

---
 ethtool.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

 1 file changed, 115 insertions(+)
ethtool.c +0 -0
index 3289e0f6e8ec..f02cd3560197 100644
@@ -4854,6 +4854,118 @@ static int do_reset(struct cmd_context *ctx)
         return 0;
  }

ethtool.c +112 -0
+static int do_gphytest(struct cmd_context *ctx)
+{
+       struct ethtool_gstrings *strings;
+       struct ethtool_phy_test test;
+       unsigned int i;
+       int max_len = 0, cur_len, rc;
+
+       if (ctx->argc != 0)
+               exit_bad_args();
+
+       strings = get_stringset(ctx, ETH_SS_PHY_TESTS, 0, 1);
+       if (!strings) {
+               perror("Cannot get PHY tests strings");
+               return 1;
+       }
+       if (strings->len == 0) {
+               fprintf(stderr, "No PHY tests defined\n");
+               rc = 1;
+               goto err;
+       }
+
+       /* Find longest string and align all strings accordingly */
+       for (i = 0; i < strings->len; i++) {
+               cur_len = strlen((const char*)strings->data +
+                                i * ETH_GSTRING_LEN);
+               if (cur_len > max_len)
+                       max_len = cur_len;
+       }
+
+       printf("PHY tests %s:\n", ctx->devname);
+       for (i = 0; i < strings->len; i++) {
+               memset(&test, 0, sizeof(test));
+               test.cmd = ETHTOOL_GPHYTEST;
+               test.mode = i;
+
+               rc = send_ioctl(ctx, &test);
+               if (rc < 0)
+                       continue;
+
+               fprintf(stdout, "     %.*s (Test data: %s)\n",
+                      max_len,
+                      (const char *)strings->data + i * ETH_GSTRING_LEN,
+                      test.len ? "Yes" : "No");
+       }
+
+       rc = 0;
+
+err:
+       free(strings);
+       return rc;
+}
+
+static int do_sphytest(struct cmd_context *ctx)
+{
+       struct ethtool_gstrings *strings;
+       struct ethtool_phy_test gtest;
+       struct ethtool_phy_test *stest;
+       unsigned int i;
+       int rc;
+
+       if (ctx->argc < 1)
+               exit_bad_args();
+
+       strings = get_stringset(ctx, ETH_SS_PHY_TESTS, 0, 1);
+       if (!strings) {
+               perror("Cannot get PHY test modes");
+               return 1;
+       }
+
+       if (strings->len == 0) {
+               fprintf(stderr, "No PHY tests defined\n");
+               rc = 1;
+               goto err;
+       }
+
+       for (i = 0; i < strings->len; i++) {
+               if (!strcmp(ctx->argp[0],
+                           (const char *)strings->data + i * ETH_GSTRING_LEN))
+                       break;
+       }
+
+       if (i == strings->len)
+               exit_bad_args();
+
+       memset(&gtest, 0, sizeof(gtest));
+       gtest.cmd = ETHTOOL_GPHYTEST;
+       gtest.mode = i;
+       rc = send_ioctl(ctx, &gtest);
+       if (rc < 0) {
+               rc = 1;
+               goto err;
+       }
+
+       stest = calloc(1, sizeof(*stest) + gtest.len);
+       if (!stest) {
+               perror("Unable to allocate memory");
+               rc = 1;
+               goto err;
+       }
+
+       stest->cmd = ETHTOOL_SPHYTEST;
+       stest->len = gtest.len;
+       stest->mode = i;
+
+       rc = send_ioctl(ctx, stest);
+       free(stest);
+err:
+       free(strings);
+       return rc;
+}
+
+

 static int parse_named_bool(struct cmd_context *ctx, const char *name, u8 *on) {
        if (ctx->argc < 2)

ethtool.c +3 -0
@@ -5223,6 +5335,9 @@ static const struct option {
         { "--show-fec", 1, do_gfec, "Show FEC settings"},
         { "--set-fec", 1, do_sfec, "Set FEC settings",
           "             [ encoding auto|off|rs|baser ]\n"},
+       { "--get-phy-tests", 1, do_gphytest,"Get PHY test mode(s)" },
+       { "--set-phy-test", 1, do_sphytest, "Set PHY test mode",
+         "             [ test options ]\n" },
         { "-h|--help", 0, show_usage, "Show this help" },
         { "--version", 0, do_version, "Show version number" },
         {}
Signature · 3 lines
Alexander Sverdlin
Siemens AG
www.siemens.com
maintainer
#16 lore raw
In reply to Florian Fainelli

From: Florian Fainelli <[email protected]>
Date: Fri, 27 Apr 2018 17:32:30 -0700

Quoted 18 lines

This patch series adds support for specifying PHY test modes through ethtool and paves the ground for adding support for more complex test modes that might require data to be exchanged between user and kernel space.

As an example, patches are included to add support for the IEEE electrical test modes for 100BaseT2 and 1000BaseT. Those do not require data to be passed back and forth.

I believe the infrastructure to be usable enough to add support for other things like:

- cable diagnostics
- pattern generator/waveform generator with specific pattern being indicated for instance

Questions for Andrew, and others:

Quoted 7 lines

- there could be room for adding additional ETH_TEST_FL_* values in order to help determine how the test should be running
- some of these tests can be disruptive to connectivity, the minimum we could do is stop the PHY state machine and restart it when "normal" is used to exit those test modes

Comments welcome!

Generally, no objection to providing this in the general manner you have implemented it via ethtool.

I think in order to answer the disruptive question, you need to give some information about what kind of context this stuff would be used in, and if in those contexts what the user expectations are or might be.

Are these test modes something that usually would be initiated with the interface down?

author
#17 lore raw
Quoted 27 lines · David Miller

From: Florian Fainelli <[email protected]>
Date: Fri, 27 Apr 2018 17:32:30 -0700

Quoted 24 lines

This patch series adds support for specifying PHY test modes through ethtool and paves the ground for adding support for more complex test modes that might require data to be exchanged between user and kernel space.

As an example, patches are included to add support for the IEEE electrical test modes for 100BaseT2 and 1000BaseT. Those do not require data to be passed back and forth.

I believe the infrastructure to be usable enough to add support for other things like:

- cable diagnostics
- pattern generator/waveform generator with specific pattern being indicated for instance

Questions for Andrew, and others:

- there could be room for adding additional ETH_TEST_FL_* values in order to help determine how the test should be running
- some of these tests can be disruptive to connectivity, the minimum we could do is stop the PHY state machine and restart it when "normal" is used to exit those test modes

Quoted 4 lines
Quoted 1 line

Comments welcome!

Generally, no objection to providing this in the general manner you have implemented it via ethtool.

Thanks for taking a look!

Quoted 8 lines

I think in order to answer the disruptive question, you need to give some information about what kind of context this stuff would be used in, and if in those contexts what the user expectations are or might be.

Are these test modes something that usually would be initiated with the interface down?

We expect that these commands/tests would likely be issued when the interface is up (not necessarily with a carrier state ON though) because we know for sure that drivers will have successfully connected to their PHY and there is no power management (or there is, like runtime PM) which will not prevent accesses to the MDIO interface from working.

Turning these tests on will typically result in the link partner dropping the link with us, and the interface will be non-functional as far as the data path is concerned (similar to an isolation mode). This might warrant properly reporting that to user-space through e.g: a private IFF_* value maybe?

Signature · 1 line
Florian
maintainer
#18 lore raw
Quoted 5 lines

Turning these tests on will typically result in the link partner dropping the link with us, and the interface will be non-functional as far as the data path is concerned (similar to an isolation mode). This might warrant properly reporting that to user-space through e.g: a private IFF_* value maybe?

Hi Florian

I've not looked at the code yet....

Is it also necessary to kick off auto-neg again after the test has finished, in order to reestablish the link?

Andrew

author
#19 lore raw
Quoted 6 lines · Andrew Lunn
Quoted 5 lines

Turning these tests on will typically result in the link partner dropping the link with us, and the interface will be non-functional as far as the data path is concerned (similar to an isolation mode). This might warrant properly reporting that to user-space through e.g: a private IFF_* value maybe?

Quoted 6 lines

Hi Florian

I've not looked at the code yet....

Is it also necessary to kick off auto-neg again after the test has finished, in order to reestablish the link?

It would yes. Right now there is a test mode exposed named "normal" which really, means: bring back to the PHY to an operation state. This state likely does not belong here and we would have to introduce flags instead such as start and stop the test instead.

Please review the patches when we get a chance, because I suspect this is not the only issue they have ;)

Signature · 1 line
Florian
maintainer
#20 lore raw
In reply to Florian Fainelli
Quoted 5 lines

Turning these tests on will typically result in the link partner dropping the link with us, and the interface will be non-functional as far as the data path is concerned (similar to an isolation mode). This might warrant properly reporting that to user-space through e.g: a private IFF_* value maybe?

Hi Florian

I think a IFF_* value would be a good idea. We want to give the user some indicate why they don't have working networking. ip link show showing PHY-TEST-MODE would help.

Andrew

author
#21 lore raw
Quoted 11 lines · Andrew Lunn
Quoted 5 lines

Turning these tests on will typically result in the link partner dropping the link with us, and the interface will be non-functional as far as the data path is concerned (similar to an isolation mode). This might warrant properly reporting that to user-space through e.g: a private IFF_* value maybe?

Hi Florian

I think a IFF_* value would be a good idea. We want to give the user some indicate why they don't have working networking. ip link show showing PHY-TEST-MODE would help.

IF_OPER_TESTING as defined in RFC 2863 looks like the correct way to signal that. I did a quick test and setting operstate to IFF_OPER_TESTING seems to correctly get reflected by iproute2/ifconfig which no longer see RUNNING though the interface is still UP. If we couple that with a proper phy_stop(), this would IMHO be consistent from an user experience perspective.

David, would that look reasonable to you?

Signature · 1 line
Florian
maintainer
#22 lore raw
Quoted 9 lines · Florian Fainelli
Quoted 8 lines · Andrew Lunn
Quoted 5 lines

Turning these tests on will typically result in the link partner dropping the link with us, and the interface will be non-functional as far as the data path is concerned (similar to an isolation mode). This might warrant properly reporting that to user-space through e.g: a private IFF_* value maybe?

Hi Florian

Quoted 8 lines
Quoted 3 lines

I think a IFF_* value would be a good idea. We want to give the user some indicate why they don't have working networking. ip link show showing PHY-TEST-MODE would help.

IF_OPER_TESTING as defined in RFC 2863 looks like the correct way to signal that. I did a quick test and setting operstate to IFF_OPER_TESTING seems to correctly get reflected by iproute2/ifconfig which no longer see RUNNING though the interface is still UP.

Hi Florian

I should really play with this.... but is the opstate printed by ip link show? Not showing RUNNING is not the best hint something else is going on. Actually saying TESTING somewhere is much clearer.

Andrew

author
#23 lore raw
Quoted 19 lines · Andrew Lunn
Quoted 9 lines · Florian Fainelli
Quoted 8 lines · Andrew Lunn
Quoted 5 lines

Turning these tests on will typically result in the link partner dropping the link with us, and the interface will be non-functional as far as the data path is concerned (similar to an isolation mode). This might warrant properly reporting that to user-space through e.g: a private IFF_* value maybe?

Hi Florian

Quoted 8 lines
Quoted 3 lines

I think a IFF_* value would be a good idea. We want to give the user some indicate why they don't have working networking. ip link show showing PHY-TEST-MODE would help.

IF_OPER_TESTING as defined in RFC 2863 looks like the correct way to signal that. I did a quick test and setting operstate to IFF_OPER_TESTING seems to correctly get reflected by iproute2/ifconfig which no longer see RUNNING though the interface is still UP.

Quoted 5 lines

Hi Florian

I should really play with this.... but is the opstate printed by ip link show? Not showing RUNNING is not the best hint something else is going on. Actually saying TESTING somewhere is much clearer.

The operstate is settable and gettable from iproute2:

# ip link show gphy
4: gphy@eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue
switchid 00000000 state UP mode DEFAULT group default qlen 1000
# ip link set gphy state testing

Although the kernel refuses to allow user space to set it to something different from up or down AFAICT. With my local hack to allow setting operstate from user-space through sysfs, we do see that iproute2 correctly show it:

# echo 4 > /sys/class/net/gphy/operstate
# ip link show gphy
4: gphy@eth1: <NO-CARRIER,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500
qdisc noqueue switchid 00000000 state TESTING mode DEFAULT group default
qlen 1000
    link/ether 00:10:18:de:38:1f brd ff:ff:ff:ff:ff:ff

but not from ifconfig. I was not too keen on adding a new IFF_* flag because it usually means there was nothing else that could be done, and it is disruptive to include/uapi/linux/if.h which I am afraid to break (especially for non glibc systems).

Do you think that is acceptable? There are lots of things ifconfig can't report, but at least, the state of the interface would not be "UP".

Signature · 1 line
Florian
maintainer
#24 lore raw
Quoted 6 lines
# echo 4 > /sys/class/net/gphy/operstate
# ip link show gphy
4: gphy@eth1: <NO-CARRIER,BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500
qdisc noqueue switchid 00000000 state TESTING mode DEFAULT group default
qlen 1000
    link/ether 00:10:18:de:38:1f brd ff:ff:ff:ff:ff:ff

This looks good.

I stopped using ifconfig years ago, so i personally don't care about it. If you are using old tools, you have to expect some surprises and missing information.

Andrew

maintainer
#25 lore raw
In reply to Florian Fainelli

From: Florian Fainelli <[email protected]>
Date: Tue, 1 May 2018 10:21:54 -0700

Quoted 13 lines · Andrew Lunn
Quoted 11 lines
Quoted 5 lines

Turning these tests on will typically result in the link partner dropping the link with us, and the interface will be non-functional as far as the data path is concerned (similar to an isolation mode). This might warrant properly reporting that to user-space through e.g: a private IFF_* value maybe?

Hi Florian

I think a IFF_* value would be a good idea. We want to give the user some indicate why they don't have working networking. ip link show showing PHY-TEST-MODE would help.

Quoted 8 lines

IF_OPER_TESTING as defined in RFC 2863 looks like the correct way to signal that. I did a quick test and setting operstate to IFF_OPER_TESTING seems to correctly get reflected by iproute2/ifconfig which no longer see RUNNING though the interface is still UP. If we couple that with a proper phy_stop(), this would IMHO be consistent from an user experience perspective.

David, would that look reasonable to you?

Yes, it does.