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

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Wed, May 13, 2026 at 07:07:07PM +0000, sashiko-bot@kernel.org wrote:
> 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)
> 

Will address all of this in next rev.

Best,
Bobby