diff mbox series

app/testpmd: handle IEEE1588 init fail

Message ID 20240330074409.273916-1-huangdengdui@huawei.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series app/testpmd: handle IEEE1588 init fail |

Checks

Context Check Description
ci/loongarch-compilation success Compilation OK
ci/checkpatch success coding style OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-abi-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS

Commit Message

Dengdui Huang March 30, 2024, 7:44 a.m. UTC 
When the port's timestamping function failed to initialize
(for example, the device does not support PTP), the packets
received by the hardware do not contain the timestamp.
In this case, IEEE1588 packet forwarding should not start.
This patch fix it.

Plus, adding a failure message when failed to disable PTP.

Fixes: a78040c990cb ("app/testpmd: update forward engine beginning")
Cc: stable@dpdk.org

Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
---
 app/test-pmd/ieee1588fwd.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Singh, Aman Deep April 5, 2024, 3:36 p.m. UTC | #1 
On 3/30/2024 1:14 PM, Dengdui Huang wrote:
> When the port's timestamping function failed to initialize
> (for example, the device does not support PTP), the packets
> received by the hardware do not contain the timestamp.
> In this case, IEEE1588 packet forwarding should not start.
> This patch fix it.
>
> Plus, adding a failure message when failed to disable PTP.
>
> Fixes: a78040c990cb ("app/testpmd: update forward engine beginning")
> Cc: stable@dpdk.org
>
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>

Acked-by: Aman Singh <aman.deep.singh@intel.com>

> ---
>   app/test-pmd/ieee1588fwd.c | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/app/test-pmd/ieee1588fwd.c b/app/test-pmd/ieee1588fwd.c
> index 3371771751..afea7735c7 100644
> --- a/app/test-pmd/ieee1588fwd.c
> +++ b/app/test-pmd/ieee1588fwd.c
> @@ -197,14 +197,23 @@ ieee1588_packet_fwd(struct fwd_stream *fs)
>   static int
>   port_ieee1588_fwd_begin(portid_t pi)
>   {
> -	rte_eth_timesync_enable(pi);
> -	return 0;
> +	int ret;
> +
> +	ret = rte_eth_timesync_enable(pi);
> +	if (ret)
> +		printf("Port %u enable PTP failed, ret = %d\n", pi, ret);
> +
> +	return ret;
>   }
>   
>   static void
>   port_ieee1588_fwd_end(portid_t pi)
>   {
> -	rte_eth_timesync_disable(pi);
> +	int ret;
> +
> +	ret = rte_eth_timesync_disable(pi);
> +	if (ret)
> +		printf("Port %u disable PTP failed, ret = %d\n", pi, ret);
>   }
>   
>   static void
Stephen Hemminger April 5, 2024, 4:44 p.m. UTC | #2 
On Sat, 30 Mar 2024 15:44:09 +0800
Dengdui Huang <huangdengdui@huawei.com> wrote:

> When the port's timestamping function failed to initialize
> (for example, the device does not support PTP), the packets
> received by the hardware do not contain the timestamp.
> In this case, IEEE1588 packet forwarding should not start.
> This patch fix it.
> 
> Plus, adding a failure message when failed to disable PTP.
> 
> Fixes: a78040c990cb ("app/testpmd: update forward engine beginning")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>

Noticed that ieee1588 part is printing errors to stdout,
but other parts of test-pmd are using stderr or TEST_PMD_LOG.

It would be good to decide on one good way to handle this
across all of testpmd.
Dengdui Huang April 8, 2024, 5:52 a.m. UTC | #3 
On 2024/4/6 0:44, Stephen Hemminger wrote:
> On Sat, 30 Mar 2024 15:44:09 +0800
> Dengdui Huang <huangdengdui@huawei.com> wrote:
> 
>> When the port's timestamping function failed to initialize
>> (for example, the device does not support PTP), the packets
>> received by the hardware do not contain the timestamp.
>> In this case, IEEE1588 packet forwarding should not start.
>> This patch fix it.
>>
>> Plus, adding a failure message when failed to disable PTP.
>>
>> Fixes: a78040c990cb ("app/testpmd: update forward engine beginning")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> 
> Noticed that ieee1588 part is printing errors to stdout,
> but other parts of test-pmd are using stderr or TEST_PMD_LOG.
> 
> It would be good to decide on one good way to handle this
> across all of testpmd.

