Re: [PATCH net-next v9 09/11] onsemi: s2500: Added selftest support to onsemi's S2500 driver

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When CONFIG_NET_SELFTESTS=n, s2500_get_sset_count() returns 0
  for ETH_SS_TEST, because the include/net/selftests.h stub…

--

onsemi: s2500: Added selftest support to onsemi's S2500 driver

This patch adds ethtool selftest support to the onsemi S2500 MAC-PHY
driver. For ETH_SS_TEST it reports the generic net_selftest count and
strings. It also sets net_selftest() as the self_test callback in
s2500_ethtool_ops.

> diff --git a/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c b/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c
> index 062cab9460dcf..12e59e8d44a62 100644
> --- a/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c

[ ... ]

> @@ -201,6 +202,8 @@ static int s2500_get_sset_count(struct net_device *ndev, int sset)
>  	switch (sset) {
>  	case ETH_SS_STATS:
>  		return S2500_MAC_STATS_LEN;
> +	case ETH_SS_TEST:
> +		return net_selftest_get_count();

[Severity: Medium]
What happens here when CONFIG_NET_SELFTESTS is not enabled?

In that case include/net/selftests.h provides stubs:

static inline int net_selftest_get_count(void)
{
	return 0;
}

So s2500_get_sset_count() returns 0 for ETH_SS_TEST. At the same time,
s2500_ethtool_ops still sets ".self_test = net_selftest", which is the
empty stub.

The ethtool core expects a non-zero count whenever self_test is set. In
net/ethtool/ioctl.c:

ethtool_self_test() {
    ...
	test_len = ops->get_sset_count(dev, ETH_SS_TEST);
	if (test_len < 0)
		return test_len;
	WARN_ON(test_len == 0);
    ...
}

After the warning, the no-op stub runs and "ethtool -t" reports success
without running any tests.

ETHTOOL_GSTRINGS does not require CAP_NET_ADMIN. With
string_set == ETH_SS_TEST, that path also hits a warning:

ethtool_get_strings() {
    ...
	ret = __ethtool_get_sset_count(dev, gstrings.string_set);
	if (ret < 0)
		return ret;
	...
	WARN_ON_ONCE(!ret);
    ...
}

Can this configuration actually be built? NET_SELFTESTS depends on
PHYLIB && INET. S2500_MACPHY only uses imply for it and has no INET
dependency:

config S2500_MACPHY
	tristate "S2500 support"
	depends on SPI
	depends on PTP_1588_CLOCK_OPTIONAL
	imply NET_SELFTESTS

With CONFIG_INET=n, the driver can be built while NET_SELFTESTS=n.

A few existing drivers (fec, ag71xx, asix, lan78xx, smsc95xx) follow
the same pattern.

Would any of these fixes work here?

  - return -EOPNOTSUPP for ETH_SS_TEST when net_selftest_get_count()
    is 0
  - set .self_test only under IS_ENABLED(CONFIG_NET_SELFTESTS)
  - add a dependency on NET_SELFTESTS, and therefore on INET

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-s2500-mac-phy-support-v9-0-dcefe1d0bf0d%40onsemi.com