[PATCH net-next 0/3] selftests: drv-net: hds: fix state leaks and ioctl tests

[PATCH net-next 0/3] selftests: drv-net: hds: fix state leaks and ioctl tests

[Dimitri Daskalakis]

From: Dimitri Daskalakis <daskald@meta.com>

Fix hds.py leaving tcp-data-split in a non-default state on drivers
that report HDS state rather than config, and fix the ioctl XDP tests.

Dimitri Daskalakis (3):
  selftests: drv-net: hds: always restore tcp-data-split to auto
  selftests: drv-net: hds: perturb settings via ioctl in
    ioctl_enabled_set_xdp
  selftests: drv-net: hds: restore HDS settings in ioctl_set_xdp

 tools/testing/selftests/drivers/net/hds.py | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.52.0

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

[Dimitri Daskalakis]

From: Dimitri Daskalakis <daskald@meta.com>

_hds_reset() only restores tcp-data-split if the reported value
changed. Drivers may report the current HDS state rather than the
configured value, so an explicit setting made by a test can be
indistinguishable from the original "unknown" config.

bnxt reports enabled or disabled based on whether aggregation rings
are in use. mlx5 will report enabled or disabled based on whether
SHAMPO is active, and an explicit "disabled" keeps HW-GRO off. After
set_hds_disable on a default mlx5 device the reset is skipped, leaving
tcp-data-split disabled and HW-GRO unable to be enabled.

Always reset tcp-data-split to "unknown" when it was reported.

Signed-off-by: Dimitri Daskalakis <daskald@meta.com>
---
 tools/testing/selftests/drivers/net/hds.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hds.py b/tools/testing/selftests/drivers/net/hds.py
index 606e26d75951..af34dc9ce749 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:
         # 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']
         else:
             # Try the explicit setting
-- 
2.52.0

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

[netdev-bot+sashiko]

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

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

[Dimitri Daskalakis]

On 11/9/26 1:14 PM, netdev-bot+sashiko@kernel.org wrote:
> 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).
> 

The last sentence of the patch phrases it clearly,
"Always reset tcp-data-split to "unknown" when it was reported". If this
needs a re-spin I could amend the commit message, but this issue does not
justify a re-spin.

>> 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?

This is a pending change, I've held off on sending v4 until this series
gets merged
https://lore.kernel.org/all/20261009025137.2631215-1-dimitri.daskalakis1@gmail.com/

> 
> 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.
> 

This selftest behavior is not severe enough to warrant a Fixes tag IMO. I
was unaware that netdevsim had this issue, but called out the existing bnxt
behavior.

>> 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".
> 

The reset is currently failing with a KeyError since mlx5 does not report
the attribute. I will send v4 of a patch to report tcp-data-split in mlx5
after this series is merged. Without this change, mlx5 will get left with
tcp-data-split disabled, and the HW-GRO tests can end up failing.

>>          # 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.
> 

This only matters if a user explicitly changes tcp-data-split before the
test. Resetting to "unknown" is the common behavior across selftests
that modify tcp-data-split. It's not possible to get this fully right from
the information netlink gives us, since GET reports state, not config.

[PATCH net-next 2/3] selftests: drv-net: hds: perturb settings via ioctl in ioctl_enabled_set_xdp

[Dimitri Daskalakis]

From: Dimitri Daskalakis <daskald@meta.com>

ioctl_enabled_set_xdp never calls _ioctl_ringparam_modify(), so it
does not exercise the ioctl path. Add the missing call.

Signed-off-by: Dimitri Daskalakis <daskald@meta.com>
---
 tools/testing/selftests/drivers/net/hds.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/hds.py b/tools/testing/selftests/drivers/net/hds.py
index af34dc9ce749..7f052c319603 100755
--- a/tools/testing/selftests/drivers/net/hds.py
+++ b/tools/testing/selftests/drivers/net/hds.py
@@ -274,6 +274,8 @@ def ioctl_enabled_set_xdp(cfg, netnl) -> None:
     defer(netnl.rings_set, {'header': {'dev-index': cfg.ifindex},
                             'tcp-data-split': 'unknown'})
 
+    _ioctl_ringparam_modify(cfg, netnl)
+
     with ksft_raises(CmdExitFailure) as e:
         _xdp_onoff(cfg)
 
-- 
2.52.0

[PATCH net-next 3/3] selftests: drv-net: hds: restore HDS settings in ioctl_set_xdp

[Dimitri Daskalakis]

From: Dimitri Daskalakis <daskald@meta.com>

ioctl_set_xdp sets tcp-data-split to "unknown" without restoring the
original setting. Defer a reset like set_xdp does.

Signed-off-by: Dimitri Daskalakis <daskald@meta.com>
---
 tools/testing/selftests/drivers/net/hds.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/drivers/net/hds.py b/tools/testing/selftests/drivers/net/hds.py
