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

[Huisong Li <lihuisong@huawei.com>]

在 2021/6/8 18:49, Andrew Rybchenko 写道:
> 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.

Thanks for your reivew.

You're right. There are many configurations in tespmd that need to 
recall dev_configure and are not verified during configuration.

Only one of them is verified, which does not indicate that the failure 
is caused by the current configuration. So let's keep it the way it was.

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