[PATCH net-next v2 00/10] Ethernet Cable test support

[PATCH net-next v2 00/10] 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.

v2:
See individual patches but:

Remove _REPLY messages
Change length into a u32
Grammar fixes
Rename functions for consistency
Extack for cable test already running
Remove ethnl_cable_test_act_ops
Add status attributes
Rename pairs from numbers to letters

Andrew Lunn (10):
  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
  net: phy: Send notifier when starting the cable test

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

-- 
2.26.2

[PATCH net-next v2 01/10] 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.

v2:
Rename phy_cable_test_abort to phy_abort_cable_test
Return different extack when already running test
Use phy_init_hw() to reset the PHY

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/phy.c | 76 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy.h   | 28 ++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 72c69a9c8a98..4f95e605080e 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,62 @@ static void phy_trigger_machine(struct phy_device *phydev)
 	phy_queue_state_machine(phydev, 0);
 }
 
+static void phy_abort_cable_test(struct phy_device *phydev)
+{
+	int err;
+
+	err = phy_init_hw(phydev);
+	if (err)
+		phydev_err(phydev, "Error while aborting cable test");
+}
+
+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_CABLETEST) {
+		NL_SET_ERR_MSG(extack,
+			       "PHY already performing a test");
+		err = -EBUSY;
+		goto out;
+	}
+
+	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 +866,9 @@ void phy_stop(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
+	if (phydev->state == PHY_CABLETEST)
+		phy_abort_cable_test(phydev);
+
 	if (phydev->sfp_bus)
 		sfp_upstream_stop(phydev->sfp_bus);
 
@@ -870,6 +931,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 +950,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_abort_cable_test(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.2

[PATCH net-next v2 02/10] 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 4f95e605080e..cea89785bcd4 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -521,6 +521,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.2

Re: [PATCH net-next v2 02/10] net: phy: Add support for polling cable test

[Florian Fainelli]

On 5/4/2020 5:18 PM, 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>

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

[PATCH net-next v2 03/10] 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.

v2:
Remove unwanted white space change
Remove ethnl_cable_test_act_ops and use doit handler
Rename cable_test_set_policy cable_test_act_policy
Remove ETHTOOL_MSG_CABLE_TEST_ACT_REPLY

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

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 567326491f80..72d53da2bea9 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
@@ -955,13 +957,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 +1064,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..7e16a1cce20b 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,
@@ -401,6 +402,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 c5ba2d180c43..5c524c6ee75d 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..6e5782a7da80
--- /dev/null
+++ b/net/ethtool/cabletest.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/phy.h>
+#include "netlink.h"
+#include "common.h"
+#include "bitset.h"
+
+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 },
+};
+
+/* CABLE_TEST_ACT */
+
+static const struct nla_policy
+cable_test_act_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_act_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..b9c9ddf408fe 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -839,6 +839,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.2

Re: [PATCH net-next v2 03/10] net: ethtool: netlink: Add support for triggering a cable test

[Florian Fainelli]

On 5/4/2020 5:18 PM, 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.
> 
> v2:
> Remove unwanted white space change
> Remove ethnl_cable_test_act_ops and use doit handler
> Rename cable_test_set_policy cable_test_act_policy
> Remove ETHTOOL_MSG_CABLE_TEST_ACT_REPLY
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

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

Re: [PATCH net-next v2 03/10] net: ethtool: netlink: Add support for triggering a cable test

[Michal Kubecek]

On Tue, May 05, 2020 at 02:18:14AM +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.
> 
> v2:
> Remove unwanted white space change
> Remove ethnl_cable_test_act_ops and use doit handler
> Rename cable_test_set_policy cable_test_act_policy
> Remove ETHTOOL_MSG_CABLE_TEST_ACT_REPLY
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 567326491f80..72d53da2bea9 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

This was dropped.

[...]
> diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
> new file mode 100644
> index 000000000000..6e5782a7da80
> --- /dev/null
> +++ b/net/ethtool/cabletest.c
> @@ -0,0 +1,61 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/phy.h>
> +#include "netlink.h"
> +#include "common.h"
> +#include "bitset.h"
> +
> +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 },
> +};

This policy is probably a leftover from the previous version. Also, you
don't need to include bitset.h

