Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: fix Segment fault when start fwd

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

On 9/16/2019 7:37 AM, Wang, ShougangX wrote:
> Hi Ferruh
> 
> Thanks for your reply.
> 
>> Not sure if 'reset' command should do more than it says, if there is a
>> requirement that port should be stopped, why not add this condition with an
>> error message so that user can stop the port in advance if she wants.
> 
> Firstly, port must be stopped before reset. Usually, port is stopped by rte_eth_dev_reset(), 
> so testpmd does not prompt user to stop port. Although it can stop port, but testpmd does not 
> change its own port status flag to "RTE_PORT_STOPPED" and it will cause "port start" to fail. 
> So I add this patch to stop port as same as running "port stop" command.

Let's assume port already stopped before calling the reset, reset will cause
port to be started.
I am for user explicitly do the stop, reset and start commands, instead of reset
automatically stop and start later.

> 
>> - 'reset_port()' function has a loop inside, 'RTE_ETH_FOREACH_DEV()' but it
>> works on single port, the loop looked unnecessary to me, can you please check
>> and remove the loop if required?
> 
> "port reset" supports reset all ports (testpmd > port reset all), so this loop is necessary.

Got it.

> 
>> - I am not able to see 'reset' has been documented in
>> 'doc/guides/testpmd_app_ug/testpmd_funcs.rst', it may be missing, can you
>> please check and add it if required?
> 
> OK. I will add it in the next patch.
> 
> Thanks.
> Shougang
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Saturday, September 14, 2019 1:35 AM
>> To: Wang, ShougangX <shougangx.wang@intel.com>; dev@dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Yang, Qiming
>> <qiming.yang@intel.com>; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix Segment fault when start
>> fwd
>>
>> On 9/6/2019 2:28 AM, Wang ShougangX wrote:
>>> This patch fixed the reset function to avoid crash when user don't
>>> call port reset , port stop and port start functions as sequence.
>>>
>>> Fixes: 97f1e19679 ("app/testpmd: add port reset command")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Wang ShougangX <shougangx.wang@intel.com>
>>> ---
>>>  app/test-pmd/testpmd.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> e8e2a39b6..273a7aa02 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -2344,6 +2344,9 @@ reset_port(portid_t pid)
>>>  	if (port_id_is_invalid(pid, ENABLED_WARN))
>>>  		return;
>>>
>>> +	printf("Stopping ports...\n");
>>> +	stop_port(pid);
>>> +
>>>  	printf("Resetting ports...\n");
>>>
>>>  	RTE_ETH_FOREACH_DEV(pi) {
>>> @@ -2372,6 +2375,9 @@ reset_port(portid_t pid)
>>>  		}
>>>  	}
>>>
>>> +	printf("Starting ports...\n");
>>> +	start_port(pid);
>>> +
>>>  	printf("Done\n");
>>>  }
>>
>> Hi Shougang,
>>
>> Not sure if 'reset' command should do more than it says, if there is a
>> requirement that port should be stopped, why not add this condition with an
>> error message so that user can stop the port in advance if she wants.
>>
>>
>> Btw, a few things related,
>> - 'reset_port()' function has a loop inside, 'RTE_ETH_FOREACH_DEV()' but it
>> works on single port, the loop looked unnecessary to me, can you please check
>> and remove the loop if required?
>>
>> - I am not able to see 'reset' has been documented in
>> 'doc/guides/testpmd_app_ug/testpmd_funcs.rst', it may be missing, can you
>> please check and add it if required?
>>
>> Thanks,
>> ferruh