Re: [PATCH v2 4/7] net: add generic selftest support

[Florian Fainelli]

On 4/15/2021 6:07 AM, Oleksij Rempel wrote:
> Port some parts of the stmmac selftest and reuse it as basic generic selftest
> library. This patch was tested with following combinations:
> - iMX6DL FEC -> AT8035
> - iMX6DL FEC -> SJA1105Q switch -> KSZ8081
> - iMX6DL FEC -> SJA1105Q switch -> KSZ9031
> - AR9331 ag71xx -> AR9331 PHY
> - AR9331 ag71xx -> AR9331 switch -> AR9331 PHY
> 
> Signed-off-by: Oleksij Rempel 
> ---

[snip]

> +
> +struct net_packet_attrs {
> + unsigned char *src;
> + unsigned char *dst;
> + u32 ip_src;
> + u32 ip_dst;
> + int tcp;

This can be an u8 and named proto maybe?

> + int sport;
> + int dport;

These two can be u16

> + int timeout;
> + int size;
> + int max_size;
> + u8 id;
> + u16 queue_mapping;
> +};

[snip]

> +static const struct net_test {
> + char name[ETH_GSTRING_LEN];
> + int (*fn)(struct net_device *ndev);
> +} net_selftests[] = {
> + {
> + .name = "PHY Loopback, UDP  ",

This should be "PHY internal loopback, UDP"

> + .fn = net_test_phy_loopback_udp,
> + }, {
> + .name = "PHY Loopback, TCP  ",
> + .fn = net_test_phy_loopback_tcp,

and "PHY internal loopback, TCP"

to make it clear that the loopback is internal, as opposed to external.
Or if you prefer to use the line-side or MAC-side that works too.

> + },
> +};
> +
> +void net_selftest(struct net_device *ndev, struct ethtool_test *etest, u64 
> *buf)
> +{
> + int count = net_selftest_get_count();
> + int i;
> +
> + memset(buf, 0, sizeof(*buf) * count);
> + net_test_next_id = 0;
> +
> + if (etest->flags != ETH_TEST_FL_OFFLINE) {
> + netdev_err(ndev, "Only offline tests are supported\n");
> + etest->flags |= ETH_TEST_FL_FAILED;
> + return;
> + } else if (!netif_carrier_ok(ndev)) {
> + netdev_err(ndev, "You need valid Link to execute tests\n");
> + etest->flags |= ETH_TEST_FL_FAILED;
> + return;
> + }
> +
> + if (!ndev->phydev)
> + return;

Can you move that as the first test and return -EOPNOTSUPP instead?

> +
> + /* PHY loopback tests should be combined to avoid delays on each PHY
> +  * reconfiguration
> +  */
> + phy_loopback(ndev->phydev, true);
> +
> + /* give PHYs some time to establish the loopback link */
> + msleep(100);

Cannot you poll for LSTATUS instead?

> +
> + for (i = 0; i < count; i++) {
> + buf[i] = net_selftests[i].fn(ndev);
> + if (buf[i] && (buf[i] != -EOPNOTSUPP))
> + etest->flags |= ETH_TEST_FL_FAILED;
> + }
> +
> + phy_loopback(ndev->phydev, false);

Can you propagate the return value here?

As spotted by the test robot please export all of these symbols as
EXPORT_SYMBOL_GPL().
-- 
Florian

[PATCH v2 4/7] net: add generic selftest support

[Oleksij Rempel]

Port some parts of the stmmac selftest and reuse it as basic generic selftest
library. This patch was tested with following combinations:
- iMX6DL FEC -> AT8035
- iMX6DL FEC -> SJA1105Q switch -> KSZ8081
- iMX6DL FEC -> SJA1105Q switch -> KSZ9031
- AR9331 ag71xx -> AR9331 PHY
- AR9331 ag71xx -> AR9331 switch -> AR9331 PHY

Signed-off-by: Oleksij Rempel 
---
 include/net/selftests.h |  12 ++
 net/Kconfig |   4 +
 net/core/Makefile   |   1 +
 net/core/selftests.c| 366 
 4 files changed, 383 insertions(+)
 create mode 100644 include/net/selftests.h
 create mode 100644 net/core/selftests.c

diff --git a/include/net/selftests.h b/include/net/selftests.h
new file mode 100644
index ..9993b9498cf3
--- /dev/null
+++ b/include/net/selftests.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _NET_SELFTESTS
+#define _NET_SELFTESTS
+
+#include 
+
+void net_selftest(struct net_device *ndev, struct ethtool_test *etest,
+ u64 *buf);
+int net_selftest_get_count(void);
+void net_selftest_get_strings(u8 *data);
+
+#endif /* _NET_SELFTESTS */
diff --git a/net/Kconfig b/net/Kconfig
index 9c456acc379e..6e1285c8340e 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -429,6 +429,10 @@ config GRO_CELLS
 config SOCK_VALIDATE_XMIT
bool
 
+config NET_SELFTESTS
+   bool
+   default n
+
 config NET_SOCK_MSG
bool
default n
diff --git a/net/core/Makefile b/net/core/Makefile
index 0c2233c826fd..1a6168d8f23b 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -33,6 +33,7 @@ obj-$(CONFIG_NET_DEVLINK) += devlink.o
 obj-$(CONFIG_GRO_CELLS) += gro_cells.o
 obj-$(CONFIG_FAILOVER) += failover.o
 ifeq ($(CONFIG_INET),y)
+obj-$(CONFIG_NET_SELFTESTS) += selftests.o
 obj-$(CONFIG_NET_SOCK_MSG) += skmsg.o
 obj-$(CONFIG_BPF_SYSCALL) += sock_map.o
 endif
diff --git a/net/core/selftests.c b/net/core/selftests.c
new file mode 100644
index ..da1871d0f4ef
--- /dev/null
+++ b/net/core/selftests.c
@@ -0,0 +1,366 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2019 Synopsys, Inc. and/or its affiliates.
+ * stmmac Selftests Support
+ *
+ * Author: Jose Abreu 
+ *
+ * Ported from stmmac by:
+ * Copyright (C) 2021 Oleksij Rempel 
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+struct net_packet_attrs {
+   unsigned char *src;
+   unsigned char *dst;
+   u32 ip_src;
+   u32 ip_dst;
+   int tcp;
+   int sport;
+   int dport;
+   int timeout;
+   int size;
+   int max_size;
+   u8 id;
+   u16 queue_mapping;
+};
+
+struct net_test_priv {
+   struct net_packet_attrs *packet;
+   struct packet_type pt;
+   struct completion comp;
+   int double_vlan;
+   int vlan_id;
+   int ok;
+};
+
+struct netsfhdr {
+   __be32 version;
+   __be64 magic;
+   u8 id;
+} __packed;
+
+static u8 net_test_next_id;
+
+#define NET_TEST_PKT_SIZE (sizeof(struct ethhdr) + sizeof(struct iphdr) + \
+  sizeof(struct netsfhdr))
+#define NET_TEST_PKT_MAGIC 0xdeadcafecafedeadULL
+#define NET_LB_TIMEOUT msecs_to_jiffies(200)
+
+static struct sk_buff *net_test_get_udp_skb(struct net_device *ndev,
+   struct net_packet_attrs *attr)
+{
+   struct sk_buff *skb = NULL;
+   struct udphdr *uhdr = NULL;
+   struct tcphdr *thdr = NULL;
+   struct netsfhdr *shdr;
+   struct ethhdr *ehdr;
+   struct iphdr *ihdr;
+   int iplen, size;
+
+   size = attr->size + NET_TEST_PKT_SIZE;
+
+   if (attr->tcp)
+   size += sizeof(struct tcphdr);
+   else
+   size += sizeof(struct udphdr);
+
+   if (attr->max_size && attr->max_size > size)
+   size = attr->max_size;
+
+   skb = netdev_alloc_skb(ndev, size);
+   if (!skb)
+   return NULL;
+
+   prefetchw(skb->data);
+
+   ehdr = skb_push(skb, ETH_HLEN);
+   skb_reset_mac_header(skb);
+
+   skb_set_network_header(skb, skb->len);
+   ihdr = skb_put(skb, sizeof(*ihdr));
+
+   skb_set_transport_header(skb, skb->len);
+   if (attr->tcp)
+   thdr = skb_put(skb, sizeof(*thdr));
+   else
+   uhdr = skb_put(skb, sizeof(*uhdr));
+
+   eth_zero_addr(ehdr->h_dest);
+
+   if (attr->src)
+   ether_addr_copy(ehdr->h_source, attr->src);
+   if (attr->dst)
+   ether_addr_copy(ehdr->h_dest, attr->dst);
+
+   ehdr->h_proto = htons(ETH_P_IP);
+
+   if (attr->tcp) {
+   thdr->source = htons(attr->sport);
+   thdr->dest = htons(attr->dport);
+   thdr->doff = sizeof(struct tcphdr) / 4;
+   thdr->check = 0;
+   } else {
+   uhdr->source = htons(attr->sport);
+   uhdr->dest = htons(attr->dport);
+   uhdr->len = htons(sizeof(*shdr) + sizeof(*uhdr) + attr->size);
+   if