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

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

[Mohsin Bashir]

From: Mohsin Bashir <hmohsin@meta.com>

pause_setup() saves the initial link settings and queues a deferred
restore. For devices that report forced link settings but do not support
changing link settings, this can queue a restore even when setup never
changes the link. On fbnic, the deferred restore returns EOPNOTSUPP and
turns pause tests that use pause_setup() into cleanup failures.

Fix this by queuing the forced-link restore only after setup successfully
enables link autonegotiation. Keep pause_test_support coverage for devices
that cannot change link settings by falling back to forced MAC pause on the
current link. Run the extra link autoneg on/off checks only when those link
setting requests are accepted.

Results on fbnic:

  Before: # Totals: pass:0 fail:4 xfail:0 xpass:0 skip:24 error:0
  After:  # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:24 error:0

Fixes: cbd8a536cd6d ("selftests: drv-net: Introduce a selftest for ethtool flow control")
Signed-off-by: Mohsin Bashir <hmohsin@meta.com>
---
v2:
- Drop the require_link_autoneg() approach from v1.
- Move the cleanup fix into pause_setup() so all pause tests avoid
  restoring unchanged forced-link settings.
- Keep pause_test_support() coverage on devices without link setting
  support by falling back to forced MAC pause on the current link.

v1: https://lore.kernel.org/netdev/20261030173516.1002250-1-mohsin.bashr@gmail.com
---
 .../testing/selftests/drivers/net/hw/pause.py | 40 +++++++++++--------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
index ca4a55e59c30..99ce6a59dfcd 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")
-    elif "speed" in link and "duplex" in link:
-        defer(cmd, f"ethtool -s {cfg.ifname} autoneg off speed {link['speed']} "
-                   f"duplex {link['duplex'].lower()}")
 
     # 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()}")
 
     # Get remote pause params
     ret, params = ethtool_ret(f"-a {cfg.remote_ifname}", host=cfg.remote)
@@ -429,15 +429,15 @@ def pause_setup(cfg):
     if link["auto-negotiation"]:
         defer(cmd, f"ethtool -s {cfg.remote_ifname} autoneg on",
               host=cfg.remote)
-    elif "speed" in link and "duplex" in link:
-        defer(cmd, f"ethtool -s {cfg.remote_ifname} autoneg off "
-                   f"speed {link['speed']} duplex {link['duplex'].lower()}",
-                   host=cfg.remote)
 
     # Remote interface admin up, link aneg on
     ip(f"link set {cfg.remote_ifname} up", host=cfg.remote)
     if link["supports-auto-negotiation"] and not link["auto-negotiation"]:
         ethtool(f"-s {cfg.remote_ifname} autoneg on", host=cfg.remote)
+        if "speed" in link and "duplex" in link:
+            defer(cmd, f"ethtool -s {cfg.remote_ifname} autoneg off "
+                       f"speed {link['speed']} duplex {link['duplex'].lower()}",
+                       host=cfg.remote)
 
     wait_for_link(cfg)
 
@@ -470,9 +470,12 @@ def pause_test_support(cfg, pauseparams):
 
     forced = forced_link_settings(cfg)
     _, supported = get_local_pause_supported(cfg)
+    tested = False
 
     # We check that what we can configure in the pause params matches what we
-    # support under various contditions : Link aneg on/off, pause aneg on/off
+    # support under various conditions: link aneg on/off, and pause aneg
+    # on/off. If no link configuration change is supported, fall back to
+    # checking forced MAC pause on the current link.
     ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg on", is_get = False)
     if ret != 0:
         ksft_pr("link autoneg on refused, not tested")
@@ -481,18 +484,21 @@ def pause_test_support(cfg, pauseparams):
                                "link autoneg on")
         expect_pauseparams_set(cfg, pauseparams, True, supported,
                                "link autoneg on")
+        tested = True
 
     if not forced:
         ksft_pr("link speed unknown, the forced link is not tested")
