[PATCH net-next v3 0/3] selftests: drv-net: Introduce flow-control selftest

[PATCH net-next v3 0/3] selftests: drv-net: Introduce flow-control selftest

[Maxime Chevallier (Netdev Foundation)]

Hi everyone,

This is V3 for the ethtool flow control test.

This test aims at validating the flow control implementation in drivers,
using the ethtool API.

The test can run in various conditions :

DUT <-----> Anything that has link up (not controllable)
DUT <-----> A directly connected device

It enumerates the local and remote device's capabilities, to test as
much as possibly testable. I've run it on 17 different boards connected
to other boards through various means : Usb to Eth dongles, direct
connection to the board, connection through a switch. Some of the ports
tested were user ports of DSA switches too.

Contrary to V2, on V3 we really only have a collection of actual
failures that show during tests, as one of the peer's I'm running these
on has a v6.18 kernel that pre-dates the realtek phylink port and show
actual bugs in the driver.

Thanks to the Netdev Foundation for funding this work :)

Maxime

Changes in V3:
 - Use Jakub's loop on carrier state when waiting for the interface to
   be up
 - Following discussions with Andrew, we now detect the link partner
   using ethtool -r to trigger an autoneg, instead of assuming the link
   will drop when the interface is brought down
 - Numerous typo fixes and comment reworks
 - wait_for_link and wait_for_aneg have been reworked
 - We no longer expect that pause advertising is empty when pause
   autoneg is off, as documented in uAPI.

Changes in V2:
V2: https://lore.kernel.org/r/20260925182628.214667-1-maxime.chevallier@bootlin.com
 - Don't add stuff to the lib
 - Fix most pylink errors (module's too long for pylint ...)
 - Ditch the fail=False from the defer()
 - Remove duplicate patches
 - Clean the comments
 - Drop the return type annotations, most were wrong...

V1 : https://lore.kernel.org/netdev/20260920164732.349744-1-maxime.chevallier@bootlin.com/

Maxime Chevallier (Netdev Foundation) (3):
  selftests: drv-net: Introduce a selftest for ethtool flow control
  selftests: drv-net: pause: Validate the pause autonegotiation with a
    partner
  selftests: drv-net: pause: Validate pause autoneg interactions with
    link autoneg

 .../testing/selftests/drivers/net/hw/Makefile |   1 +
 .../testing/selftests/drivers/net/hw/pause.py | 944 ++++++++++++++++++
 2 files changed, 945 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/hw/pause.py

-- 
2.55.0

[PATCH net-next v3 1/3] selftests: drv-net: Introduce a selftest for ethtool flow control

[Maxime Chevallier (Netdev Foundation)]

Ethernet flow control is a tricky thing to get right, especially when it
comes to correctly handling the negotiation of the parameters with a
link partner.

On one hand, the userspace API talks in terms of the device's ability to
send Pause frames when the device is overwhelmed by ingress traffic (TX
pause) and the ability to stop sending traffic upon receiving Pause
frames (RX pause).

On the other hand, 802.3 explains that devices can exchange their
abilities over link negotiation, but instead of echanging TX and RX
abilities, they exchange Pause and Asymmetric Pause capabilities :

RX unable TX unable => None
RX able   TX unable => Pause + Asymmetric
RX unable TX able   => Asymmetric
RX able   TX able   => Pause

The reported Pause and Asym abilities depend both on the MAC, that
eventually sends and processes these frames, and the PHY, that
advertises these to the partner.

Both MAC and PHYs can have their own limitations, and getting the
correct set of supported Pause/Asym parameters is non-trivial, unless
the MAC uses phylink, which deals with the complexity.

Introduce a set of Pause selftests that verify that the local
interface's supported pause parameters reported from ethtool (in terms
of Pause + Asym ) match the accepted parameters from "ethtool -A",
corresponding to ethtool's .set_pauseparams() ops, expressed in TX and
RX abilities.

Signed-off-by: Maxime Chevallier (Netdev Foundation) <maxime.chevallier@bootlin.com>
---
 .../testing/selftests/drivers/net/hw/Makefile |   1 +
 .../testing/selftests/drivers/net/hw/pause.py | 369 ++++++++++++++++++
 2 files changed, 370 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/hw/pause.py

diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
index 18ce235a77f8..9b6b5d15f04d 100644
--- a/tools/testing/selftests/drivers/net/hw/Makefile
+++ b/tools/testing/selftests/drivers/net/hw/Makefile
@@ -40,6 +40,7 @@ TEST_PROGS = \
 	nk_netns.py \
 	nk_qlease.py \
 	ntuple.py \
+	pause.py \
 	pp_alloc_fail.py \
 	rss_api.py \
 	rss_ctx.py \
diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
new file mode 100755
index 000000000000..854428e19f55
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/pause.py
@@ -0,0 +1,369 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+"""
+Driver-related behavior tests for Pause-based Flow Control.
+"""
+
+import errno
+import json
+import os
+
+from lib.py import KsftFailEx, KsftNamedVariant, KsftSkipEx
+from lib.py import NetDrvEpEnv
+from lib.py import cmd, defer, ethtool, ip, wait_file
+from lib.py import ksft_disruptive, ksft_variants, ksft_run
+from lib.py import ksft_eq, ksft_exit, ksft_in, ksft_not_in, ksft_pr
+
+# Linkmodes to Pause params :
+# Pause bit is set if rx == 1
+# Asym_Pause bit is set if rx != tx
+pauseparams_to_linkmodes = {
+    0: {0: {"rx": 0, "tx": 0, "linkmodes": []},
+        1: {"rx": 0, "tx": 1, "linkmodes": ["Asym_Pause"]}},
+    1: {0: {"rx": 1, "tx": 0, "linkmodes": ["Pause", "Asym_Pause"]},
+        1: {"rx": 1, "tx": 1, "linkmodes": ["Pause"]}},
+}
+
+def onoff(val):
+    """ Convert a bool to on/off """
+    return "on" if val else "off"
+
+pauseparams_variants = [
+    KsftNamedVariant(f"RX {onoff(p['rx'])} TX {onoff(p['tx'])}", p)
+    for by_tx in pauseparams_to_linkmodes.values() for p in by_tx.values()
+]
+
+_strerrors = {os.strerror(e): e for e in errno.errorcode}
+
+def ethtool_ret(command, is_get=True, host=None):
+    """ Execute an ethtool command, returns the return code and JSON content
+
+    :param command: the ethtool arguments
+    :param is_get: Is the command a get or a set. Get commands return the loaded
+                   JSON attributes
+    :param host: The host on which to run the command on. None means local host.
+    """
+    json_flag = "--json" if is_get else ""
+    cmd_res = cmd(f"ethtool {json_flag} {command}", host=host, fail=False)
+
+    if cmd_res.ret != 0:
+        # ethtool returns 1 upon error, not the netlink errcode. Try to get it
+        # by parsing the stderr output, which looks like :
+        # "netlink error: Operation not supported"
+        for line in cmd_res.stderr.splitlines():
+            err = _strerrors.get(line.rsplit(": ", 1)[-1].strip())
+            if err:
+                return err, None
+        return cmd_res.ret, None
+
+    # Not a get operation, we don't have any JSON output to parse
+    if not is_get:
+        return 0, None
+
+    return 0, json.loads(cmd_res.stdout)[0]
+
+def wait_for_link(cfg, timeout=15):
+    """ Wait for the link to come up """
+
+    wait_file(f"/sys/class/net/{cfg.ifname}/carrier",
+              lambda x: x.strip() == "1", deadline=timeout)
+
+def wait_for_aneg(cfg, link_drop=False, timeout=15):
+    """ Wait for a renegotiation to complete
+
+    :param link_drop: Set to true if the link HAS to flap.
+    """
+    try:
+        wait_file(f"/sys/class/net/{cfg.ifname}/carrier",
+                  lambda x: x.strip() == "0",
+                  deadline=timeout if link_drop else 2)
+    except TimeoutError:
+        if link_drop:
+            raise
+
+    wait_for_link(cfg, timeout)
+
+def forced_link_settings(cfg):
+    """ Returns a string to pass to ethtool -s with speed/duplex corresponding
+        to the current settings.
+
+        Note that some devices don't return duplex info, so assume full duplex
+        in that case.
+    """
+    link = ethtool(f"{cfg.ifname}", json=True)[0]
+    if "speed" not in link:
+        return ""
+
+    return f"speed {link['speed']} duplex {link.get('duplex', 'Full').lower()}"
+
+def _ethtool_pause_use_to_linkmodes(use):
+    """ Convert the ethtool output for pause modes into pause linkmodes """
+    if use == "Symmetric":
+        return ["Pause"]
+    elif use == "Symmetric Receive-only":
+        return ["Pause", "Asym_Pause"]
+    elif use == "Transmit-only":
+        return ["Asym_Pause"]
+    else:
+        return []
+
+def set_local_pauseparams(cfg, rx, tx, aneg):
+    """ set pauseparams : ethtool -A """
+    rx_param = onoff(rx)
+    tx_param = onoff(tx)
+    aneg_param = onoff(aneg)
+
+    ret, _ = ethtool_ret(f"-A {cfg.ifname} rx {rx_param} tx {tx_param}"
+                         f" autoneg {aneg_param}",
+                         is_get = False)
+
+    if ret != 0 and ret != errno.EOPNOTSUPP:
+        raise KsftFailEx(f"ethtool -A {cfg.ifname} failed: {ret}")
+
+    return ret
+
+def get_local_pauseparams(cfg):
+    """ get pauseparams : ethtool -a """
+    return ethtool_ret(f"-a {cfg.ifname}", is_get=True)
+
+def get_local_pause_supported(cfg):
+    """ Return the errcode and the local pause supported linkmodes """
+
+    ret, data = ethtool_ret(f"{cfg.ifname}")
+    if ret != 0:
+        raise KsftFailEx(f"ethtool {cfg.ifname} failed: {ret}")
+
+    if "supported-pause-frame-use" not in data:
+        raise KsftSkipEx(f"{cfg.ifname} doesn't report its link modes")
+
+    return ret, _ethtool_pause_use_to_linkmodes(data["supported-pause-frame-use"])
+
+def get_local_pause_advertising(cfg):
+    """ Return the errcode and the local pause advertised linkmodes """
+
+    ret, data = ethtool_ret(f"{cfg.ifname}")
+    if ret != 0:
+        raise KsftFailEx(f"ethtool {cfg.ifname} failed: {ret}")
+
+    if "advertised-pause-frame-use" not in data:
+        raise KsftSkipEx(f"{cfg.ifname} doesn't report its link modes")
+
+    return ret, _ethtool_pause_use_to_linkmodes(data["advertised-pause-frame-use"])
+
+def require_pause_supported_allof(cfg, linkmodes):
+    """ Skip if local device doesn't support all of the passed modes """
+
+    ret, _ = get_local_pauseparams(cfg)
+    if ret != 0:
+        raise KsftSkipEx("device doesn't allow getting pauseparams")
+
+    _, pause_support = get_local_pause_supported(cfg)
+    for lm in linkmodes:
+        if lm not in pause_support:
+            raise KsftSkipEx(f"Local device doesn't support {lm}")
+
+def expect_pauseparams_set(cfg, pauseparams, aneg, supported, note):
+    """ ethtool -A must work or be refused depending on what the local
+        device supports
+    """
+    rx = pauseparams["rx"]
+    tx = pauseparams["tx"]
+
+    # Measure current pauseparams, re-setting the same params as the current
+    # ones returns 0 even if unsupported.
+    _, cur = get_local_pauseparams(cfg)
+    noop = cur and (cur["rx"], cur["tx"], cur["autonegotiate"]) == \
+           (bool(rx), bool(tx), aneg)
+
+    ret, _ = ethtool_ret(f"-A {cfg.ifname} rx {onoff(rx)} tx {onoff(tx)}"
+                         f" autoneg {onoff(aneg)}", is_get = False)
+
+    if noop:
+        ksft_eq(ret, 0, note)
+    elif not supported:
+        ksft_eq(ret, errno.EOPNOTSUPP, note)
+    elif set(pauseparams["linkmodes"]).issubset(set(supported)):
+        ksft_eq(ret, 0, note)
+    else:
+        ksft_in(ret, (errno.EOPNOTSUPP, errno.EINVAL), note)
+
+def pause_setup(cfg):
+    """ The starting conditions every test counts on, restored on exit:
+        - both ports admin up
+        - link autoneg on on both sides ifsupported
+        - carrier on
+    """
+
+    # Get init pause parameters
+    ret, params = ethtool_ret(f"-a {cfg.ifname}")
+    if ret == 0:
+        defer(cmd, f"ethtool -A {cfg.ifname} rx {onoff(params['rx'])} "
+                   f"tx {onoff(params['tx'])} "
+                   f"autoneg {onoff(params['autonegotiate'])}")
+
+    # Get init link parameters
+    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")
+
+    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()}")
+
+    # Local interface admin up, link aneg on
+    ip(f"link set {cfg.ifname} up")
+    if link["supports-auto-negotiation"] and not link["auto-negotiation"]:
+        ethtool(f"-s {cfg.ifname} autoneg on")
+
+    # Get remote pause params
+    ret, params = ethtool_ret(f"-a {cfg.remote_ifname}", host=cfg.remote)
+    if ret == 0:
+        defer(cmd, f"ethtool -A {cfg.remote_ifname} rx {onoff(params['rx'])} "
+                   f"tx {onoff(params['tx'])} "
+                   f"autoneg {onoff(params['autonegotiate'])}",
+                   host=cfg.remote)
+
+    # Get remote link params
+    link = ethtool(f"{cfg.remote_ifname}", json=True, host=cfg.remote)[0]
+    if "auto-negotiation" not in link:
+        raise KsftSkipEx(f"{cfg.remote_ifname} doesn't report its link modes")
+
+    if link["auto-negotiation"]:
+        defer(cmd, f"ethtool -s {cfg.remote_ifname} autoneg on",
+              host=cfg.remote)
+    elif "speed" in link and "duplex" in link:
+        defer(cmd, f"ethtool -s {cfg.remote_ifname} autoneg off "
+                   f"speed {link['speed']} duplex {link['duplex'].lower()}",
+                   host=cfg.remote)
+
+    # Remote interface admin up, link aneg on
+    ip(f"link set {cfg.remote_ifname} up", host=cfg.remote)
+    if link["supports-auto-negotiation"] and not link["auto-negotiation"]:
+        ethtool(f"-s {cfg.remote_ifname} autoneg on", host=cfg.remote)
+
+    wait_for_link(cfg)
+
+# Pause support : Supported linkmodes vs ability to set/get pauseparams
+@ksft_variants(pauseparams_variants)
+@ksft_disruptive
+def pause_test_support(cfg, pauseparams):
+    """ 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.
+    """
+
+    pause_setup(cfg)
+
+    forced = forced_link_settings(cfg)
+    _, supported = get_local_pause_supported(cfg)
+
+    # We check that what we can configure in the pause params matches what we
+    # support under various contditions : Link aneg on/off, pause aneg on/off
+    ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg on", is_get = False)
+    if ret != 0:
+        ksft_pr("link autoneg on refused, not tested")
+    else:
+        expect_pauseparams_set(cfg, pauseparams, False, supported,
+                               "link autoneg on")
+        expect_pauseparams_set(cfg, pauseparams, True, supported,
+                               "link autoneg on")
+
+    if not forced:
+        ksft_pr("link speed unknown, the forced link is not tested")
+        return
+
+    ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg off {forced}", is_get = False)
+    if ret != 0:
+        ksft_pr(f"link autoneg off {forced} refused, not tested")
+        return
+
+    expect_pauseparams_set(cfg, pauseparams, False, supported, "link autoneg off")
+    expect_pauseparams_set(cfg, pauseparams, True, supported, "link autoneg off")
+
+@ksft_variants(pauseparams_variants)
+@ksft_disruptive
+def pause_advertising_test(cfg, pauseparams):
+    """Pause advertisement
+
+    Validate that changing pause params through the ETHTOOL_MSG_PAUSE command
+    translates to a change in the advertised pause params, and that these
+    parameters are correct w.r.t the supported pause params and requested pause
+    params.
+
+    This exercises the .set_pauseparam() ethtool ops for MAC configuration,
+    as well as the reconfiguration of the PHY's advertising and negotiation.
+
+    On non-phylink MACs, the MAC should call phy_set_sym_pause() to update the
+    PHY's advertising, and restart a negotiation with phy_start_aneg() if
+    need be. Failure to do so will result in the wrong advertising parameters.
+
+    On phylink-enabled MACs, phylink deals with the PHY reconfiguration provided
+    the MAC driver calls phylink_ethtool_set_pauseparam().
+
+    Failing this test likely means that the PHY driver is not correctly
+    advertising pause settings, either due to the MAC not triggering a PHY
+    reconfiguration, a misconfiguration of the advertising registers by the PHY,
+    or by mis-handling the phydev->advertising bitmap in the PHY driver directly.
+
+    The validation is made by looking at the advertised modes locally, as well
+    as what the peer's 'lp_advertising' values report.
+    """
+
+    require_pause_supported_allof(cfg, pauseparams["linkmodes"])
+    pause_setup(cfg)
+
+    tx = pauseparams["tx"]
+    rx = pauseparams["rx"]
+    adv = pauseparams["linkmodes"]
+    not_adv = [ l for l in ["Pause", "Asym_Pause"] if l not in adv]
+
+    # It's OK to skip here, we're already validating the EOPNOTSUPP behaviour
+    # the pause_test_support test.
+    ret = set_local_pauseparams(cfg, rx, tx, True)
+    if ret == errno.EOPNOTSUPP:
+        raise KsftSkipEx(f"RX {rx} TX {tx} not supported")
+
+    # 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.
+    wait_for_aneg(cfg)
+
+    _, linkmodes = get_local_pause_advertising(cfg)
+    for mode in adv:
+        ksft_in(mode, linkmodes,
+                f"rx {rx} tx {tx} aneg on must advertise {adv}")
+
+    for mode in not_adv:
+        ksft_not_in(mode, linkmodes,
+                    f"rx {rx} tx {tx} aneg on must not advertise {not_adv}")
+
+def main():
+    """ Entry point """
+    with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
+        ksft_run([pause_test_support,
+                  pause_advertising_test,
+                  ],
+                 args=(cfg, ))
+    ksft_exit()
+
+if __name__ == "__main__":
+    main()
-- 
2.55.0

[PATCH net-next v3 2/3] selftests: drv-net: pause: Validate the pause autonegotiation with a partner

[Maxime Chevallier (Netdev Foundation)]

Pause autonegotiation behaves differently than the regular link params
autonegotiation, in that the user intention may differ from the
negotiated pause parameters.

Introduce a test that validates we're correctly resolving pause
parameters according to 802.3.

The validation is done based on what the local interface reports in
terms of advertised and lp_advertised linkmodes.

If the remote can express what it sees in terms of its own advertised
and lp_advertised modes, validate the autoneg results on the peer as
well.

Signed-off-by: Maxime Chevallier (Netdev Foundation) <maxime.chevallier@bootlin.com>
---
 .../testing/selftests/drivers/net/hw/pause.py | 308 ++++++++++++++++++
 1 file changed, 308 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
index 854428e19f55..cd117296be33 100755
--- a/tools/testing/selftests/drivers/net/hw/pause.py
+++ b/tools/testing/selftests/drivers/net/hw/pause.py
@@ -84,6 +84,41 @@ def wait_for_aneg(cfg, link_drop=False, timeout=15):
 
     wait_for_link(cfg, timeout)
 