Yeah, it's a bit of a mess. Is it better to use TEST_PMD_LOG?
But this is a test app, and modifying it seems unnecessary.
What should we do next?
Ferruh Yigit April 8, 2024, 8:45 a.m. UTC | #4 
On 4/8/2024 6:52 AM, huangdengdui wrote:
> 
> 
> On 2024/4/6 0:44, Stephen Hemminger wrote:
>> On Sat, 30 Mar 2024 15:44:09 +0800
>> Dengdui Huang <huangdengdui@huawei.com> wrote:
>>
>>> When the port's timestamping function failed to initialize
>>> (for example, the device does not support PTP), the packets
>>> received by the hardware do not contain the timestamp.
>>> In this case, IEEE1588 packet forwarding should not start.
>>> This patch fix it.
>>>
>>> Plus, adding a failure message when failed to disable PTP.
>>>
>>> Fixes: a78040c990cb ("app/testpmd: update forward engine beginning")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
>>
>> Noticed that ieee1588 part is printing errors to stdout,
>> but other parts of test-pmd are using stderr or TEST_PMD_LOG.
>>
>> It would be good to decide on one good way to handle this
>> across all of testpmd.
> 
> Yeah, it's a bit of a mess. Is it better to use TEST_PMD_LOG?
> But this is a test app, and modifying it seems unnecessary.
> What should we do next?
>

'TESTPMD_LOG' exists and used in a few places, but still most of the
logging done with 'printf/fprintf'.

Agree to have an agreement what to use, document it, and stick to it.


For some cases, like 'usage()' output (testpmd supported parameters), or
cmdline prompt we always want to have output, so 'printf' suits well.

Not sure where 'TESTPMD_LOG' is needed and what is the benefit. I don't
remember many cases that I want to refine testpmd app level output.
Maybe a case can be packet verbose output, but it also has its specific
command to control it.

So should we continue to 'printf/fprintf', is there any disadvantage to
do so?
Dengdui Huang April 9, 2024, 2:06 a.m. UTC | #5 
On 2024/4/8 16:45, Ferruh Yigit wrote:
> On 4/8/2024 6:52 AM, huangdengdui wrote:
>>
>>
>> On 2024/4/6 0:44, Stephen Hemminger wrote:
>>> On Sat, 30 Mar 2024 15:44:09 +0800
>>> Dengdui Huang <huangdengdui@huawei.com> wrote:
>>>
>>>> When the port's timestamping function failed to initialize
>>>> (for example, the device does not support PTP), the packets
>>>> received by the hardware do not contain the timestamp.
>>>> In this case, IEEE1588 packet forwarding should not start.
>>>> This patch fix it.
>>>>
>>>> Plus, adding a failure message when failed to disable PTP.
>>>>
>>>> Fixes: a78040c990cb ("app/testpmd: update forward engine beginning")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
>>>
>>> Noticed that ieee1588 part is printing errors to stdout,
>>> but other parts of test-pmd are using stderr or TEST_PMD_LOG.
>>>
>>> It would be good to decide on one good way to handle this
>>> across all of testpmd.
>>
>> Yeah, it's a bit of a mess. Is it better to use TEST_PMD_LOG?
>> But this is a test app, and modifying it seems unnecessary.
>> What should we do next?
>>
> 
> 'TESTPMD_LOG' exists and used in a few places, but still most of the
> logging done with 'printf/fprintf'.
> 
> Agree to have an agreement what to use, document it, and stick to it.
> 
> 
> For some cases, like 'usage()' output (testpmd supported parameters), or
> cmdline prompt we always want to have output, so 'printf' suits well.
> 
> Not sure where 'TESTPMD_LOG' is needed and what is the benefit. I don't
> remember many cases that I want to refine testpmd app level output.
> Maybe a case can be packet verbose output, but it also has its specific
> command to control it.
> 
> So should we continue to 'printf/fprintf', is there any disadvantage to
> do so?
OK, 'printf/fprintf' is really necessary. Am I to understand it as follows?

