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: sta...@dpdk.org
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yi...@amd.com>
>
> Acked-by: Aman Singh <aman.deep.si...@intel.com>
>
Applied to dpdk-next-net/main, thanks.
