Re: [PATCH net v2] selftests: drv-net: pause: Avoid cleanup failures for unsupported link settings

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] The fix is incomplete: the same cleanup failure still happens on
  the link-autoneg-on branch of pause_setup() in…
- [Low] In pause_setup()
  (tools/testing/selftests/drivers/net/hw/pause.py, local lines 410-414,…

--

selftests: drv-net: pause: Avoid cleanup failures for unsupported link settings

This patch changes pause_setup() so that the forced-link restore is queued
only after setup has successfully enabled link autonegotiation. The goal is
to avoid EOPNOTSUPP cleanup failures on devices such as fbnic. It also makes
pause_test_support() fall back to checking forced MAC pause on the current
link when no link configuration change is accepted.

> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> index ca4a55e59c30f..99ce6a59dfcd6 100755
> --- a/tools/testing/selftests/drivers/net/hw/pause.py
> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
> @@ -404,14 +404,14 @@ def pause_setup(cfg):
>  
>      if link["auto-negotiation"]:
>          defer(cmd, f"ethtool -s {cfg.ifname} autoneg on")

[Severity: High]
Can the same cleanup failure still happen on this branch?

This restore is still queued whenever the device reports link autoneg on,
even when setup never changes the link. The remote interface has the same
unconditional defer further down in pause_setup().

When the driver has no set_link_ksettings, ethnl_set_linkmodes_validate()
rejects every LINKMODES_SET before it checks for a no-op:

net/ethtool/linkmodes.c:ethnl_set_linkmodes_validate() {
    ...
	if (!ops->get_link_ksettings || !ops->set_link_ksettings)
		return -EOPNOTSUPP;
    ...
}

That means a restore to the same autoneg on value also fails with
EOPNOTSUPP.

Some in-tree drivers report autoneg on and provide only get_link_ksettings.
nicvf_get_link_ksettings() sets cmd->base.autoneg = AUTONEG_ENABLE.
be_get_link_ksettings() does the same when auto speeds are supported.
fbnic also has get_link_ksettings and no set_link_ksettings.

On these devices pause_test_support() takes the new "current link"
fallback. The deferred "ethtool -s <if> autoneg on" then fails in
ksft_flush_defer() and the test ends in a cleanup failure. The subject
line says the patch avoids exactly this.

Every other test that calls pause_setup() would hit it too, because the
defer is flushed even when the test skips.

Dropping the autoneg on restore isn't an option, since pause_test_support()
and pause_autoneg_link_autoneg() force the link and rely on it. Would it
work to queue the restore only where a link change is accepted? One place
is after a successful "autoneg off {forced}" in pause_test_support().
Another option is to let the restore tolerate EOPNOTSUPP.

>  
>      # Local interface admin up, link aneg on
>      ip(f"link set {cfg.ifname} up")
>      if link["supports-auto-negotiation"] and not link["auto-negotiation"]:
>          ethtool(f"-s {cfg.ifname} autoneg on")
> +        if "speed" in link and "duplex" in link:
> +            defer(cmd, f"ethtool -s {cfg.ifname} autoneg off speed {link['speed']} "
> +                       f"duplex {link['duplex'].lower()}")

[Severity: Low]
Could this leave a forced link in autoneg state if the ethtool call fails
partway through?

ethtool() raises on a non-zero exit. With the defer now registered after
the call, nothing restores the original forced speed/duplex if the driver
commits part of the change and then returns an error.

bnxt_set_link_ksettings() is one example. It sets
link_info->autoneg |= BNXT_AUTONEG_SPEED before calling
bnxt_hwrm_set_link_setting(), and returns that call's error without
rolling back.

This branch only runs when the device supports autoneg. Would queuing the
restore just before the ethtool call still avoid the fbnic cleanup
failure? The same ordering applies to the remote interface later in
pause_setup().

[ ... ]

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