'TESTPMD_LOG' is more suitable for printing app runtime context logs,
such as initialization logs and uninstallation logs.

'printf' is suitable for normal interaction with the user, such as
show port info <port_id>

When should we print to stderr?
Stephen Hemminger April 9, 2024, 2:50 a.m. UTC | #6 
On Tue, 9 Apr 2024 10:06:01 +0800
huangdengdui <huangdengdui@huawei.com> wrote:

> On 2024/4/8 16:45, Ferruh Yigit wrote:
> > On 4/8/2024 6:52 AM, huangdengdui wrote:  
> >>
> >>
> >> On 2024/4/6 0:44, Stephen Hemminger wrote:  
> >>> On Sat, 30 Mar 2024 15:44:09 +0800
> >>> Dengdui Huang <huangdengdui@huawei.com> wrote:
> >>>  
> >>>> When the port's timestamping function failed to initialize
> >>>> (for example, the device does not support PTP), the packets
> >>>> received by the hardware do not contain the timestamp.
> >>>> In this case, IEEE1588 packet forwarding should not start.
> >>>> This patch fix it.
> >>>>
> >>>> Plus, adding a failure message when failed to disable PTP.
> >>>>
> >>>> Fixes: a78040c990cb ("app/testpmd: update forward engine beginning")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>  
> >>>
> >>> Noticed that ieee1588 part is printing errors to stdout,
> >>> but other parts of test-pmd are using stderr or TEST_PMD_LOG.
> >>>
> >>> It would be good to decide on one good way to handle this
> >>> across all of testpmd.  
> >>
> >> Yeah, it's a bit of a mess. Is it better to use TEST_PMD_LOG?
> >> But this is a test app, and modifying it seems unnecessary.
> >> What should we do next?
> >>  
> > 
> > 'TESTPMD_LOG' exists and used in a few places, but still most of the
> > logging done with 'printf/fprintf'.
> > 
> > Agree to have an agreement what to use, document it, and stick to it.
> > 
> > 
> > For some cases, like 'usage()' output (testpmd supported parameters), or
> > cmdline prompt we always want to have output, so 'printf' suits well.
> > 
> > Not sure where 'TESTPMD_LOG' is needed and what is the benefit. I don't
> > remember many cases that I want to refine testpmd app level output.
> > Maybe a case can be packet verbose output, but it also has its specific
> > command to control it.
> > 
> > So should we continue to 'printf/fprintf', is there any disadvantage to
> > do so?  
> OK, 'printf/fprintf' is really necessary. Am I to understand it as follows?
> 
> 'TESTPMD_LOG' is more suitable for printing app runtime context logs,
> such as initialization logs and uninstallation logs.
> 
> 'printf' is suitable for normal interaction with the user, such as
> show port info <port_id>
> 
> When should we print to stderr?

Any Unix convention is that any error message should go to stderr.
For test-pmd, using TESTPMD_LOG has benefits and problems.
The benefit is following a convention across all the startup and error
codes. And with the color log patches, errors are highlighted.