[...]
> 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;

This was also dropped.

Excepts for the cleanups suggested above,

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>

Michal

[PATCH net-next v2 04/10] 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.

v2:
Grammar fixes
Change length from u16 to u32
s/DEV/HEADER/g
Add status attributes
Rename pairs from numbers to letters.

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

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 72d53da2bea9..7a8a76f08bb9 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -968,6 +968,48 @@ Request contents:
   ``ETHTOOL_A_CABLE_TEST_HEADER``       nested  request header
   ====================================  ======  ==========================
 
+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,
+depending on the specific hardware. Hence the contents of the notify
+message are 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 the notification sent when the test is completed for
+a T2 cable, i.e. two pairs. One pair is 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_STATUS``             | u8     | completed           |
+ +---------------------------------------------+--------+---------------------+
+ | ``ETHTOOL_A_CABLE_TEST_NTF_NEST``           | nested | all the results     |
+ +-+-------------------------------------------+--------+---------------------+
+ | | ``ETHTOOL_A_CABLE_TEST_STATUS``           | u8     | completed           |
+ +-+-------------------------------------------+--------+---------------------+
+ | | ``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``     | u32    | length in cm        |
+ +-+-+-----------------------------------------+--------+---------------------+
 
 Request translation
 ===================
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 7e16a1cce20b..519c37e61fe8 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -75,6 +75,7 @@ enum {
 	ETHTOOL_MSG_EEE_GET_REPLY,
 	ETHTOOL_MSG_EEE_NTF,
 	ETHTOOL_MSG_TSINFO_GET_REPLY,
+	ETHTOOL_MSG_CABLE_TEST_NTF,
 
 	/* add new constants above here */
 	__ETHTOOL_MSG_KERNEL_CNT,
@@ -402,7 +403,6 @@ enum {
 	/* add new constants above here */
 	__ETHTOOL_A_TSINFO_CNT,
 	ETHTOOL_A_TSINFO_MAX = (__ETHTOOL_A_TSINFO_CNT - 1)
-
 };
 
 /* CABLE TEST */
@@ -416,6 +416,58 @@ 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_A,
+	ETHTOOL_A_CABLE_PAIR_B,
+	ETHTOOL_A_CABLE_PAIR_C,
+	ETHTOOL_A_CABLE_PAIR_D,
+};
+
+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,	/* u32 */
+
+	__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_STATUS_UNSPEC,
+	ETHTOOL_A_CABLE_TEST_NTF_STATUS_STARTED,
+	ETHTOOL_A_CABLE_TEST_NTF_STATUS_COMPLETED
+};
+
+enum {
+	ETHTOOL_A_CABLE_TEST_NTF_UNSPEC,
+	ETHTOOL_A_CABLE_TEST_NTF_HEADER,	/* nest - ETHTOOL_A_HEADER_* */
+	ETHTOOL_A_CABLE_TEST_NTF_STATUS,	/* u8 - _STARTED/_COMPLETE */
+	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.2

Re: [PATCH net-next v2 04/10] net: ethtool: Add attributes for cable test reports

[Florian Fainelli]

On 5/4/2020 5:18 PM, 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.
> 
> v2:
> Grammar fixes
> Change length from u16 to u32
> s/DEV/HEADER/g
> Add status attributes
> Rename pairs from numbers to letters.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

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

Re: [PATCH net-next v2 04/10] net: ethtool: Add attributes for cable test reports

[Michal Kubecek]

On Tue, May 05, 2020 at 02:18:15AM +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.
> 
> v2:
> Grammar fixes
> Change length from u16 to u32
> s/DEV/HEADER/g
> Add status attributes
> Rename pairs from numbers to letters.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  Documentation/networking/ethtool-netlink.rst | 42 +++++++++++++++
>  include/uapi/linux/ethtool_netlink.h         | 54 +++++++++++++++++++-
>  2 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
> index 72d53da2bea9..7a8a76f08bb9 100644
> --- a/Documentation/networking/ethtool-netlink.rst
> +++ b/Documentation/networking/ethtool-netlink.rst
> @@ -968,6 +968,48 @@ Request contents:
>    ``ETHTOOL_A_CABLE_TEST_HEADER``       nested  request header
>    ====================================  ======  ==========================
>  
> +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,
> +depending on the specific hardware. Hence the contents of the notify
> +message are 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 the notification sent when the test is completed for
> +a T2 cable, i.e. two pairs. One pair is 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_STATUS``             | u8     | completed           |
> + +---------------------------------------------+--------+---------------------+
> + | ``ETHTOOL_A_CABLE_TEST_NTF_NEST``           | nested | all the results     |
> + +-+-------------------------------------------+--------+---------------------+
> + | | ``ETHTOOL_A_CABLE_TEST_STATUS``           | u8     | completed           |
> + +-+-------------------------------------------+--------+---------------------+

You have ETHTOOL_A_CABLE_TEST_STATUS both here and on top level. AFAICS
the top level attribute is the right one - but the name is
ETHTOOL_A_CABLE_TEST_NTF_STATUS.

> + | | ``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``     | u32    | length in cm        |
> + +-+-+-----------------------------------------+--------+---------------------+
>  
>  Request translation
>  ===================
> diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
> index 7e16a1cce20b..519c37e61fe8 100644
> --- a/include/uapi/linux/ethtool_netlink.h
> +++ b/include/uapi/linux/ethtool_netlink.h
> @@ -75,6 +75,7 @@ enum {
>  	ETHTOOL_MSG_EEE_GET_REPLY,
>  	ETHTOOL_MSG_EEE_NTF,
>  	ETHTOOL_MSG_TSINFO_GET_REPLY,
> +	ETHTOOL_MSG_CABLE_TEST_NTF,

Please add this constant to the table in ethtool-netlink.rst.

>  
>  	/* add new constants above here */
>  	__ETHTOOL_MSG_KERNEL_CNT,
> @@ -402,7 +403,6 @@ enum {
>  	/* add new constants above here */
>  	__ETHTOOL_A_TSINFO_CNT,
>  	ETHTOOL_A_TSINFO_MAX = (__ETHTOOL_A_TSINFO_CNT - 1)
> -
>  };
>  
>  /* CABLE TEST */
> @@ -416,6 +416,58 @@ 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_A,
> +	ETHTOOL_A_CABLE_PAIR_B,
> +	ETHTOOL_A_CABLE_PAIR_C,
> +	ETHTOOL_A_CABLE_PAIR_D,
> +};
> +
> +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,	/* u32 */
> +
> +	__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_STATUS_UNSPEC,
> +	ETHTOOL_A_CABLE_TEST_NTF_STATUS_STARTED,
> +	ETHTOOL_A_CABLE_TEST_NTF_STATUS_COMPLETED
> +};
> +
> +enum {
> +	ETHTOOL_A_CABLE_TEST_NTF_UNSPEC,
> +	ETHTOOL_A_CABLE_TEST_NTF_HEADER,	/* nest - ETHTOOL_A_HEADER_* */
> +	ETHTOOL_A_CABLE_TEST_NTF_STATUS,	/* u8 - _STARTED/_COMPLETE */
> +	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_ */

I would prefer to have a separate enum for last two attributes as they
belong to a different context than the rest.

Michal

> +
> +	__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.2
> 

Re: [PATCH net-next v2 04/10] net: ethtool: Add attributes for cable test reports

[Andrew Lunn]

> > + +---------------------------------------------+--------+---------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_HEADER``             | nested | reply header        |
> > + +---------------------------------------------+--------+---------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_STATUS``             | u8     | completed           |
> > + +---------------------------------------------+--------+---------------------+
> > + | ``ETHTOOL_A_CABLE_TEST_NTF_NEST``           | nested | all the results     |
> > + +-+-------------------------------------------+--------+---------------------+
> > + | | ``ETHTOOL_A_CABLE_TEST_STATUS``           | u8     | completed           |
> > + +-+-------------------------------------------+--------+---------------------+
> 
> You have ETHTOOL_A_CABLE_TEST_STATUS both here and on top level. AFAICS
> the top level attribute is the right one - but the name is
> ETHTOOL_A_CABLE_TEST_NTF_STATUS.

Hi Michal

They need better names. The first one is about the test run
status. Started vs complete. A notification is sent when the test is
started, and a second one at the end with the actual test results. The
second status is per pair, indicating open, shorted, O.K.
Maybe this second one shouldd be ETHTOOL_A_CABLE_TEST_NTF_PAIR_STATUS.

There is also a cut/paste error, the second one should not have the
comment completed.

	Andrew

Re: [PATCH net-next v2 04/10] net: ethtool: Add attributes for cable test reports

[Michal Kubecek]

On Tue, May 05, 2020 at 03:15:48PM +0200, Andrew Lunn wrote:
> > > + +---------------------------------------------+--------+---------------------+
> > > + | ``ETHTOOL_A_CABLE_TEST_HEADER``             | nested | reply header        |
> > > + +---------------------------------------------+--------+---------------------+
> > > + | ``ETHTOOL_A_CABLE_TEST_STATUS``             | u8     | completed           |
> > > + +---------------------------------------------+--------+---------------------+
> > > + | ``ETHTOOL_A_CABLE_TEST_NTF_NEST``           | nested | all the results     |
> > > + +-+-------------------------------------------+--------+---------------------+
> > > + | | ``ETHTOOL_A_CABLE_TEST_STATUS``           | u8     | completed           |
> > > + +-+-------------------------------------------+--------+---------------------+
> > 
> > You have ETHTOOL_A_CABLE_TEST_STATUS both here and on top level. AFAICS
> > the top level attribute is the right one - but the name is
> > ETHTOOL_A_CABLE_TEST_NTF_STATUS.
> 
> Hi Michal
> 
> They need better names. The first one is about the test run
> status. Started vs complete. A notification is sent when the test is
> started, and a second one at the end with the actual test results. The
> second status is per pair, indicating open, shorted, O.K.
> Maybe this second one shouldd be ETHTOOL_A_CABLE_TEST_NTF_PAIR_STATUS.

The per-pair status is ETHTOOL_A_CABLE_RESULTS_CODE (nested within
ETHTOOL_A_CABLE_TEST_NTF_RESULT), isn't it?

Based on the code, I would say second ETHTOOL_A_CABLE_TEST_STATUS line
should be dropped and first fixed to ETHTOOL_A_CABLE_TEST_NTF_STATUS.

Michal

[PATCH net-next v2 05/10] 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 b9c9ddf408fe..87bc02da74bc 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.2

Re: [PATCH net-next v2 05/10] net: ethtool: Make helpers public

[Florian Fainelli]

On 5/4/2020 5:18 PM, Andrew Lunn wrote:
> 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>

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

Re: [PATCH net-next v2 05/10] net: ethtool: Make helpers public

[Michal Kubecek]

On Tue, May 05, 2020 at 02:18:16AM +0200, Andrew Lunn wrote:
> 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>
> ---

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>

[PATCH net-next v2 06/10] 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.

v2:
Include the status complete in the netlink notification message

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

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index cea89785bcd4..039e41e15c7e 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
 
@@ -476,6 +480,8 @@ static void phy_abort_cable_test(struct phy_device *phydev)
 {
 	int err;
 
+	ethnl_cable_test_finished(phydev);
+
 	err = phy_init_hw(phydev);
 	if (err)
 		phydev_err(phydev, "Error while aborting cable test");
@@ -484,7 +490,7 @@ static void phy_abort_cable_test(struct phy_device *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 &&
@@ -510,19 +516,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);
 
@@ -962,6 +979,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 6e5782a7da80..4c888db33ef0 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"
@@ -59,3 +60,57 @@ 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_HEADER);
+	if (err)
+		goto out;
+
+	err = nla_put_u8(phydev->skb, ETHTOOL_A_CABLE_TEST_NTF_STATUS,
+			 ETHTOOL_A_CABLE_TEST_NTF_STATUS_COMPLETED);
+	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.2

Re: [PATCH net-next v2 06/10] net: ethtool: Add infrastructure for reporting cable test results

[Florian Fainelli]

On 5/4/2020 5:18 PM, Andrew Lunn wrote:
> 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.
> 
> v2:
> Include the status complete in the netlink notification message
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

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

Re: [PATCH net-next v2 06/10] net: ethtool: Add infrastructure for reporting cable test results

[Michal Kubecek]

On Tue, May 05, 2020 at 02:18:17AM +0200, Andrew Lunn wrote:
> 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.
> 
> v2:
> Include the status complete in the netlink notification message
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/phy/phy.c           | 22 +++++++++++--
>  include/linux/ethtool_netlink.h | 20 ++++++++++++
>  include/linux/phy.h             |  5 +++
>  net/ethtool/cabletest.c         | 55 +++++++++++++++++++++++++++++++++
>  4 files changed, 100 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index cea89785bcd4..039e41e15c7e 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
>  
> @@ -476,6 +480,8 @@ static void phy_abort_cable_test(struct phy_device *phydev)
>  {
>  	int err;
>  
> +	ethnl_cable_test_finished(phydev);
> +
>  	err = phy_init_hw(phydev);
>  	if (err)
>  		phydev_err(phydev, "Error while aborting cable test");
> @@ -484,7 +490,7 @@ static void phy_abort_cable_test(struct phy_device *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 &&
> @@ -510,19 +516,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);
>  
> @@ -962,6 +979,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 6e5782a7da80..4c888db33ef0 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"
> @@ -59,3 +60,57 @@ 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;

This should be -EMSGSIZE.

> +		goto out;
> +	}
> +
> +	err = ethnl_fill_reply_header(phydev->skb, phydev->attached_dev,
> +				      ETHTOOL_A_CABLE_TEST_NTF_HEADER);
> +	if (err)
> +		goto out;
> +
> +	err = nla_put_u8(phydev->skb, ETHTOOL_A_CABLE_TEST_NTF_STATUS,
> +			 ETHTOOL_A_CABLE_TEST_NTF_STATUS_COMPLETED);
> +	if (err)
> +		goto out;
> +
> +	phydev->nest = nla_nest_start(phydev->skb,
> +				      ETHTOOL_A_CABLE_TEST_NTF_NEST);
> +	if (!phydev->nest)
> +		goto out;

If nla_nest_start() fails, we still have 0 in err.

> +
> +	return 0;
> +
> +out:
> +	nlmsg_free(phydev->skb);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(ethnl_cable_test_alloc);

Do you think it would make sense to set phydev->skb to NULL on failure
and also in ethnl_cable_test_free() and ethnl_cable_test_finished() so
that if driver messes up, it hits null pointer dereference which is much
easier to debug than use after free?

Michal

Re: [PATCH net-next v2 06/10] net: ethtool: Add infrastructure for reporting cable test results

[Andrew Lunn]

> > +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;
> 
> This should be -EMSGSIZE.
> 
> > +		goto out;
> > +	}
> > +
> > +	err = ethnl_fill_reply_header(phydev->skb, phydev->attached_dev,
> > +				      ETHTOOL_A_CABLE_TEST_NTF_HEADER);
> > +	if (err)
> > +		goto out;
> > +
> > +	err = nla_put_u8(phydev->skb, ETHTOOL_A_CABLE_TEST_NTF_STATUS,
> > +			 ETHTOOL_A_CABLE_TEST_NTF_STATUS_COMPLETED);
> > +	if (err)
> > +		goto out;
> > +
> > +	phydev->nest = nla_nest_start(phydev->skb,
> > +				      ETHTOOL_A_CABLE_TEST_NTF_NEST);
> > +	if (!phydev->nest)
> > +		goto out;
> 
> If nla_nest_start() fails, we still have 0 in err.
> 
> > +
> > +	return 0;
> > +
> > +out:
> > +	nlmsg_free(phydev->skb);
> > +	return err;
> > +}
> > +EXPORT_SYMBOL_GPL(ethnl_cable_test_alloc);
> 
> Do you think it would make sense to set phydev->skb to NULL on failure
> and also in ethnl_cable_test_free() and ethnl_cable_test_finished() so
> that if driver messes up, it hits null pointer dereference which is much
> easier to debug than use after free?

