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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Uncaught `KsftSkipEx` exceptions in `main()` cause the test to fail instead of reporting as skipped.
- [High] Global NIC configuration changes are leaked if the setup phase fails, breaking exception safety and corrupting the host NIC state.
- [Medium] The commit message incorrectly claims that `nk_qlease.py` configures the NIC once in `main()` to avoid re-toggling settings.
--

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

selftests: drv-net: add netkit devmem tests

Add nk_devmem.py with four tests for TCP devmem through a netkit device.

> The NIC (HDS, RSS, queue lease) is configured once in main() before
> ksft_run() and torn down in a finally block via cleanup_nic(), mirroring
> the nk_qlease.py pattern.

Is this description accurate? Looking at nk_qlease.py, it does not configure
NIC settings in main(). Instead, it configures and re-toggles settings like
rings and RSS within individual test cases using defer() for cleanup.

Could the commit message be updated to reflect this accurately?

> 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
> @@ -0,0 +1,55 @@
[ ... ]
> +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)

If require_devmem() or configure_nic() raises KsftSkipEx because the
environment lacks devmem support or sufficient NIC channels, will this cause
the Python script to crash with an unhandled exception instead of cleanly
skipping?

Because these functions are called outside the ksft_run() test runner loop,
it appears KsftSkipEx won't be caught, resulting in a non-zero exit code
instead of Kselftest's expected SKIP exit code (4).

Furthermore, if configure_nic() raises an exception halfway through its
modifications (for example, if an ethtool command fails after initial ring
modifications), will this leak global NIC configuration changes?

Since configure_nic() is called before the try...finally block, an exception
here would cause the script to terminate immediately. This bypasses
cleanup_nic() and leaves the host NIC permanently modified.

Should configure_nic() be moved inside the try block to ensure cleanup_nic()
is always executed?

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

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