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 <o.rempel@pengutronix.de>
---

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel