patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@amd.com>
To: "Singh, Aman Deep" <aman.deep.singh@intel.com>,
	Yuying Zhang <yuying.zhang@intel.com>,
	Michael Qiu <qiudayu@chinac.com>,
	Pablo de Lara <pablo.de.lara.guarch@intel.com>
Cc: dev@dpdk.org, stable@dpdk.org, michael.qiu@intel.com
Subject: Re: [PATCH] app/testpmd: fix link check condition on port start
Date: Wed, 8 Feb 2023 22:54:49 +0000	[thread overview]
Message-ID: <09aa00fa-d040-a061-46a9-7ad85a49c159@amd.com> (raw)
In-Reply-To: <e7931c0a-9f97-2be7-3d05-7ca99945baa3@intel.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.


      reply	other threads:[~2023-02-08 22:55 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-27 22:45 Ferruh Yigit
2023-02-03  9:56 ` Singh, Aman Deep
2023-02-08 22:54   ` Ferruh Yigit [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=09aa00fa-d040-a061-46a9-7ad85a49c159@amd.com \
    --to=ferruh.yigit@amd.com \
    --cc=aman.deep.singh@intel.com \
    --cc=dev@dpdk.org \
    --cc=michael.qiu@intel.com \
    --cc=pablo.de.lara.guarch@intel.com \
    --cc=qiudayu@chinac.com \
    --cc=stable@dpdk.org \
    --cc=yuying.zhang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).