index 7f052c319603..a1ea195743b5 100755
--- a/tools/testing/selftests/drivers/net/hds.py
+++ b/tools/testing/selftests/drivers/net/hds.py
@@ -254,6 +254,7 @@ def ioctl_set_xdp(cfg, netnl) -> None:
     """
     mode = _get_hds_mode(cfg, netnl)
     if mode == 'enabled':
+        _defer_reset_hds(cfg, netnl)
         netnl.rings_set({'header': {'dev-index': cfg.ifindex},
                          'tcp-data-split': 'unknown'})
 
-- 
2.52.0

Re: [PATCH net-next 3/3] selftests: drv-net: hds: restore HDS settings in ioctl_set_xdp

[netdev-bot+sashiko]

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] The fix is incomplete: two sibling tests in
  tools/testing/selftests/drivers/net/hds.py still change tcp-data-split…
- [Low] The patch has no Fixes: tag.

--

selftests: drv-net: hds: restore HDS settings in ioctl_set_xdp

This patch makes ioctl_set_xdp() in the drv-net hds.py selftest call
_defer_reset_hds() before it sets tcp-data-split to "unknown". The
original HDS configuration is then restored when the test finishes, the
same way set_xdp() already does it.

> diff --git a/tools/testing/selftests/drivers/net/hds.py b/tools/testing/selftests/drivers/net/hds.py
> index 7f052c3196039..a1ea195743b56 100755
> --- a/tools/testing/selftests/drivers/net/hds.py
> +++ b/tools/testing/selftests/drivers/net/hds.py
> @@ -254,6 +254,7 @@ def ioctl_set_xdp(cfg, netnl) -> None:
>      """
>      mode = _get_hds_mode(cfg, netnl)
>      if mode == 'enabled':
> +        _defer_reset_hds(cfg, netnl)
>          netnl.rings_set({'header': {'dev-index': cfg.ifindex},
>                           'tcp-data-split': 'unknown'})

[Severity: Medium]
Is this fix complete? Two sibling tests in hds.py still change
tcp-data-split, and they defer only a fixed reset to "unknown" instead of
going through _defer_reset_hds():

tools/testing/selftests/drivers/net/hds.py:enabled_set_xdp() {
    ...
    netnl.rings_set({'header': {'dev-index': cfg.ifindex},
                     'tcp-data-split': 'enabled'})

    defer(netnl.rings_set, {'header': {'dev-index': cfg.ifindex},
                            'tcp-data-split': 'unknown'})
    ...
}

ioctl_enabled_set_xdp() uses the same pattern.

When "unknown" does not bring back the saved value, _hds_reset() falls
back to setting that value explicitly:

        if cur.get('tcp-data-split') == rings['tcp-data-split']:
            del arg['tcp-data-split']
        else:
            # Try the explicit setting
            arg['tcp-data-split'] = rings['tcp-data-split']

The fixed resets have no such fallback. If the operator set "enabled" or
"disabled" explicitly, and the driver reports a different value under
"unknown", that setting is lost.

Here is the test order in main():

                  set_xdp,
                  enabled_set_xdp,
                  ioctl,
                  ioctl_set_xdp,
                  ioctl_enabled_set_xdp],

In a default full run, enabled_set_xdp() has already left the device at
"unknown" before ioctl_set_xdp() starts. The new _defer_reset_hds() call
therefore only saves that already-reset state. ioctl_enabled_set_xdp()
runs last and leaves the device at "unknown" again.

Doesn't that mean this change only has a visible effect when
ioctl_set_xdp is run by itself with -t?

The previous patch in this series, "selftests: drv-net: hds: perturb
settings via ioctl in ioctl_enabled_set_xdp", edited
ioctl_enabled_set_xdp() but kept its fixed "unknown" reset.

Could enabled_set_xdp() and ioctl_enabled_set_xdp() also call
_defer_reset_hds(cfg, netnl) before they set 'enabled'?

[Severity: Low]
Should this patch carry a Fixes: tag? Commit ee3ae27721fb
("selftests: drv-net: hds: restore hds settings") added _defer_reset_hds()
to set_xdp() and the set_hds_* tests, but not to ioctl_set_xdp().
Something like:

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