+def controllable_lp(cfg):
+    """ Whether the remote interface is the link partner of the local one.
+
+    For low level ethtool tests, we need to have the remote directly connected
+    to the local host (i.e. not through a switch).
+
+    This is tested by taking triggering a link renegotiation and checking the
+    carrier_changes counter on the partner.
+
+    :returns: True if the remote is our link partner
+    """
+    controllable = getattr(cfg, "lp_controllable", None)
+    if controllable is not None:
+        return controllable
+
+    before = int(cmd(f"cat /sys/class/net/{cfg.remote_ifname}/carrier_changes",
+                     host=cfg.remote).stdout)
+
+    ret, _ = ethtool_ret(f"-r {cfg.ifname}", is_get=False)
+    if ret != 0:
+        raise KsftSkipEx(f"ethtool -r {cfg.ifname} failed: {errno.errorcode.get(ret, ret)}")
+
+    wait_for_aneg(cfg, link_drop=True)
+
+    cfg.lp_controllable = int(cmd(f"cat /sys/class/net/{cfg.remote_ifname}/carrier_changes",
+                                  host=cfg.remote).stdout) > before
+    return cfg.lp_controllable
+
+def require_controllable_lp(cfg):
+    """ Skip if the remote isn't directly controllable, e.g. accessed through
+        a switch
+    """
+    if not controllable_lp(cfg):
+        raise KsftSkipEx(f"{cfg.remote_ifname} is not directly connected to {cfg.ifname}")
+
 def forced_link_settings(cfg):
     """ Returns a string to pass to ethtool -s with speed/duplex corresponding
         to the current settings.
@@ -108,6 +143,10 @@ def _ethtool_pause_use_to_linkmodes(use):
     else:
         return []
 
+def pause_to_linkmodes(rx, tx):
+    """ Convert the bool rx/tx pause into Pause/Asym """
+    return pauseparams_to_linkmodes[rx][tx]["linkmodes"]
+
 def set_local_pauseparams(cfg, rx, tx, aneg):
     """ set pauseparams : ethtool -A """
     rx_param = onoff(rx)
@@ -127,6 +166,27 @@ def get_local_pauseparams(cfg):
     """ get pauseparams : ethtool -a """
     return ethtool_ret(f"-a {cfg.ifname}", is_get=True)
 
+def set_peer_pauseparams(cfg, rx, tx, aneg):
+    """ set pauseparams on peer : ethtool -A """
+    rx_param = onoff(rx)
+    tx_param = onoff(tx)
+    aneg_param = onoff(aneg)
+
+    ret, _ = ethtool_ret(f"-A {cfg.remote_ifname} rx {rx_param} tx {tx_param}"
+                         f" autoneg {aneg_param}",
+                         is_get = False, host=cfg.remote)
+
+    if ret != 0:
+        raise KsftSkipEx(f"Can't set pauseparams on peer: {errno.errorcode.get(ret, ret)}")
+
+    return ret
+
+def get_peer_pauseparams(cfg):
+    """ get pauseparams on peer : ethtool -a """
+
+    return ethtool_ret(f"-a {cfg.remote_ifname}", is_get=True,
+                       host=cfg.remote)
+
 def get_local_pause_supported(cfg):
     """ Return the errcode and the local pause supported linkmodes """
 
@@ -151,6 +211,61 @@ def get_local_pause_advertising(cfg):
 
     return ret, _ethtool_pause_use_to_linkmodes(data["advertised-pause-frame-use"])
 
+def get_local_pause_lp_advertising(cfg):
+    """ Return the errcode and the local pause lp_advertised linkmodes,
+        if any.
+    """
+
+    ret, data = ethtool_ret(f"{cfg.ifname}")
+    if ret != 0:
+        raise KsftFailEx(f"ethtool {cfg.ifname} failed: {errno.errorcode.get(ret, ret)}")
+
+    if "link-partner-advertised-pause-frame-use" in data:
+        return ret, _ethtool_pause_use_to_linkmodes(data["link-partner-advertised-pause-frame-use"])
+    else:
+        ksft_pr(f"Warning: {cfg.ifname} does not report the LP's advertising")
+        return errno.EOPNOTSUPP, None
+
+def get_peer_pause_supported(cfg):
+    """ Return the errcode and the remote pause supported linkmodes """
+
+    ret, data = ethtool_ret(f"{cfg.remote_ifname}", host = cfg.remote)
+    if ret != 0:
+        raise KsftFailEx(f"ethtool {cfg.remote_ifname} failed: {errno.errorcode.get(ret, ret)}")
+
+    if "supported-pause-frame-use" not in data:
+        raise KsftSkipEx(f"{cfg.remote_ifname} doesn't report its link modes")
+
+    return ret, _ethtool_pause_use_to_linkmodes(data["supported-pause-frame-use"])
+
+
+def get_peer_pause_advertising(cfg):
+    """ Return the errcode and the remote pause advertised linkmodes """
+
+    ret, data = ethtool_ret(f"{cfg.remote_ifname}", host = cfg.remote)
+    if ret != 0:
+        raise KsftFailEx(f"ethtool {cfg.remote_ifname} failed: {errno.errorcode.get(ret, ret)}")
+
+    if "advertised-pause-frame-use" not in data:
+        raise KsftSkipEx(f"{cfg.remote_ifname} doesn't report its link modes")
+
+    return ret, _ethtool_pause_use_to_linkmodes(data["advertised-pause-frame-use"])
+
+def get_peer_pause_lp_advertising(cfg):
+    """ Return the errcode and the peer pause lp_advertised linkmodes,
+        if any.
+    """
+
+    ret, data = ethtool_ret(f"{cfg.remote_ifname}", host = cfg.remote)
+    if ret != 0:
+        raise KsftFailEx(f"ethtool {cfg.remote_ifname} failed: {errno.errorcode.get(ret, ret)}")
+
+    if "link-partner-advertised-pause-frame-use" in data:
+        return ret, _ethtool_pause_use_to_linkmodes(data["link-partner-advertised-pause-frame-use"])
+    else:
+        ksft_pr(f"Warning: {cfg.remote_ifname} does not report the LP's advertising")
+        return errno.EOPNOTSUPP, None
+
 def require_pause_supported_allof(cfg, linkmodes):
     """ Skip if local device doesn't support all of the passed modes """
 
@@ -163,6 +278,18 @@ def require_pause_supported_allof(cfg, linkmodes):
         if lm not in pause_support:
             raise KsftSkipEx(f"Local device doesn't support {lm}")
 
