Re: [PATCH net-next v3 8/8] selftests: drv-net: add netkit devmem tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] If configure_nic(cfg) fails halfway, the script exits without executing cleanup_nic(cfg), permanently leaving the physical NIC in a disrupted state.
- [High] The script performs highly disruptive physical NIC configuration changes unconditionally, bypassing the framework's DISRUPTIVE=False skip mechanism.
--

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

selftests: drv-net: add netkit devmem tests

This commit adds nk_devmem.py with four tests for TCP devmem through a netkit
device. The script configures the NIC once before running the test cases and
cleans it up in a finally block to avoid toggling settings around every test.

> diff --git a/tools/testing/selftests/drivers/net/hw/nk_devmem.py b/tools/testing/selftests/drivers/net/hw/nk_devmem.py
> new file mode 100755
> index 0000000000000..0e36a0fa96888
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/nk_devmem.py

[ ... ]

> +def main() -> None:
> +    """Configure the NIC once, then run the netkit devmem test cases."""
> +    with NetDrvContEnv(__file__, rxqueues=2, primary_rx_redirect=True) as cfg:
> +        setup_test(cfg,
> +                   os.path.join(os.path.dirname(os.path.abspath(__file__)),
> +                                "ncdevmem"))
> +
> +        require_devmem(cfg)
> +        configure_nic(cfg)

Because configure_nic(cfg) is called here in main() outside of ksft_run(),
does this bypass the DISRUPTIVE=False skip mechanism?

Since the test functions themselves are marked with @ksft_disruptive, running
with DISRUPTIVE=False will skip the tests, but it appears the disruptive NIC
configuration will still unconditionally execute.

> +        try:

If configure_nic(cfg) fails halfway through, will this permanently leave the
physical NIC in a disrupted state?

If an exception is raised before the try block is entered (for example, if
the driver doesn't support queue leases during configuration),
cleanup_nic(cfg) would be entirely bypassed.

Could configure_nic() be moved inside the try block to ensure cleanup
always runs?

> +            ksft_run([check_nk_rx, check_nk_tx, check_nk_tx_chunks,
> +                      check_nk_rx_hds], args=(cfg,))
> +        finally:
> +            cleanup_nic(cfg)
> +
> +    ksft_exit()

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507-tcp-dm-netkit-v3-0-52821445867c@meta.com?part=8