[PATCH net-next v1 0/9] Ethernet Cable test support

[PATCH net-next v1 0/9] Ethernet Cable test support

[Andrew Lunn]

Many copper Ethernet PHY have support for performing diagnostics of
the cable. Are the cable shorted, broken, not plugged into anything at
the other end? And they can report roughly how far along the cable any
fault is.

Add infrastructure in ethtool and phylib support for triggering a
cable test and reporting the results. The Marvell 1G PHY driver is
then extended to make use of this infrastructure.

For testing, a modified ethtool(1) can be found here:
https://github.com/lunn/ethtool.git feature/cable-test-v2. This also
contains extra code for TDR dump, which will be added to the kernel in
a later patch series.

Thanks to Chris Healy for extensive testing.

Andrew Lunn (9):
  net: phy: Add cable test support to state machine
  net: phy: Add support for polling cable test
  net: ethtool: netlink: Add support for triggering a cable test
  net: ethtool: Add attributes for cable test reports
  net: ethtool: Make helpers public
  net: ethtool: Add infrastructure for reporting cable test results
  net: ethtool: Add helpers for reporting test results
  net: phy: marvell: Add cable test support
  net: phy: Put interface into oper testing during cable test

 Documentation/networking/ethtool-netlink.rst |  53 ++++-
 drivers/net/phy/marvell.c                    | 202 +++++++++++++++++++
 drivers/net/phy/phy.c                        |  94 +++++++++
 include/linux/ethtool_netlink.h              |  33 +++
 include/linux/phy.h                          |  42 ++++
 include/uapi/linux/ethtool_netlink.h         |  59 ++++++
 net/Kconfig                                  |   1 +
 net/ethtool/Makefile                         |   2 +-
 net/ethtool/cabletest.c                      | 179 ++++++++++++++++
 net/ethtool/netlink.c                        |  10 +-
 net/ethtool/netlink.h                        |   4 +
 11 files changed, 674 insertions(+), 5 deletions(-)
 create mode 100644 net/ethtool/cabletest.c

-- 
2.26.1

[PATCH net-next v1 1/9] net: phy: Add cable test support to state machine

[Andrew Lunn]

Running a cable test is desruptive to normal operation of the PHY and
can take a 5 to 10 seconds to complete. The RTNL lock cannot be held
for this amount of time, and add a new state to the state machine for
running a cable test.

The driver is expected to implement two functions. The first is used
to start a cable test. Once the test has started, it should return.

The second function is called once per second, or on interrupt to
check if the cable test is complete, and to allow the PHY to report
the status.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy.c | 65 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h   | 28 +++++++++++++++++++
 2 files changed, 93 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 72c69a9c8a98..4a6279c4a3a3 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -15,6 +15,7 @@
 #include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/netdevice.h>
+#include <linux/netlink.h>
 #include <linux/etherdevice.h>
 #include <linux/skbuff.h>
 #include <linux/mm.h>
@@ -44,6 +45,7 @@ static const char *phy_state_to_str(enum phy_state st)
 	PHY_STATE_STR(UP)
 	PHY_STATE_STR(RUNNING)
 	PHY_STATE_STR(NOLINK)
+	PHY_STATE_STR(CABLETEST)
 	PHY_STATE_STR(HALTED)
 	}
 
@@ -470,6 +472,51 @@ static void phy_trigger_machine(struct phy_device *phydev)
 	phy_queue_state_machine(phydev, 0);
 }
 
+static void phy_cable_test_abort(struct phy_device *phydev)
+{
+	genphy_soft_reset(phydev);
+}
+
+int phy_start_cable_test(struct phy_device *phydev,
+			 struct netlink_ext_ack *extack)
+{
+	int err;
+
+	if (!(phydev->drv &&
+	      phydev->drv->cable_test_start &&
+	      phydev->drv->cable_test_get_status)) {
+		NL_SET_ERR_MSG(extack,
+			       "PHY driver does not support cable testing");
+		return -EOPNOTSUPP;
+	}
+
+	mutex_lock(&phydev->lock);
+	if (phydev->state < PHY_UP ||
+	    phydev->state >= PHY_CABLETEST) {
+		NL_SET_ERR_MSG(extack,
+			       "PHY not configured. Try setting interface up");
+		err = -EBUSY;
+		goto out;
+	}
+
+	/* Mark the carrier down until the test is complete */
+	phy_link_down(phydev, true);
+
+	err = phydev->drv->cable_test_start(phydev);
+	if (err) {
+		phy_link_up(phydev);
+		goto out;
+	}
+
+	phydev->state = PHY_CABLETEST;
+
+out:
+	mutex_unlock(&phydev->lock);
+
+	return err;
+}
+EXPORT_SYMBOL(phy_start_cable_test);
+
 static int phy_config_aneg(struct phy_device *phydev)
 {
 	if (phydev->drv->config_aneg)
@@ -808,6 +855,9 @@ void phy_stop(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
+	if (phydev->state == PHY_CABLETEST)
+		phy_cable_test_abort(phydev);
+
 	if (phydev->sfp_bus)
 		sfp_upstream_stop(phydev->sfp_bus);
 
@@ -870,6 +920,7 @@ void phy_state_machine(struct work_struct *work)
 			container_of(dwork, struct phy_device, state_queue);
 	bool needs_aneg = false, do_suspend = false;
 	enum phy_state old_state;
+	bool finished = false;
 	int err = 0;
 
 	mutex_lock(&phydev->lock);
@@ -888,6 +939,20 @@ void phy_state_machine(struct work_struct *work)
 	case PHY_RUNNING:
 		err = phy_check_link_status(phydev);
 		break;
+	case PHY_CABLETEST:
+		err = phydev->drv->cable_test_get_status(phydev, &finished);
+		if (err) {
+			phy_cable_test_abort(phydev);
+			needs_aneg = true;
+			phydev->state = PHY_UP;
+			break;
+		}
+
+		if (finished) {
+			needs_aneg = true;
+			phydev->state = PHY_UP;
+		}
+		break;
 	case PHY_HALTED:
 		if (phydev->link) {
 			phydev->link = 0;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e2bfb9240587..916a5eb969a1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -15,6 +15,7 @@
 #include <linux/spinlock.h>
 #include <linux/ethtool.h>
 #include <linux/linkmode.h>
+#include <linux/netlink.h>
 #include <linux/mdio.h>
 #include <linux/mii.h>
 #include <linux/mii_timestamper.h>
@@ -343,6 +344,12 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
  * - irq or timer will set NOLINK if link goes down
  * - phy_stop moves to HALTED
  *
+ * CABLETEST: PHY is performing a cable test. Packet reception/sending
+ * is not expected to work, carrier will be indicated as down. PHY will be
+ * poll once per second, or on interrupt for it current state.
+ * Once complete, move to UP to restart the PHY.
+ * - phy_stop aborts the running test and moves to HALTED
+ *
  * HALTED: PHY is up, but no polling or interrupts are done. Or
  * PHY is in an error state.
  * - phy_start moves to UP
@@ -354,6 +361,7 @@ enum phy_state {
 	PHY_UP,
 	PHY_RUNNING,
 	PHY_NOLINK,
+	PHY_CABLETEST,
 };
 
 /**
@@ -653,6 +661,13 @@ struct phy_driver {
 	int (*module_eeprom)(struct phy_device *dev,
 			     struct ethtool_eeprom *ee, u8 *data);
 
+	/* Start a cable test */
+	int (*cable_test_start)(struct phy_device *dev);
+	/* Once per second, or on interrupt, request the status of the
+	 * test.
+	 */
+	int (*cable_test_get_status)(struct phy_device *dev, bool *finished);
+
 	/* Get statistics from the phy using ethtool */
 	int (*get_sset_count)(struct phy_device *dev);
 	void (*get_strings)(struct phy_device *dev, u8 *data);
@@ -1191,6 +1206,19 @@ int phy_speed_up(struct phy_device *phydev);
 int phy_restart_aneg(struct phy_device *phydev);
 int phy_reset_after_clk_enable(struct phy_device *phydev);
 
+#if IS_ENABLED(CONFIG_PHYLIB)
+int phy_start_cable_test(struct phy_device *phydev,
+			 struct netlink_ext_ack *extack);
+#else
+static inline
+int phy_start_cable_test(struct phy_device *phydev,
+			 struct netlink_ext_ack *extack)
+{
+	NL_SET_ERR_MSG(extack, "Kernel not compiled with PHYLIB support");
+	return -EOPNOTSUPP;
+}
+#endif
+
 static inline void phy_device_reset(struct phy_device *phydev, int value)
 {
 	mdio_device_reset(&phydev->mdio, value);
-- 
2.26.1

[PATCH net-next v1 2/9] net: phy: Add support for polling cable test

[Andrew Lunn]

Some PHYs are not capable of generating interrupts when a cable test
finished. They do however support interrupts for normal operations,
like link up/down. As such, the PHY state machine would normally not
poll the PHY.

Add support for indicating the PHY state machine must poll the PHY
when performing a cable test.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy.c | 2 ++
 include/linux/phy.h   | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 4a6279c4a3a3..242a7c1c4b6c 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -510,6 +510,8 @@ int phy_start_cable_test(struct phy_device *phydev,
 
 	phydev->state = PHY_CABLETEST;
 
+	if (phy_polling_mode(phydev))
+		phy_trigger_machine(phydev);
 out:
 	mutex_unlock(&phydev->lock);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 916a5eb969a1..593da2c6041d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -78,6 +78,7 @@ extern const int phy_10gbit_features_array[1];
 
 #define PHY_IS_INTERNAL		0x00000001
 #define PHY_RST_AFTER_CLK_EN	0x00000002
+#define PHY_POLL_CABLE_TEST	0x00000004
 #define MDIO_DEVICE_IS_PHY	0x80000000
 
 /* Interface Mode definitions */
@@ -1025,6 +1026,10 @@ static inline bool phy_interrupt_is_valid(struct phy_device *phydev)
  */
 static inline bool phy_polling_mode(struct phy_device *phydev)
 {
+	if (phydev->state == PHY_CABLETEST)
+		if (phydev->drv->flags & PHY_POLL_CABLE_TEST)
+			return true;
+
 	return phydev->irq == PHY_POLL;
 }
 
-- 
2.26.1

[PATCH net-next v1 3/9] net: ethtool: netlink: Add support for triggering a cable test

[Andrew Lunn]

Add new ethtool netlink calls to trigger the starting of a PHY cable
test.

Add Kconfig'ury to ETHTOOL_NETLINK so that PHYLIB is not a module when
ETHTOOL_NETLINK is builtin, which would result in kernel linking errors.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/networking/ethtool-netlink.rst | 18 ++++-
 include/uapi/linux/ethtool_netlink.h         | 14 ++++
 net/Kconfig                                  |  1 +
 net/ethtool/Makefile                         |  2 +-
 net/ethtool/cabletest.c                      | 82 ++++++++++++++++++++
 net/ethtool/netlink.c                        |  6 ++
 net/ethtool/netlink.h                        |  2 +
 7 files changed, 122 insertions(+), 3 deletions(-)
 create mode 100644 net/ethtool/cabletest.c

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 567326491f80..0c354567e991 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -204,6 +204,7 @@ Userspace to kernel:
   ``ETHTOOL_MSG_EEE_GET``               get EEE settings
   ``ETHTOOL_MSG_EEE_SET``               set EEE settings
   ``ETHTOOL_MSG_TSINFO_GET``		get timestamping info
+  ``ETHTOOL_MSG_CABLE_TEST_ACT``        action start cable test
   ===================================== ================================
 
 Kernel to userspace:
@@ -235,6 +236,7 @@ Kernel to userspace:
   ``ETHTOOL_MSG_EEE_GET_REPLY``         EEE settings
   ``ETHTOOL_MSG_EEE_NTF``               EEE settings
   ``ETHTOOL_MSG_TSINFO_GET_REPLY``	timestamping info
+  ``ETHTOOL_MSG_CABLE_TEST_ACT_REPLY``  Cable test action result
   ===================================== =================================
 
 ``GET`` requests are sent by userspace applications to retrieve device
@@ -758,7 +760,6 @@ Kernel checks that requested channel counts do not exceed limits reported by
 driver. Driver may impose additional constraints and may not suspport all
 attributes.
 
-
 COALESCE_GET
 ============
 
@@ -955,13 +956,25 @@ Kernel response contents:
 is no special value for this case). The bitset attributes are omitted if they
 would be empty (no bit set).
 
+CABLE_TEST
+==========
+
+Start a cable test.
+
+Request contents:
+
+  ====================================  ======  ==========================
+  ``ETHTOOL_A_CABLE_TEST_HEADER``       nested  request header
+  ====================================  ======  ==========================
+
 
 Request translation
 ===================
 
 The following table maps ioctl commands to netlink commands providing their
 functionality. Entries with "n/a" in right column are commands which do not
-have their netlink replacement yet.
+have their netlink replacement yet. Entries which "n/a" in the left column
+are netlink only.
 
   =================================== =====================================
   ioctl command                       netlink command
@@ -1050,4 +1063,5 @@ have their netlink replacement yet.
   ``ETHTOOL_PHY_STUNABLE``            n/a
   ``ETHTOOL_GFECPARAM``               n/a
   ``ETHTOOL_SFECPARAM``               n/a
+  n/a                                 ''ETHTOOL_MSG_CABLE_TEST_ACT''
   =================================== =====================================
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 7fde76366ba4..598d0b502ebd 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -39,6 +39,7 @@ enum {
 	ETHTOOL_MSG_EEE_GET,
 	ETHTOOL_MSG_EEE_SET,
 	ETHTOOL_MSG_TSINFO_GET,
+	ETHTOOL_MSG_CABLE_TEST_ACT,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_USER_CNT,
@@ -74,6 +75,7 @@ enum {
 	ETHTOOL_MSG_EEE_GET_REPLY,
 	ETHTOOL_MSG_EEE_NTF,
 	ETHTOOL_MSG_TSINFO_GET_REPLY,
+	ETHTOOL_MSG_CABLE_TEST_ACT_REPLY,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -401,6 +403,18 @@ enum {
 	/* add new constants above here */
 	__ETHTOOL_A_TSINFO_CNT,
 	ETHTOOL_A_TSINFO_MAX = (__ETHTOOL_A_TSINFO_CNT - 1)
+
+};
+
+/* CABLE TEST */
+
+enum {
+	ETHTOOL_A_CABLE_TEST_UNSPEC,
+	ETHTOOL_A_CABLE_TEST_HEADER,		/* nest - _A_HEADER_* */
+
+	/* add new constants above here */
+	__ETHTOOL_A_CABLE_TEST_CNT,
+	ETHTOOL_A_CABLE_TEST_MAX = __ETHTOOL_A_CABLE_TEST_CNT - 1
 };
 
 /* generic netlink info */
diff --git a/net/Kconfig b/net/Kconfig
index df8d8c9bd021..d72d0d804456 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -455,6 +455,7 @@ config FAILOVER
 config ETHTOOL_NETLINK
 	bool "Netlink interface for ethtool"
 	default y
+	depends on PHYLIB=y || PHYLIB=n
 	help
 	  An alternative userspace interface for ethtool based on generic
 	  netlink. It provides better extensibility and some new features,
diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
index 6c360c9c9370..0c2b94f20499 100644
--- a/net/ethtool/Makefile
+++ b/net/ethtool/Makefile
@@ -6,4 +6,4 @@ obj-$(CONFIG_ETHTOOL_NETLINK)	+= ethtool_nl.o
 
 ethtool_nl-y	:= netlink.o bitset.o strset.o linkinfo.o linkmodes.o \
 		   linkstate.o debug.o wol.o features.o privflags.o rings.o \
-		   channels.o coalesce.o pause.o eee.o tsinfo.o
+		   channels.o coalesce.o pause.o eee.o tsinfo.o cabletest.o
diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
new file mode 100644
index 000000000000..44b8165ac66e
--- /dev/null
+++ b/net/ethtool/cabletest.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/phy.h>
+#include "netlink.h"
+#include "common.h"
+#include "bitset.h"
+
+struct cable_test_req_info {
+	struct ethnl_req_info		base;
+};
+
+struct cable_test_reply_data {
+	struct ethnl_reply_data		base;
+};
+
+#define CABLE_TEST_REPDATA(__reply_base) \
+	container_of(__reply_base, struct cable_test_reply_data, base)
+
+static const struct nla_policy
+cable_test_get_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
+	[ETHTOOL_A_CABLE_TEST_UNSPEC]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_CABLE_TEST_HEADER]		= { .type = NLA_NESTED },
+};
+
+const struct ethnl_request_ops ethnl_cable_test_act_ops = {
+	.request_cmd		= ETHTOOL_MSG_CABLE_TEST_ACT,
+	.reply_cmd		= ETHTOOL_MSG_CABLE_TEST_ACT_REPLY,
+	.hdr_attr		= ETHTOOL_A_CABLE_TEST_HEADER,
+	.max_attr		= ETHTOOL_A_CABLE_TEST_MAX,
+	.req_info_size		= sizeof(struct cable_test_req_info),
+	.reply_data_size	= sizeof(struct cable_test_reply_data),
+	.request_policy		= cable_test_get_policy,
+};
+
+/* CABLE_TEST_ACT */
+
+static const struct nla_policy
+cable_test_set_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
+	[ETHTOOL_A_CABLE_TEST_UNSPEC]		= { .type = NLA_REJECT },
+	[ETHTOOL_A_CABLE_TEST_HEADER]		= { .type = NLA_NESTED },
+};
+
+int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
+{
+	struct nlattr *tb[ETHTOOL_A_CABLE_TEST_MAX + 1];
+	struct ethnl_req_info req_info = {};
+	struct net_device *dev;
+	int ret;
+
+	ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
+			  ETHTOOL_A_CABLE_TEST_MAX,
+			  cable_test_set_policy, info->extack);
+	if (ret < 0)
+		return ret;
+
+	ret = ethnl_parse_header_dev_get(&req_info,
+					 tb[ETHTOOL_A_CABLE_TEST_HEADER],
+					 genl_info_net(info), info->extack,
+					 true);
+	if (ret < 0)
+		return ret;
+
+	dev = req_info.dev;
+	if (!dev->phydev) {
+		ret = -EOPNOTSUPP;
+		goto out_dev_put;
+	}
+
+	rtnl_lock();
+	ret = ethnl_ops_begin(dev);
+	if (ret < 0)
+		goto out_rtnl;
+
+	ret = phy_start_cable_test(dev->phydev, info->extack);
+
+	ethnl_ops_complete(dev);
+out_rtnl:
+	rtnl_unlock();
+out_dev_put:
+	dev_put(dev);
+	return ret;
+}
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 0c772318c023..56efd12c58ba 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -231,6 +231,7 @@ ethnl_default_requests[__ETHTOOL_MSG_USER_CNT] = {
 	[ETHTOOL_MSG_PAUSE_GET]		= &ethnl_pause_request_ops,
 	[ETHTOOL_MSG_EEE_GET]		= &ethnl_eee_request_ops,
 	[ETHTOOL_MSG_TSINFO_GET]	= &ethnl_tsinfo_request_ops,
+	[ETHTOOL_MSG_CABLE_TEST_ACT]	= &ethnl_cable_test_act_ops,
 };
 
 static struct ethnl_dump_ctx *ethnl_dump_context(struct netlink_callback *cb)
@@ -839,6 +840,11 @@ static const struct genl_ops ethtool_genl_ops[] = {
 		.dumpit	= ethnl_default_dumpit,
 		.done	= ethnl_default_done,
 	},
+	{
+		.cmd	= ETHTOOL_MSG_CABLE_TEST_ACT,
+		.flags	= GENL_UNS_ADMIN_PERM,
+		.doit	= ethnl_act_cable_test,
+	},
 };
 
 static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 81b8fa020bcb..03e65e2b9e3d 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -345,6 +345,7 @@ extern const struct ethnl_request_ops ethnl_coalesce_request_ops;
 extern const struct ethnl_request_ops ethnl_pause_request_ops;
 extern const struct ethnl_request_ops ethnl_eee_request_ops;
 extern const struct ethnl_request_ops ethnl_tsinfo_request_ops;
+extern const struct ethnl_request_ops ethnl_cable_test_act_ops;
 
 int ethnl_set_linkinfo(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info);
@@ -357,5 +358,6 @@ int ethnl_set_channels(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_coalesce(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_pause(struct sk_buff *skb, struct genl_info *info);
 int ethnl_set_eee(struct sk_buff *skb, struct genl_info *info);
+int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info);
 
 #endif /* _NET_ETHTOOL_NETLINK_H */
-- 
2.26.1

[PATCH net-next v1 4/9] net: ethtool: Add attributes for cable test reports

[Andrew Lunn]

Add the attributes needed to report cable test results to userspace.
The reports are expected to be per twisted pair. A nested property per
pair can report the result of the cable test. A nested property can
also report the length of the cable to any fault.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/networking/ethtool-netlink.rst | 35 +++++++++++++++
 include/uapi/linux/ethtool_netlink.h         | 47 +++++++++++++++++++-
 2 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 0c354567e991..89fd321b0e29 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -967,6 +967,41 @@ Request contents:
   ``ETHTOOL_A_CABLE_TEST_HEADER``       nested  request header
   ====================================  ======  ==========================
 
+Notify contents:
+
+An Ethernet cable typically contains 1, 2 or 4 pairs. The length of
+the pair can only be measured when there is a fault in the pair and
+hence a reflection. Information about the fault may not be available,
+depends on the specific hardware. Hence the contents of the notify
+message is mostly optional. The attributes can be repeated an
+arbitrary number of times, in an arbitrary order, for an arbitrary
+number of pairs.
+
+The example shows a T2 cable, i.e. two pairs. One pair is O.K, and
+hence has no length information. The second pair has a fault and does
+have length information.
+
+ +-------------------------------------------+--------+-----------------------+
+ | ``ETHTOOL_A_CABLE_TEST_HEADER``           | nested | reply header          |
+ +-------------------------------------------+--------+-----------------------+
+ | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test result     |
+ +-+-----------------------------------------+--------+-----------------------+
+ | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
+ +-+-----------------------------------------+--------+-----------------------+
+ | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
+ +-+-----------------------------------------+--------+-----------------------+
+ | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test results    |
+ +-+-----------------------------------------+--------+-----------------------+
+ | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
+ +-+-----------------------------------------+--------+-----------------------+
+ | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
+ +-+-----------------------------------------+--------+-----------------------+
+ | ``ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH`` | nested | cable length          |
+ +-+-----------------------------------------+--------+-----------------------+
+ | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR``   | u8     | pair number           |
+ +-+-----------------------------------------+--------+-----------------------+
+ | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_CM``     | u8     | length in cm          |
+ +-+-----------------------------------------+--------+-----------------------+
 
 Request translation
 ===================
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 598d0b502ebd..05ef5048e4fc 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -76,6 +76,7 @@ enum {
 	ETHTOOL_MSG_EEE_NTF,
 	ETHTOOL_MSG_TSINFO_GET_REPLY,
 	ETHTOOL_MSG_CABLE_TEST_ACT_REPLY,
+	ETHTOOL_MSG_CABLE_TEST_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -403,7 +404,6 @@ enum {
 	/* add new constants above here */
 	__ETHTOOL_A_TSINFO_CNT,
 	ETHTOOL_A_TSINFO_MAX = (__ETHTOOL_A_TSINFO_CNT - 1)
-
 };
 
 /* CABLE TEST */
@@ -417,6 +417,51 @@ enum {
 	ETHTOOL_A_CABLE_TEST_MAX = __ETHTOOL_A_CABLE_TEST_CNT - 1
 };
 
+/* CABLE TEST NOTIFY */
+enum {
+	ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC,
+	ETHTOOL_A_CABLE_RESULT_CODE_OK,
+	ETHTOOL_A_CABLE_RESULT_CODE_OPEN,
+	ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT,
+	ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT,
+};
+
+enum {
+	ETHTOOL_A_CABLE_PAIR_0,
+	ETHTOOL_A_CABLE_PAIR_1,
+	ETHTOOL_A_CABLE_PAIR_2,
+	ETHTOOL_A_CABLE_PAIR_3,
+};
+
+enum {
+	ETHTOOL_A_CABLE_RESULT_UNSPEC,
+	ETHTOOL_A_CABLE_RESULT_PAIR,		/* u8 ETHTOOL_A_CABLE_PAIR_ */
+	ETHTOOL_A_CABLE_RESULT_CODE,		/* u8 ETHTOOL_A_CABLE_RESULT_CODE_ */
+
+	__ETHTOOL_A_CABLE_RESULT_CNT,
+	ETHTOOL_A_CABLE_RESULT_MAX = (__ETHTOOL_A_CABLE_RESULT_CNT - 1)
+};
+
+enum {
+	ETHTOOL_A_CABLE_FAULT_LENGTH_UNSPEC,
+	ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR,	/* u8 ETHTOOL_A_CABLE_PAIR_ */
+	ETHTOOL_A_CABLE_FAULT_LENGTH_CM,	/* u16 */
+
+	__ETHTOOL_A_CABLE_FAULT_LENGTH_CNT,
+	ETHTOOL_A_CABLE_FAULT_LENGTH_MAX = (__ETHTOOL_A_CABLE_FAULT_LENGTH_CNT - 1)
+};
+
+enum {
+	ETHTOOL_A_CABLE_TEST_NTF_UNSPEC,
+	ETHTOOL_A_CABLE_TEST_NTF_DEV,		/* nest - ETHTOOL_A_DEV_* */
+	ETHTOOL_A_CABLE_TEST_NTF_NEST,		/* nest - of results: */
+	ETHTOOL_A_CABLE_TEST_NTF_RESULT,	/* nest - ETHTOOL_A_CABLE_RESULT_ */
+	ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH,	/* nest - ETHTOOL_A_CABLE_FAULT_LENGTH_ */
+
+	__ETHTOOL_A_CABLE_TEST_NTF_CNT,
+	ETHTOOL_A_CABLE_TEST_NTF_MAX = (__ETHTOOL_A_CABLE_TEST_NTF_CNT - 1)
+};
+
 /* generic netlink info */
 #define ETHTOOL_GENL_NAME "ethtool"
 #define ETHTOOL_GENL_VERSION 1
-- 
2.26.1

[PATCH net-next v1 5/9] net: ethtool: Make helpers public

[Andrew Lunn]

Make some helpers for building ethtool netlink messages available
outside the compilation unit, so they can be used for building
messages which are not simple get/set.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/ethtool/netlink.c | 4 ++--
 net/ethtool/netlink.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 56efd12c58ba..ffd3d1e40293 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -181,13 +181,13 @@ struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd,
 	return NULL;
 }
 
-static void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd)
+void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd)
 {
 	return genlmsg_put(skb, 0, ++ethnl_bcast_seq, &ethtool_genl_family, 0,
 			   cmd);
 }
 
