Re: [PATCH] app/testpmd: fix link check condition on port start

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

On 2/3/2023 9:56 AM, Singh, Aman Deep wrote:
> 
> On 1/28/2023 4:15 AM, Ferruh Yigit wrote:
>> In testpmd port start function, 'need_check_link_status' variable is
>> used to detect if a link check is required after port is started.
>>
>> Intention is if at least one port is started, link check should be
>> called, and initially 'need_check_link_status' used as following:
>> ```
>> start_port
>> 	need_check_link_status <- 0
>> 	for each p in port
>> 		ret <- config & start p
>> 		if ret is failure
>> 			break
>> 		need_check_link_status <- 1
>> 	if need_check_link_status
>> 		check link
>> 	else
>> 		log failure message
>> ```
>>
>> Later above logic is modified [1] because when there is no port at all,
>> 'need_check_link_status' remains zero and it causes and error message
>> although this is a valid use case.
>>
>> For this code updated as following:
>>
>> ```
>> start_port
>> 	need_check_link_status <- -1
>> 	for each p in port
>> 		need_check_link_status <- 0
>> 		ret <- config & start p
>> 		if ret is failure
>> 			break
>> 		need_check_link_status <- 1
>> 	if need_check_link_status == 1
>> 		check link
>> 	else if need_check_link_status == 0
>> 		log failure message
>> ```
>>
>> This modification works fine if 'start_port()' called for a single port,
>> but function support both single port and all ports with 'RTE_PORT_ALL'
>> parameter to the function.
>>
>> When it is called for all ports, above logic is wrong because
>> 'need_check_link_status' value reset on each iteration of the loop.
>>
>> For multi port case, if last port fails to start,
>> 'need_check_link_status' will be zero and no link check will be done and
>> it will log a wrong error message.
>>
>> Overall there are three cases to cover:
>> * No port exist at all
>> * All ports are already started
>> * At least on port started successfully
> 
> Minor typo, on => one
> 

Fixed while merging.

>> To cover all three cases, one option is to use 'need_check_link_status'
>> have multiple values to reflect above cases.
>> But meaning of values are not obvious which can lead more issues in the
>> future.
>>
>> Instead converting 'need_check_link_status' to multiple boolean
>> variables whose names are self explanatory.
>>
>> This fixes issue and link check called if at least one port started
>> successfully as intended.
>> Also log message only printed when at least one port exists and all
>> ports are already in started state.
>>
>> [1]
>> Fixes: 92d2703e2c43 ("app/testpmd: fix log with no bound device")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@amd.com>
> 
> Acked-by: Aman Singh <aman.deep.singh@intel.com>
> 

Applied to dpdk-next-net/main, thanks.