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

From: Florian Fainelli
Date: Thu Apr 15 2021 - 16:06:53 EST




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@xxxxxxxxxxxxxx>
> ---

[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