[PATCH net-next 0/4] cxgb4: add ethtool self_test support

[PATCH net-next 0/4] cxgb4: add ethtool self_test support

[Vishal Kulkarni]

This series of patches add support for below tests.
1. Adapter status test
2. Link test
3. Link speed test
4. Loopback test

Vishal Kulkarni (4):
  cxgb4: Add ethtool self-test support
  cxgb4: Add link test to ethtool self test.
  cxgb4: Add adapter status check to ethtool
  cxgb4: Add speed link test to ethtool self_test

 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h    |  10 ++
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    | 137 ++++++++++++++++++
 drivers/net/ethernet/chelsio/cxgb4/sge.c      | 117 ++++++++++++++-
 3 files changed, 261 insertions(+), 3 deletions(-)

-- 
2.18.2

[PATCH net-next 1/4] cxgb4: Add ethtool self-test support

[Vishal Kulkarni]

This test checks whether adapter has crashed or not

Signed-off-by: Vishal Kulkarni <vishal@chelsio.com>
---
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index b66a2e6cbbeb..1ec6157f8ba7 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -25,6 +25,15 @@ static void set_msglevel(struct net_device *dev, u32 val)
 	netdev2adap(dev)->msg_enable = val;
 }
 
+enum cxgb4_ethtool_tests {
+	CXGB4_ETHTOOL_ADAPTER_TEST,
+	CXGB4_ETHTOOL_MAX_TEST,
+};
+
+static const char cxgb4_selftest_strings[CXGB4_ETHTOOL_MAX_TEST][ETH_GSTRING_LEN] = {
+	"Adapter health status",
+};
+
 static const char * const flash_region_strings[] = {
 	"All",
 	"Firmware",
@@ -166,6 +175,8 @@ static int get_sset_count(struct net_device *dev, int sset)
 		       ARRAY_SIZE(loopback_stats_strings);
 	case ETH_SS_PRIV_FLAGS:
 		return ARRAY_SIZE(cxgb4_priv_flags_strings);
+	case ETH_SS_TEST:
+		return ARRAY_SIZE(cxgb4_selftest_strings);
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -228,6 +239,9 @@ static void get_strings(struct net_device *dev, u32 stringset, u8 *data)
 	} else if (stringset == ETH_SS_PRIV_FLAGS) {
 		memcpy(data, cxgb4_priv_flags_strings,
 		       sizeof(cxgb4_priv_flags_strings));
+	} else if (stringset == ETH_SS_TEST) {
+		memcpy(data, cxgb4_selftest_strings,
+		       sizeof(cxgb4_selftest_strings));
 	}
 }
 
@@ -2056,6 +2070,31 @@ static int cxgb4_set_priv_flags(struct net_device *netdev, u32 flags)
 	return 0;
 }
 
+static void cxgb4_adapter_status_test(struct net_device *netdev, u64 *data)
+{
+	struct port_info *pi = netdev_priv(netdev);
+	struct adapter *adap = pi->adapter;
+	u32 pcie_fw;
+
+	pcie_fw = t4_read_reg(adap, PCIE_FW_A);
+	if (pcie_fw & PCIE_FW_ERR_F) {
+		*data = 1;
+		return;
+	}
+
+	*data = 0;
+}
+
+static void cxgb4_self_test(struct net_device *netdev,
+			    struct ethtool_test *eth_test, u64 *data)
+{
+	memset(data, 0, sizeof(u64) * CXGB4_ETHTOOL_MAX_TEST);
+
+	cxgb4_adapter_status_test(netdev, &data[CXGB4_ETHTOOL_ADAPTER_TEST]);
+	if (data[CXGB4_ETHTOOL_ADAPTER_TEST])
+		eth_test->flags |= ETH_TEST_FL_FAILED;
+}
+
 static const struct ethtool_ops cxgb_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_RX_MAX_FRAMES |
@@ -2090,6 +2129,7 @@ static const struct ethtool_ops cxgb_ethtool_ops = {
 	.get_rxfh_indir_size = get_rss_table_size,
 	.get_rxfh	   = get_rss_table,
 	.set_rxfh	   = set_rss_table,
+	.self_test	   = cxgb4_self_test,
 	.flash_device      = set_flash,
 	.get_ts_info       = get_ts_info,
 	.set_dump          = set_dump,
-- 
2.21.1

[PATCH net-next 2/4] cxgb4: Add link test to ethtool self test

[Vishal Kulkarni]

This test checks whether link is up or not

Signed-off-by: Vishal Kulkarni <vishal@chelsio.com>
---
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    | 35 ++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index 1ec6157f8ba7..f374757e15c8 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -27,11 +27,13 @@ static void set_msglevel(struct net_device *dev, u32 val)
 
 enum cxgb4_ethtool_tests {
 	CXGB4_ETHTOOL_ADAPTER_TEST,
+	CXGB4_ETHTOOL_LINK_TEST,
 	CXGB4_ETHTOOL_MAX_TEST,
 };
 
 static const char cxgb4_selftest_strings[CXGB4_ETHTOOL_MAX_TEST][ETH_GSTRING_LEN] = {
 	"Adapter health status",
+	"Link test",
 };
 
 static const char * const flash_region_strings[] = {
@@ -2085,14 +2087,45 @@ static void cxgb4_adapter_status_test(struct net_device *netdev, u64 *data)
 	*data = 0;
 }
 
+static void cxgb4_link_test(struct net_device *netdev, u64 *data)
+{
+	struct port_info *pi = netdev_priv(netdev);
+	unsigned int link = 0;
+	int ret;
+
+	ret = t4_get_link_params(pi, &link, NULL, NULL);
+	if (ret) {
+		*data = ret;
+		return;
+	}
+
+	*data = !link;
+}
+
 static void cxgb4_self_test(struct net_device *netdev,
 			    struct ethtool_test *eth_test, u64 *data)
 {
+	int i;
+
 	memset(data, 0, sizeof(u64) * CXGB4_ETHTOOL_MAX_TEST);
 
 	cxgb4_adapter_status_test(netdev, &data[CXGB4_ETHTOOL_ADAPTER_TEST]);
-	if (data[CXGB4_ETHTOOL_ADAPTER_TEST])
+	if (data[CXGB4_ETHTOOL_ADAPTER_TEST]) {
+		for (i = CXGB4_ETHTOOL_ADAPTER_TEST + 1;
+		       i < CXGB4_ETHTOOL_MAX_TEST; i++)
+			data[i] = 1;
+
 		eth_test->flags |= ETH_TEST_FL_FAILED;
+		return;
+	}
+
+	cxgb4_link_test(netdev, &data[CXGB4_ETHTOOL_LINK_TEST]);
+	for (i = CXGB4_ETHTOOL_ADAPTER_TEST; i < CXGB4_ETHTOOL_MAX_TEST; i++) {
+		if (data[i]) {
+			eth_test->flags |= ETH_TEST_FL_FAILED;
+			break;
+		}
+	}
 }
 
 static const struct ethtool_ops cxgb_ethtool_ops = {
-- 
2.21.1

[PATCH net-next 3/4] cxgb4: Add speed link test to ethtool

[Vishal Kulkarni]

This test checks whether the current speed is supported or not

Signed-off-by: Vishal Kulkarni <vishal@chelsio.com>
---
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index f374757e15c8..5d3eb44dee46 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -28,12 +28,14 @@ static void set_msglevel(struct net_device *dev, u32 val)
 enum cxgb4_ethtool_tests {
 	CXGB4_ETHTOOL_ADAPTER_TEST,
 	CXGB4_ETHTOOL_LINK_TEST,
+	CXGB4_ETHTOOL_LINK_SPEED_TEST,
 	CXGB4_ETHTOOL_MAX_TEST,
 };
 
 static const char cxgb4_selftest_strings[CXGB4_ETHTOOL_MAX_TEST][ETH_GSTRING_LEN] = {
 	"Adapter health status",
 	"Link test",
+	"Link speed test",
 };
 
 static const char * const flash_region_strings[] = {
@@ -2102,6 +2104,26 @@ static void cxgb4_link_test(struct net_device *netdev, u64 *data)
 	*data = !link;
 }
 
+static void cxgb4_link_speed_test(struct net_device *netdev, u64 *data)
+{
+	struct port_info *pi = netdev_priv(netdev);
+	unsigned int speed;
+	int ret;
+
+	ret = t4_get_link_params(pi, NULL, &speed, NULL);
+	if (ret) {
+		*data = ret;
+		return;
+	}
+
+	if (!speed_to_fw_caps(speed)) {
+		*data = 1;
+		return;
+	}
+
+	*data = 0;
+}
+
 static void cxgb4_self_test(struct net_device *netdev,
 			    struct ethtool_test *eth_test, u64 *data)
 {
@@ -2120,6 +2142,8 @@ static void cxgb4_self_test(struct net_device *netdev,
 	}
 
 	cxgb4_link_test(netdev, &data[CXGB4_ETHTOOL_LINK_TEST]);
+	cxgb4_link_speed_test(netdev, &data[CXGB4_ETHTOOL_LINK_SPEED_TEST]);
+
 	for (i = CXGB4_ETHTOOL_ADAPTER_TEST; i < CXGB4_ETHTOOL_MAX_TEST; i++) {
 		if (data[i]) {
 			eth_test->flags |= ETH_TEST_FL_FAILED;
-- 
2.21.1

[PATCH net-next 4/4] cxgb4: add loop back test to ethtool

[Vishal Kulkarni]

In this test, loopback pkt is created and sent on default queue.

Signed-off-by: Vishal Kulkarni <vishal@chelsio.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h    |   8 ++
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    |  22 ++++
 drivers/net/ethernet/chelsio/cxgb4/sge.c      | 107 +++++++++++++++++-
 3 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index c59e9ccc2f18..adbc0d088070 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -532,6 +532,12 @@ static inline struct mbox_cmd *mbox_cmd_log_entry(struct mbox_cmd_log *log,
 		FW_HDR_FW_VER_BUILD_G(chip##FW_VERSION_BUILD))
 #define FW_INTFVER(chip, intf) (FW_HDR_INTFVER_##intf)
 
+struct cxgb4_ethtool_lb_test {
+	struct completion completion;
+	int result;
+	int loopback;
+};
+
 struct fw_info {
 	u8 chip;
 	char *fs_name;
@@ -685,6 +691,7 @@ struct port_info {
 	u16 nmirrorqsets;
 	u32 vi_mirror_count;
 	struct mutex vi_mirror_mutex; /* Sync access to Mirror VI info */
+	struct cxgb4_ethtool_lb_test ethtool_lb;
 };
 
 struct dentry;
@@ -1595,6 +1602,7 @@ void t4_free_sge_resources(struct adapter *adap);
 void t4_free_ofld_rxqs(struct adapter *adap, int n, struct sge_ofld_rxq *q);
 irq_handler_t t4_intr_handler(struct adapter *adap);
 netdev_tx_t t4_start_xmit(struct sk_buff *skb, struct net_device *dev);
+int cxgb4_selftest_lb_pkt(struct net_device *netdev);
 int t4_ethrx_handler(struct sge_rspq *q, const __be64 *rsp,
 		     const struct pkt_gl *gl);
 int t4_mgmt_tx(struct adapter *adap, struct sk_buff *skb);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index 5d3eb44dee46..9773a4ab435b 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -29,6 +29,7 @@ enum cxgb4_ethtool_tests {
 	CXGB4_ETHTOOL_ADAPTER_TEST,
 	CXGB4_ETHTOOL_LINK_TEST,
 	CXGB4_ETHTOOL_LINK_SPEED_TEST,
+	CXGB4_ETHTOOL_LB_TEST,
 	CXGB4_ETHTOOL_MAX_TEST,
 };
 
@@ -36,6 +37,7 @@ static const char cxgb4_selftest_strings[CXGB4_ETHTOOL_MAX_TEST][ETH_GSTRING_LEN
 	"Adapter health status",
 	"Link test",
 	"Link speed test",
+	"Loop back test",
 };
 
 static const char * const flash_region_strings[] = {
@@ -2124,6 +2126,23 @@ static void cxgb4_link_speed_test(struct net_device *netdev, u64 *data)
 	*data = 0;
 }
 
+static void cxgb4_lb_test(struct net_device *netdev, u64 *lb_status)
+{
+	int dev_state = netif_running(netdev);
+
+	if (dev_state) {
+		netif_tx_stop_all_queues(netdev);
+		netif_carrier_off(netdev);
+	}
+
+	*lb_status = cxgb4_selftest_lb_pkt(netdev);
+
+	if (dev_state) {
+		netif_tx_start_all_queues(netdev);
+		netif_carrier_on(netdev);
+	}
+}
+
 static void cxgb4_self_test(struct net_device *netdev,
 			    struct ethtool_test *eth_test, u64 *data)
 {
@@ -2144,6 +2163,9 @@ static void cxgb4_self_test(struct net_device *netdev,
 	cxgb4_link_test(netdev, &data[CXGB4_ETHTOOL_LINK_TEST]);
 	cxgb4_link_speed_test(netdev, &data[CXGB4_ETHTOOL_LINK_SPEED_TEST]);
 
+	if (eth_test->flags == ETH_TEST_FL_OFFLINE)
+		cxgb4_lb_test(netdev, &data[CXGB4_ETHTOOL_LB_TEST]);
+
 	for (i = CXGB4_ETHTOOL_ADAPTER_TEST; i < CXGB4_ETHTOOL_MAX_TEST; i++) {
 		if (data[i]) {
 			eth_test->flags |= ETH_TEST_FL_FAILED;
diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index f9c6f13d7c99..3f0fdffdb2b5 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -2537,6 +2537,80 @@ static void ctrlq_check_stop(struct sge_ctrl_txq *q, struct fw_wr_hdr *wr)
 	}
 }
 
+#define CXGB4_SELFTEST_LB_STR "CHELSIO_SELFTEST"
+
+int cxgb4_selftest_lb_pkt(struct net_device *netdev)
+{
+	struct port_info *pi = netdev_priv(netdev);
+	struct adapter *adap = pi->adapter;
+	struct cxgb4_ethtool_lb_test *lb;
+	int ret, i = 0, pkt_len, credits;
+	struct fw_eth_tx_pkt_wr *wr;
+	struct cpl_tx_pkt_core *cpl;
+	u32 ctrl0, ndesc, flits;
+	struct sge_eth_txq *q;
+	u8 *sgl;
+
+	pkt_len = ETH_HLEN + sizeof(CXGB4_SELFTEST_LB_STR);
+
+	flits = DIV_ROUND_UP(pkt_len + sizeof(struct cpl_tx_pkt) +
+			     sizeof(*wr), sizeof(__be64));
+	ndesc = flits_to_desc(flits);
+
+	lb = &pi->ethtool_lb;
+	lb->loopback = 1;
+
+	q = &adap->sge.ethtxq[pi->first_qset];
+
+	reclaim_completed_tx(adap, &q->q, -1, true);
+	credits = txq_avail(&q->q) - ndesc;
+	if (unlikely(credits < 0))
+		return -ENOMEM;
+
+	wr = (void *)&q->q.desc[q->q.pidx];
+	memset(wr, 0, sizeof(struct tx_desc));
+
+	wr->op_immdlen = htonl(FW_WR_OP_V(FW_ETH_TX_PKT_WR) |
+			       FW_WR_IMMDLEN_V(pkt_len +
+			       sizeof(*cpl)));
+	wr->equiq_to_len16 = htonl(FW_WR_LEN16_V(DIV_ROUND_UP(flits, 2)));
+	wr->r3 = cpu_to_be64(0);
+
+	cpl = (void *)(wr + 1);
+	sgl = (u8 *)(cpl + 1);
+
+	ctrl0 = TXPKT_OPCODE_V(CPL_TX_PKT_XT) | TXPKT_PF_V(adap->pf) |
+		TXPKT_INTF_V(pi->tx_chan + 4);
+
+	cpl->ctrl0 = htonl(ctrl0);
+	cpl->pack = htons(0);
+	cpl->len = htons(pkt_len);
+	cpl->ctrl1 = cpu_to_be64(TXPKT_L4CSUM_DIS_F | TXPKT_IPCSUM_DIS_F);
+
+	eth_broadcast_addr(sgl);
+	i += ETH_ALEN;
+	ether_addr_copy(&sgl[i], netdev->dev_addr);
+	i += ETH_ALEN;
+
+	snprintf(&sgl[i], sizeof(CXGB4_SELFTEST_LB_STR), "%s",
+		 CXGB4_SELFTEST_LB_STR);
+
+	init_completion(&lb->completion);
+	txq_advance(&q->q, ndesc);
+	cxgb4_ring_tx_db(adap, &q->q, ndesc);
+
+	/* wait for the pkt to return */
+	ret = wait_for_completion_timeout(&lb->completion, 10 * HZ);
+	if (!ret)
+		ret = -ETIMEDOUT;
+	else
+		ret = lb->result;
+
+	lb->loopback = 0;
+
+	return ret;
+}
+
 /**
  *	ctrl_xmit - send a packet through an SGE control Tx queue
  *	@q: the control queue
@@ -3413,6 +3487,31 @@ static void t4_tx_completion_handler(struct sge_rspq *rspq,
 	t4_sge_eth_txq_egress_update(adapter, txq, -1);
 }
 
+static int cxgb4_validate_lb_pkt(struct port_info *pi, const struct pkt_gl *si)
+{
+	struct adapter *adap = pi->adapter;
+	struct cxgb4_ethtool_lb_test *lb;
+	struct sge *s = &adap->sge;
+	struct net_device *netdev;
+	u8 *data;
+	int i;
+
+	netdev = adap->port[pi->port_id];
+	lb = &pi->ethtool_lb;
+	data = si->va + s->pktshift;
+
+	i = ETH_ALEN;
+	if (!ether_addr_equal(data + i, netdev->dev_addr))
+		return -1;
+
+	i += ETH_ALEN;
+	if (strcmp(&data[i], CXGB4_SELFTEST_LB_STR))
+		lb->result = -EIO;
+
+	complete(&lb->completion);
+	return 0;
+}
+
 /**
  *	t4_ethrx_handler - process an ingress ethernet packet
  *	@q: the response queue that received the packet
@@ -3436,6 +3535,7 @@ int t4_ethrx_handler(struct sge_rspq *q, const __be64 *rsp,
 	struct port_info *pi;
 	int ret = 0;
 
+	pi = netdev_priv(q->netdev);
 	/* If we're looking at TX Queue CIDX Update, handle that separately
 	 * and return.
 	 */
@@ -3463,6 +3563,12 @@ int t4_ethrx_handler(struct sge_rspq *q, const __be64 *rsp,
 	if (err_vec)
 		rxq->stats.bad_rx_pkts++;
 
+	if (unlikely(pi->ethtool_lb.loopback && pkt->iff >= NCHAN)) {
+		ret = cxgb4_validate_lb_pkt(pi, si);
+		if (!ret)
+			return 0;
+	}
+
 	if (((pkt->l2info & htonl(RXF_TCP_F)) ||
 	     tnl_hdr_len) &&
 	    (q->netdev->features & NETIF_F_GRO) && csum_ok && !pkt->ip_frag) {
@@ -3476,7 +3582,6 @@ int t4_ethrx_handler(struct sge_rspq *q, const __be64 *rsp,
 		rxq->stats.rx_drops++;
 		return 0;
 	}
-	pi = netdev_priv(q->netdev);
 
 	/* Handle PTP Event Rx packet */
 	if (unlikely(pi->ptp_enable)) {
-- 
2.21.1

Re: [PATCH net-next 0/4] cxgb4: add ethtool self_test support

[Andrew Lunn]

On Fri, Jul 17, 2020 at 07:17:55PM +0530, Vishal Kulkarni wrote:
> This series of patches add support for below tests.
> 1. Adapter status test
> 2. Link test
> 3. Link speed test
> 4. Loopback test

Hi Vishal

The loopback test is pretty usual for an ethtool self test. But the
first 3 are rather odd. They don't really seem to be self tests. What
reason do you have for adding these? Are you trying to debug a
specific problem?

	 Andrew

Re: [PATCH net-next 0/4] cxgb4: add ethtool self_test support

[Vishal Kulkarni]

On Friday, July 07/17/20, 2020 at 20:02:51 +0200, Andrew Lunn wrote:
> On Fri, Jul 17, 2020 at 07:17:55PM +0530, Vishal Kulkarni wrote:
> > This series of patches add support for below tests.
> > 1. Adapter status test
> > 2. Link test
> > 3. Link speed test
> > 4. Loopback test
> 
> Hi Vishal
> 
> The loopback test is pretty usual for an ethtool self test. But the
> first 3 are rather odd. They don't really seem to be self tests. What
> reason do you have for adding these? Are you trying to debug a
> specific problem?
> 
> 	 Andrew
Hi Andrew,

Our requirement is to add a list of self tests that can summarize if the adapter is functioning
properly in a single command during system init. The above tests are the most common ones run by
our on-field diagnostics team. Besides, these tests seem to be the most common among other drivers as well.

Hence we have added
1. Adapter status test: Tests whether the adapter is alive or crashed
2. Link test: Adapter PHY is up or not.
3. Link speed test: Adapter has negotiated link speed correctly or not.

-Vishal

Re: [PATCH net-next 0/4] cxgb4: add ethtool self_test support

[Andrew Lunn]

On Mon, Jul 20, 2020 at 11:58:37AM +0530, Vishal Kulkarni wrote:
> On Friday, July 07/17/20, 2020 at 20:02:51 +0200, Andrew Lunn wrote:
> > On Fri, Jul 17, 2020 at 07:17:55PM +0530, Vishal Kulkarni wrote:
> > > This series of patches add support for below tests.
> > > 1. Adapter status test
> > > 2. Link test
> > > 3. Link speed test
> > > 4. Loopback test
> > 
> > Hi Vishal
> > 
> > The loopback test is pretty usual for an ethtool self test. But the
> > first 3 are rather odd. They don't really seem to be self tests. What
> > reason do you have for adding these? Are you trying to debug a
> > specific problem?
> > 
> > 	 Andrew
> Hi Andrew,
> 
> Our requirement is to add a list of self tests that can summarize if the adapter is functioning
> properly in a single command during system init. The above tests are the most common ones run by
> our on-field diagnostics team. Besides, these tests seem to be the most common among other drivers as well.
> 
> Hence we have added
> 1. Adapter status test: Tests whether the adapter is alive or crashed
> 2. Link test: Adapter PHY is up or not.
> 3. Link speed test: Adapter has negotiated link speed correctly or not.

Hi Vishal

Knowing that the field team does this is useful. But i still don't see
these as self tests.

From the man page:

       -t --test
              Executes adapter selftest on the specified network
	      device. Possible test modes are:

           offline
                  Perform full set of tests, possibly interrupting normal
		  operation during the tests,

           online Perform limited set of tests, not interrupting normal
	   operation,

           external_lb
                  Perform full set of tests, as for offline, and additionally
		  an external-loopback test.


Maybe a crashed adaptor could be considered a self test, but

1) I expect nearly everything else is failing so it is pretty obvious
2) devlink health seems like a better API

The PHY is up or not is only partially to do with self. It has a lot
to do with the link partner and the cable. Plus ip link show will tell
you this.

3) This actually sounds like a bug. Why would it of negotiated a link
speed it cannot support? If you have non-overlapping sets of
advertised link modes, i.e. there is no common mode to select, the
link should remain down, but this is not an error. You can use ethtool
to list both the local and peer advertised modes. You could also
report this via the new link state properties Mellanox just added.

       Andrew

Re: [PATCH net-next 0/4] cxgb4: add ethtool self_test support

[Vishal Kulkarni]

On Monday, July 07/20/20, 2020 at 15:35:54 +0200, Andrew Lunn wrote:
> On Mon, Jul 20, 2020 at 11:58:37AM +0530, Vishal Kulkarni wrote:
> > On Friday, July 07/17/20, 2020 at 20:02:51 +0200, Andrew Lunn wrote:
> > > On Fri, Jul 17, 2020 at 07:17:55PM +0530, Vishal Kulkarni wrote:
> > > > This series of patches add support for below tests.
> > > > 1. Adapter status test
> > > > 2. Link test
> > > > 3. Link speed test
> > > > 4. Loopback test
> > > 
> > > Hi Vishal
> > > 
> > > The loopback test is pretty usual for an ethtool self test. But the
> > > first 3 are rather odd. They don't really seem to be self tests. What
> > > reason do you have for adding these? Are you trying to debug a
> > > specific problem?
> > > 
> > > 	 Andrew
> > Hi Andrew,
> > 
> > Our requirement is to add a list of self tests that can summarize if the adapter is functioning
> > properly in a single command during system init. The above tests are the most common ones run by
> > our on-field diagnostics team. Besides, these tests seem to be the most common among other drivers as well.
> > 
> > Hence we have added
> > 1. Adapter status test: Tests whether the adapter is alive or crashed
> > 2. Link test: Adapter PHY is up or not.
> > 3. Link speed test: Adapter has negotiated link speed correctly or not.
> 
> Hi Vishal
> 
> Knowing that the field team does this is useful. But i still don't see
> these as self tests.
> 
> From the man page:
> 
>        -t --test
>               Executes adapter selftest on the specified network
> 	      device. Possible test modes are:
> 
>            offline
>                   Perform full set of tests, possibly interrupting normal
> 		  operation during the tests,
> 
>            online Perform limited set of tests, not interrupting normal
> 	   operation,
> 
>            external_lb
>                   Perform full set of tests, as for offline, and additionally
> 		  an external-loopback test.
> 
> 
> Maybe a crashed adaptor could be considered a self test, but
> 
> 1) I expect nearly everything else is failing so it is pretty obvious
> 2) devlink health seems like a better API
> 
> The PHY is up or not is only partially to do with self. It has a lot
> to do with the link partner and the cable. Plus ip link show will tell
> you this.
> 
> 3) This actually sounds like a bug. Why would it of negotiated a link
> speed it cannot support? If you have non-overlapping sets of
> advertised link modes, i.e. there is no common mode to select, the
> link should remain down, but this is not an error. You can use ethtool
> to list both the local and peer advertised modes. You could also
> report this via the new link state properties Mellanox just added.
> 
>        Andrew

Hi Andrew,

Our requirement is to get overall adapter health from single tool and command.
Using devlink and ip will require multiple tools and commands.

-Vishal

Re: [PATCH net-next 0/4] cxgb4: add ethtool self_test support

[Andrew Lunn]

> Hi Andrew,
> 
> Our requirement is to get overall adapter health from single tool and command.
> Using devlink and ip will require multiple tools and commands.

That is not a good reason to abuse the Kernel norms and do odd things.

     Andrew

Re: [PATCH net-next 0/4] cxgb4: add ethtool self_test support

[Jakub Kicinski]

On Tue, 21 Jul 2020 15:41:45 +0200 Andrew Lunn wrote:
> > Hi Andrew,
> > 
> > Our requirement is to get overall adapter health from single tool and command.
> > Using devlink and ip will require multiple tools and commands.  
> 
> That is not a good reason to abuse the Kernel norms and do odd things.

+1 

You should probably build your own tool if you have this single tool
requirement. This single tool fallacy leads to very bad outcomes, like
people trying to report system state in device dumps, 'cause they want
system state in their customer bug reports :/

Re: [PATCH net-next 0/4] cxgb4: add ethtool self_test support

[David Miller]

From: Vishal Kulkarni <vishal@chelsio.com>
Date: Tue, 21 Jul 2020 19:08:35 +0530

> Our requirement is to get overall adapter health from single tool and command.
> Using devlink and ip will require multiple tools and commands.

This is an invalid argument.

We have multiple facilities, each of which handles a specific task that it
was designed for.  You shall use such facilities, as appropriate, to fulfill
your needs.

Re: [PATCH net-next 0/4] cxgb4: add ethtool self_test support

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Tue, 21 Jul 2020 15:41:45 +0200

>> Hi Andrew,
>> 
>> Our requirement is to get overall adapter health from single tool and command.
>> Using devlink and ip will require multiple tools and commands.
> 
> That is not a good reason to abuse the Kernel norms and do odd things.

+1