Re: [PATCH net-next 09/11] selftests: drv-net: refactor devmem command builders into lib module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing TEST_FILES entry for new python library file.
- [Medium] Uncaught KsftSkipEx exception causes test script crashes instead of graceful skips.
- [Medium] Silent test coverage loss: payload validation and hardware flow steering are disabled for the plain environment.
--

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

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

Adding netkit-based devmem tests is a straight-forward copy of devmem
test commands plus some args for the nk cases, so this patch breaks out
these command builders into helpers used by both.

> diff --git a/tools/testing/selftests/drivers/net/hw/devmem.py b/tools/testing/selftests/drivers/net/hw/devmem.py
> index ee863e90d1e0..33648e39577a 100755
> --- a/tools/testing/selftests/drivers/net/hw/devmem.py
> +++ b/tools/testing/selftests/drivers/net/hw/devmem.py

[ ... ]

> @@ -76,14 +42,9 @@ def check_rx_hds(cfg) -> None:
>  
>  def main() -> None:
>      with NetDrvEpEnv(__file__) as cfg:
> -        cfg.bin_local = path.abspath(path.dirname(__file__) + "/ncdevmem")
> -        cfg.bin_remote = cfg.remote.deploy(cfg.bin_local)
> -
> +        setup_test(cfg, path.abspath(path.dirname(__file__) + "/ncdevmem"))
>          ksft_run([check_rx, check_tx, check_tx_chunks, check_rx_hds],
> -                 args=(cfg, ))
>      ksft_exit()

By calling setup_test() directly in main() before ksft_run(), does this
prevent ksft_run() from catching the KsftSkipEx exception that is now
raised inside setup_test()?

Previously, the require_devmem() check was inside individual test cases,
where ksft_run() could safely intercept the exception and emit a standard TAP
SKIP message. It seems this might now abort the script with a Python
traceback on systems where devmem is unsupported.

> 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 000000000000..e95fc38337fa
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/lib/py/devmem.py
> @@ -0,0 +1,215 @@
> +# SPDX-License-Identifier: GPL-2.0
> +"""Shared helpers for devmem TCP selftests."""

Since this patch creates a new shared Python utility file that is imported
by test scripts, does it need to be added to TEST_FILES in the subsystem
Makefile?

Without a Makefile update, tests may fail with a "No such file or directory"
error when executed from an installed location after running make install.

[ ... ]

> +def ncdevmem_rx(cfg, port, verify=True, fail_on_linear=False):
> +    if hasattr(cfg, 'netns'):
> +        flow_rule_id = set_flow_rule(cfg, port)
> +        defer(ethtool, f"-N {cfg.ifname} delete {flow_rule_id}")
> +
> +        ifname = cfg._nk_guest_ifname
> +        addr = cfg.nk_guest_ipv6
> +        extras = f" -t {cfg.nk_queue} -q 1 -n"
> +        if verify:
> +            extras += " -v 7"
> +    else:
> +        ifname = cfg.ifname
> +        addr = cfg.addr
> +        extras = ""

The commit message notes there are no functional changes to the existing tests,
but in this else branch for the non-netns environment, extras is
unconditionally set to an empty string.

Does this ignore the verify=True argument and silently disable the "-v 7"
validation flag, skipping payload validation?

[ ... ]

> +def run_rx(cfg):
> +    if hasattr(cfg, 'netns'):
> +        configure_nic(cfg)
> +    port = rand_port()
> +    socat = socat_send(cfg, port)
> +    data_pipe = (f"yes $(echo -e \x01\x02\x03\x04\x05\x06) | head -c 1K"
> +                 f" | {socat}")
> +    ns = getattr(cfg, "netns", None)
> +
> +    listen_cmd = ncdevmem_rx(cfg, port)
> +    with bkg(listen_cmd, exit_wait=True, ns=ns) as ncdevmem:

In the original check_rx() test, the listen command included
"-c {cfg.remote_addr}" and socat used the "bind" argument.

Are those arguments intentionally omitted here? It looks like dropping them
removes the 5-tuple hardware flow steering setup present in the original test.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428-tcp-dm-netkit-v1-0-719280eba4d2@meta.com?part=9