+def require_peer_pause_supported_allof(cfg, linkmodes):
+    """ Skip if local device doesn't support all of the passed modes """
+
+    ret, _ = get_peer_pauseparams(cfg)
+    if ret != 0:
+        raise KsftSkipEx("Remote device doesn't allow getting pauseparams")
+
+    _, pause_support = get_peer_pause_supported(cfg)
+    for lm in linkmodes:
+        if lm not in pause_support:
+            raise KsftSkipEx(f"Remote device doesn't support {lm}")
+
 def expect_pauseparams_set(cfg, pauseparams, aneg, supported, note):
     """ ethtool -A must work or be refused depending on what the local
         device supports
@@ -330,6 +457,7 @@ def pause_advertising_test(cfg, pauseparams):
 
     require_pause_supported_allof(cfg, pauseparams["linkmodes"])
     pause_setup(cfg)
+    lp = controllable_lp(cfg)
 
     tx = pauseparams["tx"]
     rx = pauseparams["rx"]
@@ -356,11 +484,191 @@ def pause_advertising_test(cfg, pauseparams):
         ksft_not_in(mode, linkmodes,
                     f"rx {rx} tx {tx} aneg on must not advertise {not_adv}")
 
+    if not lp:
+        return
+
+    returncode, remote_linkmodes = get_peer_pause_lp_advertising(cfg)
+    if returncode == errno.EOPNOTSUPP:
+        return
+
+    for mode in adv:
+        ksft_in(mode, remote_linkmodes, f"PHY does not advertise {adv}")
+
+    for mode in not_adv:
+        ksft_not_in(mode, remote_linkmodes,
+                    f"PHY incorrectly advertises {not_adv}")
+
+
+# Pause autonegotiation resolution : Resolved pause settings vs configured
+# pauseparams on local device and link partner
+@ksft_variants([
+    # We advertise nothing, all off
+    KsftNamedVariant("local rx off tx off, remote rx off tx off",
+        {"rx": 0, "tx": 0, "lp_rx": 0, "lp_tx": 0, "neg_rx": 0, "neg_tx": 0}),
+
+    # We advertise nothing, all off
+    KsftNamedVariant("local rx off tx off, remote rx off tx on",
+        {"rx": 0, "tx": 0, "lp_rx": 0, "lp_tx": 1, "neg_rx": 0, "neg_tx": 0}),
+
+    # We advertise nothing, all off
+    KsftNamedVariant("local rx off tx off, remote rx on tx off",
+        {"rx": 0, "tx": 0, "lp_rx": 1, "lp_tx": 0, "neg_rx": 0, "neg_tx": 0}),
+
+    # We advertise nothing, all off
+    KsftNamedVariant("local rx off tx off, remote rx on tx on",
+        {"rx": 0, "tx": 0, "lp_rx": 1, "lp_tx": 1, "neg_rx": 0, "neg_tx": 0}),
+
+    # LP advertises nothing, all off
+    KsftNamedVariant("local rx off tx on, remote rx off tx off",
+        {"rx": 0, "tx": 1, "lp_rx": 0, "lp_tx": 0, "neg_rx": 0, "neg_tx": 0}),
+
+    # We advertise Asym, LP advertises Asym, all off
+    KsftNamedVariant("local rx off tx on, remote rx off tx on",
+        {"rx": 0, "tx": 1, "lp_rx": 0, "lp_tx": 1, "neg_rx": 0, "neg_tx": 0}),
+
+    # We advertise Asym, LP advertises Pause + Asym, tx on
+    KsftNamedVariant("local rx off tx on, remote rx on tx off",
+        {"rx": 0, "tx": 1, "lp_rx": 1, "lp_tx": 0, "neg_rx": 0, "neg_tx": 1}),
+
+    # Tricky case :
+    # We advertise Asym, LP advertises Pause, resolves to all off
+    KsftNamedVariant("local rx off tx on, remote rx on tx on",
+        {"rx": 0, "tx": 1, "lp_rx": 1, "lp_tx": 1, "neg_rx": 0, "neg_tx": 0}),
+
+    # LP advertises nothing, all off
+    KsftNamedVariant("local rx on tx off, remote rx off tx off",
+        {"rx": 1, "tx": 0, "lp_rx": 0, "lp_tx": 0, "neg_rx": 0, "neg_tx": 0}),
+
+    # We advertise Pause + Asym , LP advertises Asym, rx on
+    KsftNamedVariant("local rx on tx off, remote rx off tx on",
+        {"rx": 1, "tx": 0, "lp_rx": 0, "lp_tx": 1, "neg_rx": 1, "neg_tx": 0}),
+
+    # Also tricky: Only rx enabled on both ends, but we negotiate rx/tx
+    # We advertise Pause + Asym, LP advertises Pause + Asym, all on
+    KsftNamedVariant("local rx on tx off, remote rx on tx off",
+        {"rx": 1, "tx": 0, "lp_rx": 1, "lp_tx": 0, "neg_rx": 1, "neg_tx": 1}),
+
+    # We advertise Pause + Asym, LP advertises Pause, all on
+    KsftNamedVariant("local rx on tx off, remote rx on tx on",
+        {"rx": 1, "tx": 0, "lp_rx": 1, "lp_tx": 1, "neg_rx": 1, "neg_tx": 1}),
+
+    # LP advertises nothing, all off
+    KsftNamedVariant("local rx on tx on, remote rx off tx off",
+        {"rx": 1, "tx": 1, "lp_rx": 0, "lp_tx": 0, "neg_rx": 0, "neg_tx": 0}),
+
+    # Tricky case :
+    # We advertise Pause, LP advertises Asym, resolves to all off
+    KsftNamedVariant("local rx on tx on, remote rx off tx on",
+        {"rx": 1, "tx": 1, "lp_rx": 0, "lp_tx": 1, "neg_rx": 0, "neg_tx": 0}),
+
+    # We advertise Pause, LP advertises Pause + Asym, all on
+    KsftNamedVariant("local rx on tx on, remote rx on tx off",
+        {"rx": 1, "tx": 1, "lp_rx": 1, "lp_tx": 0, "neg_rx": 1, "neg_tx": 1}),
+
+    # We advertise Pause, LP advertises Pause, all on
+    KsftNamedVariant("local rx on tx on, remote rx on tx on",
+        {"rx": 1, "tx": 1, "lp_rx": 1, "lp_tx": 1, "neg_rx": 1, "neg_tx": 1}),
+])
+@ksft_disruptive
+def pause_aneg_resolution(cfg, settings):
+    """ Verify that rx and tx pause parameters are negotiated according to 802.3
+
+    802.3 dictates the rules for pause negotiation, all 16 cases are tested, one
+    for each combination of Pause and Asym_Pause advertising on the local device
+    and the link-partner.
+
+    This test also verifies that the peer resolved the parameters correctly,
+    to ensure the negotiation is triggered correctly.
+
+    Failing this test can happen if :
+     - The MAC accepts the pause parameters but doesn't trigger a link
+       renegotiation
+     - that the PHY driver manually overwrites the Pause negotiation result
+     - that the MAC driver ignores the Pause resolution and sets its own
+       pause parameters regardless
+    """
+
+    expected_local_rx = settings["neg_rx"]
+    expected_local_tx = settings["neg_tx"]
+
+    required_local_linkmodes = pause_to_linkmodes(settings["rx"],
+                                                  settings["tx"])
+    required_remote_linkmodes = pause_to_linkmodes(settings["lp_rx"],
+                                                   settings["lp_tx"])
+
+    require_pause_supported_allof(cfg, required_local_linkmodes)
+    require_peer_pause_supported_allof(cfg, required_remote_linkmodes)
+    pause_setup(cfg)
+    require_controllable_lp(cfg)
+
+    # There's symmetry between local device and LP on pause negotiation:
+    # - if local resolves all off or all on, LP must resolve the same
+    # - if local resolves RX only, remote must resolve to TX only
+    # - if local resolves TX only, remote must resolve to RX only
+    if expected_local_rx == expected_local_tx:
+        expected_lp_rx = expected_local_rx
+        expected_lp_tx = expected_local_tx
+    else:
+        expected_lp_rx = expected_local_tx
+        expected_lp_tx = expected_local_rx
+
+    # Set pauseparams
+    ret = set_local_pauseparams(cfg, settings["rx"], settings["tx"], True)
+    if ret == errno.EOPNOTSUPP:
+        raise KsftSkipEx(f"RX {settings['rx']} TX {settings['tx']} not supported")
+
+    wait_for_aneg(cfg)
+
+    set_peer_pauseparams(cfg, settings["lp_rx"], settings["lp_tx"], True)
+
+    # Wait for link to re-negotiate
+    wait_for_aneg(cfg)
+
+    if get_local_pause_lp_advertising(cfg)[0] != 0:
+        raise KsftSkipEx("Local device doesn't report the LP's advertising")
+
+    ret, local_pauseparams = get_local_pauseparams(cfg)
+    if ret != 0 or "negotiated" not in local_pauseparams:
+        raise KsftSkipEx("Local device doesn't report the negotiated pause params")
+
+    # check adv
+    _, linkmodes = get_local_pause_advertising(cfg)
+    ksft_eq(linkmodes, required_local_linkmodes,
+            f"local rx {settings['rx']} tx {settings['tx']} must advertise "
+            f"{required_local_linkmodes}")
+
+    _, linkmodes = get_peer_pause_advertising(cfg)
+    ksft_eq(linkmodes, required_remote_linkmodes,
+            f"remote rx {settings['lp_rx']} tx {settings['lp_tx']} must advertise "
+            f"{required_remote_linkmodes}")
+
+    # check lp_adv if available
+    _, linkmodes = get_local_pause_lp_advertising(cfg)
+    ksft_eq(linkmodes, required_remote_linkmodes,
+            f"local lp_adv must show the remote's {required_remote_linkmodes}")
+
+    # check lp_adv on remote
+    ret, linkmodes = get_peer_pause_lp_advertising(cfg)
+    if ret == 0:
+        ksft_eq(linkmodes, required_local_linkmodes,
+                f"remote lp_adv must show our {required_local_linkmodes}")
+
+    # Check resolution
+    _, local_pauseparams = get_local_pauseparams(cfg)
+    ksft_eq(local_pauseparams["negotiated"]["rx"], expected_local_rx)
+    ksft_eq(local_pauseparams["negotiated"]["tx"], expected_local_tx)
+
+    ret, remote_pauseparams = get_peer_pauseparams(cfg)
+    if ret == 0 and "negotiated" in remote_pauseparams:
+        ksft_eq(remote_pauseparams["negotiated"]["rx"], expected_lp_rx)
+        ksft_eq(remote_pauseparams["negotiated"]["tx"], expected_lp_tx)
+
 def main():
     """ Entry point """
     with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
         ksft_run([pause_test_support,
                   pause_advertising_test,
+                  pause_aneg_resolution,
                   ],
                  args=(cfg, ))
     ksft_exit()
-- 
2.55.0

[PATCH net-next v3 3/3] selftests: drv-net: pause: Validate pause autoneg interactions with link autoneg

[Maxime Chevallier (Netdev Foundation)]

Pause autonegotiation happens with the link partner using the same words
as the link negotiation, used for speed and duplex exchanges. However,
pause and link autonegotiation can be separately toggled :

ethtool -s eth0 autoneg on # Enable link negotiation
ethtool -A eth0 autoneg on # Enable Pause negotiation

Pause can't be negotiated if the link autoneg isn't enabled.

Pause autoneg and link autoneg settings must not interfere with one
another when user is configuring them :
 - If Pause autoneg is on, it must stay on when link autoneg is disabled
   (even though Pause won't actually be negotiated)
 - If Pause autoneg is off, it must stay off when link autoneg is
   enabled

Introduce a set of tests to verify that pause and link autoneg are
behaving correctly, in particular that the user intent is correctly
cached when toggling link autoneg.

Signed-off-by: Maxime Chevallier (Netdev Foundation) <maxime.chevallier@bootlin.com>
---
 .../testing/selftests/drivers/net/hw/pause.py | 269 +++++++++++++++++-
 1 file changed, 268 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
index cd117296be33..ca4a55e59c30 100755
--- a/tools/testing/selftests/drivers/net/hw/pause.py
+++ b/tools/testing/selftests/drivers/net/hw/pause.py
@@ -119,6 +119,17 @@ def require_controllable_lp(cfg):
     if not controllable_lp(cfg):
         raise KsftSkipEx(f"{cfg.remote_ifname} is not directly connected to {cfg.ifname}")
 
+def require_link_autoneg(cfg):
+    """ Skip if local or remote device don't support link aneg """
+    # Does local device support link aneg
+    if not ethtool(f"{cfg.ifname}", json=True)[0]["supports-auto-negotiation"]:
+        raise KsftSkipEx(f"{cfg.ifname} doesn't support link autoneg")
+
+    # Does remote device support link aneg
+    if not ethtool(f"{cfg.remote_ifname}",
+                   json=True, host=cfg.remote)[0]["supports-auto-negotiation"]:
+        raise KsftSkipEx(f"Remote {cfg.remote_ifname} doesn't support link autoneg")
+
 def forced_link_settings(cfg):
     """ Returns a string to pass to ethtool -s with speed/duplex corresponding
         to the current settings.
@@ -266,6 +277,20 @@ def get_peer_pause_lp_advertising(cfg):
         ksft_pr(f"Warning: {cfg.remote_ifname} does not report the LP's advertising")
         return errno.EOPNOTSUPP, None
 
+def require_pause_supported_anyof(cfg, linkmodes):
+    """ Skip if local device doesn't support any of the passed modes """
+
+    ret, _ = get_local_pauseparams(cfg)
+    if ret != 0:
+        raise KsftSkipEx("device doesn't allow getting pauseparams")
+
+    _, pause_support = get_local_pause_supported(cfg)
+    for lm in linkmodes:
+        if lm in pause_support:
+            return
+
+    raise KsftSkipEx(f"Local device doesn't support any of {linkmodes}")
+
 def require_pause_supported_allof(cfg, linkmodes):
     """ Skip if local device doesn't support all of the passed modes """
 
@@ -290,8 +315,51 @@ def require_peer_pause_supported_allof(cfg, linkmodes):
         if lm not in pause_support:
             raise KsftSkipEx(f"Remote device doesn't support {lm}")
 
+def supported_pauseparams(cfg):
+    """ The rx/tx params covering every mode the local device supports """
+
+    _, pause_support = get_local_pause_supported(cfg)
+    if "Pause" in pause_support:
+        return 1, 1
+    if "Asym_Pause" in pause_support:
+        return 0, 1
+
+    raise KsftSkipEx("Local device doesn't support pause")
+
+def set_local_pause_autoneg(cfg, aneg):
+    """ Enable/Disable local pause autoneg """
+
+    ret, _ = ethtool_ret(f"-A {cfg.ifname} autoneg {onoff(aneg)}",
+                         is_get=False)
+    return ret
+
+def check_local_pauseparams(cfg, aneg, rx, tx):
+    """ Check that the local pauseparams are the passed parameters """
+
+    ret, params = get_local_pauseparams(cfg)
+    ksft_eq(ret, 0)
+    if ret != 0:
+        return
+
+    ksft_eq(params["autonegotiate"], bool(aneg), "pause autoneg")
+    ksft_eq(params["rx"], bool(rx), "rx pause")
+    ksft_eq(params["tx"], bool(tx), "tx pause")
+
+def check_local_advertising(cfg, linkmodes):
+    """ Check that the local advertised modes are the passed parameters """
+
+    _, adv = get_local_pause_advertising(cfg)
+    ksft_eq(adv, linkmodes, "advertised pause modes")
+
+def check_local_lp_advertising(cfg, linkmodes):
+    """ Check that the local lp_advertised modes are the passed parameters """
+
+    ret, adv = get_local_pause_lp_advertising(cfg)
+    if ret == 0:
+        ksft_eq(adv, linkmodes, "link partner advertised pause modes")
+
 def expect_pauseparams_set(cfg, pauseparams, aneg, supported, note):
