* [PATCH net-next v2 0/3] selftests: drv-net: Introduce flow-control selftest
@ 2026-09-25 18:26 Maxime Chevallier (Netdev Foundation)
2026-09-25 18:26 ` [PATCH net-next v2 1/3] selftests: drv-net: Introduce a selftest for ethtool flow control Maxime Chevallier (Netdev Foundation)
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Maxime Chevallier (Netdev Foundation) @ 2026-09-25 18:26 UTC (permalink / raw)
To: Andrew Lunn, Jakub Kicinski, davem, Eric Dumazet, Paolo Abeni,
Simon Horman, Russell King, Heiner Kallweit, Jonathan Corbet,
Shuah Khan
Cc: Maxime Chevallier (Netdev Foundation),
Oleksij Rempel, Vladimir Oltean, Florian Fainelli,
thomas.petazzoni, netdev, linux-kernel, linux-doc
Hi everyone,
This is V2 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.
Overall, this produced 4 different results (duplicates removed):
dev A : (stmmac or mvpp2), LP that can't use pause
dev B : stmmac, fully controllable LP
dev C : mv88exxx DSA port
dev D : stmmac + KSZ9031 (masks Asym)
[dev A] [dev B] [dev C] [dev D]
test_support RX off TX off ok ok ok ok
test_support RX off TX on ok ok ok ok
test_support RX on TX off ok ok ok ok
test_support RX on TX on ok ok ok ok
advertising_test RX off TX off FAIL FAIL ok FAIL
advertising_test RX off TX on ok ok skip skip
advertising_test RX on TX off ok ok skip skip
advertising_test RX on TX on ok ok ok ok
aneg_resol local rx off tx off, remote rx off tx off skip ok skip skip
aneg_resol local rx off tx off, remote rx off tx on skip ok skip skip
aneg_resol local rx off tx off, remote rx on tx off skip ok skip skip
aneg_resol local rx off tx off, remote rx on tx on skip ok skip skip
aneg_resol local rx off tx on, remote rx off tx off skip ok skip skip
aneg_resol local rx off tx on, remote rx off tx on skip ok skip skip
aneg_resol local rx off tx on, remote rx on tx off skip ok skip skip
aneg_resol local rx off tx on, remote rx on tx on skip ok skip skip
aneg_resol local rx on tx off, remote rx off tx off skip ok skip skip
aneg_resol local rx on tx off, remote rx off tx on skip ok skip skip
aneg_resol local rx on tx off, remote rx on tx off skip ok skip skip
aneg_resol local rx on tx off, remote rx on tx on skip ok skip skip
aneg_resol local rx on tx on, remote rx off tx off skip ok skip skip
aneg_resol local rx on tx on, remote rx off tx on skip ok skip skip
aneg_resol local rx on tx on, remote rx on tx off skip ok skip skip
aneg_resol local rx on tx on, remote rx on tx on skip ok skip skip
autoneg_state_adv skip FAIL skip skip
autoneg_state_params skip ok skip skip
autoneg_off_while_link_autoneg_on skip ok skip skip
autoneg_link_autoneg skip ok skip skip
We immediately note 2 things :
- advertising_test.RX off TX off Fails
- autoneg_state_adv Fails
These are common to all boards that use phylink in the following scenario :
MAC - PHY <-------------> Peer
It doesn't appear on DSA user ports, even if they user phylink themselves.
The first issue : advertising_test.RX off TX off
ethtool -A eth0 tx off rx off autoneg on
=> We keep advertising Pause and/or Asym
This seems like a legit issue in phylink.
The second issue : autoneg_state_adv
With link aneg on :
ethtool -A eth0 rx on tx on autoneg off
=> We keep advertising Pause + Asym
So, disabling pause autoneg doesn't disable pause advertising, that's not
expected either.
This is worth looking into as a followup.
Thanks to the Netdev Foundation for funding this work :)
Maxime
Changes in V2:
- 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 | 1035 +++++++++++++++++
2 files changed, 1036 insertions(+)
create mode 100755 tools/testing/selftests/drivers/net/hw/pause.py
--
2.55.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next v2 1/3] selftests: drv-net: Introduce a selftest for ethtool flow control
2026-09-25 18:26 [PATCH net-next v2 0/3] selftests: drv-net: Introduce flow-control selftest Maxime Chevallier (Netdev Foundation)
@ 2026-09-25 18:26 ` Maxime Chevallier (Netdev Foundation)
2026-09-29 3:26 ` netdev-bot+sashiko
2026-09-25 18:26 ` [PATCH net-next v2 2/3] selftests: drv-net: pause: Validate the pause autonegotiation with a partner Maxime Chevallier (Netdev Foundation)
2026-09-25 18:26 ` [PATCH net-next v2 3/3] selftests: drv-net: pause: Validate pause autoneg interactions with link autoneg Maxime Chevallier (Netdev Foundation)
2 siblings, 1 reply; 7+ messages in thread
From: Maxime Chevallier (Netdev Foundation) @ 2026-09-25 18:26 UTC (permalink / raw)
To: Andrew Lunn, Jakub Kicinski, davem, Eric Dumazet, Paolo Abeni,
Simon Horman, Russell King, Heiner Kallweit, Jonathan Corbet,
Shuah Khan
Cc: Maxime Chevallier (Netdev Foundation),
Oleksij Rempel, Vladimir Oltean, Florian Fainelli,
thomas.petazzoni, netdev, linux-kernel, linux-doc
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 | 411 ++++++++++++++++++
2 files changed, 412 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 bd3b8d2fa47e..f0aeb70c20fd 100644
--- a/tools/testing/selftests/drivers/net/hw/Makefile
+++ b/tools/testing/selftests/drivers/net/hw/Makefile
@@ -44,6 +44,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..f07db3271500
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/pause.py
@@ -0,0 +1,411 @@
+#!/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
+import time
+
+from lib.py import KsftFailEx, KsftNamedVariant, KsftSkipEx
+from lib.py import NetDrvEpEnv, EthtoolFamily
+from lib.py import cmd, defer, ethtool, ip
+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_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.
+ :returns: True if link is UP, False if timeout
+ """
+ deadline = time.monotonic() + timeout
+
+ # Link has 2 seconds to come back up
+ restart_by = time.monotonic() + 2
+
+ # The link may still be up for a short while when we trigger an autoneg
+ # restart, we need to wait for it to drop, then come back up again
+ while time.monotonic() < deadline:
+ if not ethtool(f"{cfg.ifname}", json=True)[0]["link-detected"]:
+ return wait_for_link(cfg)
+
+ if not link_drop and time.monotonic() > restart_by:
+ return wait_for_link(cfg)
+
+ time.sleep(0.1)
+
+ return False
+
+def wait_for_link_local(cfg, timeout=15):
+ """ Wait for the local link to be up """
+ deadline = time.monotonic() + timeout
+
+ while time.monotonic() < deadline:
+ link = ethtool(f"{cfg.ifname}", json=True)[0]["link-detected"]
+ if link:
+ return True
+
+ time.sleep(0.1)
+
+ return False
+
+def wait_for_link_remote(cfg, timeout=15):
+ """ Wait for the far end of the link to be up """
+ deadline = time.monotonic() + timeout
+
+ while time.monotonic() < deadline:
+ link = ethtool(f"{cfg.remote_ifname}", json=True,
+ host=cfg.remote)[0]["link-detected"]
+ if link:
+ return True
+
+ time.sleep(0.1)
+
+ return False
+
+def wait_for_link(cfg):
+ """ Wait for both ends of the link to be up """
+ if not wait_for_link_local(cfg):
+ return False
+
+ # Local link is UP, we shouldn't have to wait for a whole 8 seconds for
+ # the remote to report link up, let's wait a bit less
+ return wait_for_link_remote(cfg, timeout = 3)
+
+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
+
+ Raise an error if the return is not 0 or EOPNOTSUPP
+ """
+ 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)
+
+ return ret
+
+def get_local_pauseparams(cfg):
+ """ get pauseparams : ethtool -a
+
+ Raise an error if the return is not 0 or EOPNOTSUPP
+ """
+ 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}")
+
+ 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}")
+
+ 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(ret, linkmodes, supported, note):
+ """ Whether ethtool -A had to work or to be refused, given what the local
+ device supports
+ """
+
+ # If supported is empty, ethtool -A must return -EOPNOTSUPP
+ if not supported :
+ ksft_eq(ret, errno.EOPNOTSUPP, note)
+ elif set(linkmodes).issubset(set(supported)) :
+ # The configured pauseparams are supposed to be supported,
+ #ethtool -A must have worked.
+ ksft_eq(ret, 0, note)
+ else :
+ # We tried to configure parameters that aren't supporteed,
+ # ethtool -A must have failed.
+ 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
+ - link actually up (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 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 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 to become up on both ends
+ if not wait_for_link(cfg):
+ raise KsftFailEx("No link before the test")
+
+# 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.
+
+ The sequence runs with link autoneg on, then with the link forced
+ (ethtool -s ethX autoneg off): the pause params are accepted or rejected
+ the same way in both cases, and both with pause autoneg off and on.
+ """
+
+ rx = onoff(pauseparams["rx"])
+ tx = onoff(pauseparams["tx"])
+ linkmodes = pauseparams["linkmodes"]
+
+ 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:
+ ret, _ = ethtool_ret(f"-A {cfg.ifname} rx {rx} tx {tx} autoneg off",
+ is_get = False)
+ expect_pauseparams_set(ret, linkmodes, supported, "link autoneg on")
+
+ ret, _ = ethtool_ret(f"-A {cfg.ifname} rx {rx} tx {tx} autoneg on",
+ is_get = False)
+ expect_pauseparams_set(ret, linkmodes, 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
+
+ ret, _ = ethtool_ret(f"-A {cfg.ifname} rx {rx} tx {tx} autoneg off",
+ is_get = False)
+ expect_pauseparams_set(ret, linkmodes, supported, "link autoneg off")
+
+ ret, _ = ethtool_ret(f"-A {cfg.ifname} rx {rx} tx {tx} autoneg on",
+ is_get = False)
+ expect_pauseparams_set(ret, linkmodes, 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.
+ ret = wait_for_aneg(cfg)
+ ksft_eq(ret, True)
+
+ _, 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():
+ with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
+ cfg.ethnl = EthtoolFamily()
+ ksft_run([pause_test_support,
+ pause_advertising_test,
+ ],
+ args=(cfg, ))
+ ksft_exit()
+
+if __name__ == "__main__":
+ main()
--
2.55.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next v2 2/3] selftests: drv-net: pause: Validate the pause autonegotiation with a partner
2026-09-25 18:26 [PATCH net-next v2 0/3] selftests: drv-net: Introduce flow-control selftest Maxime Chevallier (Netdev Foundation)
2026-09-25 18:26 ` [PATCH net-next v2 1/3] selftests: drv-net: Introduce a selftest for ethtool flow control Maxime Chevallier (Netdev Foundation)
@ 2026-09-25 18:26 ` Maxime Chevallier (Netdev Foundation)
2026-09-29 3:26 ` netdev-bot+sashiko
2026-09-25 18:26 ` [PATCH net-next v2 3/3] selftests: drv-net: pause: Validate pause autoneg interactions with link autoneg Maxime Chevallier (Netdev Foundation)
2 siblings, 1 reply; 7+ messages in thread
From: Maxime Chevallier (Netdev Foundation) @ 2026-09-25 18:26 UTC (permalink / raw)
To: Andrew Lunn, Jakub Kicinski, davem, Eric Dumazet, Paolo Abeni,
Simon Horman, Russell King, Heiner Kallweit, Jonathan Corbet,
Shuah Khan
Cc: Maxime Chevallier (Netdev Foundation),
Oleksij Rempel, Vladimir Oltean, Florian Fainelli,
thomas.petazzoni, netdev, linux-kernel, linux-doc
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 | 350 ++++++++++++++++++
1 file changed, 350 insertions(+)
diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
index f07db3271500..96ee730074f8 100755
--- a/tools/testing/selftests/drivers/net/hw/pause.py
+++ b/tools/testing/selftests/drivers/net/hw/pause.py
@@ -124,6 +124,62 @@ def wait_for_link(cfg):
# the remote to report link up, let's wait a bit less
return wait_for_link_remote(cfg, timeout = 3)
+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 the local link down and checking that the
+ remote's link drops.
+
+ :returns: True if the remote is our link partner
+ """
+ known = getattr(cfg, "lp_controllable", None)
+ if known is not None:
+ return known
+
+ # Set both ends up, wait for up
+ ip(f"link set {cfg.ifname} up")
+ ip(f"link set {cfg.remote_ifname} up", host=cfg.remote)
+
+ # No link established
+ if not wait_for_link(cfg):
+ ksft_pr(f"{cfg.remote_ifname} is not the link partner: no link with both ends up")
+ cfg.lp_controllable = False
+ return False
+
+ ip(f"link set {cfg.ifname} down")
+
+ deadline = time.monotonic() + 3
+ dropped = False
+ while time.monotonic() < deadline and not dropped:
+ dropped = not ethtool(f"{cfg.remote_ifname}", json=True,
+ host=cfg.remote)[0]["link-detected"]
+ time.sleep(0.1)
+
+ ip(f"link set {cfg.ifname} up")
+
+ if not dropped:
+ ksft_pr(f"{cfg.remote_ifname} is not the link partner: "
+ f"it kept its link through {cfg.ifname} going down")
+ cfg.lp_controllable = False
+ wait_for_link(cfg)
+ return False
+
+ cfg.lp_controllable = wait_for_link(cfg)
+ if not cfg.lp_controllable:
+ ksft_pr(f"{cfg.remote_ifname} is not the link partner: "
+ f"no link back after {cfg.ifname} came up")
+ 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.
@@ -148,6 +204,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
@@ -170,6 +230,33 @@ def get_local_pauseparams(cfg):
"""
return ethtool_ret(f"-a {cfg.ifname}", is_get=True)
+def set_peer_pauseparams(cfg, rx, tx, aneg):
+ """ set pauseparams : ethtool -A
+
+ Raise an error if the return is not 0 or EOPNOTSUPP
+ """
+ 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 : ethtool -a
+
+ Raise an error if the return is not 0 or EOPNOTSUPP
+ """
+
+ 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 """
@@ -188,6 +275,55 @@ 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)}")
+
+ 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)}")
+
+ return ret, _ethtool_pause_use_to_linkmodes(data["advertised-pause-frame-use"])
+
+def get_peer_pause_lp_advertising(cfg):
+ """ Return the errcode and the local 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 """
@@ -200,6 +336,32 @@ 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_anyof(cfg, linkmodes):
+ """ Skip if remote device doesn't support any 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 in pause_support:
+ return
+
+ raise KsftSkipEx(f"Local device doesn't support any of {linkmodes}")
+
+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(ret, linkmodes, supported, note):
""" Whether ethtool -A had to work or to be refused, given what the local
device supports
@@ -371,6 +533,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"]
@@ -398,11 +561,198 @@ 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)
+ require_controllable_lp(cfg)
+ pause_setup(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")
+
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ set_peer_pauseparams(cfg, settings["lp_rx"], settings["lp_tx"], True)
+
+ # Wait for link to re-negotiate
+ ret = wait_for_aneg(cfg)
+
+ # Fail if it doesn't
+ ksft_eq(ret, True)
+
+ 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)
+ for mode in required_local_linkmodes:
+ ksft_in(mode, linkmodes,
+ f"local rx {settings['rx']} tx {settings['tx']} must advertise "
+ f"{required_local_linkmodes}")
+
+ _, linkmodes = get_peer_pause_advertising(cfg)
+ for mode in required_remote_linkmodes:
+ ksft_in(mode, 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)
+ for mode in required_remote_linkmodes:
+ ksft_in(mode, 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:
+ for mode in required_local_linkmodes:
+ ksft_in(mode, 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():
with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
cfg.ethnl = EthtoolFamily()
ksft_run([pause_test_support,
pause_advertising_test,
+ pause_aneg_resolution,
],
args=(cfg, ))
ksft_exit()
--
2.55.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next v2 3/3] selftests: drv-net: pause: Validate pause autoneg interactions with link autoneg
2026-09-25 18:26 [PATCH net-next v2 0/3] selftests: drv-net: Introduce flow-control selftest Maxime Chevallier (Netdev Foundation)
2026-09-25 18:26 ` [PATCH net-next v2 1/3] selftests: drv-net: Introduce a selftest for ethtool flow control Maxime Chevallier (Netdev Foundation)
2026-09-25 18:26 ` [PATCH net-next v2 2/3] selftests: drv-net: pause: Validate the pause autonegotiation with a partner Maxime Chevallier (Netdev Foundation)
@ 2026-09-25 18:26 ` Maxime Chevallier (Netdev Foundation)
2026-09-29 3:26 ` netdev-bot+sashiko
2 siblings, 1 reply; 7+ messages in thread
From: Maxime Chevallier (Netdev Foundation) @ 2026-09-25 18:26 UTC (permalink / raw)
To: Andrew Lunn, Jakub Kicinski, davem, Eric Dumazet, Paolo Abeni,
Simon Horman, Russell King, Heiner Kallweit, Jonathan Corbet,
Shuah Khan
Cc: Maxime Chevallier (Netdev Foundation),
Oleksij Rempel, Vladimir Oltean, Florian Fainelli,
thomas.petazzoni, netdev, linux-kernel, linux-doc
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 | 274 ++++++++++++++++++
1 file changed, 274 insertions(+)
diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
index 96ee730074f8..74bc21c60695 100755
--- a/tools/testing/selftests/drivers/net/hw/pause.py
+++ b/tools/testing/selftests/drivers/net/hw/pause.py
@@ -180,6 +180,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.
@@ -324,6 +335,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 """
@@ -362,6 +387,49 @@ 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(ret, linkmodes, supported, note):
""" Whether ethtool -A had to work or to be refused, given what the local
device supports
@@ -747,12 +815,218 @@ 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 toggling pause advertising changes the advertised linkmodes
+
+ When disabling pause autoneg, we enforce the pause params based on what user
+ asks, instead of relying on the negociation process (which may not be what
+ the user asked for). In forced pause settings, we don't advertise pause and
+ asym_pause bits.
+
+ Failing this test means that .set_pauseparam in the MAC driver doesn't
+ forward to the PHY (in charge of advertising these bits) that we are in
+ fixed pause mode.
+ """
+
+ require_pause_supported_anyof(cfg, ["Pause", "Asym_Pause"])
+ require_controllable_lp(cfg)
+ pause_setup(cfg)
+
+ set_peer_pauseparams(cfg, True, True, True)
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ rx, tx = supported_pauseparams(cfg)
+
+ # Enable all possible pauseparams with pause autoneg
+ ret = set_local_pauseparams(cfg, rx, tx, True)
+ ksft_eq(ret, 0)
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ # 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
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ # We shouldn't be advertising anything anymore
+ ret, adv = get_local_pause_advertising(cfg)
+ ksft_eq(ret, 0)
+ ksft_eq(adv, [])
+
+ # Validate on the LP that we aren't advertising anything
+ ret, adv = get_peer_pause_lp_advertising(cfg)
+ if ret == errno.EOPNOTSUPP:
+ return
+
+ ksft_eq(ret, 0)
+ ksft_eq(adv, [])
+
+@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_controllable_lp(cfg)
+ require_peer_pause_supported_allof(cfg, ["Pause"])
+ require_link_autoneg(cfg)
+ pause_setup(cfg)
+
+ # Set peer user intent to RX on TX on, with Pause autoneg on
+ set_peer_pauseparams(cfg, 1, 1, True)
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ # Set the local intent to RX on TX off with pause autoneg
+ ksft_eq(set_local_pauseparams(cfg, 1, 0, True), 0)
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ check_local_pauseparams(cfg, True, 1, 0)
+ # Peer advertisiong Pause + Asym and us advertising Pause means we are
+ # actually using RX on TX on here, which is different than the intent.
+ 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)
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ # 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)
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ 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_controllable_lp(cfg)
+ require_peer_pause_supported_allof(cfg, ["Pause"])
+ require_link_autoneg(cfg)
+ pause_setup(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)
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ # Disable Pause autoneg
+ ksft_eq(set_local_pauseparams(cfg, rx, tx, False), 0)
+ ksft_eq(wait_for_aneg(cfg), True)
+ # Pause autoneg must read "disabled"
+ check_local_pauseparams(cfg, False, rx, tx)
+
+ set_peer_pauseparams(cfg, 0, 0, True)
+ ksft_eq(wait_for_aneg(cfg, link_drop=True), True)
+
+ ip(f"link set {cfg.remote_ifname} down", host=cfg.remote)
+ ip(f"link set {cfg.remote_ifname} up", host=cfg.remote)
+ ksft_eq(wait_for_aneg(cfg, link_drop=True), 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_controllable_lp(cfg)
+ require_peer_pause_supported_allof(cfg, ["Pause"])
+ require_link_autoneg(cfg)
+ pause_setup(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)
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ check_local_pauseparams(cfg, True, rx, tx)
+ check_local_advertising(cfg, adv)
+ check_local_lp_advertising(cfg, ["Pause"])
+
+ # 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}")
+
+ if not wait_for_aneg(cfg):
+ 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")
+ ksft_eq(wait_for_aneg(cfg), True)
+
+ # 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():
with NetDrvEpEnv(__file__, nsim_test=False) 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()
--
2.55.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2 1/3] selftests: drv-net: Introduce a selftest for ethtool flow control
2026-09-25 18:26 ` [PATCH net-next v2 1/3] selftests: drv-net: Introduce a selftest for ethtool flow control Maxime Chevallier (Netdev Foundation)
@ 2026-09-29 3:26 ` netdev-bot+sashiko
0 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-09-29 3:26 UTC (permalink / raw)
To: maxime.chevallier
Cc: andrew, kuba, davem, edumazet, pabeni, horms, linux, hkallweit1,
corbet, skhan, o.rempel, vladimir.oltean, f.fainelli,
thomas.petazzoni, netdev, linux-kernel, linux-doc
Thank you for your contribution! Sashiko AI review found 12 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 6 · Low: 6
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] The accept/reject rule in expect_pauseparams_set()
(tools/testing/selftests/drivers/net/hw/pause.py) does not match…
- [Medium] The docstrings of set_local_pauseparams() (pause.py:154) and
get_local_pauseparams() (pause.py:169) say 'Raise an error if the…
- [Medium] When the device supports no pause modes,
expect_pauseparams_set() requires every 'ethtool -A' to return…
- [Medium] pause_setup() changes link autoneg without always registering
a restore.
- [Medium] pause_test_support() uses expect_pauseparams_set() to require
that 'ethtool -A' returns 0 for every rx/tx pair covered by the…
- [Medium] The defer queue is LIFO (ksft_flush_defer() pops from the end,
ksft.py:233).
- [Low] In this patch, pause_advertising_test() only checks the local
'advertised-pause-frame-use'.
- [Low] The pause_test_support() docstring says drivers are expected to
refuse unsupported pauseparams 'with a -EOPNOTSUPP return value'.
- [Low] Two timing comments don't match the code.
- [Low] pause_setup() and
get_local_pause_supported()/get_local_pause_advertising() index the…
- [Low] pause_test_support() can report PASS without running a single
pause-parameter assertion.
- [Low] main() assigns cfg.ethnl = EthtoolFamily().
--
selftests: drv-net: Introduce a selftest for ethtool flow control
This commit adds a new drv-net hardware selftest, pause.py. The test checks
that the Pause/Asym_Pause bits in a device's supported link modes match the
rx/tx combinations that "ethtool -A" accepts, with link autoneg on and with
the link forced. It also checks that the locally advertised pause modes
follow the configured pause parameters.
> 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..f07db32715003
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
> @@ -0,0 +1,411 @@
[ ... ]
> +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.
> + :returns: True if link is UP, False if timeout
> + """
> + deadline = time.monotonic() + timeout
> +
> + # Link has 2 seconds to come back up
> + restart_by = time.monotonic() + 2
[ ... ]
> +def wait_for_link(cfg):
> + """ Wait for both ends of the link to be up """
> + if not wait_for_link_local(cfg):
> + return False
> +
> + # Local link is UP, we shouldn't have to wait for a whole 8 seconds for
> + # the remote to report link up, let's wait a bit less
> + return wait_for_link_remote(cfg, timeout = 3)
[Severity: Low]
This isn't a bug, but do these two timing comments match the code?
wait_for_link_local() and wait_for_link_remote() default to a 15 second
timeout, and this call lowers it to 3 seconds. No 8 second value appears
anywhere.
In wait_for_aneg(), restart_by is the window in which the link is expected
to drop when link_drop is False. The time allowed for the link to come back
up is set by wait_for_link().
[ ... ]
> +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}")
> +
> + return ret, _ethtool_pause_use_to_linkmodes(data["supported-pause-frame-use"])
[Severity: Low]
What happens here on a device without get_link_ksettings?
ethtool only prints "supported-pause-frame-use", "advertised-pause-frame-use",
"auto-negotiation" and "supports-auto-negotiation" when the link modes reply
succeeds. Plain "ethtool <dev>" still exits 0 when other replies, such as
link-detected, are present.
In that case this line raises an uncaught KeyError. ksft_run() reports that
as a failure with a traceback, not as a skip.
require_pause_supported_allof() calls this function before any capability
check. pause_setup() also indexes link["auto-negotiation"] and
link["supports-auto-negotiation"] directly, for both the local and the
remote interface.
Speed and duplex are already guarded with "in" checks. Could these keys be
guarded the same way?
[ ... ]
> +def expect_pauseparams_set(ret, linkmodes, supported, note):
> + """ Whether ethtool -A had to work or to be refused, given what the local
> + device supports
> + """
> +
> + # If supported is empty, ethtool -A must return -EOPNOTSUPP
> + if not supported :
> + ksft_eq(ret, errno.EOPNOTSUPP, note)
[Severity: Medium]
Can any driver pass this for the RX off TX off variant?
ethnl_set_pause() returns early when a request changes nothing, so the
driver's set_pauseparam is never called:
net/ethtool/pause.c:ethnl_set_pause() {
...
dev->ethtool_ops->get_pauseparam(dev, ¶ms);
ethnl_update_bool32(¶ms.autoneg, tb[ETHTOOL_A_PAUSE_AUTONEG], &mod);
ethnl_update_bool32(¶ms.rx_pause, tb[ETHTOOL_A_PAUSE_RX], &mod);
ethnl_update_bool32(¶ms.tx_pause, tb[ETHTOOL_A_PAUSE_TX], &mod);
if (!mod)
return 0;
...
}
pause_test_support() sends each rx/tx pair twice, first with "autoneg off"
and then with "autoneg on". After the first request is correctly rejected,
the state is unchanged. For rx off / tx off, one of the two requests then
matches the current state and returns 0.
Take a phylink MAC without MAC_SYM_PAUSE/MAC_ASYM_PAUSE, such as ksz88x3
port 0. It starts with link_config.pause = MLO_PAUSE_AN and rx/tx off:
- "-A rx off tx off autoneg off" gets -EOPNOTSUPP, as expected.
- "-A rx off tx off autoneg on" is a no-op and returns 0, so
ksft_eq(0, EOPNOTSUPP) fails.
The forced link phase runs the same sequence and fails the same way.
> + elif set(linkmodes).issubset(set(supported)) :
> + # The configured pauseparams are supposed to be supported,
> + #ethtool -A must have worked.
> + ksft_eq(ret, 0, note)
> + else :
> + # We tried to configure parameters that aren't supporteed,
> + # ethtool -A must have failed.
> + ksft_in(ret, (errno.EOPNOTSUPP, errno.EINVAL), note)
[Severity: Medium]
Does this subset rule match phylink_ethtool_set_pauseparam()? The commit
message names phylink as the layer that gets pause right.
The rule here matches phylib's phy_validate_pause(), which rejects rx_pause
when Pause is not supported. phylink behaves differently:
drivers/net/phy/phylink.c:phylink_ethtool_set_pauseparam() {
...
if (pl->req_link_an_mode == MLO_AN_FIXED)
return -EOPNOTSUPP;
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;
...
}
With supported = [Asym_Pause], phylink accepts "rx on tx on" and
"rx on tx off", but this test requires both to be rejected.
include/linux/phylink.h documents that case explicitly: "If you set
MAC_ASYM_PAUSE, the user may request any combination of tx_pause and
rx_pause." I could not find an in-tree phylink driver with pause ops that
sets only MAC_ASYM_PAUSE, so the fixed link case below is the more concrete
one.
For fixed links, phylink_fill_fixedlink_supported() sets Pause and
Asym_Pause in pl->supported, so ethtool reports pause as supported. Yet
phylink_ethtool_set_pauseparam() returns -EOPNOTSUPP for every request.
In the forced phase, phylink_ethtool_ksettings_set() accepts "autoneg off"
when speed and duplex match the fixed link. Every variant this function
treats as supported would then fail.
Is the test meant to encode a stricter contract than phylink, or does
phylink need fixing? As written, the two disagree.
> +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
> + - link actually up (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 link["auto-negotiation"]:
> + defer(cmd, f"ethtool -s {cfg.ifname} autoneg on")
[Severity: Medium]
The defer queue is LIFO (ksft_flush_defer() pops from the end), so at
teardown the link restore runs first. The pause restore follows right
after it, with no wait for autoneg to complete.
Can the pause restore be refused in that window? i40e ties pause autoneg
to the link autoneg state:
drivers/net/ethernet/intel/i40e/i40e_ethtool.c:i40e_set_pauseparam() {
...
is_an = hw_link_info->an_info & I40E_AQ_AN_COMPLETED;
if (pause->autoneg != is_an) {
netdev_info(netdev, "To change autoneg please use: ethtool -s <dev> autoneg <on|off>\n");
return -EOPNOTSUPP;
}
...
}
On a negotiated link, the saved original is "autoneg on". If the forced
phase of pause_test_support() ran, AN_COMPLETED is still clear right after
"ethtool -s <if> autoneg on" restarts autoneg.
The restore is then rejected, and the device keeps the test's last rx/tx
pause settings. The deferred cmd() uses fail=True, so ksft_flush_defer()
also marks the case as failed.
I haven't confirmed the exact firmware timing, or that i40e accepts the
forced link on every PHY type.
> + 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")
[Severity: Medium]
Is the original forced link configuration lost if autoneg is off but speed
or duplex is missing from the JSON?
In that case no defer is registered, but "ethtool -s <if> autoneg on" still
runs.
The patch already expects both keys can be missing. forced_link_settings()
falls back when duplex is missing. Speed is omitted when unknown, for
example while the port is admin down, and this function only brings the
port up after reading the settings.
The remote side below has the same if/elif with no fallback before
"ethtool -s <remote_ifname> autoneg on". That can leave a shared peer in
autoneg mode after the test exits.
[ ... ]
> +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.
[ ... ]
> + 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 expect_pauseparams_set() also accepts EINVAL for
unsupported combinations:
ksft_in(ret, (errno.EOPNOTSUPP, errno.EINVAL), note)
phylink_ethtool_set_pauseparam() also returns -EINVAL for rx != tx when
Asym_Pause is not supported. Should the docstring mention EINVAL too?
[ ... ]
> + # 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:
[ ... ]
> + 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]
Can a variant be reported as passed without running a single pause
parameter assertion?
If "ethtool -s <if> autoneg on" fails, the first group is skipped and only
logged with ksft_pr(). If forced is then empty, or the forced
"ethtool -s ... autoneg off" fails, the function returns normally.
ksft_run() sets KSFT_RESULT = True before each case and does not check
whether the case made any assertions. The case is counted as a pass rather
than a skip.
Should these paths raise KsftSkipEx when nothing was tested?
> +
> + ret, _ = ethtool_ret(f"-A {cfg.ifname} rx {rx} tx {tx} autoneg off",
> + is_get = False)
> + expect_pauseparams_set(ret, linkmodes, supported, "link autoneg off")
> +
> + ret, _ = ethtool_ret(f"-A {cfg.ifname} rx {rx} tx {tx} autoneg on",
> + is_get = False)
> + expect_pauseparams_set(ret, linkmodes, supported, "link autoneg off")
[Severity: Medium]
Does this require pause autoneg to be accepted on a forced link? That goes
against the uAPI documentation for struct ethtool_pauseparam in
include/uapi/linux/ethtool.h:
* Drivers should reject a non-zero setting of @autoneg when
* autoneogotiation is disabled (or not supported) for the link.
expect_pauseparams_set() decides success only from Pause/Asym_Pause. Those
bits describe the MAC's pause frame abilities. They don't say whether pause
can be negotiated, or forced while link autoneg is running.
Several drivers fail at least one of the four link autoneg / pause autoneg
combinations:
bnxt_set_pauseparam() returns -EINVAL for pause autoneg without
BNXT_AUTONEG_SPEED, which is what the uAPI describes.
mlx5e_ethtool_set_pauseparam() returns -EINVAL for any pause autoneg
request.
ixgbe_set_pauseparam() returns -EINVAL when
!ixgbe_device_supports_autoneg_fc().
i40e_set_pauseparam() returns -EOPNOTSUPP whenever pause->autoneg !=
is_an. That already fails the link autoneg on / pause autoneg off step.
pause_advertising_test() has a related problem because it only skips on
EOPNOTSUPP. It treats an EINVAL refusal of pause autoneg from mlx5, bnxt or
ixgbe as applied. It then checks the advertisement against a configuration
the device never accepted.
[ ... ]
> +def pause_advertising_test(cfg, pauseparams):
> + """Pause advertisement
[ ... ]
> + The validation is made by looking at the advertised modes locally, as well
> + as what the peer's 'lp_advertising' values report.
> + """
[Severity: Low]
Is the lp_advertising part of this docstring accurate for this patch?
The body never queries the peer. It only reads the local
"advertised-pause-frame-use", which phy_ethtool_ksettings_get() copies from
the software phydev->advertising bitmap.
wait_for_aneg(cfg) also returns success if the link hasn't dropped within
2 seconds:
if not link_drop and time.monotonic() > restart_by:
return wait_for_link(cfg)
Together, these mean a MAC that never restarts autoneg, or a PHY that
misprograms its advertisement register, would still pass.
A later commit in this series, "selftests: drv-net: pause: Validate the
pause autonegotiation with a partner", adds controllable_lp() and a
get_peer_pause_lp_advertising() check to pause_advertising_test(). That
closes the gap by the end of the series.
[ ... ]
> + # 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.
> + ret = wait_for_aneg(cfg)
[Severity: Medium]
What happens when set_local_pauseparams() fails with something other than
EOPNOTSUPP?
The docstrings of set_local_pauseparams() and get_local_pauseparams() say
"Raise an error if the return is not 0 or EOPNOTSUPP". Neither function
raises. Both just return the result of ethtool_ret(), which uses
cmd(..., fail=False).
Here, an EINVAL, an EBUSY, or ethtool's raw exit code 1 is silently dropped,
and ret is overwritten by wait_for_aneg(). The advertising checks then run
against whatever state was there before, because pause_setup() does not
put the pause params into a known state.
require_pause_supported_allof() has already confirmed that the modes are
supported. So the result is either a false pass, or a failure with a
"must advertise" message that blames the PHY. For example,
mlx5e_ethtool_set_pauseparam() returns -EINVAL whenever pauseparam->autoneg
is set.
[ ... ]
> +def main():
> + with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
> + cfg.ethnl = EthtoolFamily()
[Severity: Low]
This isn't a bug, but cfg.ethnl doesn't seem to be used anywhere in
pause.py, since everything goes through the ethtool CLI. Can this
assignment and the EthtoolFamily import be dropped? As it stands, if the
ethtool genl family can't be resolved, the whole test aborts even though it
doesn't need that family.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925182628.214667-1-maxime.chevallier%40bootlin.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2 2/3] selftests: drv-net: pause: Validate the pause autonegotiation with a partner
2026-09-25 18:26 ` [PATCH net-next v2 2/3] selftests: drv-net: pause: Validate the pause autonegotiation with a partner Maxime Chevallier (Netdev Foundation)
@ 2026-09-29 3:26 ` netdev-bot+sashiko
0 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-09-29 3:26 UTC (permalink / raw)
To: maxime.chevallier
Cc: andrew, kuba, davem, edumazet, pabeni, horms, linux, hkallweit1,
corbet, skhan, o.rempel, vladimir.oltean, f.fainelli,
thomas.petazzoni, netdev, linux-kernel, linux-doc
Thank you for your contribution! Sashiko AI review found 7 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 4 · Low: 3
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, the
docstring of pause_aneg_resolution() says the test fails if 'the PHY…
- [Medium] controllable_lp() in pause.py runs `ip link set {cfg.ifname}
down` (line 152), then a remote `ethtool --json` poll loop, then `ip…
- [Medium] controllable_lp() only treats the remote as the direct link
partner if `ip link set <local> down` makes the remote lose carrier…
- [Medium] selftests: drv-net: pause: false test failure for unsupported
devices on no-op changes
- [Low] Several new peer helpers in pause.py have docstrings or messages
copied from the local helpers that do not match the code.
- [Low] pause_aneg_resolution() only handles EOPNOTSUPP from
set_local_pauseparams() (lines 693-695).
- [Low] All four advertisement checks in pause_aneg_resolution() (lines
715-738) only test that required bits are present (ksft_in over…
--
selftests: drv-net: pause: Validate the pause autonegotiation with a partner
This commit adds a pause_aneg_resolution test to pause.py. The test sets
pause parameters on both the local device and the link partner, then
checks the resulting 802.3 pause resolution for all 16 combinations. It
also adds helpers that detect whether the remote is the direct link
partner, and extends pause_advertising_test to check the remote's
lp_advertising.
> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> index f07db32715003..96ee730074f8c 100755
> --- a/tools/testing/selftests/drivers/net/hw/pause.py
> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
> @@ -124,6 +124,62 @@ def wait_for_link(cfg):
> # the remote to report link up, let's wait a bit less
> return wait_for_link_remote(cfg, timeout = 3)
>
> +def controllable_lp(cfg):
[ ... ]
> + # Set both ends up, wait for up
> + ip(f"link set {cfg.ifname} up")
> + ip(f"link set {cfg.remote_ifname} up", host=cfg.remote)
> +
> + # No link established
> + if not wait_for_link(cfg):
> + ksft_pr(f"{cfg.remote_ifname} is not the link partner: no link with both ends up")
> + cfg.lp_controllable = False
> + return False
> +
> + ip(f"link set {cfg.ifname} down")
> +
> + deadline = time.monotonic() + 3
> + dropped = False
> + while time.monotonic() < deadline and not dropped:
> + dropped = not ethtool(f"{cfg.remote_ifname}", json=True,
> + host=cfg.remote)[0]["link-detected"]
> + time.sleep(0.1)
> +
> + ip(f"link set {cfg.ifname} up")
[Severity: Medium]
What happens to cfg.ifname if something raises between the link down and
the link up here?
ethtool() goes through tool() and cmd() with the default fail=True. So any
of these would skip the "link set ... up" call:
- a failure or timeout of the remote command
- a KeyError on "link-detected"
- a JSON parse error
- a KeyboardInterrupt
Nothing like try/finally or defer() protects this window.
ksft_run() flushes defers when a test raises, but pause_setup() never
defers an admin-up. In pause_aneg_resolution(), require_controllable_lp()
also runs before pause_setup(), so that test has nothing deferred at all.
lp_controllable is not cached on exception, so a later test would retry
and bring the link back up. But if the exception happens in the last
caller of controllable_lp() in the run (pause_autoneg_link_autoneg by the
end of the series), or on an interrupt, is the DUT interface left
admin-down after the selftest exits?
Other drv-net tests restore the link with a defer. For example, queues.py
and stats.py use:
defer(ip, f"link set dev {cfg.dev['ifname']} up")
Also, both ends are forced admin-up here, and their original admin state
is never recorded or restored.
> +
> + if not dropped:
> + ksft_pr(f"{cfg.remote_ifname} is not the link partner: "
> + f"it kept its link through {cfg.ifname} going down")
> + cfg.lp_controllable = False
> + wait_for_link(cfg)
> + return False
[Severity: Medium]
Does taking the local interface admin-down reliably drop the physical
link on directly cabled setups?
In phylib, phy_suspend() leaves the PHY active on purpose in some cases:
drivers/net/phy/phy_device.c:phy_suspend() {
...
if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
return -EBUSY;
if (!phydrv->suspend)
return 0;
...
}
Firmware-managed NICs such as ice also keep the link on close by default.
In drivers/net/ethernet/intel/ice/ice_main.c, the PHY is only taken down
on close when ICE_FLAG_LINK_DOWN_ON_CLOSE_ENA is set:
if (test_bit(ICE_FLAG_LINK_DOWN_ON_CLOSE_ENA, vsi->back->flags)) {
int link_err = ice_phy_cfg(vsi, false);
On this kind of hardware, a directly connected peer would be cached as
cfg.lp_controllable = False. pause_aneg_resolution(), and the four later
tests in the series, would then skip with "is not directly connected".
That message points at cabling, when the real cause is that the PHY
stayed powered.
The new remote lp_advertising checks in pause_advertising_test() would
also be skipped without any notice.
This detection is not changed later in the series.
[ ... ]
> @@ -170,6 +230,33 @@ def get_local_pauseparams(cfg):
> """
> return ethtool_ret(f"-a {cfg.ifname}", is_get=True)
>
> +def set_peer_pauseparams(cfg, rx, tx, aneg):
> + """ set pauseparams : ethtool -A
> +
> + Raise an error if the return is not 0 or EOPNOTSUPP
> + """
> + 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 : ethtool -a
> +
> + Raise an error if the return is not 0 or EOPNOTSUPP
> + """
> +
> + return ethtool_ret(f"-a {cfg.remote_ifname}", is_get=True,
> + host=cfg.remote)
[Severity: Low]
This isn't a bug, but the docstring of set_peer_pauseparams() says it
raises an error unless the return is 0 or EOPNOTSUPP. The code instead
raises KsftSkipEx on any non-zero return, EOPNOTSUPP included. A real
peer error such as EINVAL or EIO is reported as SKIP.
Several other new peer helpers have docstrings or messages that don't
match the code:
- get_peer_pauseparams() says "Raise an error if the return is not 0 or
EOPNOTSUPP", but it only returns the ethtool_ret() tuple.
- get_peer_pause_lp_advertising() says "Return the errcode and the local
pause lp_advertised linkmodes", but it queries cfg.remote_ifname.
- require_peer_pause_supported_allof() says "Skip if local device doesn't
support all of the passed modes", but it checks the remote.
- require_peer_pause_supported_anyof() checks the remote, but its skip
message is "Local device doesn't support any of ...". Nothing calls it,
in this patch or by the end of the series.
[ ... ]
> @@ -398,11 +561,198 @@ def pause_advertising_test(cfg, pauseparams):
[ ... ]
> +@ksft_disruptive
> +def pause_aneg_resolution(cfg, settings):
[ ... ]
> + # 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")
> +
> + ksft_eq(wait_for_aneg(cfg), True)
> +
> + set_peer_pauseparams(cfg, settings["lp_rx"], settings["lp_tx"], True)
> +
> + # Wait for link to re-negotiate
> + ret = wait_for_aneg(cfg)
> +
> + # Fail if it doesn't
> + ksft_eq(ret, True)
> +
> + 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")
[Severity: Low]
Should errors other than EOPNOTSUPP from set_local_pauseparams() fail
the test?
set_local_pauseparams() returns the error code without raising, so
EINVAL, EIO and similar errors fall through to the checks below.
In the "local rx off tx off" variants, required_local_linkmodes is empty,
so the local advertising loop checks nothing. If the state left by
pause_setup() happens to give the expected resolution, the variant can
pass even though configuring the DUT failed.
pause_test_support() does catch such a driver through
expect_pauseparams_set(). A later patch in the series ("Validate pause
autoneg interactions with link autoneg") also uses ksft_eq(ret, 0) for
the same setter. This call site is unchanged at the end of the series,
though.
The same pattern shows up with get_local_pauseparams() above. Any failure
there is reported as a missing capability, although the same call already
succeeded in require_pause_supported_allof(). Would it be better to fail
when ret != 0?
> +
> + # check adv
> + _, linkmodes = get_local_pause_advertising(cfg)
> + for mode in required_local_linkmodes:
> + ksft_in(mode, linkmodes,
> + f"local rx {settings['rx']} tx {settings['tx']} must advertise "
> + f"{required_local_linkmodes}")
> +
> + _, linkmodes = get_peer_pause_advertising(cfg)
> + for mode in required_remote_linkmodes:
> + ksft_in(mode, 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)
> + for mode in required_remote_linkmodes:
> + ksft_in(mode, 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:
> + for mode in required_local_linkmodes:
> + ksft_in(mode, linkmodes,
> + f"remote lp_adv must show our {required_local_linkmodes}")
[Severity: Low]
These four checks only confirm that the required bits are present. Should
they also reject Pause or Asym_Pause bits that ought to be absent?
When a required list is empty, the loop checks nothing. Take "local rx
off tx off, remote rx off tx off": the resolution is 0/0 either way, so
two faults would go unnoticed:
- a peer that keeps Pause after a successful set
- a local PHY that reports extra lp_advertising bits
pause_advertising_test() uses ksft_not_in() for absent bits, and a later
patch in the series compares advertising lists with ksft_eq(). However,
pause_aneg_resolution() is unchanged at the end of the series.
> +
> + # 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]
Where does the "negotiated" object come from?
The PAUSE netlink uAPI only has the HEADER, AUTONEG, RX, TX, STATS and
STATS_SRC attributes. pause_fill_reply() in net/ethtool/pause.c sends no
negotiated or resolved RX/TX result.
The ethtool utility builds "negotiated" in userspace from the
ETHTOOL_A_LINKMODES_OURS and PEER bitsets. Those are the same advertised
and lp_advertised bits checked just above.
On the kernel side, phylink_ethtool_get_pauseparam() in
drivers/net/phy/phylink.c reports the configured intent, not the resolved
state:
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);
So once the adv and lp_adv checks pass, these asserts are a pure function
of those bits. Would a MAC that programs its own flow control still pass
all 16 variants? What about a PHY driver that overwrites phydev->pause or
asym_pause without touching lp_advertising?
If so, the test can't detect the failure cases its docstring lists:
- 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
The same applies to the docstring's "This test also verifies that the peer
resolved the parameters correctly". It also applies to this line in the
commit message:
Introduce a test that validates we're correctly resolving pause
parameters according to 802.3.
In practice, these checks test ethtool's userspace 802.3 resolution table.
This is not addressed later in the series.
[ ... ]
> @@ -200,6 +336,32 @@ 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_anyof(cfg, linkmodes):
> + """ Skip if remote device doesn't support any 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 in pause_support:
> + return
> +
> + raise KsftSkipEx(f"Local device doesn't support any of {linkmodes}")
> +
> +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(ret, linkmodes, supported, note):
> """ Whether ethtool -A had to work or to be refused, given what the local
> device supports
[Severity: Medium]
expect_pauseparams_set() comes from the previous patch, but it shows
up in the context here. Can the empty supported branch fail on a
driver that behaves correctly?
ethnl_set_pause_validate() in net/ethtool/pause.c only returns
-EOPNOTSUPP when get_pauseparam or set_pauseparam is missing. After
that, ethnl_set_pause() reads the current state with get_pauseparam()
and merges the requested attributes with ethnl_update_bool32(). If
nothing changed, it returns without calling the driver:
if (!mod)
return 0;
Take a phylink MAC with neither Pause nor Asym_Pause in pl->supported.
phylink_ethtool_set_pauseparam() would reject a real change with
-EOPNOTSUPP. It is never called, though, when the requested rx, tx
and autoneg values match what get_pauseparam() already reports.
For such a request ethtool -A succeeds and ret is 0. The
ksft_eq(ret, errno.EOPNOTSUPP, note) check then reports a failure,
even though the device has no pause support and the kernel did
nothing wrong. On that hardware pause_test_support() would fail in
CI instead of passing or skipping.
Should expect_pauseparams_set() get the pauseparams before the set
and accept 0 when the request is a no-op? Or should the no-op
variants be skipped on devices whose supported list is empty?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925182628.214667-1-maxime.chevallier%40bootlin.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next v2 3/3] selftests: drv-net: pause: Validate pause autoneg interactions with link autoneg
2026-09-25 18:26 ` [PATCH net-next v2 3/3] selftests: drv-net: pause: Validate pause autoneg interactions with link autoneg Maxime Chevallier (Netdev Foundation)
@ 2026-09-29 3:26 ` netdev-bot+sashiko
0 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-09-29 3:26 UTC (permalink / raw)
To: maxime.chevallier
Cc: andrew, kuba, davem, edumazet, pabeni, horms, linux, hkallweit1,
corbet, skhan, o.rempel, vladimir.oltean, f.fainelli,
thomas.petazzoni, netdev, linux-kernel, linux-doc
Thank you for your contribution! Sashiko AI review found 8 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 · Low: 6
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] pause_autoneg_state_adv() turns pause autoneg off with
`ethtool -A <if> rx R tx T autoneg off` (rx/tx unchanged, link autoneg…
- [Medium] pause_autoneg_off_while_link_autoneg_on() calls
wait_for_aneg(cfg, link_drop=True) twice and treats a missed carrier…
- [Low] The commit message says the tests cover 'If Pause autoneg is off,
it must stay off when link autoneg is enabled' and 'that the user…
- [Low] pause_autoneg_off_while_link_autoneg_on() says it checks that 'we
do not use negotiated pause parameters' when pause autoneg is off, and…
- [Low] In pause_autoneg_state_params(), the comment at line 899 says
'Peer advertisiong Pause + Asym and us advertising Pause'.
- [Low] pause_autoneg_link_autoneg() makes several soft ksft_eq checks
first: set_local_pauseparams at line ~982, wait_for_aneg, and…
- [Low] pause_autoneg_state_adv() checks pause advertising, including
what the link partner sees, but unlike the other three new tests it…
- [Low] pause_autoneg_off_while_link_autoneg_on() runs `ip link set
<remote> down` then `ip link set <remote> up` (lines 954-955) as bare…
--
selftests: drv-net: pause: Validate pause autoneg interactions with link autoneg
This adds four disruptive tests to the drv-net pause selftest. They check
how pause autoneg interacts with link autoneg, and whether the user's rx/tx
pause intent survives changes to either autoneg setting. The commit also
adds helpers that check link autoneg support and pause support, and that
verify the local pauseparams and pause advertising.
> 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.
[Severity: Low]
Do the new tests cover the second bullet, or the part about toggling link
autoneg?
pause_autoneg_off_while_link_autoneg_on() never runs "ethtool -s ...
autoneg", so link autoneg stays on for the whole test. The only things that
change are the peer's pause settings and the remote admin state.
pause_autoneg_link_autoneg() is the only test that forces the link and then
turns link autoneg back on. It starts with pause autoneg on.
No test runs this sequence: pause autoneg off -> link autoneg off -> link
autoneg on -> pause autoneg still off, with rx/tx kept. Wouldn't a driver
that turns pause autoneg back on whenever link autoneg is enabled pass the
whole suite?
> diff --git a/tools/testing/selftests/drivers/net/hw/pause.py b/tools/testing/selftests/drivers/net/hw/pause.py
> index 96ee730074f8c..74bc21c606958 100755
> --- a/tools/testing/selftests/drivers/net/hw/pause.py
> +++ b/tools/testing/selftests/drivers/net/hw/pause.py
[ ... ]
> @@ -747,12 +815,218 @@ 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 toggling pause advertising changes the advertised linkmodes
> +
> + When disabling pause autoneg, we enforce the pause params based on what user
> + asks, instead of relying on the negociation process (which may not be what
> + the user asked for). In forced pause settings, we don't advertise pause and
> + asym_pause bits.
> +
> + Failing this test means that .set_pauseparam in the MAC driver doesn't
> + forward to the PHY (in charge of advertising these bits) that we are in
> + fixed pause mode.
> + """
> +
> + require_pause_supported_anyof(cfg, ["Pause", "Asym_Pause"])
> + require_controllable_lp(cfg)
> + pause_setup(cfg)
> +
> + set_peer_pauseparams(cfg, True, True, True)
> + ksft_eq(wait_for_aneg(cfg), True)
> +
> + rx, tx = supported_pauseparams(cfg)
> +
> + # Enable all possible pauseparams with pause autoneg
> + ret = set_local_pauseparams(cfg, rx, tx, True)
> + ksft_eq(ret, 0)
[Severity: Low]
Should pause_autoneg_state_adv() also call require_link_autoneg()? The other
three new tests do.
pause_setup() turns on link autoneg only when "supports-auto-negotiation"
is true, and it doesn't skip when that's false. The test then requires
the pause autoneg set above to succeed.
The struct ethtool_pauseparam kernel-doc tells drivers to "reject a
non-zero setting of @autoneg when autonegotiation is disabled (or not
supported)". On a non-phylink driver that follows the doc and has no link
autoneg, wouldn't this report FAIL instead of SKIP?
phylink_ethtool_set_pauseparam() doesn't reject this case, so phylink
drivers aren't affected.
[ ... ]
> + # Disable pause autoneg
> + ret = set_local_pauseparams(cfg, rx, tx, False)
> + ksft_eq(ret, 0)
> +
> + # This may trigger a link renegociation
> + ksft_eq(wait_for_aneg(cfg), True)
> +
> + # We shouldn't be advertising anything anymore
> + ret, adv = get_local_pause_advertising(cfg)
> + ksft_eq(ret, 0)
> + ksft_eq(adv, [])
> +
> + # Validate on the LP that we aren't advertising anything
> + ret, adv = get_peer_pause_lp_advertising(cfg)
> + if ret == errno.EOPNOTSUPP:
> + return
> +
> + ksft_eq(ret, 0)
> + ksft_eq(adv, [])
[Severity: Medium]
Do these two ksft_eq(adv, []) checks match the uAPI contract? The struct
ethtool_pauseparam kernel-doc in include/uapi/linux/ethtool.h says:
* If the link is autonegotiated, drivers should use
* mii_advertise_flowctrl() or similar code to set the advertised
* pause frame capabilities based on the @rx_pause and @tx_pause flags,
* even if @autoneg is zero.
phylink does this. phylink_ethtool_set_pauseparam() calls
phylink_update_pause_state(), which sets the advertisement from rx/tx
without checking MLO_PAUSE_AN:
drivers/net/phy/phylink.c:phylink_update_pause_state() {
...
linkmode_set_pause(config->advertising, tx_pause,
rx_pause);
...
if (pl->phydev)
phy_set_asym_pause(pl->phydev, rx_pause, tx_pause);
}
phy_set_asym_pause() in drivers/net/phy/phy_device.c also sets the
advertisement from rx/tx alone.
The test runs "ethtool -A <if> rx R tx T autoneg off" with rx/tx unchanged
and link autoneg still on. After that, won't every phylink driver, and
every phylib driver that uses phy_set_asym_pause(), still advertise Pause
(or Asym_Pause) and fail both checks?
The docstring also says a failure means ".set_pauseparam in the MAC driver
doesn't forward to the PHY ... that we are in fixed pause mode". That
points driver authors toward behaviour the uAPI doc rules out. Also, the
pause_advertising_test docstring earlier in this file says phylink handles
the PHY reconfiguration correctly.
[ ... ]
> + # Set peer user intent to RX on TX on, with Pause autoneg on
> + set_peer_pauseparams(cfg, 1, 1, True)
> + ksft_eq(wait_for_aneg(cfg), True)
> +
> + # Set the local intent to RX on TX off with pause autoneg
> + ksft_eq(set_local_pauseparams(cfg, 1, 0, True), 0)
> + ksft_eq(wait_for_aneg(cfg), True)
> +
> + check_local_pauseparams(cfg, True, 1, 0)
> + # Peer advertisiong Pause + Asym and us advertising Pause means we are
> + # actually using RX on TX on here, which is different than the intent.
> + check_local_advertising(cfg, ["Pause", "Asym_Pause"])
> + check_local_lp_advertising(cfg, ["Pause"])
[Severity: Low]
This isn't a bug, but in pause_autoneg_state_params() the comment has the
two sides swapped.
The peer is set with set_peer_pauseparams(cfg, 1, 1, True), so it
advertises Pause. The local side is set with set_local_pauseparams(cfg, 1,
0, True), so it advertises Pause + Asym_Pause. The asserts right below
match the code, not the comment.
The comment also has a typo: "advertisiong".
[ ... ]
> +@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)
> + ksft_eq(wait_for_aneg(cfg, link_drop=True), True)
> +
> + ip(f"link set {cfg.remote_ifname} down", host=cfg.remote)
> + ip(f"link set {cfg.remote_ifname} up", host=cfg.remote)
[Severity: Low]
If the "up" call fails after "down" succeeded, what brings the remote
interface back up?
These are bare ip() calls, and no defer() restores the remote admin
state. pause_setup() only defers restores for the ethtool -A and ethtool
-s settings, even though its docstring says the ports being admin up is
restored on exit.
The next test, pause_autoneg_link_autoneg(), brings the port up only inside
pause_setup(), and it can skip before it gets there. If it skips, or if
this is the last test to run, the remote interface stays admin down.
Would registering something like this before the down call help?
defer(ip, f"link set {cfg.remote_ifname} up", host=cfg.remote)
> + ksft_eq(wait_for_aneg(cfg, link_drop=True), True)
[Severity: Medium]
Can the two wait_for_aneg(cfg, link_drop=True) calls in this function fail
on a correct DUT?
With link_drop=True, wait_for_aneg() no longer returns early for a link
that is already stable:
if not link_drop and time.monotonic() > restart_by:
return wait_for_link(cfg)
It returns True only if it sees the local "link-detected" go false.
Otherwise it waits the full 15 seconds, returns False, and the ksft_eq()
fails.
Neither trigger here reliably causes a local carrier drop that the test
can see:
- Not every peer driver restarts AN when only its pause advertisement
changes. An in-band 1000BASE-X restart through
phylink_change_inband_advert() and pcs_an_restart can also finish well
within the 100 ms poll interval.
- Admin-down on the peer doesn't always drop the physical link. Examples
are i40e/ice with link-down-on-close off (the default) and PHYs kept
powered for WoL or NC-SI. controllable_lp() only checks the other
direction, where local down drops the remote carrier.
- Polling starts only after both remote ip commands return, so a fast PHY
or PCS can drop and recover before the first poll.
The first link_drop wait also adds no coverage, because the remote bounce
right after it already forces renegotiation.
> +
> + # Pause autoneg must still be off even after a link renegotiation
> + check_local_pauseparams(cfg, False, rx, tx)
[Severity: Low]
Can this check detect the bug the docstring describes? The docstring says
a failure means the MAC "incorrectly accounts for the negotiated pause
parameters even with pause aneg off".
check_local_pauseparams() only reads the configured autonegotiate/rx/tx
values from ethtool -a. On phylink, those values come straight from the
stored user intent:
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);
}
This test would still pass in either of these cases:
- mac_link_up() applies the negotiated pause anyway.
- phylink's resolve path stops replacing state->pause with
link_config.pause when MLO_PAUSE_AN is clear.
pause_autoneg_link_autoneg() has the same gap. Its docstring says that once
link autoneg is re-enabled, "pause params must be derived from the
negotiation". It never reads the "negotiated" section that
pause_aneg_resolution() checks.
> +
> +@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)
> + ksft_eq(wait_for_aneg(cfg), True)
> +
> + check_local_pauseparams(cfg, True, rx, tx)
> + check_local_advertising(cfg, adv)
> + check_local_lp_advertising(cfg, ["Pause"])
> +
> + # 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}")
> +
> + if not wait_for_aneg(cfg):
> + raise KsftSkipEx(f"No link when forced at {forced}")
[Severity: Low]
What happens to failures already recorded in this test when one of these
skips is raised?
Before this point pause_autoneg_link_autoneg() has already run several
ksft_eq() checks: set_local_pauseparams(), wait_for_aneg(), and the
check_local_* helpers. ksft_run() in
tools/testing/selftests/net/lib/py/ksft.py handles the skip like this:
except KsftSkipEx as e:
comment = "SKIP " + str(e)
cnt_key = 'skip'
That skips the later pass/fail choice based on KSFT_RESULT. The test is
then reported as "not ok ... # SKIP". KTAP consumers treat that as a skip,
and the totals count it as a skip instead of a fail.
This path looks likely. Forcing 1000BASE-T copper usually gives no link,
and forced_link_settings() returns an empty string when the speed is
unknown. ksft_exit() still exits non-zero, but the per-test result hides a
real pause or advertising mismatch.
Could these skips be raised only when none of the earlier checks has
failed?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925182628.214667-1-maxime.chevallier%40bootlin.com
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-09-29 3:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-25 18:26 [PATCH net-next v2 0/3] selftests: drv-net: Introduce flow-control selftest Maxime Chevallier (Netdev Foundation)
2026-09-25 18:26 ` [PATCH net-next v2 1/3] selftests: drv-net: Introduce a selftest for ethtool flow control Maxime Chevallier (Netdev Foundation)
2026-09-29 3:26 ` netdev-bot+sashiko
2026-09-25 18:26 ` [PATCH net-next v2 2/3] selftests: drv-net: pause: Validate the pause autonegotiation with a partner Maxime Chevallier (Netdev Foundation)
2026-09-29 3:26 ` netdev-bot+sashiko
2026-09-25 18:26 ` [PATCH net-next v2 3/3] selftests: drv-net: pause: Validate pause autoneg interactions with link autoneg Maxime Chevallier (Netdev Foundation)
2026-09-29 3:26 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®