Re: [PATCH RFC net-next 1/2] net: selftest: add net_selftest_custom() interface

[Gerhard Engleder]

On 21.04.25 15:43, Jijie Shao wrote:
> In net/core/selftests.c,
> net_selftest() supports loopback tests.
> However, the loopback content of this interface is a fixed common test
> and cannot be expanded to add the driver's own test.
>
> In this patch, the net_selftest_custom() interface is added
> to support driver customized loopback tests and
> extra common loopback tests.
>
> Signed-off-by: Jijie Shao <shaojijie@xxxxxxxxxx>
> ---
>   include/net/selftests.h |  61 +++++++++++++
>   net/core/selftests.c    | 188 +++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 245 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/selftests.h b/include/net/selftests.h
> index e65e8d230d33..a36e6ee0a41f 100644
> --- a/include/net/selftests.h
> +++ b/include/net/selftests.h
> @@ -4,6 +4,48 @@
>   
>   #include <linux/ethtool.h>
>   
> +#define NET_TEST_NETIF_CARRIER		BIT(0)
> +#define NET_TEST_FULL_DUPLEX		BIT(1)
> +#define NET_TEST_TCP			BIT(2)
> +#define NET_TEST_UDP			BIT(3)
> +#define NET_TEST_UDP_MAX_MTU		BIT(4)
> +
> +#define NET_EXTRA_CARRIER_TEST		BIT(0)
> +#define NET_EXTRA_FULL_DUPLEX_TEST	BIT(1)
> +#define NET_EXTRA_PHY_TEST		BIT(2)

What is the difference between NET_TEST_NETIF_CARRIER and
NET_EXTRA_CARRIER_TEST? Aren't these the same tests?

> +struct net_test_entry {
> +	char name[ETH_GSTRING_LEN];
> +
> +	/* can set to NULL */
> +	int (*enable)(struct net_device *ndev, bool enable);
> +
> +	/* can set to NULL */
> +	int (*fn)(struct net_device *ndev);
> +
> +	/* if flag is set, fn() will be ignored,
> +	 * and will do test according to the flag,
> +	 * such as NET_TEST_UDP...
> +	 */
> +	unsigned long flags;

Looks limited, this interface does not scale as the bits in flags are
limited.

> +};
> +
> +#define NET_TEST_E(_name, _enable, _flags) { \
> +	.name = _name, \
> +	.enable = _enable, \
> +	.fn = NULL, \
> +	.flags = _flags }
> +
> +#define NET_TEST_ENTRY_MAX_COUNT	10

I expect that you have to eliminate this limitation too.

> +struct net_test {
> +	/* extra tests will be added based on this flag */
> +	unsigned long extra_flags;

Why this extra_flags to trigger tests? AFAIU the same tests can be
triggered with entries.

> +
> +	struct net_test_entry entries[NET_TEST_ENTRY_MAX_COUNT];
> +	/* the count of entries, must <= NET_TEST_ENTRY_MAX_COUNT */
> +	u32 count;
> +};

You try to make net selftests more usable for drivers. I also tried
that, but Andrew Lunn argumented for controlling the selftests some
user space interface is expected. IMO the situation has not changed.

https://lore.kernel.org/netdev/20250227203138.60420-3-gerhard@xxxxxxxxxxxxxxxxxxxxx/T/#md5e4ac1ca41adbdb43755d3c189aa8b8228bf8c9

Gerhard