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

[netdev-bot+sashiko@kernel.org]

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