[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