-static int ethnl_multicast(struct sk_buff *skb, struct net_device *dev)
+int ethnl_multicast(struct sk_buff *skb, struct net_device *dev)
 {
 	return genlmsg_multicast_netns(&ethtool_genl_family, dev_net(dev), skb,
 				       0, ETHNL_MCGRP_MONITOR, GFP_KERNEL);
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 03e65e2b9e3d..f948eab5fe16 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -19,6 +19,8 @@ int ethnl_fill_reply_header(struct sk_buff *skb, struct net_device *dev,
 struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd,
 				 u16 hdr_attrtype, struct genl_info *info,
 				 void **ehdrp);
+void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd);
+int ethnl_multicast(struct sk_buff *skb, struct net_device *dev);
 
 /**
  * ethnl_strz_size() - calculate attribute length for fixed size string
-- 
2.26.1

[PATCH net-next v1 6/9] net: ethtool: Add infrastructure for reporting cable test results

[Andrew Lunn]

Provide infrastructure for PHY drivers to report the cable test
results.  A netlink skb is associated to the phydev. Helpers will be
added which can add results to this skb. Once the test has finished
the results are sent to user space.

When netlink ethtool is not part of the kernel configuration stubs are
provided. It is also impossible to trigger a cable test, so the error
code returned by the alloc function is of no consequence.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy.c           | 21 ++++++++++++--
 include/linux/ethtool_netlink.h | 20 +++++++++++++
 include/linux/phy.h             |  5 ++++
 net/ethtool/cabletest.c         | 50 +++++++++++++++++++++++++++++++++
 4 files changed, 94 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 242a7c1c4b6c..88275d971f14 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/mii.h>
 #include <linux/ethtool.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
 #include <linux/sfp.h>
@@ -30,6 +31,9 @@
 #include <linux/io.h>
 #include <linux/uaccess.h>
 #include <linux/atomic.h>
+#include <net/netlink.h>
+#include <net/genetlink.h>
+#include <net/sock.h>
 
 #define PHY_STATE_TIME	HZ
 
@@ -474,13 +478,14 @@ static void phy_trigger_machine(struct phy_device *phydev)
 
 static void phy_cable_test_abort(struct phy_device *phydev)
 {
+	ethnl_cable_test_finished(phydev);
 	genphy_soft_reset(phydev);
 }
 
 int phy_start_cable_test(struct phy_device *phydev,
 			 struct netlink_ext_ack *extack)
 {
-	int err;
+	int err = -ENOMEM;
 
 	if (!(phydev->drv &&
 	      phydev->drv->cable_test_start &&
@@ -499,19 +504,30 @@ int phy_start_cable_test(struct phy_device *phydev,
 		goto out;
 	}
 
+	err = ethnl_cable_test_alloc(phydev);
+	if (err)
+		goto out;
+
 	/* Mark the carrier down until the test is complete */
 	phy_link_down(phydev, true);
 
 	err = phydev->drv->cable_test_start(phydev);
 	if (err) {
 		phy_link_up(phydev);
-		goto out;
+		goto out_free;
 	}
 
 	phydev->state = PHY_CABLETEST;
 
 	if (phy_polling_mode(phydev))
 		phy_trigger_machine(phydev);
+
+	mutex_unlock(&phydev->lock);
+
+	return 0;
+
+out_free:
+	ethnl_cable_test_free(phydev);
 out:
 	mutex_unlock(&phydev->lock);
 
@@ -951,6 +967,7 @@ void phy_state_machine(struct work_struct *work)
 		}
 
 		if (finished) {
+			ethnl_cable_test_finished(phydev);
 			needs_aneg = true;
 			phydev->state = PHY_UP;
 		}
diff --git a/include/linux/ethtool_netlink.h b/include/linux/ethtool_netlink.h
index d01b77887f82..7d763ba22f6f 100644
--- a/include/linux/ethtool_netlink.h
+++ b/include/linux/ethtool_netlink.h
@@ -14,4 +14,24 @@ enum ethtool_multicast_groups {
 	ETHNL_MCGRP_MONITOR,
 };
 
+struct phy_device;
+
+#if IS_ENABLED(CONFIG_ETHTOOL_NETLINK)
+int ethnl_cable_test_alloc(struct phy_device *phydev);
+void ethnl_cable_test_free(struct phy_device *phydev);
+void ethnl_cable_test_finished(struct phy_device *phydev);
+#else
+static inline int ethnl_cable_test_alloc(struct phy_device *phydev)
+{
+	return -ENOTSUPP;
+}
+
+static inline void ethnl_cable_test_free(struct phy_device *phydev)
+{
+}
+
+static inline void ethnl_cable_test_finished(struct phy_device *phydev)
+{
+}
+#endif /* IS_ENABLED(ETHTOOL_NETLINK) */
 #endif /* _LINUX_ETHTOOL_NETLINK_H_ */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 593da2c6041d..ee69f781995a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -487,6 +487,11 @@ struct phy_device {
 	/* For use by PHYs to maintain extra state */
 	void *priv;
 
