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

[Andrew Lunn]

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>
> ---
>  .../testing/selftests/drivers/net/hw/pause.py | 716 ++++++++++++++++++
>  1 file changed, 716 insertions(+)
>  create mode 100644 tools/testing/selftests/drivers/net/hw/pause.py
> 
> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> new file mode 100644
> index 000000000000..c5cf81f48011
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
> @@ -0,0 +1,716 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +"""
> +Driver-related behavior tests for Pause-based Flow Control.
> +
> +Requires a controllable link-partner, with configurable autoneg parameters
> +as well as support for Pause and Asym_Pause.
> +"""
> +
> +from lib.py import ksft_run, ksft_exit, ksft_eq, ksft_ge, ksft_in, ksft_not_in, ksft_true
> +from lib.py import ksft_variants, KsftNamedVariant, KsftSkipEx, ksft_raises
> +from lib.py import defer, ethtool, CmdExitFailure
> +from lib.py import EthtoolFamily, NlError
> +from lib.py import NetDrvEpEnv
> +from lib.py import ip
> +import errno
> +
> +# Linkmodes to Pause params :
> +# Pause bit is set if rx == 1
> +# Asym_Pause bit is set if rx != tx
> +pauseparams_to_linkmodes = [
> +    {"rx": 0, "tx": 0, "linkmodes": []},
> +    {"rx": 0, "tx": 1, "linkmodes": ["Asym_Pause"]},
> +    {"rx": 1, "tx": 0, "linkmodes": ["Pause", "Asym_Pause"]},
> +    {"rx": 1, "tx": 1, "linkmodes": ["Pause"]},
> +]
> +
> +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"]

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.

> +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...

> +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.

> +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.

	 Andrew