Re: [dpdk-dev] [PATCH v2 1/2] app/testpmd: add link speed check before port start

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 4/28/21 11:36 AM, Min Hu (Connor) wrote:
> From: Huisong Li <lihuisong@huawei.com>
> 
> Currently, to check whether the configured link_speeds is valid, we
> have to run "port start". In addition, if the configuration fails,
> "port->dev_conf.link_speeds" maintained in testpmd cannot be
> restored.
> 
> This patch adds the link_speeds check before port start by calling
> dev_configure, and resolves these problems.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  app/test-pmd/cmdline.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 5fdcc1c..ddbc629 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1549,8 +1549,12 @@ cmd_config_speed_all_parsed(void *parsed_result,
>  			__rte_unused void *data)
>  {
>  	struct cmd_config_speed_all *res = parsed_result;
> +	uint32_t old_link_speeds[RTE_MAX_ETHPORTS];
> +	struct rte_port *port;
>  	uint32_t link_speed;
>  	portid_t pid;
> +	portid_t i;
> +	int ret;
>  
>  	if (!all_ports_stopped()) {
>  		printf("Please stop all ports first\n");
> @@ -1562,7 +1566,26 @@ cmd_config_speed_all_parsed(void *parsed_result,
>  		return;
>  
>  	RTE_ETH_FOREACH_DEV(pid) {
> -		ports[pid].dev_conf.link_speeds = link_speed;
> +		port = &ports[pid];
> +		old_link_speeds[pid] = port->dev_conf.link_speeds;
> +		port->dev_conf.link_speeds = link_speed;
> +		ret = rte_eth_dev_configure(pid, nb_rxq, nb_txq,
> +					    &port->dev_conf);
> +		if (ret < 0) {
> +			printf("Failed to check link speeds for port %d, ret = %d.\n",
> +				pid, ret);
> +			goto roolback;
> +		}
> +	}
> +
> +	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> +
> +	return;
> +
> +roolback:
> +	for (i = 0; i <= pid; i++) {
> +		port = &ports[i];
> +		port->dev_conf.link_speeds = old_link_speeds[i];
>  	}
>  
>  	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
> @@ -1621,7 +1644,10 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>  				__rte_unused void *data)
>  {
>  	struct cmd_config_speed_specific *res = parsed_result;
> +	uint32_t old_link_speeds;
> +	struct rte_port *port;
>  	uint32_t link_speed;
> +	int ret;
>  
>  	if (!all_ports_stopped()) {
>  		printf("Please stop all ports first\n");
> @@ -1635,8 +1661,20 @@ cmd_config_speed_specific_parsed(void *parsed_result,
>  			&link_speed) < 0)
>  		return;
>  
> -	ports[res->id].dev_conf.link_speeds = link_speed;
> +	port = &ports[res->id];
> +	old_link_speeds = port->dev_conf.link_speeds;
> +	port->dev_conf.link_speeds = link_speed;
> +	ret = rte_eth_dev_configure(res->id, nb_rxq, nb_txq,
> +				    &port->dev_conf);
> +	if (ret < 0) {
> +		printf("Failed to check link speeds for port %d, ret = %d.\n",
> +			res->id, ret);
> +		port->dev_conf.link_speeds = old_link_speeds;

In fact, you don't really know here if link speed is a problem
or something else configured while device is stopped.
So, I think there is no point to over-complicate it
with addition of possibly misleading diagnostics.

> +	}
>  
> +	/*
> +	 * If the cmd fails to execute, it is necessary to reconfigure device.
> +	 */
>  	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>  }
>  
>