-    """ ethtool -A must work or be refused depending on what the local
+    """ Whether ethtool -A had to work or to be refused, given what the local
         device supports
     """
     rx = pauseparams["rx"]
@@ -663,12 +731,211 @@ def pause_aneg_resolution(cfg, settings):
         ksft_eq(remote_pauseparams["negotiated"]["rx"], expected_lp_rx)
         ksft_eq(remote_pauseparams["negotiated"]["tx"], expected_lp_tx)
 
+@ksft_disruptive
+def pause_autoneg_state_adv(cfg):
+    """Validate that disabling pause autoneg keeps the advertised linkmodes
+
+    With pause autoneg disabled, the pause params are enforced as what the user
+    intended, but the advertising still follows the rx and tx pause params so
+    that the link partner resolves its own pause settings consistently.
+
+    Failing this test means that the MAC or PHY driver changes the
+    Pause / Asym_Pause advertising when pause autoneg is disabled.
+    """
+
+    require_pause_supported_anyof(cfg, ["Pause", "Asym_Pause"])
+    require_link_autoneg(cfg)
+
+    pause_setup(cfg)
+    require_controllable_lp(cfg)
+
+    set_peer_pauseparams(cfg, True, True, True)
+    wait_for_aneg(cfg)
+
+    rx, tx = supported_pauseparams(cfg)
+
+    # Enable all possible pauseparams with pause autoneg
+    ret = set_local_pauseparams(cfg, rx, tx, True)
+    ksft_eq(ret, 0)
+    wait_for_aneg(cfg)
+
+    # Make sure we advertise them
+    ret, adv = get_local_pause_advertising(cfg)
+    ksft_eq(ret, 0)
+    ksft_eq(adv, pause_to_linkmodes(rx, tx))
+
+    # Disable pause autoneg
+    ret = set_local_pauseparams(cfg, rx, tx, False)
+    ksft_eq(ret, 0)
+
+    # This may trigger a link renegociation
+    wait_for_aneg(cfg)
+
+    ret, adv = get_local_pause_advertising(cfg)
+    ksft_eq(ret, 0)
+    ksft_eq(adv, pause_to_linkmodes(rx, tx))
+
+    ret, adv = get_peer_pause_lp_advertising(cfg)
+    if ret == errno.EOPNOTSUPP:
+        return
+
+    ksft_eq(ret, 0)
+    ksft_eq(adv, pause_to_linkmodes(rx, tx))
+
+@ksft_disruptive
+def pause_autoneg_state_params(cfg):
+    """Validate the pause params when transitioning between fixed pause
+       params and negotiated ones. The goal is to make sure that user
+       intent on the RX and TX pause params are stored when user decides
+       to use negotiated parameters instead. The main gotcha lies on the
+       fact that when pause autoneg is used, the autoneg result may differ
+       from the user intent.
+
+    Failing this test means the MAC driver is overwriting the user intent
+    when switching to forced pause.
+    """
+
+    require_pause_supported_allof(cfg, ["Pause", "Asym_Pause"])
+    require_peer_pause_supported_allof(cfg, ["Pause"])
+    require_link_autoneg(cfg)
+    pause_setup(cfg)
+    require_controllable_lp(cfg)
+
+    # Set peer user intent to RX on TX on, with Pause autoneg on
+    set_peer_pauseparams(cfg, 1, 1, True)
+    wait_for_aneg(cfg)
+
+    # Set the local intent to RX on TX off with pause autoneg
+    ksft_eq(set_local_pauseparams(cfg, 1, 0, True), 0)
+    wait_for_aneg(cfg)
+
+    check_local_pauseparams(cfg, True, 1, 0)
+    check_local_advertising(cfg, ["Pause", "Asym_Pause"])
+    check_local_lp_advertising(cfg, ["Pause"])
+
+    # Disable pause autoneg
+    ksft_eq(set_local_pause_autoneg(cfg, False), 0)
+    wait_for_aneg(cfg)
+
+    # The pauseparams must still be what we configured before, and not the
+    # previously negotiated ones
+    check_local_pauseparams(cfg, False, 1, 0)
+
+    # Re-enable autoneg
+    ksft_eq(set_local_pause_autoneg(cfg, True), 0)
+    wait_for_aneg(cfg)
+
+    check_local_pauseparams(cfg, True, 1, 0)
+    # We must be advertising our intent again, and not RX on TX on, which would
+    # be "Pause" only.
+    check_local_advertising(cfg, ["Pause", "Asym_Pause"])
+    check_local_lp_advertising(cfg, ["Pause"])
+
+@ksft_disruptive
+def pause_autoneg_off_while_link_autoneg_on(cfg):
+    """ Validate that when link autoneg is on but pause autoneg is off, we do
+        not use negotiated pause parameters.
+
+        Failing this test means the MAC driver incorrectly accounts for the
+        negotiated pause parameters even with pause aneg off, likely due to
+        confusion between link autoneg and pause autoneg.
+    """
+
+    require_pause_supported_anyof(cfg, ["Pause", "Asym_Pause"])
+    require_peer_pause_supported_allof(cfg, ["Pause"])
+    require_link_autoneg(cfg)
+    pause_setup(cfg)
+    require_controllable_lp(cfg)
+
+    rx, tx = supported_pauseparams(cfg)
+
+    # Enable pause autoneg with all the locally supported modes enabled
+    set_peer_pauseparams(cfg, 1, 1, True)
+    ksft_eq(set_local_pauseparams(cfg, rx, tx, True), 0)
+    wait_for_aneg(cfg)
+
+    # Disable Pause autoneg
+    ksft_eq(set_local_pauseparams(cfg, rx, tx, False), 0)
+    wait_for_aneg(cfg)
+    # Pause autoneg must read "disabled"
+    check_local_pauseparams(cfg, False, rx, tx)
+
+    set_peer_pauseparams(cfg, 0, 0, True)
+    wait_for_aneg(cfg)
+
+    ethtool(f"-r {cfg.ifname}")
+    wait_for_aneg(cfg, link_drop=True)
+
+    # Pause autoneg must still be off even after a link renegotiation
+    check_local_pauseparams(cfg, False, rx, tx)
+
+@ksft_disruptive
+def pause_autoneg_link_autoneg(cfg):
+    """Validate pause autoneg and link autoneg interactions. The link autoneg's
+       admin status (i.e. do we autoneg link parameters or force them) must not
+       impact the pause autoneg status. While link autoneg is disabled, we don't
+       negotiate the pause params, however we must keep pause autoneg on as this
+       is the user intent. When link autoneg is re-enabled, pause params must be
+       derived from the negotiation.
+    """
+
+    require_pause_supported_anyof(cfg, ["Pause", "Asym_Pause"])
+    require_peer_pause_supported_allof(cfg, ["Pause"])
+    require_link_autoneg(cfg)
+    pause_setup(cfg)
+    require_controllable_lp(cfg)
+
+    rx, tx = supported_pauseparams(cfg)
+    adv = pause_to_linkmodes(rx, tx)
+
+    # Enable all possible pause modes and autoneg
+    set_peer_pauseparams(cfg, 1, 1, True)
+    ksft_eq(set_local_pauseparams(cfg, rx, tx, True), 0)
+    wait_for_aneg(cfg)
+
+    # Disable link autoneg, at the speed and duplex the link runs at
+    forced = forced_link_settings(cfg)
+    if not forced:
+        raise KsftSkipEx("Can't tell what to force the link at")
+
+    ret, _ = ethtool_ret(f"-s {cfg.remote_ifname} autoneg off {forced}",
+                         is_get=False, host=cfg.remote)
+    if ret != 0:
+        raise KsftSkipEx(f"Can't force the peer's link at {forced}")
+
+    ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg off {forced}",
+                         is_get=False)
+    if ret != 0:
+        raise KsftSkipEx(f"Can't force the link at {forced}")
+
+    try:
+        wait_for_aneg(cfg)
+    except TimeoutError:
+        raise KsftSkipEx(f"No link when forced at {forced}")
+
+    # We must have pause autoneg still enabled, even if we don't negotiate pause
+    check_local_pauseparams(cfg, True, rx, tx)
+
+    # Re-enable autoneg
+    ethtool(f"-s {cfg.remote_ifname} autoneg on", host=cfg.remote)
+    ethtool(f"-s {cfg.ifname} autoneg on")
+    wait_for_aneg(cfg)
+
+    # Pause autoneg must still be on
+    check_local_pauseparams(cfg, True, rx, tx)
+    check_local_advertising(cfg, adv)
+    check_local_lp_advertising(cfg, ["Pause"])
+
 def main():
     """ Entry point """
     with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
         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()
-- 
2.55.0

Re: [PATCH net-next v3 1/3] selftests: drv-net: Introduce a selftest for ethtool flow control

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 5 · Low: 5

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

- [Medium] In tools/testing/selftests/drivers/net/hw/pause.py,
  expect_pauseparams_set() takes an `aneg` argument but only uses it to…
- [Medium] expect_pauseparams_set() decides acceptance by checking
  whether the 802.3 advertisement encoding of a request (Pause = rx,…
- [Medium] expect_pauseparams_set() checks the 'noop' case first (lines
  182-183): requested rx/tx/autoneg equals what `ethtool -a` reports.
- [Medium] pause_setup() queues restore commands with defer(cmd, ...),
  using cmd's default fail=True, whenever the GET succeeds.
- [Medium] pause_test_support() treats phylink fixed-links as having
  writable pause settings whenever their supported mask contains Pause…
- [Low] In this patch, pause_advertising_test's docstring (lines 327-328)
  says validation uses "the advertised modes locally, as well as what…
- [Low] pause_test_support's docstring (lines 262-264) says drivers must
  refuse unsupported pauseparams "with a -EOPNOTSUPP return value".
- [Low] pause_test_support() can return normally without making a single
  assertion, and is then reported as PASS.
- [Low] pause_setup()'s docstring says the starting conditions are
  "restored on exit".
- [Low] wait_for_aneg() with link_drop=False (the default, used by
  pause_advertising_test at line 348) waits a hard-coded 2 s for carrier…

--

selftests: drv-net: Introduce a selftest for ethtool flow control

This adds a pause.py driver selftest. It checks that the Pause and
Asym_Pause bits in the supported link modes match the rx/tx combinations
that "ethtool -A" accepts. It also checks that setting pause parameters
with autoneg on is reflected in the locally advertised pause modes.

> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> new file mode 100755
> index 0000000000000..854428e19f55f
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
> @@ -0,0 +1,369 @@

[ ... ]

> +def wait_for_aneg(cfg, link_drop=False, timeout=15):
> +    """ Wait for a renegotiation to complete
> +
> +    :param link_drop: Set to true if the link HAS to flap.
> +    """
> +    try:
> +        wait_file(f"/sys/class/net/{cfg.ifname}/carrier",
> +                  lambda x: x.strip() == "0",
> +                  deadline=timeout if link_drop else 2)
> +    except TimeoutError:
> +        if link_drop:
> +            raise
> +
> +    wait_for_link(cfg, timeout)

[Severity: Low]
pause_advertising_test() calls this with link_drop=False. In that case the
carrier is only watched for 2 seconds and the TimeoutError is swallowed.

The PHY or firmware may report link loss later than that, for example
with polled phylib PHYs or firmware-managed links that report
asynchronously. Carrier then still reads the stale "1", and
wait_for_link() returns right away.

If so, is the "It must come back up" condition in pause_advertising_test()
actually verified? Could the next test case also start while
autonegotiation is still running, so a failure is blamed on the wrong
case?

[ ... ]

> +def expect_pauseparams_set(cfg, pauseparams, aneg, supported, note):
> +    """ ethtool -A must work or be refused depending on what the local
> +        device supports
> +    """
> +    rx = pauseparams["rx"]
> +    tx = pauseparams["tx"]
> +
> +    # Measure current pauseparams, re-setting the same params as the current
> +    # ones returns 0 even if unsupported.
> +    _, cur = get_local_pauseparams(cfg)
> +    noop = cur and (cur["rx"], cur["tx"], cur["autonegotiate"]) == \
> +           (bool(rx), bool(tx), aneg)
> +
> +    ret, _ = ethtool_ret(f"-A {cfg.ifname} rx {onoff(rx)} tx {onoff(tx)}"
> +                         f" autoneg {onoff(aneg)}", is_get = False)
> +
> +    if noop:
> +        ksft_eq(ret, 0, note)

[Severity: Medium]
Is the comment about re-setting the same parameters accurate for drivers
that implement get_pauseparam but not set_pauseparam?

The validate step runs before the no-op shortcut in ops->set:

net/ethtool/pause.c:ethnl_set_pause_validate() {
    ...
	return ops->get_pauseparam && ops->set_pauseparam ? 1 : -EOPNOTSUPP;
}

net/ethtool/netlink.c:ethnl_default_set_doit() {
    ...
		ret = ops->set_validate(req_info, info);
		/* 0 means nothing to do */
		if (ret <= 0)
			goto out_dev;
    ...
}

Take a get-only driver such as mlxbf_gige. It reports rx on, tx on and
autoneg off, and has no .set_pauseparam. The variant that matches its
current state gets noop=True, and then ksft_eq(EOPNOTSUPP, 0) fails.

The noop branch is also checked before "elif not supported". A get-only
driver with an empty supported mask fails here, even though the test's
own expectation for that case is EOPNOTSUPP.

Should the noop case only apply when the driver can actually set pause
parameters, or be checked after the "not supported" case?

> +    elif not supported:
> +        ksft_eq(ret, errno.EOPNOTSUPP, note)
> +    elif set(pauseparams["linkmodes"]).issubset(set(supported)):
> +        ksft_eq(ret, 0, note)

[Severity: Medium]
The aneg argument is only used to detect the no-op case. Here ret == 0 is
required whatever the pause autoneg request and the link autoneg state.

The struct ethtool_pauseparam documentation in
include/uapi/linux/ethtool.h says:

 * Drivers should reject a non-zero setting of @autoneg when
 * autoneogotiation is disabled (or not supported) for the link.

pause_test_support() calls this with aneg=True after forcing the link
with "autoneg off speed X duplex Y". Several in-tree drivers reject that,
as the uAPI documentation says they should:

  xgbe_set_pauseparam() returns -EINVAL when pause->autoneg is set and
  link autoneg is off.

  bnxt_set_pauseparam() returns -EINVAL when BNXT_AUTONEG_SPEED is clear.

Some drivers also reject combinations with link autoneg on:

  mlx5e_ethtool_set_pauseparam() always returns -EINVAL for pause
  autoneg, but always reports Pause as supported.

  i40e_set_pauseparam() returns -EOPNOTSUPP whenever pause->autoneg
  differs from the link autoneg state, so aneg=False with link autoneg on
  is rejected.

Won't these drivers be reported as failing pause_test_support()?

pause_advertising_test() has the same issue. It hardcodes aneg=True:

    ret = set_local_pauseparams(cfg, rx, tx, True)

and set_local_pauseparams() turns anything other than EOPNOTSUPP into a
hard failure:

    if ret != 0 and ret != errno.EOPNOTSUPP:
        raise KsftFailEx(f"ethtool -A {cfg.ifname} failed: {ret}")

On mlx5, the RX off/TX off and RX on/TX on variants would then fail with
"ethtool -A ... failed: 22".

The commit message describes only the Pause/Asym to rx/tx mapping. It
doesn't mention a requirement to support pause autoneg. The last commit
of the series says "Pause can't be negotiated if the link autoneg isn't
enabled", but expect_pauseparams_set() is still unchanged at the end of
the series.

Should the expected result take into account the requested pause autoneg
value and the link autoneg state?

> +    else:
> +        ksft_in(ret, (errno.EOPNOTSUPP, errno.EINVAL), note)

[Severity: Medium]
Does this match the rule phylink applies? This branch decides acceptance
by whether the 802.3 advertisement encoding of the request is a subset of
the supported bits. phylink_ethtool_set_pauseparam() checks something
different:

	if (!phylink_test(pl->supported, Pause) &&
	    !phylink_test(pl->supported, Asym_Pause))
		return -EOPNOTSUPP;

	if (!phylink_test(pl->supported, Asym_Pause) &&
	    pause->rx_pause != pause->tx_pause)
		return -EINVAL;

include/linux/phylink.h also documents:

 * If you set MAC_ASYM_PAUSE, the user may request any combination of
 * tx_pause and rx_pause. You do not have to support these
 * combinations.

Consider a phylink device whose supported set is only Asym_Pause
("Transmit-only"). The RX on/TX on (["Pause"]) and RX on/TX off
(["Pause", "Asym_Pause"]) variants are not subsets, so this branch
expects EOPNOTSUPP or EINVAL. phylink returns 0 for both.

I couldn't find an in-tree phylink MAC that implements set_pauseparam and
ends up with an Asym-only supported mask. macb uses MAC_ASYM_PAUSE without
MAC_SYM_PAUSE, but it has no pauseparam ops.

The pause_test_support() docstring cites phylink mac_capabilities as the
reference. Should the expectation follow phylink's rule?

> +
> +def pause_setup(cfg):
> +    """ The starting conditions every test counts on, restored on exit:
> +        - both ports admin up
> +        - link autoneg on on both sides ifsupported
> +        - carrier on
> +    """
> +
> +    # Get init pause parameters
> +    ret, params = ethtool_ret(f"-a {cfg.ifname}")
> +    if ret == 0:
> +        defer(cmd, f"ethtool -A {cfg.ifname} rx {onoff(params['rx'])} "
> +                   f"tx {onoff(params['tx'])} "
> +                   f"autoneg {onoff(params['autonegotiate'])}")
> +
> +    # Get init link parameters
> +    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")
> +
> +    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()}")

[Severity: Medium]
These restores are queued with cmd()'s default fail=True whenever the GET
succeeds, even if the device has no matching setter.

A restore to identical values still fails when the setter op is missing:

net/ethtool/linkmodes.c:ethnl_set_linkmodes_validate() {
    ...
	if (!ops->get_link_ksettings || !ops->set_link_ksettings)
		return -EOPNOTSUPP;
    ...
}

ethnl_set_pause_validate() does the same when set_pauseparam is missing.

The failing cmd() raises inside ksft_flush_defer(), which sets
KSFT_RESULT = False, and the case is reported as "not ok".

For example, iavf has get_link_ksettings, reports AUTONEG_DISABLE and has
no set_link_ksettings. Wouldn't every pause_test_support variant fail on
iavf, even when no assertion ran?

The remote side follows the same pattern. The remote "ethtool -A" restore
is deferred even though no test in this patch changes the remote pause
parameters. A remote that only has get_pauseparam would make every case
that reaches pause_setup() fail.

Could these restores be registered only when the setting is actually
changed, or not be treated as fatal?

> +
> +    # Local interface admin up, link aneg on
> +    ip(f"link set {cfg.ifname} up")
> +    if link["supports-auto-negotiation"] and not link["auto-negotiation"]:
> +        ethtool(f"-s {cfg.ifname} autoneg on")

[Severity: Low]
The docstring says the starting conditions are "restored on exit". Is the
admin state restored? No defer brings the interface back down if it
started admin-down. The remote "ip link set ... up" works the same way.

Autoneg is also turned on here whenever it is supported and currently
off. A restore was only registered above when the original state was
autoneg on, or autoneg off with both speed and duplex reported.

If the interface started with autoneg off and no speed/duplex (for
example with the link down), would autoneg be left on after the test? The
remote side has the same pattern.

[ ... ]

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

[Severity: Low]
This isn't a bug, but the docstring requires -EOPNOTSUPP, while
expect_pauseparams_set() also accepts EINVAL:

        ksft_in(ret, (errno.EOPNOTSUPP, errno.EINVAL), note)

phylink_ethtool_set_pauseparam(), which this docstring refers to, also
returns -EINVAL for rx != tx when Asym_Pause is not supported. Could the
docstring mention EINVAL too?

[ ... ]

> +    pause_setup(cfg)
> +
> +    forced = forced_link_settings(cfg)
> +    _, supported = get_local_pause_supported(cfg)
> +
> +    # We check that what we can configure in the pause params matches what we
> +    # support under various contditions : Link aneg on/off, pause aneg on/off
> +    ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg on", is_get = False)
> +    if ret != 0:
> +        ksft_pr("link autoneg on refused, not tested")
> +    else:
> +        expect_pauseparams_set(cfg, pauseparams, False, supported,
> +                               "link autoneg on")
> +        expect_pauseparams_set(cfg, pauseparams, True, supported,
> +                               "link autoneg on")

[Severity: Medium]
How does this behave on a phylink fixed-link DUT?

phylink_parse_fixedlink() keeps the MAC pause capabilities in
pl->supported:

	linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT, mask);
	linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, mask);
	linkmode_set_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, mask);
	linkmode_and(pl->supported, pl->supported, mask);

so ethtool reports pause as supported. However,
phylink_ethtool_set_pauseparam() refuses every change on fixed links:

	if (pl->req_link_an_mode == MLO_AN_FIXED)
		return -EOPNOTSUPP;

Re-applying the current link settings, either here or through the forced
"autoneg off speed X duplex Y" below, succeeds without reaching the
driver, because ethnl_set_linkmodes() returns early:

	if (!mod)
		return 0;

The test then reaches expect_pauseparams_set(). Every supported-subset
variant that differs from the current state expects 0 there, but gets
-EOPNOTSUPP.

The commit message presents phylink as the path that "deals with the
complexity". Would a phylink fixed-link interface be reported as a driver
bug here?

> +
> +    if not forced:
> +        ksft_pr("link speed unknown, the forced link is not tested")
> +        return
> +
> +    ret, _ = ethtool_ret(f"-s {cfg.ifname} autoneg off {forced}", is_get = False)
> +    if ret != 0:
> +        ksft_pr(f"link autoneg off {forced} refused, not tested")
> +        return

[Severity: Low]
Suppose "ethtool -s autoneg on" was refused above and one of these returns
is taken. The function then returns without running a single assertion.

ksft_run() counts a normal return without a failed ksft_* check as a pass,
and ksft_pr() doesn't mark a skip:

        if not cnt_key:
            cnt_key = 'pass' if KSFT_RESULT else 'fail'

If the device started with autoneg off and no speed/duplex, pause_setup()
registers no link restore, so a defer failure doesn't turn this into a
fail either.

Should this raise KsftSkipEx when neither configuration was exercised?

[ ... ]

> +    The validation is made by looking at the advertised modes locally, as well
> +    as what the peer's 'lp_advertising' values report.
> +    """

