[PATCH net] selftests: drv-net: pause: Skip support tests without link autonegotiation

[PATCH net] selftests: drv-net: pause: Skip support tests without link autonegotiation

[Mohsin Bashir]

From: Mohsin Bashir <hmohsin@meta.com>

Devices without link autonegotiation support may reject both link-setting
requests in pause_test_support(). On fbnic, all four variants then fail
in cleanup when the deferred restore of unchanged link settings returns
EOPNOTSUPP. Removing the restore alone would report PASS without running
any pause checks.

Fix this by calling require_link_autoneg() before pause_setup() to skip
the test when either interface lacks link autonegotiation support. This
also skips devices that support only forced link configuration.

The issue was observed in the fbnic CI/CD pipeline. It was reproduced and
the fix was tested on a local fbnic device.

Fixes: cbd8a536cd6d ("selftests: drv-net: Introduce a selftest for ethtool flow control")
Signed-off-by: Mohsin Bashir <hmohsin@meta.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/drivers/net/hw/pause.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
index ca4a55e59c30..1537f88e0e77 100755
--- a/tools/testing/selftests/drivers/net/hw/pause.py
+++ b/tools/testing/selftests/drivers/net/hw/pause.py
@@ -466,6 +466,7 @@ def pause_test_support(cfg, pauseparams):
     supported fields.
     """
 
+    require_link_autoneg(cfg)
     pause_setup(cfg)
 
     forced = forced_link_settings(cfg)
-- 
2.53.0-Meta

Re: [PATCH net] selftests: drv-net: pause: Skip support tests without link autonegotiation

[Maxime Chevallier]

Hi,

On 10/30/26 18:35, Mohsin Bashir wrote:
> From: Mohsin Bashir <hmohsin@meta.com>
> 
> Devices without link autonegotiation support may reject both link-setting
> requests in pause_test_support(). On fbnic, all four variants then fail
> in cleanup when the deferred restore of unchanged link settings returns
> EOPNOTSUPP. Removing the restore alone would report PASS without running
> any pause checks.
> 
> Fix this by calling require_link_autoneg() before pause_setup() to skip
> the test when either interface lacks link autonegotiation support. This
> also skips devices that support only forced link configuration.
> 
> The issue was observed in the fbnic CI/CD pipeline. It was reproduced and
> the fix was tested on a local fbnic device.
> 
> Fixes: cbd8a536cd6d ("selftests: drv-net: Introduce a selftest for ethtool flow control")
> Signed-off-by: Mohsin Bashir <hmohsin@meta.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/drivers/net/hw/pause.py | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> index ca4a55e59c30..1537f88e0e77 100755
> --- a/tools/testing/selftests/drivers/net/hw/pause.py
> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
> @@ -466,6 +466,7 @@ def pause_test_support(cfg, pauseparams):
>      supported fields.
>      """
>  
> +    require_link_autoneg(cfg)

This helper checks if the remote supports aneg too, so it's too restrictive.

The goal of this test is to check the local device's behaviour, without too
much dependencies on the remote, so that we can at least have it test some of
the local driver behaviour even when it's connected to a dumb remote or a switch.

Maybe the sane thing to do is to allows the defer() calls in pause_setup() to
fail silently with "fail=False" ?

Or maybe a bit more clever, have a quick check to see if the device accepts
an ethtool -s XXX command ? looks like fbnic will refuse all of'em as it
doesn't have a .set_link_ksettings.

Maxime

Re: [PATCH net] selftests: drv-net: pause: Skip support tests without link autonegotiation

[Mohsin Bashir]

