Re: [RFC PATCH net-next] selftests/net: Introduce a selftest for ethtool flow control

[Maxime Chevallier]

Hi Andrew,

On 8/9/26 18:45, Andrew Lunn wrote:
> On Fri, Jul 31, 2026 at 06:28:11PM +0200, Maxime Chevallier (Netdev Foundation) wrote:
>> From: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>
>>
>> As part of work started by the Netdev Foundation, introduce a skeleton
>> of a selftest to stress the ethtool pause APIs. The goal is to catch
>> common mistakes done in drivers by allowing these tests to be run on
>> real hardware, and derive potential implementation issues from the
>> behaviour observed at the userspace level.
>>
>> This is just the skeleton of the tests, most implementation details are
>> stubbed, the goal is to validate the test definitions, flow and results
>> here.
>>
>> A separate documentation such as the one written by Oleksij will be sent
>> as a complement for the next iterations.
>>
>> Signed-off-by: Maxime Chevallier (Netdev Foundation) <maxime.chevallier@xxxxxxxxxxx>

Thanks for the review ! Sorry for the delay, I took 2 weeks off
expecting to be able to be somewhat active on netdev, but I ended-up
not being very present. Back to work now :)

[...]

>> +def pause_to_linkmodes(rx, tx) -> list[str]:
>> +    """ Convert rx/tx pauseparams to the corresponding linkmodes
>> +    """
>> +    if rx:
>> +        if tx:
>> +            return pauseparams_to_linkmodes[3]["linkmodes"]
>> +        else:
>> +            return pauseparams_to_linkmodes[2]["linkmodes"]
>> +    elif tx:
>> +        return pauseparams_to_linkmodes[1]["linkmodes"]
>> +    else:
>> +        return pauseparams_to_linkmodes[0]["linkmodes"]
> 
> I don't want to get into the weeds, but if you have nested dict, you
> should be able to do
> 
> 	return pauseparams_to_linkmodes[rx][tx]["linkmodes"]

True, that's a better idea yes :)

> 
> but then you cannot reuse the pauseparams_to_linkmodes as a list of
> test variants.
> 
>> +def get_peer_lp_advertising(cfg) -> tuple[int, list[str]]:
>> +    """ get the lp_advertised linkmodes on the link partner
>> +
>> +    Raise an error if the return is not 0 or EOPNOTSUPP
>> +    returns ENODEV if there's no LP
>> +    Prints a warning if LP is present but doesn't report lp_advertising
>> +
>> +    :param cfg: test config
>> +    :returns: tuple containing :
>> +              - return code of the ethtool command
>> +              - list of linkmodes
>> +    """
>> +    #TODO
>> +    raise KsftSkipEx("Not implemented")
>> +
>> +def wait_for_link(cfg) -> bool:
>> +    """ Wait for both ends of the link to be up.
>> +
>> +    :param cfg: test config
>> +    :returns: True if link is UP, False if timeout
>> +    """
>> +    #TODO
>> +    raise KsftSkipEx("Not implemented")
> 
> When we are considering this as Documentation of what tests we want to
> implement, this is fine. But when it comes to the real implementation,
> i think many of these methods can be put into a library, since they
> will be used for EEE testing, and hopefully other classes of tests in
> the future.

Absolutely, I agree with that, I'll put these into a shared lib

