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

[Dimitri Daskalakis <dimitri.daskalakis1@gmail.com>]

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.