-        return
-
-    ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg off {forced}", is_get = False)
-    if ret != 0:
-        ksft_pr(f"link autoneg off {forced} refused, not tested")
-        return
-
-    expect_pauseparams_set(cfg, pauseparams, False, supported, "link autoneg off")
-    expect_pauseparams_set(cfg, pauseparams, True, supported, "link autoneg off")
+    else:
+        ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg off {forced}", is_get = False)
+        if ret != 0:
+            ksft_pr(f"link autoneg off {forced} refused, not tested")
+        else:
+            expect_pauseparams_set(cfg, pauseparams, False, supported, "link autoneg off")
+            expect_pauseparams_set(cfg, pauseparams, True, supported, "link autoneg off")
+            tested = True
+
+    if not tested:
+        expect_pauseparams_set(cfg, pauseparams, False, supported, "current link")
 
 @ksft_variants(pauseparams_variants)
 @ksft_disruptive
-- 
2.53.0-Meta

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

[Maxime Chevallier]

Hi,

On 11/3/26 20:06, Mohsin Bashir wrote:
> From: Mohsin Bashir <hmohsin@meta.com>
> 
> pause_setup() saves the initial link settings and queues a deferred
> restore. For devices that report forced link settings but do not support
> changing link settings, this can queue a restore even when setup never
> changes the link. On fbnic, the deferred restore returns EOPNOTSUPP and
> turns pause tests that use pause_setup() into cleanup failures.
> 
> Fix this by queuing the forced-link restore only after setup successfully
> enables link autonegotiation. Keep pause_test_support coverage for devices
> that cannot change link settings by falling back to forced MAC pause on the
> current link. Run the extra link autoneg on/off checks only when those link
> setting requests are accepted.
> 
> Results on fbnic:
> 
>   Before: # Totals: pass:0 fail:4 xfail:0 xpass:0 skip:24 error:0
>   After:  # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:24 error:0
> 
> Fixes: cbd8a536cd6d ("selftests: drv-net: Introduce a selftest for ethtool flow control")
> Signed-off-by: Mohsin Bashir <hmohsin@meta.com>

