[RFC net-next 0/5] Support for PHY test modes

[RFC net-next 0/5] Support for PHY test modes

[Florian Fainelli]

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

-- 
2.14.1

[RFC net-next 1/5] net: phy: Pass stringset argument to ethtool operations

[Florian Fainelli]

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 <f.fainelli@gmail.com>
---
 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(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 9f561fe505cb..8201e8f5c028 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -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;
diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 0876aec7328c..e797e0863895 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -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);
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index 7c73808cbbde..bebcfe106283 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -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);
 
diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index 29b1c88b55cc..1835af147eea 100644
--- a/drivers/net/phy/bcm7xxx.c
+++ b/drivers/net/phy/bcm7xxx.c
@@ -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;
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 3bb6b66dc7bf..dd909799baf0 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -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;
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index b8f57e9b9379..cf962182297b 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -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);
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index de31c5170a5b..54f3e400a454 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -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);
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index c328208388da..c1a4d4d0879d 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -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);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 073235e70442..deba0c11647f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -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;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 462e9741b210..528388cda0a0 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -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
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index b849fdae7e87..0b9e2a44e1d1 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -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)
diff --git a/net/dsa/master.c b/net/dsa/master.c
index c90ee3227dea..4dbbaad2c8aa 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -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)
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 2413beb995be..4e836da4cdd3 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -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;
-- 
2.14.1

[RFC net-next 2/5] net: ethtool: Add UAPI for PHY test modes

