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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 15 Apr 2021 13:06:45 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Oleksij Rempel <o.rempel@...gutronix.de>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>
Cc:     kernel@...gutronix.de, netdev@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-imx@....com, Fabio Estevam <festevam@...il.com>,
        David Jander <david@...tonic.nl>,
        Russell King <linux@...linux.org.uk>,
        Philippe Schenker <philippe.schenker@...adex.com>
Subject: Re: [PATCH v2 4/7] net: add generic selftest support



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@...gutronix.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