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

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

On 4/19/2021 7:21 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.
> 

Hi Connor,

I am not sure about this patch, overall design in many commands seems store the 
config changes and mark port or queue as needs reconfiguration 
('cmd_reconfig_device_queue()'), and do the reconfigure during port_start.
Your change is against this overall design.

What is the downside of learning that speed is invalid in port start?
To not able to restore the previous speed is not good, but user can set the 
desired speed again in case of failure.

And do you think adding a check that if the requested speed is in the device 
reported speed capability help on early fail and restore?

> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>   app/test-pmd/cmdline.c | 40 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 5bf1497..09e30cf 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -1537,8 +1537,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");
> @@ -1550,7 +1554,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);
> @@ -1609,7 +1632,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");
> @@ -1623,7 +1649,17 @@ 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;
> +		return;
> +	}
>   
>   	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
>   }
>