On 6/2/2024 3:45 AM, Damodharam Ammepalli wrote:
> Add speed lanes configuration and display commands support
> to testpmd. Also provide display the lanes info show device info.
>
> testpmd>
> testpmd> port stop 0
> testpmd> port config 0 speed_lanes 4
> testpmd> port config 0 speed 200000 duplex full
> testpmd> port start 0
> testpmd> show port summary 0
> Number of available ports: 2
> Port MAC Address Name Driver Status Link Lanes
> 0 14:23:F2:C3:BA:D2 0000:b1:00.0 net_bnxt up 200 Gbps 4
> testpmd>
>
> testpmd> show port info 0
>
> ********************* Infos for port 0 *********************
> MAC address: 14:23:F2:C3:BA:D2
> Device name: 0000:b1:00.0
> Driver name: net_bnxt
> Firmware-version: 228.9.115.0
> Connect to socket: 2
> memory allocation on the socket: 2
> Link status: up
> Link speed: 200 Gbps
> Lanes: 4
> Link duplex: full-duplex
> Autoneg status: Off
>
> Signed-off-by: Damodharam Ammepalli <[email protected]>
> Reviewed-by: Kalesh AP <[email protected]>
> Reviewed-by: Ajit Khaparde <[email protected]>
> ---
> app/test-pmd/cmdline.c | 142 +++++++++++++++++++++++++++++++++++++++++
> app/test-pmd/config.c | 13 ++--
> 2 files changed, 151 insertions(+), 4 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index b7759e38a8..785e5dd4de 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1361,6 +1361,27 @@ struct cmd_config_speed_all {
> cmdline_fixed_string_t value2;
> };
>
> +static int
> +cmd_validate_lanes(portid_t pid, uint32_t *lanes)
> +{
> + struct rte_eth_speed_lanes_capa spd_lanes = {0};
> + int ret;
> +
> + ret = rte_eth_speed_lanes_get(pid, &spd_lanes);
>
Wouldn't it be better to validate value with provided capabilities?
> + /* if not supported default lanes to 0 */
> + if (ret == -ENOTSUP) {
> + *lanes = 0;
> + return 0;
> + }
> +
> + if (*lanes > spd_lanes.max_lanes_cap) {
> + fprintf(stderr, "Invalid lanes %d configuration\n", *lanes);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> static int
> parse_and_check_speed_duplex(char *speedstr, char *duplexstr, uint32_t
> *speed)
> {
> @@ -1676,6 +1697,125 @@ static cmdline_parse_inst_t
> cmd_config_loopback_specific = {
> },
> };
>
> +/* *** configure speed_lanes for all ports *** */
> +struct cmd_config_speed_lanes_all {
> + cmdline_fixed_string_t port;
> + cmdline_fixed_string_t keyword;
> + cmdline_fixed_string_t all;
> + cmdline_fixed_string_t item;
> + uint32_t lanes;
> +};
> +
> +static void
> +cmd_config_speed_lanes_all_parsed(void *parsed_result,
> + __rte_unused struct cmdline *cl,
> + __rte_unused void *data)
> +{
> + struct cmd_config_speed_lanes_all *res = parsed_result;
> + portid_t pid;
> +
> + if (!all_ports_stopped()) {
> + fprintf(stderr, "Please stop all ports first\n");
> + return;
> + }
> +
> + RTE_ETH_FOREACH_DEV(pid) {
> + if (cmd_validate_lanes(pid, &res->lanes))
> + return;
> + rte_eth_speed_lanes_set(pid, res->lanes);
> + }
> +
> + cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> +}
> +
> +static cmdline_parse_token_string_t cmd_config_speed_lanes_all_port =
> + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, port,
> "port");
> +static cmdline_parse_token_string_t cmd_config_speed_lanes_all_keyword =
> + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, keyword,
> + "config");
> +static cmdline_parse_token_string_t cmd_config_speed_lanes_all_all =
> + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, all, "all");
> +static cmdline_parse_token_string_t cmd_config_speed_lanes_all_item =
> + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, item,
> + "speed_lanes");
> +static cmdline_parse_token_num_t cmd_config_speed_lanes_all_lanes =
> + TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_all, lanes,
> RTE_UINT32);
> +
> +static cmdline_parse_inst_t cmd_config_speed_lanes_all = {
> + .f = cmd_config_speed_lanes_all_parsed,
> + .data = NULL,
> + .help_str = "port config all speed_lanes <value>",
> + .tokens = {
> + (void *)&cmd_config_speed_lanes_all_port,
> + (void *)&cmd_config_speed_lanes_all_keyword,
> + (void *)&cmd_config_speed_lanes_all_all,
> + (void *)&cmd_config_speed_lanes_all_item,
> + (void *)&cmd_config_speed_lanes_all_lanes,
> + NULL,
> + },
> +};
> +
> +/* *** configure speed_lanes for specific port *** */
> +struct cmd_config_speed_lanes_specific {
> + cmdline_fixed_string_t port;
> + cmdline_fixed_string_t keyword;
> + uint16_t port_id;
> + cmdline_fixed_string_t item;
> + uint32_t lanes;
> +};
> +
> +static void
> +cmd_config_speed_lanes_specific_parsed(void *parsed_result,
> + __rte_unused struct cmdline *cl,
> + __rte_unused void *data)
> +{
> + struct cmd_config_speed_lanes_specific *res = parsed_result;
> +
> + if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> + return;
> +
> + if (!port_is_stopped(res->port_id)) {
> + fprintf(stderr, "Please stop port %u first\n", res->port_id);
> + return;
> + }
> +
> + if (cmd_validate_lanes(res->port_id, &res->lanes))
> + return;
> + rte_eth_speed_lanes_set(res->port_id, res->lanes);
> +
> + cmd_reconfig_device_queue(res->port_id, 1, 1);
> +}
> +
> +static cmdline_parse_token_string_t cmd_config_speed_lanes_specific_port =
> + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, port,
> + "port");
> +static cmdline_parse_token_string_t cmd_config_speed_lanes_specific_keyword =
> + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific,
> keyword,
> + "config");
> +static cmdline_parse_token_num_t cmd_config_speed_lanes_specific_id =
> + TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_specific, port_id,
> + RTE_UINT16);
> +static cmdline_parse_token_string_t cmd_config_speed_lanes_specific_item =
> + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, item,
> + "speed_lanes");
> +static cmdline_parse_token_num_t cmd_config_speed_lanes_specific_lanes =
> + TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_specific, lanes,
> + RTE_UINT32);
> +
> +static cmdline_parse_inst_t cmd_config_speed_lanes_specific = {
> + .f = cmd_config_speed_lanes_specific_parsed,
> + .data = NULL,
> + .help_str = "port config <port_id> speed_lanes <value>",
> + .tokens = {
> + (void *)&cmd_config_speed_lanes_specific_port,
> + (void *)&cmd_config_speed_lanes_specific_keyword,
> + (void *)&cmd_config_speed_lanes_specific_id,
> + (void *)&cmd_config_speed_lanes_specific_item,
> + (void *)&cmd_config_speed_lanes_specific_lanes,
> + NULL,
> + },
> +};
> +
These new commands are added between 'cmd_config_loopback_all' &
'cmd_config_loopback_specific' commands, please pay more attention where
to add new code.
Can you please add them just after 'cmd_config_speed_specific' to group
related functionality together?
> /* *** configure txq/rxq, txd/rxd *** */
> struct cmd_config_rx_tx {
> cmdline_fixed_string_t port;
> @@ -13381,6 +13521,8 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
> (cmdline_parse_inst_t *)&cmd_show_port_cman_config,
> (cmdline_parse_inst_t *)&cmd_set_port_cman_config,
> (cmdline_parse_inst_t *)&cmd_config_tx_affinity_map,
> + (cmdline_parse_inst_t *)&cmd_config_speed_lanes_all,
> + (cmdline_parse_inst_t *)&cmd_config_speed_lanes_specific,
>
Can you please keep the same order where function implementations added
above?
> NULL,
> };
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index ba1007ace6..9f846c5e84 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -779,6 +779,7 @@ port_infos_display(portid_t port_id)
> struct rte_ether_addr mac_addr;
> struct rte_eth_link link;
> struct rte_eth_dev_info dev_info;
> + struct rte_eth_speed_lanes_capa spd_lanes = {0};
> int vlan_offload;
> struct rte_mempool * mp;
> static const char *info_border = "*********************";
> @@ -828,6 +829,8 @@ port_infos_display(portid_t port_id)
>
> printf("\nLink status: %s\n", (link.link_status) ? ("up") : ("down"));
> printf("Link speed: %s\n", rte_eth_link_speed_to_str(link.link_speed));
> + rte_eth_speed_lanes_get(port_id, &spd_lanes);
> + printf("Lanes: %d\n", spd_lanes.active_lanes);
>
Please print only if getting lane number is supported.
> printf("Link duplex: %s\n", (link.link_duplex ==
> RTE_ETH_LINK_FULL_DUPLEX) ?
> ("full-duplex") : ("half-duplex"));
> printf("Autoneg status: %s\n", (link.link_autoneg ==
> RTE_ETH_LINK_AUTONEG) ?
> @@ -962,8 +965,8 @@ port_summary_header_display(void)
>
> port_number = rte_eth_dev_count_avail();
> printf("Number of available ports: %i\n", port_number);
> - printf("%-4s %-17s %-12s %-14s %-8s %s\n", "Port", "MAC Address",
> "Name",
> - "Driver", "Status", "Link");
> + printf("%-4s %-17s %-12s %-14s %-8s %-8s %s\n", "Port", "MAC Address",
> "Name",
> + "Driver", "Status", "Link", "Lanes");
> }
>
> void
> @@ -972,6 +975,7 @@ port_summary_display(portid_t port_id)
> struct rte_ether_addr mac_addr;
> struct rte_eth_link link;
> struct rte_eth_dev_info dev_info;
> + struct rte_eth_speed_lanes_capa spd_lanes = {0};
> char name[RTE_ETH_NAME_MAX_LEN];
> int ret;
>
> @@ -993,10 +997,11 @@ port_summary_display(portid_t port_id)
> if (ret != 0)
> return;
>
> - printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %s\n",
> + rte_eth_speed_lanes_get(port_id, &spd_lanes);
> + printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %-8s %d\n",
> port_id, RTE_ETHER_ADDR_BYTES(&mac_addr), name,
> dev_info.driver_name, (link.link_status) ? ("up") : ("down"),
> - rte_eth_link_speed_to_str(link.link_speed));
> + rte_eth_link_speed_to_str(link.link_speed),
> spd_lanes.active_lanes);
>
This is summary info, lots of details already omitted, 'lanes'
information is not imported enough to list here, can you please drop
this change?
