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.w...@intel.com>; dev@dpdk.org
>> Cc: Lu, Wenzhuo <wenzhuo...@intel.com>; Yang, Qiming
>> <qiming.y...@intel.com>; sta...@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: sta...@dpdk.org
>>>
>>> Signed-off-by: Wang ShougangX <shougangx.w...@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