+	/* Reporting cable test results */
+	struct sk_buff *skb;
+	void *ehdr;
+	struct nlattr *nest;
+
 	/* Interrupt and Polling infrastructure */
 	struct delayed_work state_queue;
 
diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index 44b8165ac66e..7d73239b9ead 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
 #include <linux/phy.h>
+#include <linux/ethtool_netlink.h>
 #include "netlink.h"
 #include "common.h"
 #include "bitset.h"
@@ -80,3 +81,52 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	dev_put(dev);
 	return ret;
 }
+
+int ethnl_cable_test_alloc(struct phy_device *phydev)
+{
+	int err = -ENOMEM;
+
+	phydev->skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (!phydev->skb)
+		goto out;
+
+	phydev->ehdr = ethnl_bcastmsg_put(phydev->skb,
+					  ETHTOOL_MSG_CABLE_TEST_NTF);
+	if (!phydev->ehdr) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	err = ethnl_fill_reply_header(phydev->skb, phydev->attached_dev,
+				      ETHTOOL_A_CABLE_TEST_NTF_DEV);
+	if (err)
+		goto out;
+
+	phydev->nest = nla_nest_start(phydev->skb,
+				      ETHTOOL_A_CABLE_TEST_NTF_NEST);
+	if (!phydev->nest)
+		goto out;
+
+	return 0;
+
+out:
+	nlmsg_free(phydev->skb);
+	return err;
+}
+EXPORT_SYMBOL_GPL(ethnl_cable_test_alloc);
+
+void ethnl_cable_test_free(struct phy_device *phydev)
+{
+	nlmsg_free(phydev->skb);
+}
+EXPORT_SYMBOL_GPL(ethnl_cable_test_free);
+
+void ethnl_cable_test_finished(struct phy_device *phydev)
+{
+	nla_nest_end(phydev->skb, phydev->nest);
+
+	genlmsg_end(phydev->skb, phydev->ehdr);
+
+	ethnl_multicast(phydev->skb, phydev->attached_dev);
+}
+EXPORT_SYMBOL_GPL(ethnl_cable_test_finished);
-- 
2.26.1

[PATCH net-next v1 7/9] net: ethtool: Add helpers for reporting test results

[Andrew Lunn]

The PHY drivers can use these helpers for reporting the results. The
results get translated into netlink attributes which are added to the
pre-allocated skbuf.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/linux/ethtool_netlink.h | 13 +++++++++
 include/linux/phy.h             |  4 +++
 net/ethtool/cabletest.c         | 47 +++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/include/linux/ethtool_netlink.h b/include/linux/ethtool_netlink.h
index 7d763ba22f6f..31fc6224c97f 100644
--- a/include/linux/ethtool_netlink.h
+++ b/include/linux/ethtool_netlink.h
@@ -20,6 +20,8 @@ struct phy_device;
 int ethnl_cable_test_alloc(struct phy_device *phydev);
 void ethnl_cable_test_free(struct phy_device *phydev);
 void ethnl_cable_test_finished(struct phy_device *phydev);
+int ethnl_cable_test_result(struct phy_device *phydev, u8 pair, u16 result);
+int ethnl_cable_test_fault_length(struct phy_device *phydev, u8 pair, u16 cm);
 #else
 static inline int ethnl_cable_test_alloc(struct phy_device *phydev)
 {
@@ -33,5 +35,16 @@ static inline void ethnl_cable_test_free(struct phy_device *phydev)
 static inline void ethnl_cable_test_finished(struct phy_device *phydev)
 {
 }
+static inline int ethnl_cable_test_result(struct phy_device *phydev, u8 pair,
+					  u16 result)
+{
+	return -ENOTSUPP;
+}
+
+static inline int ethnl_cable_test_fault_length(struct phy_device *phydev,
+						u8 pair, u16 cm)
+{
+	return -ENOTSUPP;
+}
 #endif /* IS_ENABLED(ETHTOOL_NETLINK) */
 #endif /* _LINUX_ETHTOOL_NETLINK_H_ */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ee69f781995a..856b4293a645 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1229,6 +1229,10 @@ int phy_start_cable_test(struct phy_device *phydev,
 }
 #endif
 
+int phy_cable_test_result(struct phy_device *phydev, u8 pair, u16 result);
+int phy_cable_test_fault_length(struct phy_device *phydev, u8 pair,
+				u16 cm);
+
 static inline void phy_device_reset(struct phy_device *phydev, int value)
 {
 	mdio_device_reset(&phydev->mdio, value);
diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index 7d73239b9ead..80acf7cd358f 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -130,3 +130,50 @@ void ethnl_cable_test_finished(struct phy_device *phydev)
 	ethnl_multicast(phydev->skb, phydev->attached_dev);
 }
 EXPORT_SYMBOL_GPL(ethnl_cable_test_finished);
+
+int ethnl_cable_test_result(struct phy_device *phydev, u8 pair, u16 result)
+{
+	struct nlattr *nest;
+	int ret = -EMSGSIZE;
+
+	nest = nla_nest_start(phydev->skb, ETHTOOL_A_CABLE_TEST_NTF_RESULT);
+	if (!nest)
+		return -EMSGSIZE;
+
+	if (nla_put_u8(phydev->skb, ETHTOOL_A_CABLE_RESULT_PAIR, pair))
+		goto err;
+	if (nla_put_u8(phydev->skb, ETHTOOL_A_CABLE_RESULT_CODE, result))
+		goto err;
+
+	nla_nest_end(phydev->skb, nest);
+	return 0;
+
+err:
+	nla_nest_cancel(phydev->skb, nest);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ethnl_cable_test_result);
+
+int ethnl_cable_test_fault_length(struct phy_device *phydev, u8 pair, u16 cm)
+{
+	struct nlattr *nest;
+	int ret = -EMSGSIZE;
+
+	nest = nla_nest_start(phydev->skb,
+			      ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH);
+	if (!nest)
+		return -EMSGSIZE;
+
+	if (nla_put_u8(phydev->skb, ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR, pair))
+		goto err;
+	if (nla_put_u16(phydev->skb, ETHTOOL_A_CABLE_FAULT_LENGTH_CM, cm))
+		goto err;
+
+	nla_nest_end(phydev->skb, nest);
+	return 0;
+
+err:
+	nla_nest_cancel(phydev->skb, nest);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(ethnl_cable_test_fault_length);
-- 
2.26.1

[PATCH net-next v1 8/9] net: phy: marvell: Add cable test support

[Andrew Lunn]

The Marvell PHYs have a couple of different register sets for
performing cable tests. Page 7 provides the simplest to use.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/marvell.c | 202 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 202 insertions(+)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 7fc8e10c5f33..7ad381ae4e49 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -27,6 +27,7 @@
 #include <linux/module.h>
 #include <linux/mii.h>
 #include <linux/ethtool.h>
+#include <linux/ethtool_netlink.h>
 #include <linux/phy.h>
 #include <linux/marvell_phy.h>
 #include <linux/bitfield.h>
@@ -35,6 +36,7 @@
 #include <linux/io.h>
 #include <asm/irq.h>
 #include <linux/uaccess.h>
+#include <uapi/linux/ethtool_netlink.h>
 
 #define MII_MARVELL_PHY_PAGE		22
 #define MII_MARVELL_COPPER_PAGE		0x00
@@ -42,6 +44,7 @@
 #define MII_MARVELL_MSCR_PAGE		0x02
 #define MII_MARVELL_LED_PAGE		0x03
 #define MII_MARVELL_MISC_TEST_PAGE	0x06
+#define MII_MARVELL_VCT7_PAGE		0x07
 #define MII_MARVELL_WOL_PAGE		0x11
 
 #define MII_M1011_IEVENT		0x13
@@ -162,6 +165,36 @@
 #define MII_88E1510_GEN_CTRL_REG_1_MODE_SGMII	0x1	/* SGMII to copper */
 #define MII_88E1510_GEN_CTRL_REG_1_RESET	0x8000	/* Soft reset */
 
+#define MII_VCT7_PAIR_0_DISTANCE	0x10
+#define MII_VCT7_PAIR_1_DISTANCE	0x11
+#define MII_VCT7_PAIR_2_DISTANCE	0x12
+#define MII_VCT7_PAIR_3_DISTANCE	0x13
+
+#define MII_VCT7_RESULTS	0x14
+#define MII_VCT7_RESULTS_PAIR3_MASK	0xf000
+#define MII_VCT7_RESULTS_PAIR2_MASK	0x0f00
+#define MII_VCT7_RESULTS_PAIR1_MASK	0x00f0
+#define MII_VCT7_RESULTS_PAIR0_MASK	0x000f
+#define MII_VCT7_RESULTS_PAIR3_SHIFT	12
+#define MII_VCT7_RESULTS_PAIR2_SHIFT	8
+#define MII_VCT7_RESULTS_PAIR1_SHIFT	4
+#define MII_VCT7_RESULTS_PAIR0_SHIFT	0
+#define MII_VCT7_RESULTS_INVALID	0
+#define MII_VCT7_RESULTS_OK		1
+#define MII_VCT7_RESULTS_OPEN		2
+#define MII_VCT7_RESULTS_SAME_SHORT	3
+#define MII_VCT7_RESULTS_CROSS_SHORT	4
+#define MII_VCT7_RESULTS_BUSY		9
+
+#define MII_VCT7_CTRL		0x15
+#define MII_VCT7_CTRL_RUN_NOW			BIT(15)
+#define MII_VCT7_CTRL_RUN_ANEG			BIT(14)
+#define MII_VCT7_CTRL_DISABLE_CROSS		BIT(13)
+#define MII_VCT7_CTRL_RUN_AFTER_BREAK_LINK	BIT(12)
+#define MII_VCT7_CTRL_IN_PROGRESS		BIT(11)
+#define MII_VCT7_CTRL_METERS			BIT(10)
+#define MII_VCT7_CTRL_CENTIMETERS		0
+
 #define LPA_PAUSE_FIBER		0x180
 #define LPA_PAUSE_ASYM_FIBER	0x100
 
@@ -1658,6 +1691,163 @@ static void marvell_get_stats(struct phy_device *phydev,
 		data[i] = marvell_get_stat(phydev, i);
 }
 
+static int marvell_vct7_cable_test_start(struct phy_device *phydev)
+{
+	int bmcr, bmsr, ret;
+
+	/* If auto-negotiation is enabled, but not complete, the cable
+	 * test never completes. So disable auto-neg.
+	 */
+	bmcr = phy_read(phydev, MII_BMCR);
+	if (bmcr < 0)
+		return bmcr;
+
+	bmsr = phy_read(phydev, MII_BMSR);
+
+	if (bmsr < 0)
+		return bmsr;
+
+	if (bmcr & BMCR_ANENABLE) {
+		ret =  phy_modify(phydev, MII_BMCR, BMCR_ANENABLE, 0);
+		if (ret < 0)
+			return ret;
+		ret = genphy_soft_reset(phydev);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* If the link is up, allow it some time to go down */
+	if (bmsr & BMSR_LSTATUS)
+		msleep(1500);
+
+	return phy_write_paged(phydev, MII_MARVELL_VCT7_PAGE,
+			       MII_VCT7_CTRL,
+			       MII_VCT7_CTRL_RUN_NOW |
+			       MII_VCT7_CTRL_CENTIMETERS);
+}
+
+static int marvell_vct7_distance_to_length(int distance, bool meter)
+{
+	if (meter)
+		distance *= 100;
+
+	return distance;
+}
+
+static bool marvell_vct7_distance_valid(int result)
+{
+	switch (result) {
+	case MII_VCT7_RESULTS_OPEN:
+	case MII_VCT7_RESULTS_SAME_SHORT:
+	case MII_VCT7_RESULTS_CROSS_SHORT:
+		return true;
+	}
+	return false;
+}
+
+static int marvell_vct7_report_length(struct phy_device *phydev,
+				      int pair, bool meter)
+{
+	int length;
+	int ret;
+
+	ret = phy_read_paged(phydev, MII_MARVELL_VCT7_PAGE,
+			     MII_VCT7_PAIR_0_DISTANCE + pair);
+	if (ret < 0)
+		return ret;
+
+	length = marvell_vct7_distance_to_length(ret, meter);
+
+	ethnl_cable_test_fault_length(phydev, pair, length);
+
+	return 0;
+}
+
+static int mavell_vct7_cable_test_report_trans(int result)
+{
+	switch (result) {
+	case MII_VCT7_RESULTS_OK:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OK;
+	case MII_VCT7_RESULTS_OPEN:
+		return ETHTOOL_A_CABLE_RESULT_CODE_OPEN;
+	case MII_VCT7_RESULTS_SAME_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT;
+	case MII_VCT7_RESULTS_CROSS_SHORT:
+		return ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT;
+	default:
+		return ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC;
+	}
+}
+
+static int marvell_vct7_cable_test_report(struct phy_device *phydev)
+{
+	int pair0, pair1, pair2, pair3;
+	bool meter;
+	int ret;
+
+	ret = phy_read_paged(phydev, MII_MARVELL_VCT7_PAGE,
+			     MII_VCT7_RESULTS);
+	if (ret < 0)
+		return ret;
+
+	pair3 = (ret & MII_VCT7_RESULTS_PAIR3_MASK) >>
+		MII_VCT7_RESULTS_PAIR3_SHIFT;
+	pair2 = (ret & MII_VCT7_RESULTS_PAIR2_MASK) >>
+		MII_VCT7_RESULTS_PAIR2_SHIFT;
+	pair1 = (ret & MII_VCT7_RESULTS_PAIR1_MASK) >>
+		MII_VCT7_RESULTS_PAIR1_SHIFT;
+	pair0 = (ret & MII_VCT7_RESULTS_PAIR0_MASK) >>
+		MII_VCT7_RESULTS_PAIR0_SHIFT;
+
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_0,
+				mavell_vct7_cable_test_report_trans(pair0));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_1,
+				mavell_vct7_cable_test_report_trans(pair1));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_2,
+				mavell_vct7_cable_test_report_trans(pair2));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_3,
+				mavell_vct7_cable_test_report_trans(pair3));
+
+	ret = phy_read_paged(phydev, MII_MARVELL_VCT7_PAGE, MII_VCT7_CTRL);
+	if (ret < 0)
+		return ret;
+
+	meter = ret & MII_VCT7_CTRL_METERS;
+
+	if (marvell_vct7_distance_valid(pair0))
+		marvell_vct7_report_length(phydev, 0, meter);
+	if (marvell_vct7_distance_valid(pair1))
+		marvell_vct7_report_length(phydev, 1, meter);
+	if (marvell_vct7_distance_valid(pair2))
+		marvell_vct7_report_length(phydev, 2, meter);
+	if (marvell_vct7_distance_valid(pair3))
+		marvell_vct7_report_length(phydev, 3, meter);
+
+	return 0;
+}
+
+static int marvell_vct7_cable_test_get_status(struct phy_device *phydev,
+					      bool *finished)
+{
+	int ret;
+
+	*finished = false;
+
+	ret = phy_read_paged(phydev, MII_MARVELL_VCT7_PAGE,
+			     MII_VCT7_CTRL);
+
+	if (ret < 0)
+		return ret;
+
+	if (!(ret & MII_VCT7_CTRL_IN_PROGRESS)) {
+		*finished = true;
+
+		return marvell_vct7_cable_test_report(phydev);
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_HWMON
 static int m88e1121_get_temp(struct phy_device *phydev, long *temp)
 {
@@ -2353,6 +2543,7 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1510",
 		.features = PHY_GBIT_FIBRE_FEATURES,
+		.flags = PHY_POLL_CABLE_TEST,
 		.probe = &m88e1510_probe,
 		.config_init = &m88e1510_config_init,
 		.config_aneg = &m88e1510_config_aneg,
@@ -2372,12 +2563,15 @@ static struct phy_driver marvell_drivers[] = {
 		.set_loopback = genphy_loopback,
 		.get_tunable = m88e1011_get_tunable,
 		.set_tunable = m88e1011_set_tunable,
+		.cable_test_start = marvell_vct7_cable_test_start,
+		.cable_test_get_status = marvell_vct7_cable_test_get_status,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1540,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1540",
 		/* PHY_GBIT_FEATURES */
+		.flags = PHY_POLL_CABLE_TEST,
 		.probe = m88e1510_probe,
 		.config_init = &marvell_config_init,
 		.config_aneg = &m88e1510_config_aneg,
@@ -2394,6 +2588,8 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
+		.cable_test_start = marvell_vct7_cable_test_start,
+		.cable_test_get_status = marvell_vct7_cable_test_get_status,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E1545,
@@ -2401,6 +2597,7 @@ static struct phy_driver marvell_drivers[] = {
 		.name = "Marvell 88E1545",
 		.probe = m88e1510_probe,
 		/* PHY_GBIT_FEATURES */
+		.flags = PHY_POLL_CABLE_TEST,
 		.config_init = &marvell_config_init,
 		.config_aneg = &m88e1510_config_aneg,
 		.read_status = &marvell_read_status,
@@ -2416,6 +2613,8 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
+		.cable_test_start = marvell_vct7_cable_test_start,
+		.cable_test_get_status = marvell_vct7_cable_test_get_status,
 	},
 	{
 		.phy_id = MARVELL_PHY_ID_88E3016,
@@ -2442,6 +2641,7 @@ static struct phy_driver marvell_drivers[] = {
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E6390",
 		/* PHY_GBIT_FEATURES */
+		.flags = PHY_POLL_CABLE_TEST,
 		.probe = m88e6390_probe,
 		.config_init = &marvell_config_init,
 		.config_aneg = &m88e6390_config_aneg,
@@ -2458,6 +2658,8 @@ static struct phy_driver marvell_drivers[] = {
 		.get_stats = marvell_get_stats,
 		.get_tunable = m88e1540_get_tunable,
 		.set_tunable = m88e1540_set_tunable,
+		.cable_test_start = marvell_vct7_cable_test_start,
+		.cable_test_get_status = marvell_vct7_cable_test_get_status,
 	},
 };
 
-- 
2.26.1

[PATCH net-next v1 9/9] net: phy: Put interface into oper testing during cable test

[Andrew Lunn]

Since running a cable test is disruptive, put the interface into
operative state testing while the test is running.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 88275d971f14..ddd8550dc08d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -485,6 +485,7 @@ static void phy_cable_test_abort(struct phy_device *phydev)
 int phy_start_cable_test(struct phy_device *phydev,
 			 struct netlink_ext_ack *extack)
 {
+	struct net_device *dev = phydev->attached_dev;
 	int err = -ENOMEM;
 
 	if (!(phydev->drv &&
@@ -511,8 +512,10 @@ int phy_start_cable_test(struct phy_device *phydev,
 	/* Mark the carrier down until the test is complete */
 	phy_link_down(phydev, true);
 
+	netif_testing_on(dev);
 	err = phydev->drv->cable_test_start(phydev);
 	if (err) {
+		netif_testing_off(dev);
 		phy_link_up(phydev);
 		goto out_free;
 	}
@@ -865,6 +868,8 @@ EXPORT_SYMBOL(phy_free_interrupt);
  */
 void phy_stop(struct phy_device *phydev)
 {
+	struct net_device *dev = phydev->attached_dev;
+
 	if (!phy_is_started(phydev)) {
 		WARN(1, "called from state %s\n",
 		     phy_state_to_str(phydev->state));
@@ -873,8 +878,10 @@ void phy_stop(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
-	if (phydev->state == PHY_CABLETEST)
+	if (phydev->state == PHY_CABLETEST) {
 		phy_cable_test_abort(phydev);
+		 netif_testing_off(dev);
+	}
 
 	if (phydev->sfp_bus)
 		sfp_upstream_stop(phydev->sfp_bus);
@@ -936,6 +943,7 @@ void phy_state_machine(struct work_struct *work)
 	struct delayed_work *dwork = to_delayed_work(work);
 	struct phy_device *phydev =
 			container_of(dwork, struct phy_device, state_queue);
+	struct net_device *dev = phydev->attached_dev;
 	bool needs_aneg = false, do_suspend = false;
 	enum phy_state old_state;
 	bool finished = false;
@@ -961,6 +969,7 @@ void phy_state_machine(struct work_struct *work)
 		err = phydev->drv->cable_test_get_status(phydev, &finished);
 		if (err) {
 			phy_cable_test_abort(phydev);
+			netif_testing_off(dev);
 			needs_aneg = true;
 			phydev->state = PHY_UP;
 			break;
@@ -968,6 +977,7 @@ void phy_state_machine(struct work_struct *work)
 
 		if (finished) {
 			ethnl_cable_test_finished(phydev);
+			netif_testing_off(dev);
 			needs_aneg = true;
 			phydev->state = PHY_UP;
 		}
-- 
2.26.1

Re: [PATCH net-next v1 2/9] net: phy: Add support for polling cable test

[Florian Fainelli]

Hi Andrew,

On 4/25/2020 11:06 AM, Andrew Lunn wrote:
> Some PHYs are not capable of generating interrupts when a cable test
> finished. They do however support interrupts for normal operations,
> like link up/down. As such, the PHY state machine would normally not
> poll the PHY.
> 
> Add support for indicating the PHY state machine must poll the PHY
> when performing a cable test.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

If you started a cable test and killed the ethtool process before the
cable diagnostics are available, the state machine gets stuck in that
state, so we should find a way to propagate the signal all the way to
the PHY library somehow.
-- 
Florian

Re: [PATCH net-next v1 4/9] net: ethtool: Add attributes for cable test reports

[Randy Dunlap]

Hi,
Here are a few edit comments for you to consider:

On 4/25/20 11:06 AM, Andrew Lunn wrote:
> Add the attributes needed to report cable test results to userspace.
> The reports are expected to be per twisted pair. A nested property per
> pair can report the result of the cable test. A nested property can
> also report the length of the cable to any fault.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  Documentation/networking/ethtool-netlink.rst | 35 +++++++++++++++
>  include/uapi/linux/ethtool_netlink.h         | 47 +++++++++++++++++++-
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 0c354567e991..89fd321b0e29 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -967,6 +967,41 @@ Request contents:
>    ``ETHTOOL_A_CABLE_TEST_HEADER``       nested  request header
>    ====================================  ======  ==========================
>  
> +Notify contents:
> +
> +An Ethernet cable typically contains 1, 2 or 4 pairs. The length of
> +the pair can only be measured when there is a fault in the pair and
> +hence a reflection. Information about the fault may not be available,
> +depends on the specific hardware. Hence the contents of the notify

   depending on

> +message is mostly optional. The attributes can be repeated an

           are

> +arbitrary number of times, in an arbitrary order, for an arbitrary
> +number of pairs.
> +
> +The example shows a T2 cable, i.e. two pairs. One pair is O.K, and

                                                             OK and

> +hence has no length information. The second pair has a fault and does
> +have length information.
> +
> + +-------------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_HEADER``           | nested | reply header          |
> + +-------------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test result     |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
> + +-+-----------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test results    |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
> + +-+-----------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH`` | nested | cable length          |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR``   | u8     | pair number           |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_CM``     | u8     | length in cm          |
> + +-+-----------------------------------------+--------+-----------------------+
>  
>  Request translation
>  ===================



-- 
~Randy

Re: [PATCH net-next v1 2/9] net: phy: Add support for polling cable test

[Andrew Lunn]

On Sat, Apr 25, 2020 at 12:49:46PM -0700, Florian Fainelli wrote:
> Hi Andrew,
> 
> On 4/25/2020 11:06 AM, Andrew Lunn wrote:
> > Some PHYs are not capable of generating interrupts when a cable test
> > finished. They do however support interrupts for normal operations,
> > like link up/down. As such, the PHY state machine would normally not
> > poll the PHY.
> > 
> > Add support for indicating the PHY state machine must poll the PHY
> > when performing a cable test.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> If you started a cable test and killed the ethtool process before the
> cable diagnostics are available, the state machine gets stuck in that
> state, so we should find a way to propagate the signal all the way to
> the PHY library somehow.

Hi Florian

It should not matter if the user space tool goes away. As you read
later patches you will see why. But:

ETHTOOL_MSG_CABLE_TEST_ACT is an action to trigger cable
test. Assuming the driver supports it etc, the cable test is started,
and user space immediately gets a ETHTOOL_MSG_CABLE_TEST_ACT_REPLY.
The state is changed to PHY_CABLETEST.

Sometime later, the driver indicates the cable test has
completed. This can be an interrupt, or because it is being polled.  A
ETHTOOL_MSG_CABLE_TEST_NTF is multicast to user space with the
results. And the PHY then leaves state PHY_CABLETEST. If there is no
user space listening for the ETHTOOL_MSG_CABLE_TEST_NTF, it does not
matter.

At least with the Marvell PHY, there is no documented way to tell it
to abort a cable test. So knowing the user space has lost interest in
the results does not help us.

    Andrew
    

Re: [PATCH net-next v1 3/9] net: ethtool: netlink: Add support for triggering a cable test

[Michal Kubecek]

On Sat, Apr 25, 2020 at 08:06:15PM +0200, Andrew Lunn wrote:
> Add new ethtool netlink calls to trigger the starting of a PHY cable
> test.
> 
> Add Kconfig'ury to ETHTOOL_NETLINK so that PHYLIB is not a module when
> ETHTOOL_NETLINK is builtin, which would result in kernel linking errors.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  Documentation/networking/ethtool-netlink.rst | 18 ++++-
>  include/uapi/linux/ethtool_netlink.h         | 14 ++++
>  net/Kconfig                                  |  1 +
>  net/ethtool/Makefile                         |  2 +-
>  net/ethtool/cabletest.c                      | 82 ++++++++++++++++++++
>  net/ethtool/netlink.c                        |  6 ++
>  net/ethtool/netlink.h                        |  2 +
>  7 files changed, 122 insertions(+), 3 deletions(-)
>  create mode 100644 net/ethtool/cabletest.c
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 567326491f80..0c354567e991 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
[...]
> @@ -758,7 +760,6 @@ Kernel checks that requested channel counts do not exceed limits reported by
>  driver. Driver may impose additional constraints and may not suspport all
>  attributes.
>  
> -
>  COALESCE_GET
>  ============
>  

Nitpick: this probably wasn't intended.

[...]
> diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
> new file mode 100644
> index 000000000000..44b8165ac66e
> --- /dev/null
> +++ b/net/ethtool/cabletest.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/phy.h>
> +#include "netlink.h"
> +#include "common.h"
> +#include "bitset.h"
> +
> +struct cable_test_req_info {
> +	struct ethnl_req_info		base;
> +};
> +
> +struct cable_test_reply_data {
> +	struct ethnl_reply_data		base;
> +};
> +
> +#define CABLE_TEST_REPDATA(__reply_base) \
> +	container_of(__reply_base, struct cable_test_reply_data, base)
> +
> +static const struct nla_policy
> +cable_test_get_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
> +	[ETHTOOL_A_CABLE_TEST_UNSPEC]		= { .type = NLA_REJECT },
> +	[ETHTOOL_A_CABLE_TEST_HEADER]		= { .type = NLA_NESTED },
> +};
> +
> +const struct ethnl_request_ops ethnl_cable_test_act_ops = {
> +	.request_cmd		= ETHTOOL_MSG_CABLE_TEST_ACT,
> +	.reply_cmd		= ETHTOOL_MSG_CABLE_TEST_ACT_REPLY,
> +	.hdr_attr		= ETHTOOL_A_CABLE_TEST_HEADER,
> +	.max_attr		= ETHTOOL_A_CABLE_TEST_MAX,
> +	.req_info_size		= sizeof(struct cable_test_req_info),
> +	.reply_data_size	= sizeof(struct cable_test_reply_data),
> +	.request_policy		= cable_test_get_policy,
> +};
> +

As you register ethnl_act_cable_test() as doit handler and don't use any
of ethnl_default_*() handlers, you don't need to define
ethnl_cable_test_act_ops (and also struct cable_test_req_info and struct
cable_test_reply_data). These would be only used by default doit/dumpit
handlers and default notification handler.

> +/* CABLE_TEST_ACT */
> +
> +static const struct nla_policy
> +cable_test_set_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
> +	[ETHTOOL_A_CABLE_TEST_UNSPEC]		= { .type = NLA_REJECT },
> +	[ETHTOOL_A_CABLE_TEST_HEADER]		= { .type = NLA_NESTED },
> +};

This should be probably rather named cable_test_act_policy - or maybe
cable_test_policy would suffice as you have only one request message
type (I've been using *_get_policy for *_GET request and *_set_policy
for *_SET).

> +
> +int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct nlattr *tb[ETHTOOL_A_CABLE_TEST_MAX + 1];
> +	struct ethnl_req_info req_info = {};
> +	struct net_device *dev;
> +	int ret;
> +
> +	ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
> +			  ETHTOOL_A_CABLE_TEST_MAX,
> +			  cable_test_set_policy, info->extack);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ethnl_parse_header_dev_get(&req_info,
> +					 tb[ETHTOOL_A_CABLE_TEST_HEADER],
> +					 genl_info_net(info), info->extack,
> +					 true);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev = req_info.dev;
> +	if (!dev->phydev) {
> +		ret = -EOPNOTSUPP;
> +		goto out_dev_put;
> +	}
> +
> +	rtnl_lock();
> +	ret = ethnl_ops_begin(dev);
> +	if (ret < 0)
> +		goto out_rtnl;
> +
> +	ret = phy_start_cable_test(dev->phydev, info->extack);
> +
> +	ethnl_ops_complete(dev);
> +out_rtnl:
> +	rtnl_unlock();
> +out_dev_put:
> +	dev_put(dev);
> +	return ret;
> +}

As you don't send a reply message, you don't need
ETHTOOL_MSG_CABLE_TEST_ACT_REPLY either (we may introduce it later if
there is a reply message).

Michal

Re: [PATCH net-next v1 1/9] net: phy: Add cable test support to state machine

[Michal Kubecek]

On Sat, Apr 25, 2020 at 08:06:13PM +0200, Andrew Lunn wrote:
> Running a cable test is desruptive to normal operation of the PHY and
> can take a 5 to 10 seconds to complete. The RTNL lock cannot be held
> for this amount of time, and add a new state to the state machine for
> running a cable test.
> 
> The driver is expected to implement two functions. The first is used
> to start a cable test. Once the test has started, it should return.
> 
> The second function is called once per second, or on interrupt to
> check if the cable test is complete, and to allow the PHY to report
> the status.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/phy/phy.c | 65 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h   | 28 +++++++++++++++++++
>  2 files changed, 93 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 72c69a9c8a98..4a6279c4a3a3 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
[...]
> @@ -470,6 +472,51 @@ static void phy_trigger_machine(struct phy_device *phydev)
>  	phy_queue_state_machine(phydev, 0);
>  }
>  
> +static void phy_cable_test_abort(struct phy_device *phydev)
> +{
> +	genphy_soft_reset(phydev);
> +}
> +
> +int phy_start_cable_test(struct phy_device *phydev,
> +			 struct netlink_ext_ack *extack)

Nitpick: the naming (phy_cable_test_abort() vs phy_start_cable_test())
should probably be consistent.

> +{
> +	int err;
> +
> +	if (!(phydev->drv &&
> +	      phydev->drv->cable_test_start &&
> +	      phydev->drv->cable_test_get_status)) {
> +		NL_SET_ERR_MSG(extack,
> +			       "PHY driver does not support cable testing");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	mutex_lock(&phydev->lock);
> +	if (phydev->state < PHY_UP ||
> +	    phydev->state >= PHY_CABLETEST) {
> +		NL_SET_ERR_MSG(extack,
> +			       "PHY not configured. Try setting interface up");
> +		err = -EBUSY;
> +		goto out;
> +	}

If I read the code correctly, this check would also catch an attempt to
start a cable test while the test is already in progress in which case
the error message would be rather misleading. I'm not sure what would be
more appropriate in such case: we can return -EBUSY (with more fitting
error message) but we could also silently ignore the request and let the
client wait for the notification from the test which is already in
progress.

Michal

Re: [PATCH net-next v1 4/9] net: ethtool: Add attributes for cable test reports

[Michal Kubecek]

On Sat, Apr 25, 2020 at 08:06:16PM +0200, Andrew Lunn wrote:
> Add the attributes needed to report cable test results to userspace.
> The reports are expected to be per twisted pair. A nested property per
> pair can report the result of the cable test. A nested property can
> also report the length of the cable to any fault.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  Documentation/networking/ethtool-netlink.rst | 35 +++++++++++++++
>  include/uapi/linux/ethtool_netlink.h         | 47 +++++++++++++++++++-
>  2 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 0c354567e991..89fd321b0e29 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -967,6 +967,41 @@ Request contents:
>    ``ETHTOOL_A_CABLE_TEST_HEADER``       nested  request header
>    ====================================  ======  ==========================
>  
> +Notify contents:

Perhaps rather "Notification contents".

> +
> +An Ethernet cable typically contains 1, 2 or 4 pairs. The length of
> +the pair can only be measured when there is a fault in the pair and
> +hence a reflection. Information about the fault may not be available,
> +depends on the specific hardware. Hence the contents of the notify
> +message is mostly optional. The attributes can be repeated an
> +arbitrary number of times, in an arbitrary order, for an arbitrary
> +number of pairs.
> +
> +The example shows a T2 cable, i.e. two pairs. One pair is O.K, and
> +hence has no length information. The second pair has a fault and does
> +have length information.
> +
> + +-------------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_HEADER``           | nested | reply header          |
> + +-------------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test result     |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
> + +-+-----------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test results    |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
> + +-+-----------------------------------------+--------+-----------------------+
> + | ``ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH`` | nested | cable length          |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR``   | u8     | pair number           |
> + +-+-----------------------------------------+--------+-----------------------+
> + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_CM``     | u8     | length in cm          |
> + +-+-----------------------------------------+--------+-----------------------+

Would it be complicated to gather all information for each pair
together? I.e. to have one nest for each pair with pair number, result
code and possibly other information (if available). I believe it would
make the message easier to process.

>  
>  Request translation
>  ===================
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 598d0b502ebd..05ef5048e4fc 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -76,6 +76,7 @@ enum {
>  	ETHTOOL_MSG_EEE_NTF,
>  	ETHTOOL_MSG_TSINFO_GET_REPLY,
>  	ETHTOOL_MSG_CABLE_TEST_ACT_REPLY,
> +	ETHTOOL_MSG_CABLE_TEST_NTF,
>  
>  	/* add new constants above here */
>  	__ETHTOOL_MSG_KERNEL_CNT,
> @@ -403,7 +404,6 @@ enum {
>  	/* add new constants above here */
>  	__ETHTOOL_A_TSINFO_CNT,
>  	ETHTOOL_A_TSINFO_MAX = (__ETHTOOL_A_TSINFO_CNT - 1)
> -
>  };
>  
>  /* CABLE TEST */
> @@ -417,6 +417,51 @@ enum {
>  	ETHTOOL_A_CABLE_TEST_MAX = __ETHTOOL_A_CABLE_TEST_CNT - 1
>  };
>  
> +/* CABLE TEST NOTIFY */
> +enum {
> +	ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC,
> +	ETHTOOL_A_CABLE_RESULT_CODE_OK,
> +	ETHTOOL_A_CABLE_RESULT_CODE_OPEN,
> +	ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT,
> +	ETHTOOL_A_CABLE_RESULT_CODE_CROSS_SHORT,
> +};
> +
> +enum {
> +	ETHTOOL_A_CABLE_PAIR_0,
> +	ETHTOOL_A_CABLE_PAIR_1,
> +	ETHTOOL_A_CABLE_PAIR_2,
> +	ETHTOOL_A_CABLE_PAIR_3,
> +};

Do we really need this enum, couldn't we simply use a number (possibly
with a sanity check of maximum value)?

> +
> +enum {
> +	ETHTOOL_A_CABLE_RESULT_UNSPEC,
> +	ETHTOOL_A_CABLE_RESULT_PAIR,		/* u8 ETHTOOL_A_CABLE_PAIR_ */
> +	ETHTOOL_A_CABLE_RESULT_CODE,		/* u8 ETHTOOL_A_CABLE_RESULT_CODE_ */
> +
> +	__ETHTOOL_A_CABLE_RESULT_CNT,
> +	ETHTOOL_A_CABLE_RESULT_MAX = (__ETHTOOL_A_CABLE_RESULT_CNT - 1)
> +};
> +
> +enum {
> +	ETHTOOL_A_CABLE_FAULT_LENGTH_UNSPEC,
> +	ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR,	/* u8 ETHTOOL_A_CABLE_PAIR_ */
> +	ETHTOOL_A_CABLE_FAULT_LENGTH_CM,	/* u16 */

The example above says "u8" (which is obviously wrong). I would rather
suggest u32 here to be as future proof as possible. Using NLA_U16
doesn't save anything after all, as both NLA_U16 and NLA_U32 spend the
same space due to padding.

> +
> +	__ETHTOOL_A_CABLE_FAULT_LENGTH_CNT,
> +	ETHTOOL_A_CABLE_FAULT_LENGTH_MAX = (__ETHTOOL_A_CABLE_FAULT_LENGTH_CNT - 1)
> +};
> +
> +enum {
> +	ETHTOOL_A_CABLE_TEST_NTF_UNSPEC,
> +	ETHTOOL_A_CABLE_TEST_NTF_DEV,		/* nest - ETHTOOL_A_DEV_* */

s/DEV/HEADER/g, I suppose (looks like a relic from an older version).

> +	ETHTOOL_A_CABLE_TEST_NTF_NEST,		/* nest - of results: */

This is missing in the example above.

> +	ETHTOOL_A_CABLE_TEST_NTF_RESULT,	/* nest - ETHTOOL_A_CABLE_RESULT_ */
> +	ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH,	/* nest - ETHTOOL_A_CABLE_FAULT_LENGTH_ */
> +
> +	__ETHTOOL_A_CABLE_TEST_NTF_CNT,
> +	ETHTOOL_A_CABLE_TEST_NTF_MAX = (__ETHTOOL_A_CABLE_TEST_NTF_CNT - 1)
> +};

One more idea: it would be IMHO useful to also send a notification when
the test is started. It could be distinguished by a status attribute
which would describe status of the test as a whole (not a specific
pair), e.g. started, finished, aborted.

Michal

> +
>  /* generic netlink info */
>  #define ETHTOOL_GENL_NAME "ethtool"
>  #define ETHTOOL_GENL_VERSION 1
> -- 
> 2.26.1
> 

Re: [PATCH net-next v1 1/9] net: phy: Add cable test support to state machine

[Andrew Lunn]

> > +	mutex_lock(&phydev->lock);
> > +	if (phydev->state < PHY_UP ||
> > +	    phydev->state >= PHY_CABLETEST) {
> > +		NL_SET_ERR_MSG(extack,
> > +			       "PHY not configured. Try setting interface up");
> > +		err = -EBUSY;
> > +		goto out;
> > +	}
> 
> If I read the code correctly, this check would also catch an attempt to
> start a cable test while the test is already in progress in which case
> the error message would be rather misleading. I'm not sure what would be
> more appropriate in such case: we can return -EBUSY (with more fitting
> error message) but we could also silently ignore the request and let the
> client wait for the notification from the test which is already in
> progress.

Hi Michal

Yes, i will change this to handle phydev->state == PHY_CABLETEST
differently. Probably -EBUSY is the simplest.

Thanks
	Andrew

Re: [PATCH net-next v1 3/9] net: ethtool: netlink: Add support for triggering a cable test

[Andrew Lunn]

> > +const struct ethnl_request_ops ethnl_cable_test_act_ops = {
> > +	.request_cmd		= ETHTOOL_MSG_CABLE_TEST_ACT,
> > +	.reply_cmd		= ETHTOOL_MSG_CABLE_TEST_ACT_REPLY,
> > +	.hdr_attr		= ETHTOOL_A_CABLE_TEST_HEADER,
> > +	.max_attr		= ETHTOOL_A_CABLE_TEST_MAX,
> > +	.req_info_size		= sizeof(struct cable_test_req_info),
> > +	.reply_data_size	= sizeof(struct cable_test_reply_data),
> > +	.request_policy		= cable_test_get_policy,
> > +};
> > +
> 
> As you register ethnl_act_cable_test() as doit handler and don't use any
> of ethnl_default_*() handlers, you don't need to define
> ethnl_cable_test_act_ops (and also struct cable_test_req_info and struct
> cable_test_reply_data). These would be only used by default doit/dumpit
> handlers and default notification handler.

O.K, than

> 
> > +/* CABLE_TEST_ACT */
> > +
> > +static const struct nla_policy
> > +cable_test_set_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
> > +	[ETHTOOL_A_CABLE_TEST_UNSPEC]		= { .type = NLA_REJECT },
> > +	[ETHTOOL_A_CABLE_TEST_HEADER]		= { .type = NLA_NESTED },
> > +};
> 
> This should be probably rather named cable_test_act_policy - or maybe
> cable_test_policy would suffice as you have only one request message
> type (I've been using *_get_policy for *_GET request and *_set_policy
> for *_SET).

I probably just cut/paste and changes the name, but not set to act. I
will fix this.

> > +int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +	struct nlattr *tb[ETHTOOL_A_CABLE_TEST_MAX + 1];
> > +	struct ethnl_req_info req_info = {};
> > +	struct net_device *dev;
> > +	int ret;
> > +
> > +	ret = nlmsg_parse(info->nlhdr, GENL_HDRLEN, tb,
> > +			  ETHTOOL_A_CABLE_TEST_MAX,
> > +			  cable_test_set_policy, info->extack);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = ethnl_parse_header_dev_get(&req_info,
> > +					 tb[ETHTOOL_A_CABLE_TEST_HEADER],
> > +					 genl_info_net(info), info->extack,
> > +					 true);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	dev = req_info.dev;
> > +	if (!dev->phydev) {
> > +		ret = -EOPNOTSUPP;
> > +		goto out_dev_put;
> > +	}
> > +
> > +	rtnl_lock();
> > +	ret = ethnl_ops_begin(dev);
> > +	if (ret < 0)
> > +		goto out_rtnl;
> > +
> > +	ret = phy_start_cable_test(dev->phydev, info->extack);
> > +
> > +	ethnl_ops_complete(dev);
> > +out_rtnl:
> > +	rtnl_unlock();
> > +out_dev_put:
> > +	dev_put(dev);
> > +	return ret;
> > +}
> 
> As you don't send a reply message, you don't need
> ETHTOOL_MSG_CABLE_TEST_ACT_REPLY either (we may introduce it later if
> there is a reply message).

One thing i was thinking about is sending user space a cookie at this
point, to help pair the request to the multicasted results. Then the
reply would be useful.

      Andrew

Re: [PATCH net-next v1 3/9] net: ethtool: netlink: Add support for triggering a cable test

[Michal Kubecek]

On Sun, Apr 26, 2020 at 10:38:33PM +0200, Andrew Lunn wrote:
> > 
> > As you don't send a reply message, you don't need
> > ETHTOOL_MSG_CABLE_TEST_ACT_REPLY either (we may introduce it later if
> > there is a reply message).
> 
> One thing i was thinking about is sending user space a cookie at this
> point, to help pair the request to the multicasted results. Then the
> reply would be useful.

You could use nl_set_extack_cookie_u64() for this - it would be similar
to the nl80211 use case it was introduced for. Of course, there may be
other reasons to introduce a reply, I only suggest not to add the
message type id until we actually use it.

Michal

Re: [PATCH net-next v1 4/9] net: ethtool: Add attributes for cable test reports

[Andrew Lunn]

> > +
> > + +-------------------------------------------+--------+-----------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_HEADER``           | nested | reply header          |
> > + +-------------------------------------------+--------+-----------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test result     |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test results    |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH`` | nested | cable length          |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR``   | u8     | pair number           |
> > + +-+-----------------------------------------+--------+-----------------------+
> > + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_CM``     | u8     | length in cm          |
> > + +-+-----------------------------------------+--------+-----------------------+
> 
> Would it be complicated to gather all information for each pair
> together? I.e. to have one nest for each pair with pair number, result
> code and possibly other information (if available). I believe it would
> make the message easier to process.

It is something i considered, but decided against. Attributes give us
flexibility to report whatever the PHY gives us. There is no
standardisation here. Some PHYs will report the first fault on a
pair. Others will report multiple faults on a pair. Some PHYs can tell
you pair X is shorted to pair Y, etc, while some PHYs just tell you it
is shorted. It also keeps the driver code simple. Report whatever you
have in any order and it does not matter. And it means i don't need
complex helper code trying to coordinating information from the
driver.

So far, a plain dump of the netlink message in user space also seems
readable. But when we have more PHYs supported and more variability
between PHYs we might need to consider if user space should do some
sorting before printing the test results.

> > +enum {
> > +	ETHTOOL_A_CABLE_PAIR_0,
> > +	ETHTOOL_A_CABLE_PAIR_1,
> > +	ETHTOOL_A_CABLE_PAIR_2,
> > +	ETHTOOL_A_CABLE_PAIR_3,
> > +};
> 
> Do we really need this enum, couldn't we simply use a number (possibly
> with a sanity check of maximum value)?

They are not strictly required. But it helps with consistence. Are the
pairs numbered 0, 1, 2, 3, or 1, 2, 3, 4?

> > +enum {
> > +	ETHTOOL_A_CABLE_RESULT_UNSPEC,
> > +	ETHTOOL_A_CABLE_RESULT_PAIR,		/* u8 ETHTOOL_A_CABLE_PAIR_ */
> > +	ETHTOOL_A_CABLE_RESULT_CODE,		/* u8 ETHTOOL_A_CABLE_RESULT_CODE_ */
> > +
> > +	__ETHTOOL_A_CABLE_RESULT_CNT,
> > +	ETHTOOL_A_CABLE_RESULT_MAX = (__ETHTOOL_A_CABLE_RESULT_CNT - 1)
> > +};
> > +
> > +enum {
> > +	ETHTOOL_A_CABLE_FAULT_LENGTH_UNSPEC,
> > +	ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR,	/* u8 ETHTOOL_A_CABLE_PAIR_ */
> > +	ETHTOOL_A_CABLE_FAULT_LENGTH_CM,	/* u16 */
> 
> The example above says "u8" (which is obviously wrong).

Yep, will fix that.

> I would rather suggest u32 here to be as future proof as possible.

Yes, i've been going backwards and forwards on that. I did not realize
netlink messages were space inefficient. A u8 takes as much space as a
u32. I picked u16 because that allows up 65535cm, or 655.35m. All the
IEEE 802.3 Base-T standards have a maximum cable length of 100m, or
shorter. They might work with longer cables, but i doubt a cable 6
times longer than the specified max will work. So a u16 is ample.

The only argument i can see for a u32 is if somebody can implement
cable testing for fibre optical cables. Then a u16 is not big enough.
But so far, i've never seen an SFP module offering this.

> One more idea: it would be IMHO useful to also send a notification when
> the test is started. It could be distinguished by a status attribute
> which would describe status of the test as a whole (not a specific
> pair), e.g. started, finished, aborted.
 
Yes, give how long these tests take, i could be useful. There is also
already some hints about this, in that the last patch in the series
changes the RFC 2863 status of the interface, which i hope sends a
message to user space about the interface change of status.

	Andrew

Re: [PATCH net-next v1 2/9] net: phy: Add support for polling cable test

[Florian Fainelli]

On 4/25/2020 1:10 PM, Andrew Lunn wrote:
> On Sat, Apr 25, 2020 at 12:49:46PM -0700, Florian Fainelli wrote:
>> Hi Andrew,
>>
>> On 4/25/2020 11:06 AM, Andrew Lunn wrote:
>>> Some PHYs are not capable of generating interrupts when a cable test
>>> finished. They do however support interrupts for normal operations,
>>> like link up/down. As such, the PHY state machine would normally not
>>> poll the PHY.
>>>
>>> Add support for indicating the PHY state machine must poll the PHY
>>> when performing a cable test.
>>>
>>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>>
>> If you started a cable test and killed the ethtool process before the
>> cable diagnostics are available, the state machine gets stuck in that
>> state, so we should find a way to propagate the signal all the way to
>> the PHY library somehow.
> 
> Hi Florian
> 
> It should not matter if the user space tool goes away. As you read
> later patches you will see why. But:
> 
> ETHTOOL_MSG_CABLE_TEST_ACT is an action to trigger cable
> test. Assuming the driver supports it etc, the cable test is started,
> and user space immediately gets a ETHTOOL_MSG_CABLE_TEST_ACT_REPLY.
> The state is changed to PHY_CABLETEST.
> 
> Sometime later, the driver indicates the cable test has
> completed. This can be an interrupt, or because it is being polled.  A
> ETHTOOL_MSG_CABLE_TEST_NTF is multicast to user space with the
> results. And the PHY then leaves state PHY_CABLETEST. If there is no
> user space listening for the ETHTOOL_MSG_CABLE_TEST_NTF, it does not
> matter.
> 
> At least with the Marvell PHY, there is no documented way to tell it
> to abort a cable test. So knowing the user space has lost interest in
> the results does not help us.

The same is true with Broadcom PHYs, but I think we should be guarding
somehow against the PHY state machine being left in PHY_CABLETEST state
with no other way to recover than do an ip link set down/up in order to
recover it. We can try to tackle this as an improvement later on.
-- 
Florian

Re: [PATCH net-next v1 1/9] net: phy: Add cable test support to state machine

[Florian Fainelli]

On 4/25/2020 11:06 AM, Andrew Lunn wrote:
> Running a cable test is desruptive to normal operation of the PHY and
> can take a 5 to 10 seconds to complete. The RTNL lock cannot be held
> for this amount of time, and add a new state to the state machine for
> running a cable test.
> 
> The driver is expected to implement two functions. The first is used
> to start a cable test. Once the test has started, it should return.
> 
> The second function is called once per second, or on interrupt to
> check if the cable test is complete, and to allow the PHY to report
> the status.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---

[snip]

>  
> +static void phy_cable_test_abort(struct phy_device *phydev)
> +{
> +	genphy_soft_reset(phydev);

Some drivers implement a soft_reset callback which would not be covered
here, so I believe you need to call phy_init_hw() here. With that fixed

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>	
-- 
Florian

Re: [PATCH net-next v1 2/9] net: phy: Add support for polling cable test

[Andrew Lunn]

> > At least with the Marvell PHY, there is no documented way to tell it
> > to abort a cable test. So knowing the user space has lost interest in
> > the results does not help us.
> 
> The same is true with Broadcom PHYs, but I think we should be guarding
> somehow against the PHY state machine being left in PHY_CABLETEST state
> with no other way to recover than do an ip link set down/up in order to
> recover it. We can try to tackle this as an improvement later on.

I did wonder about setting a big timeout in the core. If after 20
seconds it has not returned any results, we probably need to move to
the error state? But that only works for polling. For interrupt driven
PHYs we don't have any sort of timer, we would need to add more
infrastructure to the core.

If the PHY never completed the cable test, we are probably in the land
of driver bugs, and how do we recover from that?

    Andrew

Re: [PATCH net-next v1 4/9] net: ethtool: Add attributes for cable test reports

[Michal Kubecek]

On Sun, Apr 26, 2020 at 11:12:42PM +0200, Andrew Lunn wrote:
> > > +
> > > + +-------------------------------------------+--------+-----------------------+
> > > + | ``ETHTOOL_A_CABLE_TEST_HEADER``           | nested | reply header          |
> > > + +-------------------------------------------+--------+-----------------------+
> > > + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test result     |
> > > + +-+-----------------------------------------+--------+-----------------------+
> > > + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
> > > + +-+-----------------------------------------+--------+-----------------------+
> > > + | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
> > > + +-+-----------------------------------------+--------+-----------------------+
> > > + | ``ETHTOOL_A_CABLE_TEST_NTF_RESULT``       | nested | cable test results    |
> > > + +-+-----------------------------------------+--------+-----------------------+
> > > + | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number           |
> > > + +-+-----------------------------------------+--------+-----------------------+
> > > + | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code           |
> > > + +-+-----------------------------------------+--------+-----------------------+
> > > + | ``ETHTOOL_A_CABLE_TEST_NTF_FAULT_LENGTH`` | nested | cable length          |
> > > + +-+-----------------------------------------+--------+-----------------------+
> > > + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR``   | u8     | pair number           |
> > > + +-+-----------------------------------------+--------+-----------------------+
> > > + | | ``ETHTOOL_A_CABLE_FAULT_LENGTH_CM``     | u8     | length in cm          |
> > > + +-+-----------------------------------------+--------+-----------------------+
> > 
> > Would it be complicated to gather all information for each pair
> > together? I.e. to have one nest for each pair with pair number, result
> > code and possibly other information (if available). I believe it would
> > make the message easier to process.
> 
> It is something i considered, but decided against. Attributes give us
> flexibility to report whatever the PHY gives us. There is no
> standardisation here. Some PHYs will report the first fault on a
> pair. Others will report multiple faults on a pair. Some PHYs can tell
> you pair X is shorted to pair Y, etc, while some PHYs just tell you it
> is shorted. It also keeps the driver code simple. Report whatever you
> have in any order and it does not matter. And it means i don't need
> complex helper code trying to coordinating information from the
> driver.
> 
> So far, a plain dump of the netlink message in user space also seems
> readable. But when we have more PHYs supported and more variability
> between PHYs we might need to consider if user space should do some
> sorting before printing the test results.

OK, if it would make driver code more complicated, let's keep it like
this. But it's still a bit unclear what exactly does the structure look
like as the example does not seem to match the enums below; I'll have to
take a look at the code composing the messages.

> > > +enum {
> > > +	ETHTOOL_A_CABLE_PAIR_0,
> > > +	ETHTOOL_A_CABLE_PAIR_1,
> > > +	ETHTOOL_A_CABLE_PAIR_2,
> > > +	ETHTOOL_A_CABLE_PAIR_3,
> > > +};
> > 
> > Do we really need this enum, couldn't we simply use a number (possibly
> > with a sanity check of maximum value)?
> 
> They are not strictly required. But it helps with consistence. Are the
> pairs numbered 0, 1, 2, 3, or 1, 2, 3, 4?

OK, I'm not strictly opposed to it, it just felt a bit weird.

> > > +enum {
> > > +	ETHTOOL_A_CABLE_RESULT_UNSPEC,
> > > +	ETHTOOL_A_CABLE_RESULT_PAIR,		/* u8 ETHTOOL_A_CABLE_PAIR_ */
> > > +	ETHTOOL_A_CABLE_RESULT_CODE,		/* u8 ETHTOOL_A_CABLE_RESULT_CODE_ */
> > > +
> > > +	__ETHTOOL_A_CABLE_RESULT_CNT,
> > > +	ETHTOOL_A_CABLE_RESULT_MAX = (__ETHTOOL_A_CABLE_RESULT_CNT - 1)
> > > +};
> > > +
> > > +enum {
> > > +	ETHTOOL_A_CABLE_FAULT_LENGTH_UNSPEC,
> > > +	ETHTOOL_A_CABLE_FAULT_LENGTH_PAIR,	/* u8 ETHTOOL_A_CABLE_PAIR_ */
> > > +	ETHTOOL_A_CABLE_FAULT_LENGTH_CM,	/* u16 */
> > 
> > The example above says "u8" (which is obviously wrong).
> 
> Yep, will fix that.
> 
> > I would rather suggest u32 here to be as future proof as possible.
> 
> Yes, i've been going backwards and forwards on that. I did not realize
> netlink messages were space inefficient. A u8 takes as much space as a
> u32. I picked u16 because that allows up 65535cm, or 655.35m. All the
> IEEE 802.3 Base-T standards have a maximum cable length of 100m, or
> shorter. They might work with longer cables, but i doubt a cable 6
> times longer than the specified max will work. So a u16 is ample.
> 
> The only argument i can see for a u32 is if somebody can implement
> cable testing for fibre optical cables. Then a u16 is not big enough.
> But so far, i've never seen an SFP module offering this.

The problem is that this is UAPI which is "cast in stone". Thus we
should ask "Can I imagine someone implementing this in next 10-20
years?" rather than "Is there a real life hardware implementing this?".
Switching from u16 to u32 in internal kernel API may be a headache if
there are many drivers using it but switching from u16 to u32 in UAPI is
out of question so that we would have to resort to a workaround like
adding another attribute for "long fault length".

Michal

> 
> > One more idea: it would be IMHO useful to also send a notification when
> > the test is started. It could be distinguished by a status attribute
> > which would describe status of the test as a whole (not a specific
> > pair), e.g. started, finished, aborted.
>  
> Yes, give how long these tests take, i could be useful. There is also
> already some hints about this, in that the last patch in the series
> changes the RFC 2863 status of the interface, which i hope sends a
> message to user space about the interface change of status.
> 
> 	Andrew

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Michael Walle]

Hi Andrew,

> Add infrastructure in ethtool and phylib support for triggering a
> cable test and reporting the results. The Marvell 1G PHY driver is
> then extended to make use of this infrastructure.

I'm currently trying this with the AR8031 PHY. With this PHY, you
have to select the pair which you want to start the test on. So
you'd have to start the test four times in a row for a normal
gigabit cable. Right now, I don't see a way how to do that
efficiently if there is no interrupt. One could start another test
in the get_status() polling if the former was completed
successfully. But then you'd have to wait at least four polling
intervals to get the final result (given a cable with four pairs).

Any other ideas?

-michael

Re: [PATCH net-next v1 4/9] net: ethtool: Add attributes for cable test reports

[Michael Walle]

Hi,

> > > > +enum {
> > > > +	ETHTOOL_A_CABLE_PAIR_0,
> > > > +	ETHTOOL_A_CABLE_PAIR_1,
> > > > +	ETHTOOL_A_CABLE_PAIR_2,
> > > > +	ETHTOOL_A_CABLE_PAIR_3,
> > > > +};
> > > 
> > > Do we really need this enum, couldn't we simply use a number (possibly
> > > with a sanity check of maximum value)?
> > 
> > They are not strictly required. But it helps with consistence. Are the
> > pairs numbered 0, 1, 2, 3, or 1, 2, 3, 4?
> 
> OK, I'm not strictly opposed to it, it just felt a bit weird.

Speaking of the pairs. What is PAIR_0 and what is PAIR_3? Maybe
it is specified somewhere in a standard, but IMHO an example for
a normal TP cable would help to prevent wild growth amongst the
PHY drivers and would help to provide consistent reporting towards
the user space.

-michael

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Andrew Lunn]

On Wed, Apr 29, 2020 at 06:02:13PM +0200, Michael Walle wrote:
> Hi Andrew,
> 
> > Add infrastructure in ethtool and phylib support for triggering a
> > cable test and reporting the results. The Marvell 1G PHY driver is
> > then extended to make use of this infrastructure.
> 
> I'm currently trying this with the AR8031 PHY. With this PHY, you
> have to select the pair which you want to start the test on. So
> you'd have to start the test four times in a row for a normal
> gigabit cable. Right now, I don't see a way how to do that
> efficiently if there is no interrupt. One could start another test
> in the get_status() polling if the former was completed
> successfully. But then you'd have to wait at least four polling
> intervals to get the final result (given a cable with four pairs).
> 
> Any other ideas?

Hi Michael

Nice to see some more PHYs getting support for this.

It is important that the start function returns quickly. However, the
get status function can block. So you could do all the work in the
first call to get status, polling for completion at a faster rate,
etc.

	Andrew

Re: [PATCH net-next v1 4/9] net: ethtool: Add attributes for cable test reports

[Andrew Lunn]

On Wed, Apr 29, 2020 at 06:16:05PM +0200, Michael Walle wrote:
> Hi,
> 
> > > > > +enum {
> > > > > +	ETHTOOL_A_CABLE_PAIR_0,
> > > > > +	ETHTOOL_A_CABLE_PAIR_1,
> > > > > +	ETHTOOL_A_CABLE_PAIR_2,
> > > > > +	ETHTOOL_A_CABLE_PAIR_3,
> > > > > +};
> > > > 
> > > > Do we really need this enum, couldn't we simply use a number (possibly
> > > > with a sanity check of maximum value)?
> > > 
> > > They are not strictly required. But it helps with consistence. Are the
> > > pairs numbered 0, 1, 2, 3, or 1, 2, 3, 4?
> > 
> > OK, I'm not strictly opposed to it, it just felt a bit weird.
> 
> Speaking of the pairs. What is PAIR_0 and what is PAIR_3? Maybe
> it is specified somewhere in a standard, but IMHO an example for
> a normal TP cable would help to prevent wild growth amongst the
> PHY drivers and would help to provide consistent reporting towards
> the user space.

Hi Michael

Good question

Section 25.4.3 gives the pin out for 100BaseT. There is no pair
numbering, just transmit+, transmit- and receive+, receive- signals.

1000BaseT calls the signals BI_DA+, BI_DA-, BI_DB+, BI_DB-, BI_DC+,
BI_DC-, BI_DDA+, BI_DD-. Comparing the pinout 100BaseT would use
BI_DA+, BI_DA-, BI_DB+, BI_DB. But 1000BaseT does not really have
transmit and receive pairs due to Auto MDI-X.

BroadReach calls the one pair it has BI_DA+/BI_DA-.

Maybe it would be better to have:

enum {
	ETHTOOL_A_CABLE_PAIR_A,
	ETHTOOL_A_CABLE_PAIR_B,
	ETHTOOL_A_CABLE_PAIR_C,
	ETHTOOL_A_CABLE_PAIR_D,
};

	Andrew

Re: [PATCH net-next v1 4/9] net: ethtool: Add attributes for cable test reports

[Florian Fainelli]

On 4/29/20 11:57 AM, Andrew Lunn wrote:
> On Wed, Apr 29, 2020 at 06:16:05PM +0200, Michael Walle wrote:
>> Hi,
>>
>>>>>> +enum {
>>>>>> +	ETHTOOL_A_CABLE_PAIR_0,
>>>>>> +	ETHTOOL_A_CABLE_PAIR_1,
>>>>>> +	ETHTOOL_A_CABLE_PAIR_2,
>>>>>> +	ETHTOOL_A_CABLE_PAIR_3,
>>>>>> +};
>>>>>
>>>>> Do we really need this enum, couldn't we simply use a number (possibly
>>>>> with a sanity check of maximum value)?
>>>>
>>>> They are not strictly required. But it helps with consistence. Are the
>>>> pairs numbered 0, 1, 2, 3, or 1, 2, 3, 4?
>>>
>>> OK, I'm not strictly opposed to it, it just felt a bit weird.
>>
>> Speaking of the pairs. What is PAIR_0 and what is PAIR_3? Maybe
>> it is specified somewhere in a standard, but IMHO an example for
>> a normal TP cable would help to prevent wild growth amongst the
>> PHY drivers and would help to provide consistent reporting towards
>> the user space.
> 
> Hi Michael
> 
> Good question
> 
> Section 25.4.3 gives the pin out for 100BaseT. There is no pair
> numbering, just transmit+, transmit- and receive+, receive- signals.
> 
> 1000BaseT calls the signals BI_DA+, BI_DA-, BI_DB+, BI_DB-, BI_DC+,
> BI_DC-, BI_DDA+, BI_DD-. Comparing the pinout 100BaseT would use
> BI_DA+, BI_DA-, BI_DB+, BI_DB. But 1000BaseT does not really have
> transmit and receive pairs due to Auto MDI-X.
> 
> BroadReach calls the one pair it has BI_DA+/BI_DA-.
> 
> Maybe it would be better to have:
> 
> enum {
> 	ETHTOOL_A_CABLE_PAIR_A,
> 	ETHTOOL_A_CABLE_PAIR_B,
> 	ETHTOOL_A_CABLE_PAIR_C,
> 	ETHTOOL_A_CABLE_PAIR_D,
> };

Yes, that would be clearer IMHO. Broadcom PHYs tend to refer to pairs A,
B, C and D in their datasheets.
-- 
Florian

Re: [PATCH net-next v1 4/9] net: ethtool: Add attributes for cable test reports

[Michael Walle]

Am 2020-04-29 20:58, schrieb Florian Fainelli:
> On 4/29/20 11:57 AM, Andrew Lunn wrote:
>> On Wed, Apr 29, 2020 at 06:16:05PM +0200, Michael Walle wrote:
>>> Hi,
>>> 
>>>>>>> +enum {
>>>>>>> +	ETHTOOL_A_CABLE_PAIR_0,
>>>>>>> +	ETHTOOL_A_CABLE_PAIR_1,
>>>>>>> +	ETHTOOL_A_CABLE_PAIR_2,
>>>>>>> +	ETHTOOL_A_CABLE_PAIR_3,
>>>>>>> +};
>>>>>> 
>>>>>> Do we really need this enum, couldn't we simply use a number 
>>>>>> (possibly
>>>>>> with a sanity check of maximum value)?
>>>>> 
>>>>> They are not strictly required. But it helps with consistence. Are 
>>>>> the
>>>>> pairs numbered 0, 1, 2, 3, or 1, 2, 3, 4?
>>>> 
>>>> OK, I'm not strictly opposed to it, it just felt a bit weird.
>>> 
>>> Speaking of the pairs. What is PAIR_0 and what is PAIR_3? Maybe
>>> it is specified somewhere in a standard, but IMHO an example for
>>> a normal TP cable would help to prevent wild growth amongst the
>>> PHY drivers and would help to provide consistent reporting towards
>>> the user space.
>> 
>> Hi Michael
>> 
>> Good question
>> 
>> Section 25.4.3 gives the pin out for 100BaseT. There is no pair
>> numbering, just transmit+, transmit- and receive+, receive- signals.
>> 
>> 1000BaseT calls the signals BI_DA+, BI_DA-, BI_DB+, BI_DB-, BI_DC+,
>> BI_DC-, BI_DDA+, BI_DD-. Comparing the pinout 100BaseT would use
>> BI_DA+, BI_DA-, BI_DB+, BI_DB. But 1000BaseT does not really have
>> transmit and receive pairs due to Auto MDI-X.
>> 
>> BroadReach calls the one pair it has BI_DA+/BI_DA-.
>> 
>> Maybe it would be better to have:
>> 
>> enum {
>> 	ETHTOOL_A_CABLE_PAIR_A,
>> 	ETHTOOL_A_CABLE_PAIR_B,
>> 	ETHTOOL_A_CABLE_PAIR_C,
>> 	ETHTOOL_A_CABLE_PAIR_D,
>> };
> 
> Yes, that would be clearer IMHO. Broadcom PHYs tend to refer to pairs 
> A,
> B, C and D in their datasheets.

Qualcomm Atheros calls them MDI[0], MDI[1], etc.. maybe mention
the corresponding pin on an RJ45 connector for reference?

-michael

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Michael Walle]

Hi Andrew,

Am 2020-04-29 18:32, schrieb Andrew Lunn:
> On Wed, Apr 29, 2020 at 06:02:13PM +0200, Michael Walle wrote:
>> Hi Andrew,
>> 
>> > Add infrastructure in ethtool and phylib support for triggering a
>> > cable test and reporting the results. The Marvell 1G PHY driver is
>> > then extended to make use of this infrastructure.
>> 
>> I'm currently trying this with the AR8031 PHY. With this PHY, you
>> have to select the pair which you want to start the test on. So
>> you'd have to start the test four times in a row for a normal
>> gigabit cable. Right now, I don't see a way how to do that
>> efficiently if there is no interrupt. One could start another test
>> in the get_status() polling if the former was completed
>> successfully. But then you'd have to wait at least four polling
>> intervals to get the final result (given a cable with four pairs).
>> 
>> Any other ideas?
> 
> Hi Michael
> 
> Nice to see some more PHYs getting support for this.
> 
> It is important that the start function returns quickly. However, the
> get status function can block. So you could do all the work in the
> first call to get status, polling for completion at a faster rate,
> etc.

Ok. I do have one problem. TDR works fine for the AR8031 and the
BCM54140 as long as there is no link partner, i.e. open cable,
shorted pairs etc. But as soon as there is a link partner and a
link, both PHYs return garbage. As far as I understand TDR, there
must not be a link, correct? The link partner may send data or
link pulses. No how do you silence the local NIC or even the peer?

-michael

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Andrew Lunn]

> Ok. I do have one problem. TDR works fine for the AR8031 and the
> BCM54140 as long as there is no link partner, i.e. open cable,
> shorted pairs etc. But as soon as there is a link partner and a
> link, both PHYs return garbage. As far as I understand TDR, there
> must not be a link, correct?

Correct.

The Marvell PHY will down the link and then wait 1.5 seconds before
starting TDR, if you set a bit. I _think_ it downs the link by turning
off autoneg.

Maybe there is some hints in 802.3 clause 22?

      Andrew

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Florian Fainelli]

On 4/30/20 10:48 AM, Michael Walle wrote:
> Hi Andrew,
> 
> Am 2020-04-29 18:32, schrieb Andrew Lunn:
>> On Wed, Apr 29, 2020 at 06:02:13PM +0200, Michael Walle wrote:
>>> Hi Andrew,
>>>
>>> > Add infrastructure in ethtool and phylib support for triggering a
>>> > cable test and reporting the results. The Marvell 1G PHY driver is
>>> > then extended to make use of this infrastructure.
>>>
>>> I'm currently trying this with the AR8031 PHY. With this PHY, you
>>> have to select the pair which you want to start the test on. So
>>> you'd have to start the test four times in a row for a normal
>>> gigabit cable. Right now, I don't see a way how to do that
>>> efficiently if there is no interrupt. One could start another test
>>> in the get_status() polling if the former was completed
>>> successfully. But then you'd have to wait at least four polling
>>> intervals to get the final result (given a cable with four pairs).
>>>
>>> Any other ideas?
>>
>> Hi Michael
>>
>> Nice to see some more PHYs getting support for this.
>>
>> It is important that the start function returns quickly. However, the
>> get status function can block. So you could do all the work in the
>> first call to get status, polling for completion at a faster rate,
>> etc.
> 
> Ok. I do have one problem. TDR works fine for the AR8031 and the
> BCM54140 as long as there is no link partner, i.e. open cable,
> shorted pairs etc. But as soon as there is a link partner and a
> link, both PHYs return garbage. As far as I understand TDR, there
> must not be a link, correct? The link partner may send data or
> link pulses. No how do you silence the local NIC or even the peer?

Michael do you use the enhanced cable diagnostics (ECD) or the simple
cable diagnostics? Having tried to get older Broadcom PHYs to work with
cable diagnostics, you need to calibrate the PHY prior to running
diagnostics and you need to soft reset it.
-- 
Florian

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Michael Walle]