On 10/30/26 10:54 AM, Maxime Chevallier wrote:
> Hi,
> 
> On 10/30/26 18:35, Mohsin Bashir wrote:
>> From: Mohsin Bashir <hmohsin@meta.com>
>>
>> Devices without link autonegotiation support may reject both link-setting
>> requests in pause_test_support(). On fbnic, all four variants then fail
>> in cleanup when the deferred restore of unchanged link settings returns
>> EOPNOTSUPP. Removing the restore alone would report PASS without running
>> any pause checks.
>>
>> Fix this by calling require_link_autoneg() before pause_setup() to skip
>> the test when either interface lacks link autonegotiation support. This
>> also skips devices that support only forced link configuration.
>>
>> The issue was observed in the fbnic CI/CD pipeline. It was reproduced and
>> the fix was tested on a local fbnic device.
>>
>> Fixes: cbd8a536cd6d ("selftests: drv-net: Introduce a selftest for ethtool flow control")
>> Signed-off-by: Mohsin Bashir <hmohsin@meta.com>
>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>> ---
>>   tools/testing/selftests/drivers/net/hw/pause.py | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
>> index ca4a55e59c30..1537f88e0e77 100755
>> --- a/tools/testing/selftests/drivers/net/hw/pause.py
>> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
>> @@ -466,6 +466,7 @@ def pause_test_support(cfg, pauseparams):
>>       supported fields.
>>       """
>>   
>> +    require_link_autoneg(cfg)
> 
> This helper checks if the remote supports aneg too, so it's too restrictive.
> 
> The goal of this test is to check the local device's behaviour, without too
> much dependencies on the remote, so that we can at least have it test some of
> the local driver behaviour even when it's connected to a dumb remote or a switch.
> 

Right, it may be too restrictive however cleanup handling is still 
required on both interfaces even when the DUT accepts the configuration 
and runs its checks, the remote can reject the deferred restore and fail 
the test. Ideally, this test should not depend on remote config in the 
first place.

> Maybe the sane thing to do is to allows the defer() calls in pause_setup() to
> fail silently with "fail=False" ?
> 

IMO using fail=False would also suppress genuine restoration errors when 
settings actually changed and can leaving the interface in an unexpected 
configuration. No?


> Or maybe a bit more clever, have a quick check to see if the device accepts
> an ethtool -s XXX command ? looks like fbnic will refuse all of'em as it
> doesn't have a .set_link_ksettings.
> 

Yes, checking whether the DUT accepts ethtool -s is one options. We 
could reuse the existing autonegotiated and forced-link attempts, and 
report SKIP if neither allows pause checks to run. Alternatively, we can 
add a remote arg to require_link_autoneg(), default to True, and pass 
False for this test so it checks only the DUT’s autonegotiation support.

Re: [PATCH net] selftests: drv-net: pause: Skip support tests without link autonegotiation

[Andrew Lunn]

> > Or maybe a bit more clever, have a quick check to see if the device accepts
> > an ethtool -s XXX command ? looks like fbnic will refuse all of'em as it
> > doesn't have a .set_link_ksettings.
> > 
> 
> Yes, checking whether the DUT accepts ethtool -s is one options. We could
> reuse the existing autonegotiated and forced-link attempts, and report SKIP
> if neither allows pause checks to run. Alternatively, we can add a remote
> arg to require_link_autoneg(), default to True, and pass False for this test
> so it checks only the DUT’s autonegotiation support.

There is a subset of pause configurations which don't require autoneg.

ethtool --pause eth42 autoneg off [rx on|off] [tx on|off]

These can be implemented by a device which does not support
autoneg. These setting go straight to the MAC. They are never
negotiated.

	 Andrew

Re: [PATCH net] selftests: drv-net: pause: Skip support tests without link autonegotiation

[Mohsin Bashir]

On 10/30/26 1:12 PM, Andrew Lunn wrote:
>>> Or maybe a bit more clever, have a quick check to see if the device accepts
>>> an ethtool -s XXX command ? looks like fbnic will refuse all of'em as it
>>> doesn't have a .set_link_ksettings.
>>>
>>
>> Yes, checking whether the DUT accepts ethtool -s is one options. We could
>> reuse the existing autonegotiated and forced-link attempts, and report SKIP
>> if neither allows pause checks to run. Alternatively, we can add a remote
>> arg to require_link_autoneg(), default to True, and pass False for this test
>> so it checks only the DUT’s autonegotiation support.
> 
> There is a subset of pause configurations which don't require autoneg.
> 
> ethtool --pause eth42 autoneg off [rx on|off] [tx on|off]
> 
> These can be implemented by a device which does not support
> autoneg. These setting go straight to the MAC. They are never
> negotiated.
> 
> 	 Andrew

Hi Andrew,

Right. The autoneg setup and MAC pause checks need to be decoupled. Let 
me respin the fix.

Re: [PATCH net] selftests: drv-net: pause: Skip support tests without link autonegotiation

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · 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).

- [High] The fix is incomplete.
- [Medium] The new require_link_autoneg(cfg) call in pause_test_support()
  (pause.py:469) now runs before pause_setup()'s guard for devices that…

--

selftests: drv-net: pause: Skip support tests without link autonegotiation

This commit calls require_link_autoneg() before pause_setup() in
pause_test_support(), so the test is skipped when either interface lacks
link autonegotiation. The goal is to stop cleanup failures on fbnic, where
the deferred restore of unchanged link settings returns EOPNOTSUPP.

> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> index ca4a55e59c30f..1537f88e0e77e 100755
> --- a/tools/testing/selftests/drivers/net/hw/pause.py
> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
> @@ -466,6 +466,7 @@ def pause_test_support(cfg, pauseparams):
>      supported fields.
>      """
>  
> +    require_link_autoneg(cfg)

