Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 4/7] net: add generic selftest support
To:     Oleksij Rempel <o.rempel@pengutronix.de>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>
Cc:     kernel@pengutronix.de, netdev@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-imx@nxp.com, Fabio Estevam <festevam@gmail.com>,
        David Jander <david@protonic.nl>,
        Russell King <linux@armlinux.org.uk>,
        Philippe Schenker <philippe.schenker@toradex.com>
References: <20210415130738.19603-1-o.rempel@pengutronix.de>
 <20210415130738.19603-5-o.rempel@pengutronix.de>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <347139c5-d7be-04c0-8857-3bb4891522cf@gmail.com>
Date:   Thu, 15 Apr 2021 13:06:45 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Firefox/78.0 Thunderbird/78.9.0
MIME-Version: 1.0
In-Reply-To: <20210415130738.19603-5-o.rempel@pengutronix.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


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