Re: [PATCH net-next v4 6/8] selftests: drv-net: refactor devmem command builders into lib module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] NIC ring and RSS configurations are permanently leaked on the physical host if `configure_nic()` encounters an error midway through execution.
- [Low] The newly created shared helper file `lib/py/devmem.py` is missing from `TEST_FILES` in the subsystem Makefile.
--

commit c5e5518c4c4e4b7d059077276ec59958fe094dd8
Author: Bobby Eshleman <bobbyeshleman@meta.com>

selftests: drv-net: refactor devmem command builders into lib module

This commit refactors netkit-based devmem test commands into a shared library
module. This allows the command builders to be reused transparently across
both plain and netkit environments while keeping a shared environment across
test runs.

> diff --git a/tools/testing/selftests/drivers/net/hw/lib/py/devmem.py b/tools/testing/selftests/drivers/net/hw/lib/py/devmem.py
> new file mode 100644
> index 0000000000000..d3e7a3645cba5
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/lib/py/devmem.py

This isn't a bug, but should this new shared helper file be added to
TEST_FILES in the subsystem Makefile? 

According to the selftests documentation, any file referenced via import in
test scripts must be added to TEST_FILES to ensure it is installed correctly.

[ ... ]

> +def configure_nic(cfg):
> +    """Channels, rings, RSS, queue lease for netkit devmem."""
> +    cfg.require_ipver('6')
> +    ethnl = EthtoolFamily()

[ ... ]

> +    ethnl.rings_set({'header': {'dev-index': cfg.ifindex},
> +                     'tcp-data-split': 'enabled',
> +                     'hds-thresh': 0,
> +                     'rx': min(64, cfg.orig_rx_rings)})
> +
> +    cfg.src_queue = channels - 1
> +    ethtool(f"-X {cfg.ifname} equal {cfg.src_queue}")
> +
> +    with NetNSEnter(str(cfg.netns)):
> +        netdevnl = NetdevFamily()
> +        lease_result = netdevnl.queue_create({
> +            "ifindex": cfg.nk_guest_ifindex,
> +            "type": "rx",
> +            "lease": {
> +                "ifindex": cfg.ifindex,
> +                "queue": {"id": cfg.src_queue, "type": "rx"},
> +                "netns-id": 0,
> +            },
> +        })
> +        cfg.nk_queue = lease_result['id']

If queue_create() or the ethtool command fails and raises an exception, does
this leave the physical NIC rings and RSS configuration permanently modified?

Since the cleanup_nic() function wouldn't be called if this setup fails
midway, would it make sense to handle exceptions here to restore the
original ring and RSS settings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-tcp-dm-netkit-v4-0-841b78b99d74@meta.com?part=6