Hi Michal

The use after free is not that hard to debug, i had to do it myself :-)

But yes, i can poison phydev->skb.

    Andrew

[PATCH net-next v2 07/10] 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..0d12abbdf3c3 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, u32 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 4c888db33ef0..f500454a54eb 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -114,3 +114,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, u32 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.2

Re: [PATCH net-next v2 07/10] net: ethtool: Add helpers for reporting test results

[Florian Fainelli]

On 5/4/2020 5:18 PM, Andrew Lunn wrote:
> 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>

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

Re: [PATCH net-next v2 07/10] net: ethtool: Add helpers for reporting test results

[Michal Kubecek]

On Tue, May 05, 2020 at 02:18:18AM +0200, Andrew Lunn wrote:
> 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>
> ---
[...]
> diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
> index 4c888db33ef0..f500454a54eb 100644
> --- a/net/ethtool/cabletest.c
> +++ b/net/ethtool/cabletest.c
> @@ -114,3 +114,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)

Is there a reason to use u16 for result when the attribute is NLA_U8?

> +{
> +	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, u32 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;

This should be nla_put_u32().

Michal

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

Re: [PATCH net-next v2 07/10] net: ethtool: Add helpers for reporting test results

[Andrew Lunn]

> > +int ethnl_cable_test_fault_length(struct phy_device *phydev, u8 pair, u32 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;
> 
> This should be nla_put_u32().

Yes. I think i messed up a rebase merge conflict somewhere. I'm also
surprised user space is not complaining.

	  Andrew

Re: [PATCH net-next v2 07/10] net: ethtool: Add helpers for reporting test results

[Michal Kubecek]

On Tue, May 05, 2020 at 03:22:03PM +0200, Andrew Lunn wrote:
> > > +int ethnl_cable_test_fault_length(struct phy_device *phydev, u8 pair, u32 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;
> > 
> > This should be nla_put_u32().
> 
> Yes. I think i messed up a rebase merge conflict somewhere. I'm also
> surprised user space is not complaining.

There is no difference on little endian architectures as nla_put_*()
helpers all call __nla_reserve() which fills the padding with zero
bytes. IIRC there was a case where wrong attribute type had been used
for quite long without anyone noticing.

Michal

[PATCH net-next v2 08/10] 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..72afc67f7f35 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_A,
+				mavell_vct7_cable_test_report_trans(pair0));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_B,
+				mavell_vct7_cable_test_report_trans(pair1));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_C,
+				mavell_vct7_cable_test_report_trans(pair2));
+	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
+				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.2