[Severity: Medium]
Can this turn a SKIP into a failure on devices that don't report link
modes?

require_link_autoneg() reads the JSON key directly for both interfaces,
without checking that it exists:

def require_link_autoneg(cfg):
    ...
    if not ethtool(f"{cfg.ifname}", json=True)[0]["supports-auto-negotiation"]:
    ...
    if not ethtool(f"{cfg.remote_ifname}",
                   json=True, host=cfg.remote)[0]["supports-auto-negotiation"]:

ethtool fills both "auto-negotiation" and "supports-auto-negotiation" from
the same LINKMODES_GET reply. If the driver has no get_link_ksettings,
ethtool still exits 0, but the JSON has neither key.

Before this patch, pause_test_support() reached this guard in pause_setup()
and skipped:

    link = ethtool(f"{cfg.ifname}", json=True)[0]
    if "auto-negotiation" not in link:
        raise KsftSkipEx(f"{cfg.ifname} doesn't report its link modes")

With this patch, require_link_autoneg() runs first and raises KeyError.
ksft_run() catches that and reports a failure for all four variants.

The other callers of require_link_autoneg() first go through the
require_pause_supported helpers. Those check "supported-pause-frame-use"
from the same reply. pause_test_support() is the only caller that has no
earlier guard.

Would using .get("supports-auto-negotiation") avoid this? Checking for the
key and skipping would also work.

>      pause_setup(cfg)

[Severity: High]
Does this fully fix the fbnic CI failure described in the commit message?

pause_setup() always defers a restore of the current link settings. It
does this for both the local and the remote interface, even when nothing
will change those settings:

    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()}")

pause_advertising_test() and pause_aneg_resolution() still call
pause_setup() without a require_link_autoneg() check:

pause_advertising_test():
    require_pause_supported_allof(cfg, pauseparams["linkmodes"])
    pause_setup(cfg)
    lp = controllable_lp(cfg)

pause_aneg_resolution():
    require_pause_supported_allof(cfg, required_local_linkmodes)
    require_peer_pause_supported_allof(cfg, required_remote_linkmodes)
    pause_setup(cfg)
    require_controllable_lp(cfg)

fbnic has .get_link_ksettings, but it has no .set_link_ksettings and no
.nway_reset.

Take the "RX off TX off" variant of pause_advertising_test(). Its linkmodes
list is empty, so the support check passes, and then:

pause_advertising_test()
  pause_setup()
    defer(cmd, "ethtool -s ... autoneg off speed X duplex Y")
  controllable_lp()
    ethtool -r -> EOPNOTSUPP
    raise KsftSkipEx(...)
ksft_flush_defer()
  ethtool -s -> EOPNOTSUPP
  KSFT_RESULT = False

The result is "not ok ... # SKIP", and ksft_exit() returns 1.

pause_aneg_resolution() takes the same path through
require_controllable_lp(), for example in the all-off/all-off variant.

controllable_lp() also does not cache a result when it skips. Every later
test runs ethtool -r again and fails the same way.

Would it be better to add require_link_autoneg() to these two tests? The
other option is for pause_setup() to defer the link-settings restore only
when it actually changes the link settings.

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