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

[Mohsin Bashir <mohsin.bashr@gmail.com>]

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.