But the current way rte_log works, the testpmd stuff ends up going
to syslog/journal which is not necessary and overly chatty.
Dengdui Huang April 17, 2024, 9:26 a.m. UTC | #7 
On 2024/4/9 10:50, Stephen Hemminger wrote:
> On Tue, 9 Apr 2024 10:06:01 +0800
> huangdengdui <huangdengdui@huawei.com> wrote:
> 
>> On 2024/4/8 16:45, Ferruh Yigit wrote:
>>> On 4/8/2024 6:52 AM, huangdengdui wrote:  
>>>>
>>>>
>>>> On 2024/4/6 0:44, Stephen Hemminger wrote:  
>>>>> On Sat, 30 Mar 2024 15:44:09 +0800
>>>>> Dengdui Huang <huangdengdui@huawei.com> wrote:
>>>>>  
>>>>>> When the port's timestamping function failed to initialize
>>>>>> (for example, the device does not support PTP), the packets
>>>>>> received by the hardware do not contain the timestamp.
>>>>>> In this case, IEEE1588 packet forwarding should not start.
>>>>>> This patch fix it.
>>>>>>
>>>>>> Plus, adding a failure message when failed to disable PTP.
>>>>>>
>>>>>> Fixes: a78040c990cb ("app/testpmd: update forward engine beginning")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>  
>>>>>
>>>>> Noticed that ieee1588 part is printing errors to stdout,
>>>>> but other parts of test-pmd are using stderr or TEST_PMD_LOG.
>>>>>
>>>>> It would be good to decide on one good way to handle this
>>>>> across all of testpmd.  
>>>>
>>>> Yeah, it's a bit of a mess. Is it better to use TEST_PMD_LOG?
>>>> But this is a test app, and modifying it seems unnecessary.
>>>> What should we do next?
>>>>  
>>>
>>> 'TESTPMD_LOG' exists and used in a few places, but still most of the
>>> logging done with 'printf/fprintf'.
>>>
>>> Agree to have an agreement what to use, document it, and stick to it.
>>>
>>>
>>> For some cases, like 'usage()' output (testpmd supported parameters), or
>>> cmdline prompt we always want to have output, so 'printf' suits well.
>>>
>>> Not sure where 'TESTPMD_LOG' is needed and what is the benefit. I don't
>>> remember many cases that I want to refine testpmd app level output.
>>> Maybe a case can be packet verbose output, but it also has its specific
>>> command to control it.
>>>
>>> So should we continue to 'printf/fprintf', is there any disadvantage to
>>> do so?  
>> OK, 'printf/fprintf' is really necessary. Am I to understand it as follows?
>>
>> 'TESTPMD_LOG' is more suitable for printing app runtime context logs,
>> such as initialization logs and uninstallation logs.
>>
>> 'printf' is suitable for normal interaction with the user, such as
>> show port info <port_id>
>>
>> When should we print to stderr?
> 
> Any Unix convention is that any error message should go to stderr.
> For test-pmd, using TESTPMD_LOG has benefits and problems.
> The benefit is following a convention across all the startup and error
> codes. And with the color log patches, errors are highlighted.
> 
> But the current way rte_log works, the testpmd stuff ends up going
> to syslog/journal which is not necessary and overly chatty.

That seems hard to unify. I don't have a better idea at the moment.
Do you have a better suggestion?
Ferruh Yigit July 5, 2024, 11:27 a.m. UTC | #8 
On 4/5/2024 4:36 PM, Singh, Aman Deep wrote:
> On 3/30/2024 1:14 PM, Dengdui Huang wrote:
>> When the port's timestamping function failed to initialize
>> (for example, the device does not support PTP), the packets
>> received by the hardware do not contain the timestamp.
>> In this case, IEEE1588 packet forwarding should not start.
>> This patch fix it.
>>
>> Plus, adding a failure message when failed to disable PTP.
>>
>> Fixes: a78040c990cb ("app/testpmd: update forward engine beginning")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Dengdui Huang <huangdengdui@huawei.com>
> 
> Acked-by: Aman Singh <aman.deep.singh@intel.com>
>

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


Will continue with 'printf', as most of the file already use it,
unifying logs can be done later.
diff mbox series

Patch

diff --git a/app/test-pmd/ieee1588fwd.c b/app/test-pmd/ieee1588fwd.c
index 3371771751..afea7735c7 100644
--- a/app/test-pmd/ieee1588fwd.c
+++ b/app/test-pmd/ieee1588fwd.c
@@ -197,14 +197,23 @@  ieee1588_packet_fwd(struct fwd_stream *fs)
 static int
 port_ieee1588_fwd_begin(portid_t pi)
 {
-	rte_eth_timesync_enable(pi);
-	return 0;
+	int ret;
+
+	ret = rte_eth_timesync_enable(pi);
+	if (ret)
+		printf("Port %u enable PTP failed, ret = %d\n", pi, ret);
+
+	return ret;
 }
 
 static void
 port_ieee1588_fwd_end(portid_t pi)
 {
-	rte_eth_timesync_disable(pi);
+	int ret;
+
+	ret = rte_eth_timesync_disable(pi);
+	if (ret)
+		printf("Port %u disable PTP failed, ret = %d\n", pi, ret);
 }
 
 static void