That looks better, this patch ran on the netdev CI and no regressions on
stmmac,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
> v2:
> - Drop the require_link_autoneg() approach from v1.
> - Move the cleanup fix into pause_setup() so all pause tests avoid
>   restoring unchanged forced-link settings.
> - Keep pause_test_support() coverage on devices without link setting
>   support by falling back to forced MAC pause on the current link.
> 
> v1: https://lore.kernel.org/netdev/20261030173516.1002250-1-mohsin.bashr@gmail.com
> ---
>  .../testing/selftests/drivers/net/hw/pause.py | 40 +++++++++++--------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> index ca4a55e59c30..99ce6a59dfcd 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")
> -    elif "speed" in link and "duplex" in link:
> -        defer(cmd, f"ethtool -s {cfg.ifname} autoneg off speed {link['speed']} "
> -                   f"duplex {link['duplex'].lower()}")
>  
>      # 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()}")
>  
>      # Get remote pause params
>      ret, params = ethtool_ret(f"-a {cfg.remote_ifname}", host=cfg.remote)
> @@ -429,15 +429,15 @@ def pause_setup(cfg):
>      if link["auto-negotiation"]:
>          defer(cmd, f"ethtool -s {cfg.remote_ifname} autoneg on",
>                host=cfg.remote)
> -    elif "speed" in link and "duplex" in link:
> -        defer(cmd, f"ethtool -s {cfg.remote_ifname} autoneg off "
> -                   f"speed {link['speed']} duplex {link['duplex'].lower()}",
> -                   host=cfg.remote)
>  
>      # Remote interface admin up, link aneg on
>      ip(f"link set {cfg.remote_ifname} up", host=cfg.remote)
>      if link["supports-auto-negotiation"] and not link["auto-negotiation"]:
>          ethtool(f"-s {cfg.remote_ifname} autoneg on", host=cfg.remote)
> +        if "speed" in link and "duplex" in link:
> +            defer(cmd, f"ethtool -s {cfg.remote_ifname} autoneg off "
> +                       f"speed {link['speed']} duplex {link['duplex'].lower()}",
> +                       host=cfg.remote)
>  
>      wait_for_link(cfg)
>  
> @@ -470,9 +470,12 @@ def pause_test_support(cfg, pauseparams):
>  
>      forced = forced_link_settings(cfg)
>      _, supported = get_local_pause_supported(cfg)
> +    tested = False
>  
>      # We check that what we can configure in the pause params matches what we
> -    # support under various contditions : Link aneg on/off, pause aneg on/off
> +    # support under various conditions: link aneg on/off, and pause aneg
> +    # on/off. If no link configuration change is supported, fall back to
> +    # checking forced MAC pause on the current link.
>      ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg on", is_get = False)
>      if ret != 0:
>          ksft_pr("link autoneg on refused, not tested")
> @@ -481,18 +484,21 @@ def pause_test_support(cfg, pauseparams):
>                                 "link autoneg on")
>          expect_pauseparams_set(cfg, pauseparams, True, supported,
>                                 "link autoneg on")
> +        tested = True
>  
>      if not forced:
>          ksft_pr("link speed unknown, the forced link is not tested")
> -        return
> -
> -    ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg off {forced}", is_get = False)
> -    if ret != 0:
> -        ksft_pr(f"link autoneg off {forced} refused, not tested")
> -        return
> -
> -    expect_pauseparams_set(cfg, pauseparams, False, supported, "link autoneg off")
> -    expect_pauseparams_set(cfg, pauseparams, True, supported, "link autoneg off")
> +    else:
> +        ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg off {forced}", is_get = False)
> +        if ret != 0:
> +            ksft_pr(f"link autoneg off {forced} refused, not tested")
> +        else:
> +            expect_pauseparams_set(cfg, pauseparams, False, supported, "link autoneg off")
> +            expect_pauseparams_set(cfg, pauseparams, True, supported, "link autoneg off")
> +            tested = True
> +
> +    if not tested:
> +        expect_pauseparams_set(cfg, pauseparams, False, supported, "current link")
>  
>  @ksft_variants(pauseparams_variants)
>  @ksft_disruptive

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

[Mohsin Bashir]

On 11/4/26 2:57 AM, Maxime Chevallier wrote:
> Hi,
> 
> On 11/3/26 20:06, Mohsin Bashir wrote:
>> From: Mohsin Bashir <hmohsin@meta.com>
>>
>> pause_setup() saves the initial link settings and queues a deferred
>> restore. For devices that report forced link settings but do not support
>> changing link settings, this can queue a restore even when setup never
>> changes the link. On fbnic, the deferred restore returns EOPNOTSUPP and
>> turns pause tests that use pause_setup() into cleanup failures.
>>
>> Fix this by queuing the forced-link restore only after setup successfully
>> enables link autonegotiation. Keep pause_test_support coverage for devices
>> that cannot change link settings by falling back to forced MAC pause on the
>> current link. Run the extra link autoneg on/off checks only when those link
>> setting requests are accepted.
>>
>> Results on fbnic:
>>
>>    Before: # Totals: pass:0 fail:4 xfail:0 xpass:0 skip:24 error:0
>>    After:  # Totals: pass:4 fail:0 xfail:0 xpass:0 skip:24 error:0
>>
>> Fixes: cbd8a536cd6d ("selftests: drv-net: Introduce a selftest for ethtool flow control")
>> Signed-off-by: Mohsin Bashir <hmohsin@meta.com>
> 
> That looks better, this patch ran on the netdev CI and no regressions on
> stmmac,
> 
> Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> 
> Maxime
> 

Hi Maxime,

Thanks for the feedback on V1 and review of V2.

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

[netdev-bot+sashiko]

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