[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 <f.fainelli@gmail.com>
---
 include/uapi/linux/ethtool.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 4ca65b56084f..a8befecfe853 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -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
-- 
2.14.1

[RFC net-next 3/5] net: ethtool: Add plumbing to get/set PHY test modes

[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 <f.fainelli@gmail.com>
---
 include/linux/phy.h | 63 ++++++++++++++++++++++++++++++++++++++++--
 net/core/ethtool.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 135 insertions(+), 7 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index deba0c11647f..449afde7ca7c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -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 {
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 0b9e2a44e1d1..52d2c9bc49b4 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -227,8 +227,9 @@ static int __ethtool_get_sset_count(struct net_device *dev, int sset)
 	if (sset == ETH_SS_PHY_TUNABLES)
 		return ARRAY_SIZE(phy_tunable_strings);
 
-	if (sset == ETH_SS_PHY_STATS && dev->phydev &&
-	    !ops->get_ethtool_phy_stats)
+	if ((sset == ETH_SS_PHY_STATS && dev->phydev &&
+	    !ops->get_ethtool_phy_stats) ||
+	    (sset == ETH_SS_PHY_TESTS && dev->phydev))
 		return phy_ethtool_get_sset_count(dev->phydev, sset);
 
 	if (ops->get_sset_count && ops->get_strings)
@@ -252,8 +253,9 @@ static void __ethtool_get_strings(struct net_device *dev,
 		memcpy(data, tunable_strings, sizeof(tunable_strings));
 	else if (stringset == ETH_SS_PHY_TUNABLES)
 		memcpy(data, phy_tunable_strings, sizeof(phy_tunable_strings));
-	else if (stringset == ETH_SS_PHY_STATS && dev->phydev &&
-		 !ops->get_ethtool_phy_stats)
+	else if ((stringset == ETH_SS_PHY_STATS && dev->phydev &&
+		 !ops->get_ethtool_phy_stats) ||
+		 (stringset == ETH_SS_PHY_TESTS && dev->phydev))
 		phy_ethtool_get_strings(dev->phydev, stringset, data);
 	else
 		/* ops->get_strings is valid because checked earlier */
@@ -2016,6 +2018,68 @@ static int ethtool_get_phy_stats(struct net_device *dev, void __user *useraddr)
 	return ret;
 }
 
+static int ethtool_get_phy_test(struct net_device *dev, void __user *useraddr)
+{
+	struct phy_device *phydev = dev->phydev;
+	struct ethtool_phy_test test;
+	int ret, test_len;
+	void *data;
+
+	if (!phydev)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&test, useraddr, sizeof(test)))
+		return -EFAULT;
+
+	test_len = phy_ethtool_get_test_len(phydev, test.mode);
+	if (test_len < 0)
+		return test_len;
+
+	test.len = test_len;
+	data = kmalloc(test_len, GFP_USER);
+	if (test_len && !data)
+		return -ENOMEM;
+
+	ret = phy_ethtool_get_test(phydev, &test, data);
+	if (ret < 0)
+		goto out;
+
+	ret = -EFAULT;
+	if (copy_to_user(useraddr, &test, sizeof(test)))
+		goto out;
+	useraddr += sizeof(test);
+	if (test_len && copy_to_user(useraddr, data, test_len))
+		goto out;
+	ret = 0;
+out:
+	kfree(data);
+	return ret;
+}
+
+static int ethtool_set_phy_test(struct net_device *dev, void __user *useraddr)
+{
+	struct phy_device *phydev = dev->phydev;
+	struct ethtool_phy_test test;
+	void *data;
+	int ret;
+
+	if (!phydev)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&test, useraddr, sizeof(test)))
+		return -EFAULT;
+
+	useraddr += sizeof(test);
+	data = memdup_user(useraddr, test.len);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	ret = phy_ethtool_set_test(phydev, &test, data);
+
+	kfree(data);
+	return ret;
+}
+
 static int ethtool_get_perm_addr(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_perm_addr epaddr;
@@ -2637,6 +2701,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_PHY_GTUNABLE:
 	case ETHTOOL_GLINKSETTINGS:
 	case ETHTOOL_GFECPARAM:
+	case ETHTOOL_GPHYTEST:
 		break;
 	default:
 		if (!ns_capable(net->user_ns, CAP_NET_ADMIN))
@@ -2852,6 +2917,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_SFECPARAM:
 		rc = ethtool_set_fecparam(dev, useraddr);
 		break;
+	case ETHTOOL_GPHYTEST:
+		rc = ethtool_get_phy_test(dev, useraddr);
+		break;
+	case ETHTOOL_SPHYTEST:
+		rc = ethtool_set_phy_test(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
2.14.1

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

[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 <f.fainelli@gmail.com>
---
 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

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index edb8b9ab827f..ef3f2f1ae990 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -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"
 
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 701ca0b8717e..e9905432e164 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -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
diff --git a/drivers/net/phy/phy-tests.c b/drivers/net/phy/phy-tests.c
new file mode 100644
index 000000000000..5709d7821925
--- /dev/null
+++ b/drivers/net/phy/phy-tests.c
@@ -0,0 +1,159 @@
+// SPDX-License-Identifier: GPL-2.0
+/* PHY library common test modes
+ */
+#include <linux/export.h>
+#include <linux/phy.h>
+
+/* genphy_get_test - Get PHY test specific data
+ * @phydev: the PHY device instance
+ * @test: the desired test mode
+ * @data: test specific data (none)
+ */
+int genphy_get_test(struct phy_device *phydev, struct ethtool_phy_test *test,
+		    u8 *data)
+{
+	if (test->mode >= PHY_STD_TEST_MODE_MAX)
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(genphy_get_test);
+
+/* 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)
+		return ret;
+
+	/* Set the desired test mode bit */
+	return phy_write(phydev, MII_CTRL1000, (test->mode + base) << shift);
+}
+EXPORT_SYMBOL_GPL(genphy_set_test);
+
+static const char *const phy_std_test_mode_str[] = {
+	"normal",
+	"100baseT2-tx-waveform",
+	"100baseT2-tx-jitter",
+	"100baseT2-tx-idle",
+	"1000baseT-tx-waveform",
+	"1000baseT-tx-jitter-master",
+	"1000baseT-tx-jitter-slave",
+	"1000BaseT-tx-distorsion"
+};
+
+/* genphy_get_test_count - Get PHY test count
+ * @phydev: the PHY device instance
+ *
+ * Returns the number of supported test modes for this PHY
+ */
+int genphy_get_test_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(phy_std_test_mode_str);
+}
+EXPORT_SYMBOL_GPL(genphy_get_test_count);
+
+/* genphy_get_test_len - Return the amount of test specific data given
+ * a specific test mode
+ * @phydev: the PHY device instance
+ * @mode: the desired test mode
+ */
+int genphy_get_test_len(struct phy_device *phydev, u32 mode)
+{
+	switch (mode) {
+	case PHY_STD_TEST_MODE_NORMAL:
+	case PHY_STD_TEST_MODE_100BASET2_1:
+	case PHY_STD_TEST_MODE_100BASET2_2:
+	case PHY_STD_TEST_MODE_100BASET2_3:
+	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:
+		/* no test specific data */
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+EXPORT_SYMBOL_GPL(genphy_get_test_len);
+
+/* genphy_get_test_strings - Obtain the PHY device supported test modes
+ * text representations
+ * @phydev: the PHY device instance
+ * @data: buffer to store strings
+ */
+void genphy_get_test_strings(struct phy_device *phydev, u8 *data)
+{
+	unsigned int i;
+
+	if (!(phydev->supported & PHY_100BT_FEATURES))
+		return;
+
+	for (i = 0; i < PHY_STD_TEST_MODE_100BASET2_MAX; i++)
+		strlcpy(data + i * ETH_GSTRING_LEN,
+			phy_std_test_mode_str[i], ETH_GSTRING_LEN);
+
+	if (!(phydev->supported & PHY_1000BT_FEATURES))
+		return;
+
+	for (; i < PHY_STD_TEST_MODE_MAX; i++)
+		strlcpy(data + i * ETH_GSTRING_LEN,
+			phy_std_test_mode_str[i], ETH_GSTRING_LEN);
+}
+EXPORT_SYMBOL_GPL(genphy_get_test_strings);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 449afde7ca7c..7155187cf268 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -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);
-- 
2.14.1

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

[Andrew Lunn]

> +/* 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);
> +	}

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?

> +
> +	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)
> +		return ret;

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

       Andrew

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

[Florian Fainelli]

On 04/30/2018 04:20 PM, Andrew Lunn wrote:
>> +/* 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);
>> +	}
> 
> 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?

We certainly should reset the PHY, thanks!

> 
>> +
>> +	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)
>> +		return ret;
> 
> Should there be something to undo this when returning to normal mode?

Yes, resetting the PHY would perform that.
-- 
Florian

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

[Woojung.Huh]

Hi Florian,

> diff --git a/drivers/net/phy/phy-tests.c b/drivers/net/phy/phy-tests.c
...
> +/* 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)
> +		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

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

[Florian Fainelli]

On 05/01/2018 10:29 AM, Woojung.Huh@microchip.com wrote:
> Hi Florian,
> 
>> diff --git a/drivers/net/phy/phy-tests.c b/drivers/net/phy/phy-tests.c
> ...
>> +/* 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)
>> +		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?
-- 
Florian

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

[Woojung.Huh]

Hi Florian,

> 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();
> 
> }
Yes, I should write pseudo code. Sorry about confusion.
User can override this function or expand to other modes.

Thanks.
Woojung

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

[Florian Fainelli]

On 05/01/2018 01:07 PM, Woojung.Huh@microchip.com wrote:
> Hi Florian,
> 
>> 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();
>>
>> }
> 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?
-- 
Florian

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

[Woojung.Huh]

Hi Florian,

> 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

[RFC net-next 5/5] net: phy: broadcom: Add support for PHY test modes

[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 <f.fainelli@gmail.com>
---
 drivers/net/phy/bcm-phy-lib.c | 15 +++++++++------
 drivers/net/phy/bcm7xxx.c     |  3 +++
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index e797e0863895..cb3081e523a5 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -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);
 
diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index 1835af147eea..1efd287ed320 100644
--- a/drivers/net/phy/bcm7xxx.c
+++ b/drivers/net/phy/bcm7xxx.c
@@ -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,				\
 }
 
-- 
2.14.1

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

[Florian Fainelli]

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

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 ethtool-copy.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/ethtool-copy.h b/ethtool-copy.h
index 8cc61e9ab40b..42fb94129da5 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -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
-- 
2.14.1

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

[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 <f.fainelli@gmail.com>
---
 ethtool.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/ethtool.c b/ethtool.c
index 3289e0f6e8ec..f02cd3560197 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -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" },
 	{}
-- 
2.14.1

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

[Sverdlin, Alexander]

Hello Florian,

On Fri, 2018-04-27 at 17:32 -0700, Florian Fainelli wrote:
> 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 <f.fainelli@gmail.com>

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.

> ---
>  ethtool.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
> 
> diff --git a/ethtool.c b/ethtool.c
> index 3289e0f6e8ec..f02cd3560197 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -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" },
>         {}

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

Re: [RFC net-next 0/5] Support for PHY test modes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 27 Apr 2018 17:32:30 -0700

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

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?

Re: [RFC net-next 0/5] Support for PHY test modes

[Florian Fainelli]

On 04/29/2018 07:55 PM, David Miller wrote:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Fri, 27 Apr 2018 17:32:30 -0700
> 
>> 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!
> 
> Generally, no objection to providing this in the general manner you
> have implemented it via ethtool.

Thanks for taking a look!

> 
> 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?
-- 
Florian

Re: [RFC net-next 0/5] Support for PHY test modes

[Andrew Lunn]

> 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

Re: [RFC net-next 0/5] Support for PHY test modes

[Florian Fainelli]

On 04/30/2018 09:40 AM, Andrew Lunn wrote:
>> 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?

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 ;)
-- 
Florian

Re: [RFC net-next 0/5] Support for PHY test modes

[Andrew Lunn]

> 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

Re: [RFC net-next 0/5] Support for PHY test modes

[Florian Fainelli]

On 04/30/2018 04:24 PM, Andrew Lunn wrote:
>> 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?
-- 
Florian

Re: [RFC net-next 0/5] Support for PHY test modes

[Andrew Lunn]

On Tue, May 01, 2018 at 10:21:54AM -0700, Florian Fainelli wrote:
> On 04/30/2018 04:24 PM, Andrew Lunn wrote:
> >> 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.

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

Re: [RFC net-next 0/5] Support for PHY test modes

[Florian Fainelli]

On 05/01/2018 10:47 AM, Andrew Lunn wrote:
> On Tue, May 01, 2018 at 10:21:54AM -0700, Florian Fainelli wrote:
>> On 04/30/2018 04:24 PM, Andrew Lunn wrote:
>>>> 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.
> 
> 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".
-- 
Florian

Re: [RFC net-next 0/5] Support for PHY test modes

[Andrew Lunn]

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

Re: [RFC net-next 0/5] Support for PHY test modes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 1 May 2018 10:21:54 -0700

> On 04/30/2018 04:24 PM, Andrew Lunn wrote:
>>> 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?

Yes, it does.