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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

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