Hi Florian,

Am 2020-04-30 20:34, schrieb Florian Fainelli:
> On 4/30/20 10:48 AM, Michael Walle wrote:
>> Hi Andrew,
>> 
>> Am 2020-04-29 18:32, schrieb Andrew Lunn:
>>> On Wed, Apr 29, 2020 at 06:02:13PM +0200, Michael Walle wrote:
>>>> Hi Andrew,
>>>> 
>>>> > Add infrastructure in ethtool and phylib support for triggering a
>>>> > cable test and reporting the results. The Marvell 1G PHY driver is
>>>> > then extended to make use of this infrastructure.
>>>> 
>>>> I'm currently trying this with the AR8031 PHY. With this PHY, you
>>>> have to select the pair which you want to start the test on. So
>>>> you'd have to start the test four times in a row for a normal
>>>> gigabit cable. Right now, I don't see a way how to do that
>>>> efficiently if there is no interrupt. One could start another test
>>>> in the get_status() polling if the former was completed
>>>> successfully. But then you'd have to wait at least four polling
>>>> intervals to get the final result (given a cable with four pairs).
>>>> 
>>>> Any other ideas?
>>> 
>>> Hi Michael
>>> 
>>> Nice to see some more PHYs getting support for this.
>>> 
>>> It is important that the start function returns quickly. However, the
>>> get status function can block. So you could do all the work in the
>>> first call to get status, polling for completion at a faster rate,
>>> etc.
>> 
>> Ok. I do have one problem. TDR works fine for the AR8031 and the
>> BCM54140 as long as there is no link partner, i.e. open cable,
>> shorted pairs etc. But as soon as there is a link partner and a
>> link, both PHYs return garbage. As far as I understand TDR, there
>> must not be a link, correct? The link partner may send data or
>> link pulses. No how do you silence the local NIC or even the peer?
> 
> Michael do you use the enhanced cable diagnostics (ECD) or the simple
> cable diagnostics?

