Re: [PATCH net-next v6 13/15] onsemi: s2500: Added selftest support to onsemi's S2500 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Triggering WARN_ON(test_len == 0) in ethtool_self_test() when CONFIG_NET_SELFTESTS is disabled.
--

commit 5471ef8ce8772b7b0fc98bb44681a4f8a55bbb68
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

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

Adds selftest support for onsemi S2500 MAC-PHY.

> diff --git a/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c b/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c
> --- a/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c
> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_ethtool.c
[ ... ]
> @@ -229,6 +230,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();
>  	default:
>  		return -EOPNOTSUPP;
>  	}

[Severity: Medium]
Since the Kconfig uses "imply NET_SELFTESTS", it's possible for this driver
to be built with CONFIG_NET_SELFTESTS disabled. In that case, the fallback
stub for net_selftest_get_count() returns 0.

When the ethtool core processes the test ioctl, it expects the test array
length to be greater than 0. If a user runs "ethtool -t", could this 0 count
trigger the WARN_ON(test_len == 0) in ethtool_self_test()?

Should this return -EOPNOTSUPP when tests are disabled, rather than
forwarding a 0 count?

[ ... ]
> @@ -349,5 +355,6 @@ const struct ethtool_ops s2500_ethtool_ops = {
>  	.get_ts_info        = s2500_get_ts_info,
>  	.get_regs_len       = s2500_get_regs_len,
>  	.get_regs           = s2500_get_regs,
> +	.self_test          = net_selftest,
>  };

[Severity: Medium]
Similarly, when CONFIG_NET_SELFTESTS is disabled, the fallback for
net_selftest is still a valid function pointer stub.

Because the driver unconditionally provides this callback, the ethtool core
will bypass its (!ops->self_test) check, proceed to query the test count,
and hit the WARN_ON mentioned above.

Would it be safer to only populate this callback assignment when testing is
actually enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-s2500-mac-phy-support-v6-0-18ce79500371@onsemi.com?part=13