Re: [PATCH net-next v2 08/10] net: phy: marvell: Add cable test support

[Florian Fainelli]

On 5/4/2020 5:18 PM, Andrew Lunn wrote:
> 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>

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

Re: [PATCH net-next v2 08/10] net: phy: marvell: Add cable test support

[Michael Walle]

Hi Andrew,

Am 2020-05-05 02:18, schrieb 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..72afc67f7f35 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);

Is it mandatory to wait 1.5s unconditionally or can we poll for link 
down?

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

I've never understood the use of the meter unit. If we always use
centimeters, we have 2^16 cm = 655m, shouldn't that be enough given
that the max cable length is 100m in TP ethernet? Also you hardcode
the unit to centimeters, so this should be superfluous, making this
function a noop.

> +
> +	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:

btw on the BCM54140 I've observed, that if you have a intra-pair
short, the length is wrong; looks like it is twice the value it
should be.

Does the Marvell PHY report the correct value?

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

I'm sure you know FIELD_GET(), so there must be another
reason why you use mask and shift, consistency?

-michael

> +
> +	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
> +				mavell_vct7_cable_test_report_trans(pair0));
> +	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_B,
> +				mavell_vct7_cable_test_report_trans(pair1));
> +	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_C,
> +				mavell_vct7_cable_test_report_trans(pair2));
> +	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_D,
> +				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,
>  	},
>  };

