Re: [PATCH net-next v2 3/3] selftests: drv-net: Add VLAN test
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-22 12:01:52
Also in:
linux-kselftest, lkml
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider. Critical: 0 · High: 0 · Medium: 2 · Low: 3 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] New configuration knobs VLAN_LOCAL_V4 / VLAN_REMOTE_V4 in tools/testing/selftests/drivers/net/hw/vlan.py (lines 48-49) are read… - [Medium] _setup() reprograms RX VLAN stripping via set_ethtool_feat() (tools/testing/selftests/drivers/net/hw/vlan.py:91) and then… - [Low] Concern (assessed as a false positive, retained for policy combination): in _setup()… - [Low] The only traffic validation in test() (tools/testing/selftests/drivers/net/hw/vlan.py:115) is 'ping -c 1 -W… - [Low] Documentation/implementation mismatch: the commit message and the module docstring… -- selftests: drv-net: Add VLAN test This adds drivers/net/hw/vlan.py, which builds 802.1q, 802.1ad and QinQ VLAN stacks on the device under test and its remote endpoint, then pings across them with hardware RX VLAN stripping enabled and disabled. Six variants are registered, and the test is added to TEST_PROGS along with CONFIG_VLAN_8021Q in the hw config. No Link: tags present in the commit header.
quoted hunk ↗ jump to hunk
diff --git a/tools/testing/selftests/drivers/net/hw/vlan.py b/tools/testing/selftests/drivers/net/hw/vlan.py new file mode 100755 index 000000000000..00c90b930f67 --- /dev/null +++ b/tools/testing/selftests/drivers/net/hw/vlan.py@@ -0,0 +1,127 @@
[ ... ]
quoted hunk ↗ jump to hunk
+OUTER_DEV = f"vlout{os.getpid()}" +INNER_DEV = f"vlin{os.getpid()}" + +OUTER_VID = 100 +INNER_VID = 200 + +LOCAL_IP = os.environ.get("VLAN_LOCAL_V4", "198.51.100.1") +REMOTE_IP = os.environ.get("VLAN_REMOTE_V4", "198.51.100.2")
[Severity: Medium]
Are VLAN_LOCAL_V4 and VLAN_REMOTE_V4 reachable when the harness is
configured through net.config?
These two lookups run at module import time against os.environ, but
NetDrvEnvBase._load_env_file() in drivers/net/lib/py/env.py merges
net.config entries into a copy and exposes them as cfg.env only:
env = os.environ.copy()
...
env[pair[0]] = pair[1]
return ksft_setup(env)
os.environ is never updated, so a VLAN_LOCAL_V4/VLAN_REMOTE_V4 line in
drivers/net/hw/net.config is silently ignored and the hard-coded
198.51.100.1/198.51.100.2 defaults are used instead. Would reading these
from cfg.env inside the test body work better, given that
drivers/net/README.rst states "The variables can be set in the
environment or by creating a net.config file in the same directory as
this README file"?
The two new variables are also missing from the "Variables" section of
drivers/net/README.rst, which lists NETIF, LOCAL_V4/V6, REMOTE_V4/V6,
LOCAL_PREFIX_V6, REMOTE_TYPE and REMOTE_ARGS. Should they be documented
there as well?
[ ... ]
quoted hunk ↗ jump to hunk
+def _setup(cfg, outer_proto, inner_proto, hw_strip): + """Configure VLAN stripping and create the VLAN interfaces.""" + + names = ["rx-vlan-offload"] + if outer_proto == "802.1ad": + names.append("rx-vlan-stag-hw-parse")
[Severity: Low] The commit message and the module docstring say the "sw" test variants "disable all of them", referring to rx-vlan-offload and rx-vlan-stag-hw-parse. Here rx-vlan-stag-hw-parse is only appended when outer_proto is "802.1ad", and the same list is used for both directions. For the 8021q_sw variant on a device where rx-vlan-stag-hw-parse is active, the feature is left as-is, so the "all HW RX stripping off" baseline the docstring promises is not established. Should the wording be narrowed, or should names include every supported RX stripping feature when hw_strip is False?
quoted hunk ↗ jump to hunk
+ + feat = ethtool(f"-k {cfg.ifname}", json=True)[0] + feats = {} + for name in names: + # hw: skip the features the device does not support + # sw: request all of them off + if hw_strip and feat[name]["fixed"] and not feat[name]["active"]: + continue + feats[name] = hw_strip + + if hw_strip and not feats: + raise KsftXfailEx("Device does not support RX VLAN stripping")
[Severity: Low]
This isn't a bug, but on a device where rx-vlan-offload is changeable and
rx-vlan-stag-hw-parse is fixed-off, 8021ad_hw and qinq_hw run with only
C-tag stripping enabled and report "ok", with nothing in the output
saying which features were actually applied.
The skipping itself matches what the docstring describes, and genuinely
impossible requests still xfail through set_ethtool_feat() in
drivers/net/lib/py/feat.py:
if current[name]["fixed"]:
raise KsftXfailEx(f"Device does not support {name}")
Would it be worth printing the resulting feats dict via ksft_pr() so the
reported pass records which stripping path was exercised?
quoted hunk ↗ jump to hunk
+ + set_ethtool_feat(cfg.ifname, feat, feats) + + _vlan_setup(cfg.ifname, LOCAL_IP, outer_proto, inner_proto) + _vlan_setup(cfg.remote_ifname, REMOTE_IP, outer_proto, inner_proto, + host=cfg.remote)
[Severity: Medium] Can toggling rx-vlan-offload here make the following ping flaky on hardware that re-initialises the datapath? Some drivers implement ndo_set_features for NETIF_F_HW_VLAN_CTAG_RX with a full down/up. e1000_set_features() in drivers/net/ethernet/intel/e1000e/netdev.c keeps CTAG_RX in its changed mask: if (!(changed & (NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_RXCSUM | NETIF_F_RXHASH | NETIF_F_RXFCS | NETIF_F_RXALL))) return 0; ... if (netif_running(netdev)) e1000e_reinit_locked(adapter); so the call chain is set_ethtool_feat() -> e1000_set_features() -> e1000e_reinit_locked() -> e1000e_down() (carrier off) + e1000e_up() with PHY re-autoneg. _setup() then creates the VLANs and test() sends a single echo request without waiting for carrier, so ARP plus the 5 s reply window can expire inside the re-link. The deferred feature restore between variants has the same exposure for the next variant. Would a carrier wait (as drivers/net/lib/py/env.py does on /sys/class/net/<dev>/carrier) or a retry loop like the one in drivers/net/hw/rss_ctx.py be appropriate here?
quoted hunk ↗ jump to hunk
+ + +def _vlan_variants():
[ ... ]
quoted hunk ↗ jump to hunk
+@ksft_variants(_vlan_variants()) +def test(cfg, outer_proto, inner_proto, hw_strip): + """Run a single VLAN test""" + + _setup(cfg, outer_proto, inner_proto, hw_strip) + + cmd(f"ping -c 1 -W 5 {REMOTE_IP}")
[Severity: Low]
Is route selection guaranteed to send this ping out of the VLAN device?
_vlan_setup() installs the address with "addr add {addr}/24 dev {dev}" on
the innermost VLAN, and this ping is not bound to an interface. If
cfg.ifname or any other interface on the DUT already carries an address
inside 198.51.100.0/24, two connected routes for the same prefix exist
and the pre-existing one can win, in which case the ICMP exchange leaves
untagged and the test still reports "ok" without exercising VLAN
tagging or stripping at all.
The reverse case is that installing 198.51.100.1/24 on the VLAN hijacks
the prefix used by a REMOTE_TYPE=ssh control channel living in it.
drivers/net/macsec.py does the same VLAN-on-top-of-an-offload-device
dance with the same addresses and binds explicitly:
cmd(f"ping -I {vlan_name} -c 1 -W 5 {remote_ip}")
Would adding -I for the innermost VLAN device here be preferable?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918112529.96039-1-ovidiu.panait.rb%40renesas.com