ECD. The registers looks exactly like the one from the Marvell PHYs,
which makes me wonder if both have the same building block or if one
imitated the registers of the other. There are subtle differences
like one bit in the broadcom PHY is "break link" and is self-clearing,
while the bit on the Marvell PHY is described as "perform diagnostics
on link break".

I don't know what simple cable diagnostics should be, I guess the
BCM54140 doesn't support it or its not documented. Actually, ECD
has very little documentation in general.

> Having tried to get older Broadcom PHYs to work with
> cable diagnostics, you need to calibrate the PHY prior to running
> diagnostics and you need to soft reset it.

What do you mean by calibrate it?

-michael

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Florian Fainelli]

On 4/30/20 12:31 PM, Michael Walle wrote:
> Hi Florian,
> 
> Am 2020-04-30 20:34, schrieb Florian Fainelli:
>> On 4/30/20 10:48 AM, Michael Walle wrote:
>>> Hi Andrew,
>>>
>>> Am 2020-04-29 18:32, schrieb Andrew Lunn:
>>>> On Wed, Apr 29, 2020 at 06:02:13PM +0200, Michael Walle wrote:
>>>>> Hi Andrew,
>>>>>
>>>>> > Add infrastructure in ethtool and phylib support for triggering a
>>>>> > cable test and reporting the results. The Marvell 1G PHY driver is
>>>>> > then extended to make use of this infrastructure.
>>>>>
>>>>> I'm currently trying this with the AR8031 PHY. With this PHY, you
>>>>> have to select the pair which you want to start the test on. So
>>>>> you'd have to start the test four times in a row for a normal
>>>>> gigabit cable. Right now, I don't see a way how to do that
>>>>> efficiently if there is no interrupt. One could start another test
>>>>> in the get_status() polling if the former was completed
>>>>> successfully. But then you'd have to wait at least four polling
>>>>> intervals to get the final result (given a cable with four pairs).
>>>>>
>>>>> Any other ideas?
>>>>
>>>> Hi Michael
>>>>
>>>> Nice to see some more PHYs getting support for this.
>>>>
>>>> It is important that the start function returns quickly. However, the
>>>> get status function can block. So you could do all the work in the
>>>> first call to get status, polling for completion at a faster rate,
>>>> etc.
>>>
>>> Ok. I do have one problem. TDR works fine for the AR8031 and the
>>> BCM54140 as long as there is no link partner, i.e. open cable,
>>> shorted pairs etc. But as soon as there is a link partner and a
>>> link, both PHYs return garbage. As far as I understand TDR, there
>>> must not be a link, correct? The link partner may send data or
>>> link pulses. No how do you silence the local NIC or even the peer?
>>
>> Michael do you use the enhanced cable diagnostics (ECD) or the simple
>> cable diagnostics?
> 
> ECD. The registers looks exactly like the one from the Marvell PHYs,
> which makes me wonder if both have the same building block or if one
> imitated the registers of the other. There are subtle differences
> like one bit in the broadcom PHY is "break link" and is self-clearing,
> while the bit on the Marvell PHY is described as "perform diagnostics
> on link break".
> 
> I don't know what simple cable diagnostics should be, I guess the
> BCM54140 doesn't support it or its not documented. Actually, ECD
> has very little documentation in general.

