Re: [PATCH net-next v2 2/2] net: stmmac: Don't set or get RSS parameters when not supported

[netdev-bot+sashiko@kernel.org]

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

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

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] This patch makes stmmac_get_rxfh() in
  drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c return…
- [Low] The commit message describes a user-visible bug: on stmmac
  devices without RSS, 'ethtool -x reports an indirection table and a…

--

net: stmmac: Don't set or get RSS parameters when not supported

This commit changes the stmmac ethtool RSS ops to return early when
stmmac_rss_is_supported() is false. The key and indirection sizes now
report 0, and get_rxfh/set_rxfh return -EOPNOTSUPP. The aim is to stop
reporting bogus RSS data and to make the RSS kselftests work on stmmac.

> The RSS kselftests fail on stmmac, and this is partly due to the driver
> reporting bogus data for the RSS ops :
>
>  - ethtool -x reports an indirection table and a key while the hardware
>    doesn't have any of that
>  - ethtool -X fails with -EINVAL.
>
> Let's return early in the rss ops if we know the hardware and platform
> don't support RSS.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

[Severity: Low]
The commit message describes a bug that users can see, but there is no
Fixes: tag.

Blame shows that the unconditional stmmac_get_rxfh_key_size(),
stmmac_get_rxfh_indir_size(), stmmac_get_rxfh() and stmmac_set_rxfh()
logic came from commit 76067459c686. The later refactor fb6e30a72539 only
moved these functions to the ethtool_rxfh_param API.

Should this carry the following tag?

Fixes: 76067459c686 ("net: stmmac: Implement RSS and enable it in XGMAC core")

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 1cf0f8820b33..56019683f399 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c

[ ... ]

> @@ -943,6 +949,9 @@ static int stmmac_get_rxfh(struct net_device *dev,
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	int i;
>  
> +	if (!stmmac_rss_is_supported(priv))
> +		return -EOPNOTSUPP;
> +

[Severity: Medium]
Does this -EOPNOTSUPP break namespace-wide ETHTOOL_MSG_RSS_GET dumps?

rss_dump_one_dev() in net/ethtool/rss.c only skips a device when it has
no get_rxfh op, and stmmac still provides one:

net/ethtool/rss.c:rss_dump_one_dev() {
    ...
	if (!dev->ethtool_ops->get_rxfh)
		return 0;

	if (!ctx->ctx_idx) {
		ret = rss_dump_one_ctx(skb, cb, dev, 0);
		if (ret)
			return ret;
    ...
}

The error then comes back through this path:

ethnl_rss_dumpit()
  rss_dump_one_dev()
    rss_dump_one_ctx()
      rss_prepare()
        rss_prepare_get()
          ops->get_rxfh() -> stmmac_get_rxfh() returns -EOPNOTSUPP

When it reaches ethnl_rss_dumpit(), the loop stops:

	for_each_netdev_lock_ops_compat_scoped(net, dev, ctx->ifindex) {
		...
		ret = rss_dump_one_dev(skb, cb, dev);
		if (ret)
			break;
	}

	return ret;

So the dump fails at the stmmac netdev, and no device with a higher
ifindex gets reported. That includes NICs that do support RSS.

No in-tree platform sets plat->rss_en. The only match is the declaration
in include/linux/stmmac.h. That means stmmac_rss_is_supported() is false
for every in-tree stmmac device.

Before this patch, stmmac_get_rxfh() always returned 0. The dump used to
complete, even though it reported bogus data.

The RSS selftests use this dump path. In
tools/testing/selftests/drivers/net/hw/rss_ctx.py,
test_rss_context_dump() calls:

    ctxs = cfg.ethnl.rss_get({}, dump=True)

It also runs a dump with {'start-context': 1}. On a test host that has a
stmmac port plus the NIC under test, would these now fail, when they
passed before?

mvpp2 and mvneta already return -EOPNOTSUPP from get_rxfh, so the core
already stopped dumps this way. This patch applies that behaviour to every
in-tree stmmac device.

Would it be better to have rss_dump_one_dev() skip devices whose get_rxfh
returns -EOPNOTSUPP, or that report zero indir and key sizes? Another
option is to not provide the stmmac get_rxfh op when RSS is unsupported.

>  	if (rxfh->indir) {
>  		for (i = 0; i < ARRAY_SIZE(priv->rss.table); i++)
>  			rxfh->indir[i] = priv->rss.table[i];

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928145043.1228968-1-maxime.chevallier%40bootlin.com