Re: [PATCH net-next 1/3] selftests: drv-net: hds: always restore tcp-data-split to auto

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · 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).

- [High] The new guard in _hds_reset()
  (tools/testing/selftests/drivers/net/hds.py:68) is `if 'tcp-data-…
- [Medium] _hds_reset() now writes tcp-data-split='unknown' whenever the
  attribute was in the baseline snapshot.
- [Low] The commit message does not match the code or the tree: (1) The
  subject says 'always restore tcp-data-split to auto', but _hds_reset()…

--

selftests: drv-net: hds: always restore tcp-data-split to auto

This changes _hds_reset() in the hds.py selftest so that tcp-data-split is
reset to "unknown" whenever the attribute was in the saved ring snapshot.
Before, the reset only happened when the reported value had changed. The
reason given is that bnxt and mlx5 report the current HDS state rather than
the configured value.

> selftests: drv-net: hds: always restore tcp-data-split to auto

[Severity: Low]
The subject says "always", but the reset only runs when the baseline
snapshot contained the attribute:

    if 'tcp-data-split' in rings:

The body's "when it was reported" qualifier is the case that leaves mlx5
unrestored in this tree (see the _hds_reset() comment below).

> mlx5 will report enabled or disabled based on whether
> SHAMPO is active, and an explicit "disabled" keeps HW-GRO off.

Is this describing a pending mlx5 change? In this tree
mlx5e_ethtool_get_ringparam() never sets kernel_param->tcp_data_split. That
means this selftest change seems to depend on a driver patch that isn't
named.

Could the dependency be stated, so the patches get ordered correctly?

Should this also have a Fixes: tag? The conditional reset came in with:

Fixes: ee3ae27721fb ("selftests: drv-net: hds: restore hds settings")

and the leak already happens without the mlx5 change. On netdevsim,
nsim_get_ringparam() reports ENABLED when hds_config is UNKNOWN:

drivers/net/netdevsim/ethtool.c:nsim_get_ringparam() {
    ...
	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
		kernel_ring->tcp_data_split = ETHTOOL_TCP_DATA_SPLIT_ENABLED;
    ...
}

So after set_hds_enable, the old code sees cur == rings and skips the
reset, which leaves hds_config at ENABLED. bnxt_get_ringparam() reports the
BNXT_FLAG_AGG_RINGS state, so the same leak happens there whenever TPA keeps
the agg rings on.

> diff --git a/tools/testing/selftests/drivers/net/hds.py b/tools/testing/selftests/drivers/net/hds.py
> index 606e26d75951f..af34dc9ce7499 100755
> --- a/tools/testing/selftests/drivers/net/hds.py
> +++ b/tools/testing/selftests/drivers/net/hds.py
> @@ -65,13 +65,13 @@ def _hds_reset(cfg, netnl, rings) -> None:
>      cur = netnl.rings_get({'header': {'dev-index': cfg.ifindex}})
>  
>      arg = {'header': {'dev-index': cfg.ifindex}}
> -    if cur.get('tcp-data-split') != rings.get('tcp-data-split'):
> +    if 'tcp-data-split' in rings:

[Severity: High]
Does this new check skip the restore in the exact mlx5 case the commit
message describes?

In this tree mlx5e_ethtool_get_ringparam() never sets
kernel_param->tcp_data_split. So rings_prepare_data() reports
dev->cfg->hds_config, which defaults to ETHTOOL_TCP_DATA_SPLIT_UNKNOWN (0).
rings_fill_reply() only emits the attribute when the value is non-zero:

net/ethtool/rings.c:rings_fill_reply() {
    ...
	    (kr->tcp_data_split &&
	     (nla_put_u8(skb, ETHTOOL_A_RINGS_TCP_DATA_SPLIT,
    ...
}

That leaves the snapshot taken in _defer_reset_hds() without a
'tcp-data-split' key. mlx5 does report 'hds-thresh', so _defer_reset_hds()
still registers _hds_reset().

After set_hds_disable, hds_config is DISABLED. In the cleanup the new check
is false and hds-thresh is unchanged. len(arg) is 1, so rings_set is never
called and the device stays explicitly "disabled".

After set_hds_enable it stays "enabled", and mlx5e_fix_features() then
forces HW-GRO on:

	if (cfg && cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
	    !(features & NETIF_F_GRO_HW)) {

The old condition did send "unknown" in this case, since cur 'disabled'
differs from rings None. It then hit a KeyError on cur['tcp-data-split'].
With this patch the restore is silently skipped instead.

The fix only works if mlx5 always reports tcp-data-split, and that driver
change isn't in this tree. Would something like this cover both cases?

    if 'tcp-data-split' in rings or 'tcp-data-split' in cur:

The later comparison would also use .get() on both sides, so that a
missing attribute means "unknown".

>          # Try to reset to "unknown" first, we don't know if the setting
>          # was the default or user chose it. Default seems more likely.
>          arg['tcp-data-split'] = "unknown"
>          netnl.rings_set(arg)
>          cur = netnl.rings_get({'header': {'dev-index': cfg.ifindex}})
> -        if cur['tcp-data-split'] == rings['tcp-data-split']:
> +        if cur.get('tcp-data-split') == rings['tcp-data-split']:
>              del arg['tcp-data-split']

[Severity: Medium]
Can this replace an explicit tcp-data-split=enabled, set before the test
ran, with UNKNOWN?

With the new check, "unknown" is written for every test that calls
_defer_reset_hds(). That includes the tests that only touch hds-thresh
(set_hds_thresh_random, set_hds_thresh_zero, set_hds_thresh_max,
set_hds_thresh_gt) and tests that skip early. Whether to keep "unknown" is
then decided by comparing reported values, which can match even when the
configured values differ.

On netdevsim, nsim_get_ringparam() reports ENABLED for both UNKNOWN and an
explicit ENABLED. Say hds_config was ENABLED before the test:

  - ethnl_set_rings() commits hds_config = UNKNOWN
  - the re-read returns 'enabled', which matches the snapshot
  - the del runs, and the device is left at UNKNOWN

On bnxt, writing UNKNOWN clears the flag in bnxt_set_ringparam():

		else if (tcp_data_split == ETHTOOL_TCP_DATA_SPLIT_UNKNOWN)
			bp->flags &= ~BNXT_FLAG_HDS;

If TPA keeps the agg rings active, bnxt_get_ringparam() still reports
ENABLED, so the explicit HDS setting is dropped.

Before this patch, cur == rings in those tests, so tcp-data-split was never
written. Memory-provider binding requires an explicit ENABLED:

net/core/netdev_rx_queue.c:
	if (dev->cfg->hds_config != ETHTOOL_TCP_DATA_SPLIT_ENABLED) {
		NL_SET_ERR_MSG(extack, "tcp-data-split is disabled");
		return -EINVAL;

After running hds.py, a devmem or io_uring zcrx setup that had explicitly
enabled HDS would no longer be able to bind.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106031208.465456-1-dimitri.daskalakis1%40gmail.com