> 
>> +def pause_test_support(cfg, pauseparams) -> None:
>> +    """ Verify that the supported linkmodes Pause and Asym_Pause match the
>> +        ability to configure the rx and tx pauseparams.
>> +
>> +    Drivers are expected to reject pauseparams they don't support, and
>> +    accept the ones they support. The supported modes are exposed by
>> +    the MAC to the PHY layer through phylink mac_capabilities MAC_SYM_PAUSE
>> +    and MAC_ASYM_PAUSE, or through phylib directly with the
>> +    phy_support_sym_pause() and phy_support_asym_pause() helpers.
>> +
>> +    The expectation is for drivers to refuse setting pauseparams that don't
>> +    match the Pause and Asym_Pause bits in the supported linkmodes with a
>> +    -EOPNOTSUPP return value. Unsupported pause params must be rejected.
>> +
>> +    Failing this test likely means the MAC driver doesn't implement the
>> +    set/get_pauseparam, but still sets flow control as supported through
>> +    phylink mac_capabilities or phylib's pause API. Conversely, the MAC driver
>> +    may have omitted to indicate its supported Pause modes. Finally, the PHY
>> +    driver may incorrectly override the Pause and Asym_Pause bits in its supported
>> +    fields.
>> +    """
>> +
>> +    rx = "on" if pauseparams["rx"] else "off"
>> +    tx = "on" if pauseparams["tx"] else "off"
>> +    linkmodes = pauseparams["linkmodes"]
>> +
>> +    # Run ethtool {cfg.ifname} and extract the "Supported pause frame use"
>> +    # Run ethtool -A {cfg.ifname} rx {rx} tx {tx} autoneg off, store the return code
> 
> The autoneg off is buried in this text. On first reading, i did not
> notice this was testing forced pause configuration. I would mention
> that in the method documentation, and maybe the method name.
> 
> Should we be being this twice, once with ethtool -s autoneg on, and
> then with ethtool -s autoneg off? Maybe that is covered in a later
> test i've not got to yet...

Ah true yes

> 
>> +def pause_advertising_test(cfg, pauseparams) -> None:
> 
> ...
> 
>> +    # Wait for link parameters to re-negotiate and link to come back up. It must
>> +    # come back up, otherwise that means changing pauseparams can bring the
>> +    # link down.
>> +    ret = wait_for_link(cfg)
>> +    ksft_true(ret, "Link din't come back up after setting pauseparams")
> 
> typo: didn't
> 
>> +
>> +    _, linkmodes = get_local_advertising(cfg)
>> +    for mode in adv:
>> +        ksft_in(mode, linkmodes, f"rx {rx} tx {tx} must advertise {adv}")
>> +
>> +    for mode in not_adv:
>> +        ksft_not_in(mode, linkmodes, f"rx {rx} tx {tx} must not advertise {not_adv}")
>> +
>> +    # It's better to double-check on the LP that we are indeed correctly
>> +    # advertising these modes, but if we don't have a controllable LP let's
>> +    # just rely on what we say we're advertising
>> +    returncode, remote_linkmodes = get_peer_lp_advertising(cfg)
>> +    if returncode == errno.ENODEV:
>> +        return
> 
> I think we should be suggesting to vendors to implement returning the
> LP values. And i _guess_ most vendors with use a pair of their own
> devices, rather than a 3rd party device. So printing a warning here
> that LP is not supported would be a good hint it should be.
> 
> I think the level of details is good now. We can start the
> implementation.

Great, next round will be something we can actually run then :)

> 
>> +def main() -> None:
>> +    with NetDrvEpEnv(__file__) as cfg:
>> +        cfg.ethnl = EthtoolFamily()
>> +        ksft_run([pause_test_support,
>> +                  pause_advertising_test,
>> +                  pause_aneg_resolution,
>> +                  pause_autoneg_state_adv,
>> +                  pause_autoneg_state_params,
>> +                  pause_autoneg_off_while_link_autoneg_on,
>> +                  pause_autoneg_link_autoneg,
>> +                  ],
>> +                 args=(cfg, ))
>> +    ksft_exit()
>> +
>> +if __name__ == "__main__":
>> +    main()
> 
> An implementation detail: How robust is the test framework, getting
> the device back into a working state? The test is turning autoneg off
> and on, we might loose link. If we fail a test, throw an exception, is
> there cleanup to get the device back into a known good state?
> 
> Maybe before we run the test, we check we have link. If there is no
> link, fail the test, basic requirements are not met. Then make a note
> of the current configuration. At the end of the test, even if it
> fails, or throws an exception restore that configuration. We have to
> be careful of systems like automotive which often don't support
> autoneg. So our cleanup cannot be as simple as turn autoneg on.

>From my initial tests, we definitely have to deal with recovery. We can
lose link, get the whole interface completely stuck (crash/stall requiring
a reboot), etc.

It's more of an infrastructure thing here, people running the selftests
while developping a driver will be able to manually recover, but for NIPA
or other setups, usually reboot is the safe way.

Maxime

Pause isn't the wo

> 
> 	 Andrew