On Tue, 3 Nov 2026 15:50:33 +0000 Bruce Richardson <[email protected]> wrote:
> When a particular speed was specified for a port in testpmd, the flags > passed to the underlying API call always included the > RTE_ETH_LINK_SPEED_FIXED flag. This flag is not a mandatory part of the > bitmap when setting speeds, and is used to disable autonegotiation. > It's possible to have a link report only a single speed while > still have negotiation enabled, so testpmd needs to be extended to > support setting a speed both with and without the flag. > > This patch does so by adding an optional "fixed" keyword after the speed > specified by the user. The presence or absense of the keyword adjusts > the presence or absense of the FIXED speed bit in the ethdev > configuration structure "rte_eth_conf.link_speeds" bitmap. > > Signed-off-by: Bruce Richardson <[email protected]> > --- In general it looked ok to me. But I would like testpmd and ethtool have same syntax to follow the principle of "least surprise" for users. The AI review with its issues and that encouragement was: Review: [PATCH] app/test-pmd: add explicit parameter for fixed link speeds (patchwork 172601) Applies to main (26.11-rc2), builds with -Dwerror=true. Warning (should fix) -------------------- 1. Syntax should follow ethtool rather than invent "fixed". ethtool has autoneg as a separate on/off setting: ethtool -s eth0 speed 10000 duplex full autoneg off With autoneg on, a given speed/duplex limits what gets advertised (the kernel's ethnl_auto_linkmodes()). That is the same as this patch's default, so the semantics are right; only the keyword and its position need to change: port config (port_id|all) speed (...|auto) duplex (half|full|auto) [autoneg (on|off)] autoneg off -> set RTE_ETH_LINK_SPEED_FIXED autoneg on -> clear it absent -> leave autoneg alone. ethtool keeps the current setting, so keep the FIXED bit already in ports[pid].dev_conf.link_speeds. The default dev_conf is autoneg, so a fresh port behaves as posted. speed auto + autoneg off -> error, as now. Putting the optional tokens last leaves the existing "speed N duplex D" prefix alone. Users who know ethtool can guess it. Untested sketch for the all-ports case: struct cmd_config_speed_all { ... cmdline_fixed_string_t item3; /* "autoneg" */ cmdline_fixed_string_t value3; /* "on#off" */ }; static cmdline_parse_token_string_t cmd_config_speed_all_item3 = TOKEN_STRING_INITIALIZER(struct cmd_config_speed_all, item3, "autoneg"); static cmdline_parse_token_string_t cmd_config_speed_all_value3 = TOKEN_STRING_INITIALIZER(struct cmd_config_speed_all, value3, "on#off"); The second cmdline_parse_inst_t still needs the two extra tokens. In the handler, res->item3[0] == '\0' means autoneg was not given. The cmdline library zeroes the result buffer, so the ".data = &cmd_config_speed_all_fixed" self-reference used as a flag can go. Same for the per-port instance. 2. Default behaviour of an existing command changes silently. Before this patch "port config 0 speed 10000 duplex full" set RTE_ETH_LINK_SPEED_FIXED. After it, the same command leaves autoneg enabled and advertises only 10G. Existing scripts and test plans (DTS, vendor validation) keep parsing but now do something different. Whether the user sees a difference depends on the PMD. The new default is correct (it matches ethtool), but the release note puts it under New Features. Give the behaviour change its own sentence, e.g.: Speed settings no longer disable autonegotiation by default; add ``autoneg off`` to force the speed. 3. --eth-link-speed is now inconsistent with the runtime command. parse_link_speed() in parameters.c still starts from RTE_ETH_LINK_SPEED_FIXED, and run_app.rst calls it "a forced link speed". So the startup option forces the speed and the runtime command no longer does. Either add a matching option (e.g. --eth-link-autoneg=on|off), or at minimum document it in the speed command section: The ``--eth-link-speed`` command line option always disables autonegotiation. Info (consider) --------------- 4. testpmd_funcs.rst: the patch leaves three blank lines before "port config - queues/descriptors"; one is enough. "Support for these settings depends on the driver and hardware." adds nothing and can go. 5. Commit message: "absense" (twice) -> "absence"; "still have negotiation enabled" -> "still have autonegotiation enabled".
