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

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

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@bootlin.com>
>>
>> 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@bootlin.com>

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