Yes, there is very little information, even internally. My understanding
is that diagnostics at run at auto-neg, so you need to break the link,
and when the link comes back up is when you should have per-pair
results. There are also some caveats, like the link parnter must also
have auto-negotiation on for the diagnostics to work.

> 
>> Having tried to get older Broadcom PHYs to work with
>> cable diagnostics, you need to calibrate the PHY prior to running
>> diagnostics and you need to soft reset it.
> 
> What do you mean by calibrate it?

You need to tune the AFE and DSP of the PHY in order for it to report
accurate cable lengths. I would not think that you have access to that,
and what I got access to is not really well documented, which is why I
have not had a chance to submit those changes yet.

How accurate are your results when there is no link?
-- 
Florian

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Andrew Lunn]

> ECD. The registers looks exactly like the one from the Marvell PHYs,
> which makes me wonder if both have the same building block or if one
> imitated the registers of the other. There are subtle differences
> like one bit in the broadcom PHY is "break link" and is self-clearing,
> while the bit on the Marvell PHY is described as "perform diagnostics
> on link break".

Should we be sharing code between the two drivers?

> What do you mean by calibrate it?

Some of the Marvell documentation talks about calibrating for losses
on the PCB. Run a diagnostics with no cable plugged in, and get the
cable length to the 'fault'. This gives you the distance to the RJ45
socket. You should then subtract that from all subsequent results.
But since this is board design specific, i decided to ignore it. I
suppose it could be stuffed into a DT property, but i got the feeling
it is not worth it, given the measurement granularity of 80cm.

   Andrew

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Michael Walle]

Am 2020-04-30 20:23, schrieb Andrew Lunn:
>> Ok. I do have one problem. TDR works fine for the AR8031 and the
>> BCM54140 as long as there is no link partner, i.e. open cable,
>> shorted pairs etc. But as soon as there is a link partner and a
>> link, both PHYs return garbage. As far as I understand TDR, there
>> must not be a link, correct?
> 
> Correct.
> 
> The Marvell PHY will down the link and then wait 1.5 seconds before
> starting TDR, if you set a bit. I _think_ it downs the link by turning
> off autoneg.