The test itself was added by 29b036be1b0b ("selftests: drv-net: test XDP,
HDS auto and the ioctl path"), which would also be a reasonable target.

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

Re: [PATCH net-next 3/3] selftests: drv-net: hds: restore HDS settings in ioctl_set_xdp

[Dimitri Daskalakis]

On 11/9/26 1:14 PM, netdev-bot+sashiko@kernel.org wrote:
> 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] The fix is incomplete: two sibling tests in
>   tools/testing/selftests/drivers/net/hds.py still change tcp-data-split…
> - [Low] The patch has no Fixes: tag.
> 
> --
> 
> selftests: drv-net: hds: restore HDS settings in ioctl_set_xdp
> 
> This patch makes ioctl_set_xdp() in the drv-net hds.py selftest call
> _defer_reset_hds() before it sets tcp-data-split to "unknown". The
> original HDS configuration is then restored when the test finishes, the
> same way set_xdp() already does it.
> 
>> diff --git a/tools/testing/selftests/drivers/net/hds.py b/tools/testing/selftests/drivers/net/hds.py
>> index 7f052c3196039..a1ea195743b56 100755
>> --- a/tools/testing/selftests/drivers/net/hds.py
>> +++ b/tools/testing/selftests/drivers/net/hds.py
>> @@ -254,6 +254,7 @@ def ioctl_set_xdp(cfg, netnl) -> None:
>>      """
>>      mode = _get_hds_mode(cfg, netnl)
>>      if mode == 'enabled':
>> +        _defer_reset_hds(cfg, netnl)
>>          netnl.rings_set({'header': {'dev-index': cfg.ifindex},
>>                           'tcp-data-split': 'unknown'})
> 
> [Severity: Medium]
> Is this fix complete? Two sibling tests in hds.py still change
> tcp-data-split, and they defer only a fixed reset to "unknown" instead of
> going through _defer_reset_hds():
> 
> tools/testing/selftests/drivers/net/hds.py:enabled_set_xdp() {
>     ...
>     netnl.rings_set({'header': {'dev-index': cfg.ifindex},
>                      'tcp-data-split': 'enabled'})
> 
>     defer(netnl.rings_set, {'header': {'dev-index': cfg.ifindex},
>                             'tcp-data-split': 'unknown'})
>     ...
> }
> 
> ioctl_enabled_set_xdp() uses the same pattern.
> 
> When "unknown" does not bring back the saved value, _hds_reset() falls
> back to setting that value explicitly:
> 
>         if cur.get('tcp-data-split') == rings['tcp-data-split']:
>             del arg['tcp-data-split']
>         else:
>             # Try the explicit setting
>             arg['tcp-data-split'] = rings['tcp-data-split']
> 
> The fixed resets have no such fallback. If the operator set "enabled" or
> "disabled" explicitly, and the driver reports a different value under
> "unknown", that setting is lost.
> 
> Here is the test order in main():
> 
>                   set_xdp,
>                   enabled_set_xdp,
>                   ioctl,
>                   ioctl_set_xdp,
>                   ioctl_enabled_set_xdp],
> 
> In a default full run, enabled_set_xdp() has already left the device at
> "unknown" before ioctl_set_xdp() starts. The new _defer_reset_hds() call
> therefore only saves that already-reset state. ioctl_enabled_set_xdp()
> runs last and leaves the device at "unknown" again.
> 
> Doesn't that mean this change only has a visible effect when
> ioctl_set_xdp is run by itself with -t?
> 
> The previous patch in this series, "selftests: drv-net: hds: perturb
> settings via ioctl in ioctl_enabled_set_xdp", edited
> ioctl_enabled_set_xdp() but kept its fixed "unknown" reset.
> 
> Could enabled_set_xdp() and ioctl_enabled_set_xdp() also call
> _defer_reset_hds(cfg, netnl) before they set 'enabled'?
> 

It's true that there is no visible effect unless the test is run standalone
or with a non-default config, but the point of the patch was to bring this
in line with set_xdp().

I'd leave the fixed "unknown" resets as is. _defer_reset_hds() handles some
additional scenarios, but as in my reply on patch 1, netlink reports state
rather than config, so no reset can cover all cases.

> [Severity: Low]
> Should this patch carry a Fixes: tag? Commit ee3ae27721fb
> ("selftests: drv-net: hds: restore hds settings") added _defer_reset_hds()
> to set_xdp() and the set_hds_* tests, but not to ioctl_set_xdp().
> Something like:
> 
> Fixes: ee3ae27721fb ("selftests: drv-net: hds: restore hds settings")
> 
> The test itself was added by 29b036be1b0b ("selftests: drv-net: test XDP,
> HDS auto and the ioctl path"), which would also be a reasonable target.
> 

This change is not critical enough for a Fixes tag.

Re: [PATCH net-next 0/3] selftests: drv-net: hds: fix state leaks and ioctl tests

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu,  5 Nov 2026 19:12:05 -0800 you wrote:
> From: Dimitri Daskalakis <daskald@meta.com>
> 
> Fix hds.py leaving tcp-data-split in a non-default state on drivers
> that report HDS state rather than config, and fix the ioctl XDP tests.
> 
> Dimitri Daskalakis (3):
>   selftests: drv-net: hds: always restore tcp-data-split to auto
>   selftests: drv-net: hds: perturb settings via ioctl in
>     ioctl_enabled_set_xdp
>   selftests: drv-net: hds: restore HDS settings in ioctl_set_xdp
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] selftests: drv-net: hds: always restore tcp-data-split to auto
    https://git.kernel.org/netdev/net-next/c/c1e042cb4de2
  - [net-next,2/3] selftests: drv-net: hds: perturb settings via ioctl in ioctl_enabled_set_xdp
    https://git.kernel.org/netdev/net-next/c/2839e04a9f76
  - [net-next,3/3] selftests: drv-net: hds: restore HDS settings in ioctl_set_xdp
    https://git.kernel.org/netdev/net-next/c/f4085a199ebd

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html