Re: [PATCH v2 2/4] testpmd: Add speed lanes to testpmd config and show command

[Ferruh Yigit <ferruh.yigit@amd.com>]

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 <damodharam.ammepalli@broadcom.com>
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>  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?