Re: [PATCH net-next v2 08/10] net: phy: marvell: Add cable test support

[Andrew Lunn]

> > +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);
> 
> Is it mandatory to wait 1.5s unconditionally or can we poll for link down?

Polling is fine. I think i got this from the Marvell SDK.

> > +
> > +	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;
> 
> I've never understood the use of the meter unit. If we always use
> centimeters, we have 2^16 cm = 655m, shouldn't that be enough given
> that the max cable length is 100m in TP ethernet? Also you hardcode
> the unit to centimeters, so this should be superfluous, making this
> function a noop.

Yes, it should never be used now. But i did use it initially, to see
if the results were different/better. It is a rather odd design, and
i'm wondering if some of the older PHYs only have meters? I guess i
should go look at the SDK.

> > +	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:
> 
> btw on the BCM54140 I've observed, that if you have a intra-pair
> short, the length is wrong; looks like it is twice the value it
> should be.
> 
> Does the Marvell PHY report the correct value?

I've not tested that.

Chris, do you have results for this test?

> > +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;
> 
> I'm sure you know FIELD_GET(), so there must be another
> reason why you use mask and shift, consistency?

Consistency, and FIELD_GET() just looks odd, only taking a mask, not a
shift.

     Andrew

Re: [PATCH net-next v2 08/10] net: phy: marvell: Add cable test support