According to the Atheros datasheet the "CDT can be performed when
there is no link partner or when the link partner is auto-negotiating".
One could interpret that as is is only allowed _during_ autoneg. But
as far as I know there is no indication if autoneg is currently active,
is it?

> Maybe there is some hints in 802.3 clause 22?

I'm just skimming over that, but even if the link could be disabled
locally, shouldn't there still be link pulses from the peer?

-michael

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Michael Walle]

Am 2020-04-30 21:38, schrieb Florian Fainelli:
> On 4/30/20 12:31 PM, Michael Walle wrote:
>> Hi Florian,
>> 
>> Am 2020-04-30 20:34, schrieb Florian Fainelli:
>>> On 4/30/20 10:48 AM, Michael Walle wrote:
>>>> Hi Andrew,
>>>> 
>>>> Am 2020-04-29 18:32, schrieb Andrew Lunn:
>>>>> On Wed, Apr 29, 2020 at 06:02:13PM +0200, Michael Walle wrote:
>>>>>> Hi Andrew,
>>>>>> 
>>>>>> > Add infrastructure in ethtool and phylib support for triggering a
>>>>>> > cable test and reporting the results. The Marvell 1G PHY driver is
>>>>>> > then extended to make use of this infrastructure.
>>>>>> 
>>>>>> I'm currently trying this with the AR8031 PHY. With this PHY, you
>>>>>> have to select the pair which you want to start the test on. So
>>>>>> you'd have to start the test four times in a row for a normal
>>>>>> gigabit cable. Right now, I don't see a way how to do that
>>>>>> efficiently if there is no interrupt. One could start another test
>>>>>> in the get_status() polling if the former was completed
>>>>>> successfully. But then you'd have to wait at least four polling
>>>>>> intervals to get the final result (given a cable with four pairs).
>>>>>> 
>>>>>> Any other ideas?
>>>>> 
>>>>> Hi Michael
>>>>> 
>>>>> Nice to see some more PHYs getting support for this.
>>>>> 
>>>>> It is important that the start function returns quickly. However, 
>>>>> the
>>>>> get status function can block. So you could do all the work in the
>>>>> first call to get status, polling for completion at a faster rate,
>>>>> etc.
>>>> 
>>>> Ok. I do have one problem. TDR works fine for the AR8031 and the
>>>> BCM54140 as long as there is no link partner, i.e. open cable,
>>>> shorted pairs etc. But as soon as there is a link partner and a
>>>> link, both PHYs return garbage. As far as I understand TDR, there
>>>> must not be a link, correct? The link partner may send data or
>>>> link pulses. No how do you silence the local NIC or even the peer?
>>> 
>>> Michael do you use the enhanced cable diagnostics (ECD) or the simple
>>> cable diagnostics?
>> 
>> ECD. The registers looks exactly like the one from the Marvell PHYs,
>> which makes me wonder if both have the same building block or if one
>> imitated the registers of the other. There are subtle differences
>> like one bit in the broadcom PHY is "break link" and is self-clearing,
>> while the bit on the Marvell PHY is described as "perform diagnostics
>> on link break".
>> 
>> I don't know what simple cable diagnostics should be, I guess the
>> BCM54140 doesn't support it or its not documented. Actually, ECD
>> has very little documentation in general.
> 
> Yes, there is very little information, even internally. My 
> understanding
> is that diagnostics at run at auto-neg, so you need to break the link,
> and when the link comes back up is when you should have per-pair
> results. There are also some caveats, like the link parnter must also
> have auto-negotiation on for the diagnostics to work.

Ok, that was also my guess.

>>> Having tried to get older Broadcom PHYs to work with
>>> cable diagnostics, you need to calibrate the PHY prior to running
>>> diagnostics and you need to soft reset it.
>> 
>> What do you mean by calibrate it?
> 
> You need to tune the AFE and DSP of the PHY in order for it to report
> accurate cable lengths. I would not think that you have access to that,
> and what I got access to is not really well documented, which is why I
> have not had a chance to submit those changes yet.
> 
> How accurate are your results when there is no link?

I've only tested with a ~100m cable (or two in a row). The results were
104m to 107m.

-michael

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Michael Walle]

Am 2020-04-30 21:41, schrieb Andrew Lunn:
>> ECD. The registers looks exactly like the one from the Marvell PHYs,
>> which makes me wonder if both have the same building block or if one
>> imitated the registers of the other. There are subtle differences
>> like one bit in the broadcom PHY is "break link" and is self-clearing,
>> while the bit on the Marvell PHY is described as "perform diagnostics
>> on link break".
> 
> Should we be sharing code between the two drivers?

If they are indeed the same sure, but it doesn't look like that the
marvell procedure works for the broadcom PHY.

>> What do you mean by calibrate it?
> 
> Some of the Marvell documentation talks about calibrating for losses
> on the PCB. Run a diagnostics with no cable plugged in, and get the
> cable length to the 'fault'. This gives you the distance to the RJ45
> socket. You should then subtract that from all subsequent results.
> But since this is board design specific, i decided to ignore it. I
> suppose it could be stuffed into a DT property, but i got the feeling
> it is not worth it, given the measurement granularity of 80cm.

Oh so, the time delta counter of the Marvell PHY also runs at 125MHz? ;)

        /* Accoring to the datasheet the distance to the fault is
         * DELTA_TIME * 0.824 meters.
         *
         * The author suspect the correct formula is:
         *  distance in meters = (c * VF) / (2 * 125MHz)
         * where c is the speed of light, VF is the velocity factor of
         * the twisted pair cable, 125MHz the counter frequency and
         * the factor 2 because the hardware will measure the round
         * trip time.
         * With a VF of 0.69 we get the factor 0.824 mentioned in the
         * datasheet.
         */


-michael

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Florian Fainelli]

On 4/30/20 12:41 PM, Andrew Lunn wrote:
>> ECD. The registers looks exactly like the one from the Marvell PHYs,
>> which makes me wonder if both have the same building block or if one
>> imitated the registers of the other. There are subtle differences
>> like one bit in the broadcom PHY is "break link" and is self-clearing,
>> while the bit on the Marvell PHY is described as "perform diagnostics
>> on link break".
> 
> Should we be sharing code between the two drivers?

Yes, I am amazed how how identical they are, nearly on a bit level
identical, the coincidence is uncanny. The expansion registers are also
0x10 - 0x15 just in the reverse order, you know, so as to make it not
too obvious this looks about the same ;) I wonder if we managed to find
something here.

> 
>> What do you mean by calibrate it?
> 
> Some of the Marvell documentation talks about calibrating for losses
> on the PCB. Run a diagnostics with no cable plugged in, and get the
> cable length to the 'fault'. This gives you the distance to the RJ45
> socket. You should then subtract that from all subsequent results.
> But since this is board design specific, i decided to ignore it. I
> suppose it could be stuffed into a DT property, but i got the feeling
> it is not worth it, given the measurement granularity of 80cm.

OK, accuracy is different here, they are said to be +/- 5m accurate for
cable faults and +/- 10m accurate for good cables.
-- 
Florian

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Michael Walle]

Am 2020-04-30 22:04, schrieb Florian Fainelli:
> On 4/30/20 12:41 PM, Andrew Lunn wrote:
>>> ECD. The registers looks exactly like the one from the Marvell PHYs,
>>> which makes me wonder if both have the same building block or if one
>>> imitated the registers of the other. There are subtle differences
>>> like one bit in the broadcom PHY is "break link" and is 
>>> self-clearing,
>>> while the bit on the Marvell PHY is described as "perform diagnostics
>>> on link break".
>> 
>> Should we be sharing code between the two drivers?
> 
> Yes, I am amazed how how identical they are, nearly on a bit level
> identical, the coincidence is uncanny. The expansion registers are also
> 0x10 - 0x15 just in the reverse order,

At what PHY are you looking? I've just found some datasheets where they
are at 0xC0 to 0xC5.

> you know, so as to make it not
> too obvious this looks about the same ;) I wonder if we managed to find
> something here.
> 
>> 
>>> What do you mean by calibrate it?
>> 
>> Some of the Marvell documentation talks about calibrating for losses
>> on the PCB. Run a diagnostics with no cable plugged in, and get the
>> cable length to the 'fault'. This gives you the distance to the RJ45
>> socket. You should then subtract that from all subsequent results.
>> But since this is board design specific, i decided to ignore it. I
>> suppose it could be stuffed into a DT property, but i got the feeling
>> it is not worth it, given the measurement granularity of 80cm.
> 
> OK, accuracy is different here, they are said to be +/- 5m accurate for
> cable faults and +/- 10m accurate for good cables.

Accuracy != granularity. But yes, if one digit correspond to 80cm it
doesn't really make sense to remove the PCB trace error if you assume
that it might add just one digit at most.

-michael

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Florian Fainelli]

On 4/30/20 1:13 PM, Michael Walle wrote:
> Am 2020-04-30 22:04, schrieb Florian Fainelli:
>> On 4/30/20 12:41 PM, Andrew Lunn wrote:
>>>> ECD. The registers looks exactly like the one from the Marvell PHYs,
>>>> which makes me wonder if both have the same building block or if one
>>>> imitated the registers of the other. There are subtle differences
>>>> like one bit in the broadcom PHY is "break link" and is self-clearing,
>>>> while the bit on the Marvell PHY is described as "perform diagnostics
>>>> on link break".
>>>
>>> Should we be sharing code between the two drivers?
>>
>> Yes, I am amazed how how identical they are, nearly on a bit level
>> identical, the coincidence is uncanny. The expansion registers are also
>> 0x10 - 0x15 just in the reverse order,
> 
> At what PHY are you looking? I've just found some datasheets where they
> are at 0xC0 to 0xC5.

BCM54810 because that's what I have on my desk right now, but 0x10 would
be the offset relative to the expansion register space, which would
translate into this:

https://github.com/ffainelli/linux/commits/broadcom-cable-tests

(sorry for the mess it is a patchwork of tests on various platforms,
based on an earlier branch from Andrew).

> 
>> you know, so as to make it not
>> too obvious this looks about the same ;) I wonder if we managed to find
>> something here.
>>
>>>
>>>> What do you mean by calibrate it?
>>>
>>> Some of the Marvell documentation talks about calibrating for losses
>>> on the PCB. Run a diagnostics with no cable plugged in, and get the
>>> cable length to the 'fault'. This gives you the distance to the RJ45
>>> socket. You should then subtract that from all subsequent results.
>>> But since this is board design specific, i decided to ignore it. I
>>> suppose it could be stuffed into a DT property, but i got the feeling
>>> it is not worth it, given the measurement granularity of 80cm.
>>
>> OK, accuracy is different here, they are said to be +/- 5m accurate for
>> cable faults and +/- 10m accurate for good cables.
> 
> Accuracy != granularity. But yes, if one digit correspond to 80cm it
> doesn't really make sense to remove the PCB trace error if you assume
> that it might add just one digit at most.

One of the test racks that I use has very short cables, but I guess it
does not matter if they get reported as 80cm or 160cm...
-- 
Florian

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Andrew Lunn]

On Thu, Apr 30, 2020 at 09:44:12PM +0200, Michael Walle wrote:
> Am 2020-04-30 20:23, schrieb Andrew Lunn:
> > > Ok. I do have one problem. TDR works fine for the AR8031 and the
> > > BCM54140 as long as there is no link partner, i.e. open cable,
> > > shorted pairs etc. But as soon as there is a link partner and a
> > > link, both PHYs return garbage. As far as I understand TDR, there
> > > must not be a link, correct?
> > 
> > Correct.
> > 
> > The Marvell PHY will down the link and then wait 1.5 seconds before
> > starting TDR, if you set a bit. I _think_ it downs the link by turning
> > off autoneg.
> 
> According to the Atheros datasheet the "CDT can be performed when
> there is no link partner or when the link partner is auto-negotiating".
> One could interpret that as is is only allowed _during_ autoneg. But
> as far as I know there is no indication if autoneg is currently active,
> is it?
> 
> > Maybe there is some hints in 802.3 clause 22?
> 
> I'm just skimming over that, but even if the link could be disabled
> locally, shouldn't there still be link pulses from the peer?

I guessing here....

If the local end stops negotiating, the link peer will stop sending
traffic, and just sends link pulses. The PHY knows when the next set of
link pulses are expected from the link partner, since they should be
every 16ms. It can then avoid them when doing its TDR.

      Andrew

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Andrew Lunn]

On Thu, Apr 30, 2020 at 10:01:55PM +0200, Michael Walle wrote:
> Oh so, the time delta counter of the Marvell PHY also runs at 125MHz? ;)

No idea. But 125MHz is one of the fundamental clocks rates in
Ethernet, so it make sense to reuse it.

>        /* Accoring to the datasheet the distance to the fault is
>         * DELTA_TIME * 0.824 meters.

A calculation like that pops up in my second set of patches where i
add raw TDR data access.

    Andrew

Re: [PATCH net-next v1 0/9] Ethernet Cable test support

[Michael Walle]

Am 2020-04-30 22:19, schrieb Florian Fainelli:
> On 4/30/20 1:13 PM, Michael Walle wrote:
>> Am 2020-04-30 22:04, schrieb Florian Fainelli:
>>> On 4/30/20 12:41 PM, Andrew Lunn wrote:
>>>>> ECD. The registers looks exactly like the one from the Marvell 
>>>>> PHYs,
>>>>> which makes me wonder if both have the same building block or if 
>>>>> one
>>>>> imitated the registers of the other. There are subtle differences
>>>>> like one bit in the broadcom PHY is "break link" and is 
>>>>> self-clearing,
>>>>> while the bit on the Marvell PHY is described as "perform 
>>>>> diagnostics
>>>>> on link break".
>>>> 
>>>> Should we be sharing code between the two drivers?
>>> 
>>> Yes, I am amazed how how identical they are, nearly on a bit level
>>> identical, the coincidence is uncanny. The expansion registers are 
>>> also
>>> 0x10 - 0x15 just in the reverse order,
>> 
>> At what PHY are you looking? I've just found some datasheets where 
>> they
>> are at 0xC0 to 0xC5.
> 
> BCM54810 because that's what I have on my desk right now, but 0x10 
> would
> be the offset relative to the expansion register space, which would
> translate into this:
> 
> https://github.com/ffainelli/linux/commits/broadcom-cable-tests
> 
> (sorry for the mess it is a patchwork of tests on various platforms,
> based on an earlier branch from Andrew).

Ah thanks, now I see what you mean with CD vs ECD. In your latest WIP
you're using ECD, so did both actually work with a link?

Also according to you header files bit 14 of the ECD_CTRL is the
"run after aneg bit"; in the BCM54140 it is just marked as reserved.
I guess I'm trying the CD on the BCM54140 tomorrow.

-michael

> 
>> 
>>> you know, so as to make it not
>>> too obvious this looks about the same ;) I wonder if we managed to 
>>> find
>>> something here.
>>> 
>>>> 
>>>>> What do you mean by calibrate it?
>>>> 
>>>> Some of the Marvell documentation talks about calibrating for losses
>>>> on the PCB. Run a diagnostics with no cable plugged in, and get the
>>>> cable length to the 'fault'. This gives you the distance to the RJ45
>>>> socket. You should then subtract that from all subsequent results.
>>>> But since this is board design specific, i decided to ignore it. I
>>>> suppose it could be stuffed into a DT property, but i got the 
>>>> feeling
>>>> it is not worth it, given the measurement granularity of 80cm.
>>> 
>>> OK, accuracy is different here, they are said to be +/- 5m accurate 
>>> for
>>> cable faults and +/- 10m accurate for good cables.
>> 
>> Accuracy != granularity. But yes, if one digit correspond to 80cm it
>> doesn't really make sense to remove the PCB trace error if you assume
>> that it might add just one digit at most.
> 
> One of the test racks that I use has very short cables, but I guess it
> does not matter if they get reported as 80cm or 160cm...