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

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

On 9/29/26 05:26, netdev-bot+sashiko@kernel.org wrote:

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

There are a few of these drivers, will address.

> 
> [ ... ]
> 

> 
>> +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:
That's a i40e driver issue :) The tests aren't build around the HW behaviour.

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

Ok I'll add a restore step here

> 
> [ ... ]
> 
>> +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?

Docstrings are gonna be shrunk down as pylint says the file's too big... I'll
update.

> 
> [ ... ]
> 
>> +    # 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?

OK

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

No, MAC + PHY.

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

That's why we write those tests

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

Ok will address

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

Ok

Maxime