[Michal Kubecek]

On Tue, May 05, 2020 at 02:18:19AM +0200, Andrew Lunn wrote:
> 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..72afc67f7f35 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

This shouldn't be necessary, <linux/ethtool_netlink.h> includes the UAPI
header.

[...]
> +static int mavell_vct7_cable_test_report_trans(int result)

Typo: "mavell".

Michal

[PATCH net-next v2 09/10] 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 039e41e15c7e..2117e3347d59 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -490,6 +490,7 @@ static void phy_abort_cable_test(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 &&
@@ -523,8 +524,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;
 	}
@@ -877,6 +880,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));
@@ -885,8 +890,10 @@ void phy_stop(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
-	if (phydev->state == PHY_CABLETEST)
+	if (phydev->state == PHY_CABLETEST) {
 		phy_abort_cable_test(phydev);
+		netif_testing_off(dev);
+	}
 
 	if (phydev->sfp_bus)
 		sfp_upstream_stop(phydev->sfp_bus);
@@ -948,6 +955,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;
@@ -973,6 +981,7 @@ void phy_state_machine(struct work_struct *work)
 		err = phydev->drv->cable_test_get_status(phydev, &finished);
 		if (err) {
 			phy_abort_cable_test(phydev);
+			netif_testing_off(dev);
 			needs_aneg = true;
 			phydev->state = PHY_UP;
 			break;
@@ -980,6 +989,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.2

Re: [PATCH net-next v2 09/10] net: phy: Put interface into oper testing during cable test

[Florian Fainelli]

On 5/4/2020 5:18 PM, Andrew Lunn wrote:
> 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>

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

Re: [PATCH net-next v2 09/10] net: phy: Put interface into oper testing during cable test

[Michal Kubecek]

On Tue, May 05, 2020 at 02:18:20AM +0200, Andrew Lunn wrote:
> 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>
> ---

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>

Things could be simplified a bit by putting netif_testing_off() into
ethnl_cable_test_finished() but it's probably not worth the confusion
from setting the state in one place and clearing it in a completely
different one.

Michal

[PATCH net-next v2 10/10] net: phy: Send notifier when starting the cable test

[Andrew Lunn]

Given that it takes time to run a cable test, send a notify message at
the start, as well as when it is completed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/ethtool/cabletest.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index f500454a54eb..e59f570494c0 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -20,6 +20,41 @@ cable_test_act_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
 	[ETHTOOL_A_CABLE_TEST_HEADER]		= { .type = NLA_NESTED },
 };
 
+static int ethnl_cable_test_started(struct phy_device *phydev)
+{
+	struct sk_buff *skb;
+	int err = -ENOMEM;
+	void *ehdr;
+
+	skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
+	if (!skb)
+		goto out;
+
+	ehdr = ethnl_bcastmsg_put(skb, ETHTOOL_MSG_CABLE_TEST_NTF);
+	if (!ehdr) {
+		err = -EINVAL;
+		goto out;
+	}
+
+	err = ethnl_fill_reply_header(skb, phydev->attached_dev,
+				      ETHTOOL_A_CABLE_TEST_NTF_HEADER);
+	if (err)
+		goto out;
+
+	err = nla_put_u8(skb, ETHTOOL_A_CABLE_TEST_NTF_STATUS,
+			 ETHTOOL_A_CABLE_TEST_NTF_STATUS_STARTED);
+	if (err)
+		goto out;
+
+	genlmsg_end(skb, ehdr);
+
+	return ethnl_multicast(skb, phydev->attached_dev);
+
+out:
+	nlmsg_free(skb);
+	return err;
+}
+
 int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *tb[ETHTOOL_A_CABLE_TEST_MAX + 1];
@@ -54,6 +89,10 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 	ret = phy_start_cable_test(dev->phydev, info->extack);
 
 	ethnl_ops_complete(dev);
+
+	if (!ret)
+		ethnl_cable_test_started(dev->phydev);
+
 out_rtnl:
 	rtnl_unlock();
 out_dev_put:
-- 
2.26.2

Re: [PATCH net-next v2 10/10] net: phy: Send notifier when starting the cable test

[Florian Fainelli]

On 5/4/2020 5:18 PM, Andrew Lunn wrote:
> Given that it takes time to run a cable test, send a notify message at
> the start, as well as when it is completed.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

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

Re: [PATCH net-next v2 10/10] net: phy: Send notifier when starting the cable test

[Michal Kubecek]

On Tue, May 05, 2020 at 02:18:21AM +0200, Andrew Lunn wrote:
> Given that it takes time to run a cable test, send a notify message at
> the start, as well as when it is completed.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  net/ethtool/cabletest.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
> index f500454a54eb..e59f570494c0 100644
> --- a/net/ethtool/cabletest.c
> +++ b/net/ethtool/cabletest.c
> @@ -20,6 +20,41 @@ cable_test_act_policy[ETHTOOL_A_CABLE_TEST_MAX + 1] = {
>  	[ETHTOOL_A_CABLE_TEST_HEADER]		= { .type = NLA_NESTED },
>  };
>  
> +static int ethnl_cable_test_started(struct phy_device *phydev)
> +{
> +	struct sk_buff *skb;
> +	int err = -ENOMEM;
> +	void *ehdr;
> +
> +	skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> +	if (!skb)
> +		goto out;
> +
> +	ehdr = ethnl_bcastmsg_put(skb, ETHTOOL_MSG_CABLE_TEST_NTF);
> +	if (!ehdr) {
> +		err = -EINVAL;

This should rather be -EMSGSIZE. But as we are not going to use the
return value anyway, it's only cosmetic problem.

Michal

Reviewed-by: Michal Kubecek <mkubecek@suse.cz>

> +		goto out;
> +	}
> +
> +	err = ethnl_fill_reply_header(skb, phydev->attached_dev,
> +				      ETHTOOL_A_CABLE_TEST_NTF_HEADER);
> +	if (err)
> +		goto out;
> +
> +	err = nla_put_u8(skb, ETHTOOL_A_CABLE_TEST_NTF_STATUS,
> +			 ETHTOOL_A_CABLE_TEST_NTF_STATUS_STARTED);
> +	if (err)
> +		goto out;
> +
> +	genlmsg_end(skb, ehdr);
> +
> +	return ethnl_multicast(skb, phydev->attached_dev);
> +
> +out:
> +	nlmsg_free(skb);
> +	return err;
> +}
> +
>  int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
>  {
>  	struct nlattr *tb[ETHTOOL_A_CABLE_TEST_MAX + 1];
> @@ -54,6 +89,10 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
>  	ret = phy_start_cable_test(dev->phydev, info->extack);
>  
>  	ethnl_ops_complete(dev);
> +
> +	if (!ret)
> +		ethnl_cable_test_started(dev->phydev);
> +
>  out_rtnl:
>  	rtnl_unlock();
>  out_dev_put:
> -- 
> 2.26.2
> 

Re: [PATCH net-next v2 10/10] net: phy: Send notifier when starting the cable test

[Andrew Lunn]

> > +static int ethnl_cable_test_started(struct phy_device *phydev)
> > +{
> > +	struct sk_buff *skb;
> > +	int err = -ENOMEM;
> > +	void *ehdr;
> > +
> > +	skb = genlmsg_new(NLMSG_GOODSIZE, GFP_KERNEL);
> > +	if (!skb)
> > +		goto out;
> > +
> > +	ehdr = ethnl_bcastmsg_put(skb, ETHTOOL_MSG_CABLE_TEST_NTF);
> > +	if (!ehdr) {
> > +		err = -EINVAL;
> 
> This should rather be -EMSGSIZE. But as we are not going to use the
> return value anyway, it's only cosmetic problem.

Yes, cut/paste from the alloc function. I might put a phydev_err() in
out: so we get to know about errors.

     Andrew