Re: [PATCH net 1/2] selftests: drv-net: ring_reconfig: handle devices without channel config

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] _configure_min_ring_cnt() in
  tools/testing/selftests/drivers/net/ring_reconfig.py now catches…

--

selftests: drv-net: ring_reconfig: handle devices without channel config

This patch handles drivers that don't implement get_channels (e.g. e1000e)
in ring_reconfig.py. When channels_get returns EOPNOTSUPP, channels() is
now skipped and _configure_min_ring_cnt() returns early. Other netlink
errors are still reported as failures.

> diff --git a/tools/testing/selftests/drivers/net/ring_reconfig.py b/tools/testing/selftests/drivers/net/ring_reconfig.py
> index 6682d4badfbfc..baac20e8d3eba 100755
> --- a/tools/testing/selftests/drivers/net/ring_reconfig.py
> +++ b/tools/testing/selftests/drivers/net/ring_reconfig.py

[ ... ]

> @@ -88,7 +99,14 @@ def channels(cfg) -> None:
>  def _configure_min_ring_cnt(cfg) -> None:
>      """ Try to configure a single Rx/Tx ring. """
>      ehdr = {'header':{'dev-index': cfg.ifindex}}
> -    chans = cfg.eth.channels_get(ehdr)
> +    try:
> +        chans = cfg.eth.channels_get(ehdr)
> +    except NlError as e:
> +        # Reducing the channel count is only an optimization
> +        if e.error == errno.EOPNOTSUPP:
> +            ksft_pr("Channel configuration unsupported; continuing ring test")
> +            return
> +        raise

[Severity: Medium]
This catches EOPNOTSUPP from channels_get, but what about drivers that
implement get_channels but not set_channels? Further down in
_configure_min_ring_cnt() the set call is still unguarded:

    if chans.get('combined-count', 0) > 1:
        config['combined-count'] = 1
    elif chans.get('rx-count', 0) > 1 and chans.get('tx-count', 0) > 1:
        ...

    cfg.eth.channels_set(ehdr | config)
    defer(cfg.eth.channels_set, ehdr | restore)

On the kernel side, CHANNELS_SET is rejected unless both ops are present:

net/ethtool/channels.c:ethnl_set_channels_validate() {
    ...
	return ops->get_channels && ops->set_channels ? 1 : -EOPNOTSUPP;
}

vmxnet3 is one in-tree example. Its ethtool ops set only
.get_channels = vmxnet3_get_channels, and in MSI-X buddy-share mode
vmxnet3_get_channels() reports combined_count = num_tx_queues, which can
be greater than 1.

On such a device channels_get succeeds, so the new early return never
runs. channels_set then raises NlError(95), which propagates out of
_configure_min_ring_cnt() and ringparam(). That means ringparam() fails
before any ring size is tested, which is the symptom the commit message
says this patch fixes.

The comment here says reducing the channel count is only an optimization.
Wouldn't that reasoning also apply to a failed channels_set? Could the
channels_set call get the same handling, i.e. log with ksft_pr() and
return on EOPNOTSUPP before registering the defer?

The later patch in this series ("selftests: drv-net: ring_reconfig: skip
tx stall test without channel config") only changes reconfig_tx_stall(),
so this case still fails after the full series is applied.

>  
>      all_keys = ["rx-count", "tx-count", "combined-count"]
>      restore = {}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106150509.30458-1-ahmetbilalyazicioglu%40gmail.com