Re: [PATCH net-next v3 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

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

--

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

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

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

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

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

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

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

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

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

[ ... ]

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

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

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

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

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

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

[ ... ]

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

[ ... ]

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

[ ... ]

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

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

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

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

The pause_autoneg_link_autoneg() docstring has a similar gap:

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

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

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

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

[ ... ]

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

[ ... ]

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

[ ... ]

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

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

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

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

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

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

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

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

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

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

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