[Severity: Low]
The docstring mentions checking the peer's lp_advertising values. The body
of pause_advertising_test() in this patch only checks the local
advertised-pause-frame-use:

    _, linkmodes = get_local_pause_advertising(cfg)

The later commit in this series, "selftests: drv-net: pause: Validate the
pause autonegotiation with a partner", adds the
get_peer_pause_lp_advertising() check, so the docstring matches the code
by the end of the series. Could this sentence move to that patch?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015134350.629593-1-maxime.chevallier%40bootlin.com

Re: [PATCH net-next v3 2/3] selftests: drv-net: pause: Validate the pause autonegotiation with a partner

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 4 · Low: 1

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

- [Medium] In pause_aneg_resolution()
  (tools/testing/selftests/drivers/net/hw/pause.py), the '# Check…
- [Medium] This patch makes the existing pause_advertising_test() depend
  on a link-partner probe.
- [Medium] controllable_lp() decides whether the remote is the direct
  link partner from one timing-sensitive sample.
- [Medium] pause_advertising_test() (new remote lp_adv check, line 490)
  and pause_aneg_resolution() (remote lp_adv check at line 651, remote…
- [Low] The docstring of require_peer_pause_supported_allof()
  (tools/testing/selftests/drivers/net/hw/pause.py, line 282) says 'Skip…

--

selftests: drv-net: pause: Validate the pause autonegotiation with a partner

This adds a pause_aneg_resolution test to the drv-net pause selftest. It
runs all 16 combinations of local and link-partner rx/tx pause settings
and checks the advertised, lp_advertised and negotiated pause state. It
also adds a controllable_lp() probe to detect whether the remote is
directly connected, and extends pause_advertising_test() to check the
peer's lp_advertising.

> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> index 854428e19f55f..cd117296be333 100755
> --- a/tools/testing/selftests/drivers/net/hw/pause.py
> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
> @@ -84,6 +84,41 @@ def wait_for_aneg(cfg, link_drop=False, timeout=15):
>  
>      wait_for_link(cfg, timeout)
>  
> +def controllable_lp(cfg):

[ ... ]

> +    controllable = getattr(cfg, "lp_controllable", None)
> +    if controllable is not None:
> +        return controllable
> +
> +    before = int(cmd(f"cat /sys/class/net/{cfg.remote_ifname}/carrier_changes",
> +                     host=cfg.remote).stdout)
> +
> +    ret, _ = ethtool_ret(f"-r {cfg.ifname}", is_get=False)
> +    if ret != 0:
> +        raise KsftSkipEx(f"ethtool -r {cfg.ifname} failed: {errno.errorcode.get(ret, ret)}")
> +
> +    wait_for_aneg(cfg, link_drop=True)
> +
> +    cfg.lp_controllable = int(cmd(f"cat /sys/class/net/{cfg.remote_ifname}/carrier_changes",
> +                                  host=cfg.remote).stdout) > before
> +    return cfg.lp_controllable

[Severity: Medium]
Is a single read of the remote carrier_changes reliable enough here?

wait_for_aneg() only watches the local carrier, and nothing waits for the
remote to notice the flap. A polled phylib/phylink remote only sees
link-down on its next state machine poll (PHY_STATE_TIME, 1 s).

If the AN restart is fast (in-band SGMII/1000BASE-X) and the local link is
interrupt-driven, the local carrier can be back up before the remote has
counted the flap. In that case carrier_changes hasn't moved and the remote
is classified as not directly connected.

That False is then cached in cfg.lp_controllable. Wouldn't every
pause_aneg_resolution variant then skip with "not directly connected", and
the peer checks in pause_advertising_test() be skipped for the rest of the
run?

Would retrying, or polling the remote carrier_changes for a short time,
help? Later patches in the series don't seem to change controllable_lp().

[ ... ]

> @@ -163,6 +278,18 @@ def require_pause_supported_allof(cfg, linkmodes):
>          if lm not in pause_support:
>              raise KsftSkipEx(f"Local device doesn't support {lm}")
>  
> +def require_peer_pause_supported_allof(cfg, linkmodes):
> +    """ Skip if local device doesn't support all of the passed modes """

[Severity: Low]
This isn't a bug, but the docstring looks copied from
require_pause_supported_allof(). This helper checks the remote device, not
the local one.

[ ... ]

> @@ -330,6 +457,7 @@ def pause_advertising_test(cfg, pauseparams):
>  
>      require_pause_supported_allof(cfg, pauseparams["linkmodes"])
>      pause_setup(cfg)
> +    lp = controllable_lp(cfg)

[Severity: Medium]
Doesn't this make the existing local advertising checks in
pause_advertising_test() depend on the link partner probe?

controllable_lp() raises KsftSkipEx when ethtool -r fails:

    ret, _ = ethtool_ret(f"-r {cfg.ifname}", is_get=False)
    if ret != 0:
        raise KsftSkipEx(...)

On a driver without .nway_reset (-EOPNOTSUPP), or where
phy_ethtool_nway_reset() returns -ENODEV, every variant would now skip its
local ksft_in/ksft_not_in checks. Those checks don't need a partner.

ethtool -r can also succeed without the local carrier ever dropping. One
example is phylink_ethtool_nway_reset() with no phydev and a PCS that
doesn't restart AN. In that case wait_for_aneg(cfg, link_drop=True) lets
the TimeoutError from wait_file() escape. ksft_run() records that as a
failure, so a test that used to pass would now fail.

In both cases cfg.lp_controllable is never set. The probe, with its link
flap or 15 second timeout, then repeats for every pause_advertising_test
variant and all 16 pause_aneg_resolution variants.

In pause_aneg_resolution(), the same TimeoutError turns what should be a
"not directly connected" skip into a failure.

Could controllable_lp() return False, and cache it, in these cases? Later
patches in the series don't seem to change this.

[ ... ]

> @@ -356,11 +484,191 @@ def pause_advertising_test(cfg, pauseparams):
>          ksft_not_in(mode, linkmodes,
>                      f"rx {rx} tx {tx} aneg on must not advertise {not_adv}")
>  
> +    if not lp:
> +        return
> +
> +    returncode, remote_linkmodes = get_peer_pause_lp_advertising(cfg)
> +    if returncode == errno.EOPNOTSUPP:
> +        return
> +
> +    for mode in adv:
> +        ksft_in(mode, remote_linkmodes, f"PHY does not advertise {adv}")

[Severity: Medium]
Could the remote's lp_advertising still be stale at this point?

wait_for_aneg() only polls /sys/class/net/{cfg.ifname}/carrier. The
remote's phylib state machine runs on its own schedule. In polled mode it
only updates phydev->lp_advertising on its 1 s poll, and while AN is
incomplete genphy_read_lpa() clears the bits:

    if (!phydev->autoneg_complete) {
        mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
        mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
        return 0;
    }

Right after the local carrier comes up, the remote may still report the
previous variant's lp bits. The ksft_in/ksft_not_in checks below would
then fail intermittently.

If the bits are cleared instead, linkmodes.c omits
ETHTOOL_A_LINKMODES_PEER. get_peer_pause_lp_advertising() then returns
EOPNOTSUPP and the remote checks are silently skipped.

pause_aneg_resolution() has the same problem. After set_peer_pauseparams()
it waits only on the local carrier before checking the remote lp_adv and
negotiated values.

Would waiting for /sys/class/net/{cfg.remote_ifname}/carrier on
cfg.remote help? Later patches in the series don't seem to add such a
wait.

[ ... ]

> +@ksft_disruptive
> +def pause_aneg_resolution(cfg, settings):
> +    """ Verify that rx and tx pause parameters are negotiated according to 802.3

[ ... ]

> +    Failing this test can happen if :
> +     - The MAC accepts the pause parameters but doesn't trigger a link
> +       renegotiation
> +     - that the PHY driver manually overwrites the Pause negotiation result
> +     - that the MAC driver ignores the Pause resolution and sets its own
> +       pause parameters regardless
> +    """

[ ... ]

> +    # check lp_adv if available
> +    _, linkmodes = get_local_pause_lp_advertising(cfg)
> +    ksft_eq(linkmodes, required_remote_linkmodes,
> +            f"local lp_adv must show the remote's {required_remote_linkmodes}")
> +
> +    # check lp_adv on remote
> +    ret, linkmodes = get_peer_pause_lp_advertising(cfg)
> +    if ret == 0:
> +        ksft_eq(linkmodes, required_local_linkmodes,
> +                f"remote lp_adv must show our {required_local_linkmodes}")
> +
> +    # Check resolution
> +    _, local_pauseparams = get_local_pauseparams(cfg)
> +    ksft_eq(local_pauseparams["negotiated"]["rx"], expected_local_rx)
> +    ksft_eq(local_pauseparams["negotiated"]["tx"], expected_local_tx)
> +
> +    ret, remote_pauseparams = get_peer_pauseparams(cfg)
> +    if ret == 0 and "negotiated" in remote_pauseparams:
> +        ksft_eq(remote_pauseparams["negotiated"]["rx"], expected_lp_rx)
> +        ksft_eq(remote_pauseparams["negotiated"]["tx"], expected_lp_tx)

[Severity: Medium]
Do these resolution checks test what the kernel actually resolved?

The kernel's PAUSE_GET reply has no resolved pause attribute.
pause_fill_reply() only puts ETHTOOL_A_PAUSE_AUTONEG, RX and TX, and
phylink_ethtool_get_pauseparam() reports the configured
pl->link_config.pause.

The "negotiated" object in the ethtool -a JSON output is computed in
userspace by the ethtool tool. It comes from LINKMODES_OURS and
LINKMODES_PEER, the same advertised and lp_advertised bits that the
"# check adv" and "# check lp_adv" blocks above already compare with
ksft_eq(). Once those pass, these asserts can only fail if ethtool itself
is buggy.

With that in mind, can the test catch the PHY overwrite and MAC override
cases listed in the docstring above?

Nothing here reads the tx_pause/rx_pause passed to mac_link_up() or
phydev->pause/asym_pause. Pause frame stats and traffic aren't checked
either.

The ethtool userspace resolution also ignores duplex. A half-duplex link,
where phylink disables pause, would still pass.

The commit message says:

    Introduce a test that validates we're correctly resolving pause
    parameters according to 802.3.

Should the commit message and docstring be narrowed to what is actually
checked? Alternatively, could the test look at the pause state actually
programmed into the MAC? Later patches in the series leave this code
unchanged.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015134350.629593-1-maxime.chevallier%40bootlin.com

Re: [PATCH net-next v3 3/3] selftests: drv-net: pause: Validate pause autoneg interactions with link autoneg

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says the tests check two invariants: (1) if
  pause autoneg is on, it stays on when link autoneg is disabled; (2) if…
- [Low] Two test docstrings claim more than the tests check.
- [Low] The supported_pauseparams() docstring says it returns 'The rx/tx
  params covering every mode the local device supports', and callers say…
- [Low] require_link_autoneg() reads `ethtool(f"{cfg.remote_ifname}",
  json=True, host=cfg.remote)[0]["supports-auto-negotiation"]` (lines…

--

selftests: drv-net: pause: Validate pause autoneg interactions with link autoneg

This commit adds four tests and some helpers to the drv-net pause.py
selftest. The tests check that toggling pause autoneg keeps the user's
rx/tx intent and advertising. They also check that pause autoneg and link
autoneg do not override each other.

> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> index cd117296be333..ca4a55e59c30f 100755
> --- a/tools/testing/selftests/drivers/net/hw/pause.py
> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
> @@ -119,6 +119,17 @@ def require_controllable_lp(cfg):
>      if not controllable_lp(cfg):
>          raise KsftSkipEx(f"{cfg.remote_ifname} is not directly connected to {cfg.ifname}")
>  
> +def require_link_autoneg(cfg):
> +    """ Skip if local or remote device don't support link aneg """
> +    # Does local device support link aneg
> +    if not ethtool(f"{cfg.ifname}", json=True)[0]["supports-auto-negotiation"]:
> +        raise KsftSkipEx(f"{cfg.ifname} doesn't support link autoneg")
> +
> +    # Does remote device support link aneg
> +    if not ethtool(f"{cfg.remote_ifname}",
> +                   json=True, host=cfg.remote)[0]["supports-auto-negotiation"]:
> +        raise KsftSkipEx(f"Remote {cfg.remote_ifname} doesn't support link autoneg")

[Severity: Low]
What happens here if the remote doesn't report link modes at all? ethtool
prints supports-auto-negotiation together with the other link mode fields.
If the key is missing, this raises KeyError instead of KsftSkipEx.

In pause_autoneg_state_adv(), the only guard before this call is
require_pause_supported_anyof(), and it looks only at the local device.
pause_setup() would skip cleanly in this case:

    if "auto-negotiation" not in link:
        raise KsftSkipEx(f"{cfg.remote_ifname} doesn't report its link modes")

but pause_setup() runs after require_link_autoneg(). ksft_run() counts any
exception other than KsftSkipEx/KsftXfailEx as a failure.

Would pause_autoneg_state_adv() then fail instead of skip on such a setup?

The other three new tests call require_peer_pause_supported_allof() first,
and it already skips when the remote reports no link modes.

[ ... ]

> @@ -290,8 +315,51 @@ def require_peer_pause_supported_allof(cfg, linkmodes):
>          if lm not in pause_support:
>              raise KsftSkipEx(f"Remote device doesn't support {lm}")
>  
> +def supported_pauseparams(cfg):
> +    """ The rx/tx params covering every mode the local device supports """
> +
> +    _, pause_support = get_local_pause_supported(cfg)
> +    if "Pause" in pause_support:
> +        return 1, 1

[Severity: Low]
This isn't a bug, but does the docstring match what this function returns?
In pauseparams_to_linkmodes, rx=1/tx=1 maps to Pause only:

        1: {"rx": 1, "tx": 1, "linkmodes": ["Pause"]}},

On a device that supports both Pause and Asym_Pause, this returns (1, 1).
So pause_autoneg_state_adv(), pause_autoneg_off_while_link_autoneg_on() and
pause_autoneg_link_autoneg() never advertise Asym_Pause on that device. Only
rx=1/tx=0 advertises both modes.

The caller comments "Enable all possible pauseparams" and "Enable all
possible pause modes" describe the same wider coverage.

> +    if "Asym_Pause" in pause_support:
> +        return 0, 1
> +
> +    raise KsftSkipEx("Local device doesn't support pause")

[ ... ]

> @@ -663,12 +731,211 @@ def pause_aneg_resolution(cfg, settings):

[ ... ]

> +@ksft_disruptive
> +def pause_autoneg_off_while_link_autoneg_on(cfg):
> +    """ Validate that when link autoneg is on but pause autoneg is off, we do
> +        not use negotiated pause parameters.
> +
> +        Failing this test means the MAC driver incorrectly accounts for the
> +        negotiated pause parameters even with pause aneg off, likely due to
> +        confusion between link autoneg and pause autoneg.
> +    """

[ ... ]

> +    set_peer_pauseparams(cfg, 0, 0, True)
> +    wait_for_aneg(cfg)
> +
> +    ethtool(f"-r {cfg.ifname}")
> +    wait_for_aneg(cfg, link_drop=True)
> +
> +    # Pause autoneg must still be off even after a link renegotiation
> +    check_local_pauseparams(cfg, False, rx, tx)

[Severity: Low]
Can this check detect what the docstring describes? check_local_pauseparams()
only compares the autonegotiate, rx and tx fields from ethtool -a. For
phylink drivers those fields hold the stored request, not what was
programmed into the MAC:

drivers/net/phy/phylink.c:phylink_ethtool_get_pauseparam() {
    ...
	pause->autoneg = !!(pl->link_config.pause & MLO_PAUSE_AN);
	pause->rx_pause = !!(pl->link_config.pause & MLO_PAUSE_RX);
	pause->tx_pause = !!(pl->link_config.pause & MLO_PAUSE_TX);
}

Many non-phylink drivers also return their cached rx/tx values. Suppose the
peer moves to rx 0 / tx 0 and the MAC wrongly applies the resolved all-off
result. It would still report the forced values here and pass. The check
does catch drivers whose get_pauseparam reports the resolved state.

The pause_autoneg_link_autoneg() docstring has a similar gap:

    is the user intent. When link autoneg is re-enabled, pause params must be
    derived from the negotiation.

After link autoneg is re-enabled, that test only checks the cached intent
and the local and link partner advertising. Unlike pause_aneg_resolution(),
it never reads params["negotiated"]. Nothing checks pause frame counters or
traffic to confirm that the MAC applies the negotiated flow control.

ethtool also seems to compute its negotiated output in userspace from the
local and link partner advertising. If so, pause frame statistics may be
the only way to see what the MAC is really doing.

Should the docstrings be narrowed to what is actually checked? Or could the
tests verify the applied state, for example through pause frame statistics?

[ ... ]

> +@ksft_disruptive
> +def pause_autoneg_link_autoneg(cfg):

[ ... ]

> +    # Enable all possible pause modes and autoneg
> +    set_peer_pauseparams(cfg, 1, 1, True)
> +    ksft_eq(set_local_pauseparams(cfg, rx, tx, True), 0)
> +    wait_for_aneg(cfg)

[ ... ]

> +    # Re-enable autoneg
> +    ethtool(f"-s {cfg.remote_ifname} autoneg on", host=cfg.remote)
> +    ethtool(f"-s {cfg.ifname} autoneg on")
> +    wait_for_aneg(cfg)

[Severity: Low]
The commit message lists two invariants:

     - If Pause autoneg is on, it must stay on when link autoneg is disabled
       (even though Pause won't actually be negotiated)
     - If Pause autoneg is off, it must stay off when link autoneg is
       enabled

It also says the tests verify "that the user intent is correctly cached when
toggling link autoneg".

Is the second invariant covered when link autoneg is toggled? This is the
only test that toggles link autoneg, and pause autoneg is always on when it
does.

pause_autoneg_off_while_link_autoneg_on() is the only test that runs with
pause autoneg off. It keeps link autoneg on the whole time and only
restarts negotiation:

    ethtool(f"-r {cfg.ifname}")

pause_autoneg_state_adv() and pause_autoneg_state_params() never change
link autoneg. The steady state with link autoneg on, including a
renegotiation, is tested, but an explicit link autoneg off -> on transition
with pause autoneg off is not.

Suppose a driver's set_link_ksettings copies cmd->base.autoneg into its
cached pause autoneg. Wouldn't that driver pass every test in the suite?

Could a pause autoneg off variant of this test run the same link autoneg
off -> on cycle?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015134350.629593-1-maxime.chevallier%40bootlin.com