Re: [PATCH] app/test-pmd: add explicit parameter for fixed link speeds

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue,  3 Nov 2026 15:50:33 +0000
Bruce Richardson <bruce.richardson@intel.com> 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 <bruce.richardson@intel.com>
> ---

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