On Wed, Nov 04, 2026 at 01:29:49PM +0100, Stephen Hemminger wrote: > 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. >
Ack, will rework to use the autoneg syntax. > 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. > This becomes moot if we rework the syntax. However, when making the original change, I had AI do an analysis - only 5 drivers in tree actually take account of the FIXED flag in turning on/off autoneg. Hopefully, adding testpmd support will allow that list to increase, because nearly all drivers just ignore it. Interestingly, AI also flagged that there is a further set of 5 drivers which would reject the existing testpmd command as it currently stands since they reject any speed selection with the FIXED flag set. > 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. > If we rework this also becomes largely moot, though the option of a non-fixed link speed flag is a possibility for the future. > 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".
