Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · 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] In gro_lib._setup() the "hw" mode passes check=False to
set_ethtool_feat(), which disables the newly added read-back for the…
- [Medium] The read-back verification added to the shared helper
set_ethtool_feat() (drivers/net/lib/py/feat.py:40-43) is not mirrored…
--
selftests: drv-net: Check the features set by set_ethtool_feat()
This adds a read-back of the device features after running ethtool -K in
set_ethtool_feat(), raising KsftXfailEx when the requested state was not
applied. A new check parameter allows callers to skip the verification, and
the GRO "hw" setup mode in gro_lib.py uses it.
> diff --git a/tools/testing/selftests/drivers/net/gro_lib.py
> b/tools/testing/selftests/drivers/net/gro_lib.py
> index 7856ebdd49d3..875a715b2ae1 100644
> --- a/tools/testing/selftests/drivers/net/gro_lib.py
> +++ b/tools/testing/selftests/drivers/net/gro_lib.py
> @@ -263,7 +263,8 @@ def _setup(cfg, mode, test_name):
> set_ethtool_feat(cfg.ifname, cfg.feat,
> {"generic-receive-offload": False,
> "rx-gro-hw": True,
> - "large-receive-offload": False})
> + "large-receive-offload": False},
> + check=False)
[Severity: Medium]
The commit message justifies skipping the check only for rx-gro-hw ("the
GRO "hw" mode handles the case where HW GRO is cleared by the driver"), but
check=False disables verification for all three features in this request.
The follow-up read-back in _setup() only looks at rx-gro-hw:
feat = ethtool(f"-k {cfg.ifname}", json=True)[0]
if not feat["rx-gro-hw"]["active"]:
ksft_pr("Driver clears HW GRO and SW GRO is cleared, using generic
XDP workaround")
so generic-receive-offload is never verified in this path. If a driver
accepts rx-gro-hw but leaves SW GRO active, the XDP generic workaround
branch is not taken (it only runs when rx-gro-hw is inactive) and the
gro_hw.py tests then run with SW GRO enabled, which can coalesce in
software and pass even when the hardware offload does nothing. Isn't that
the same "test could run with a different configuration than the requested
one" case the patch is meant to catch?
Note that for the third feature the kernel does enforce the requested
state, netdev_fix_features() drops LRO when HW GRO is requested:
if ((features & NETIF_F_GRO_HW) && (features & NETIF_F_LRO)) {
netdev_dbg(dev, "Dropping LRO feature since HW-GRO is
requested.\n");
features &= ~NETIF_F_LRO;
but there is no equivalent rule tying generic GRO to HW GRO. Would a
per-feature exemption (skip verification of rx-gro-hw only) instead of the
all-or-nothing boolean work here?
> diff --git a/tools/testing/selftests/drivers/net/lib/py/feat.py
> b/tools/testing/selftests/drivers/net/lib/py/feat.py
> index b94a24214e59..f0630844f023 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/feat.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/feat.py
> @@ -33,3 +33,11 @@ def set_ethtool_feat(dev, current, feats, host=None):
> # If ethtool printed something kernel must have modified some features
> if eth_cmd.stdout:
> ksft_pr(eth_cmd)
> +
> + if not check:
> + return
> +
> + current = ethtool(f"-k {dev}", json=True, host=host)[0]
> + for name, state in feats.items():
> + if current[name]["active"] != state:
> + raise KsftXfailEx(f"{name} was not set to {s2n[state]}")
[Severity: Medium]
Should the private copy of this helper in
tools/testing/selftests/drivers/net/hw/gro_stats.py be converted as well?
After this series the tree carries two copies of the same ethtool -K helper
with different semantics. _set_ethtool_feat() in gro_stats.py still ends
with no read-back:
eth_cmd = ethtool(" ".join(new))
defer(ethtool, " ".join(old))
# If ethtool printed something kernel must have modified some features
if eth_cmd.stdout:
ksft_pr(eth_cmd)
and it raises KsftSkipEx for fixed features where the shared helper raises
KsftXfailEx.
_setup_hw_gro() in gro_stats.py repeats the gro_lib.py "hw" logic including
the XDP generic workaround, and its final request is unverified:
_set_ethtool_feat(cfg, feat,
{"generic-receive-offload": True,
"rx-gro-hw": True,
"large-receive-offload": False})
so the HW GRO qstats assertions that follow can run with rx-gro-hw
inactive, and the resulting delta mismatch gives no hint about the real
cause. drivers/net/hw/lib/py/__init__.py already imports and re-exports
the shared set_ethtool_feat, so is there a reason the consolidation done in
the earlier "selftests: drv-net: Move _set_ethtool_feat() into lib" patch
stops short of this file?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918112529.96039-1-ovidiu.panait.rb%40renesas.com
