diff mbox series

net/nfp: improve readability NFP HWINFO header

Message ID 1661492343-23225-1-git-send-email-chaoyong.he@corigine.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series net/nfp: improve readability NFP HWINFO header |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

Chaoyong He Aug. 26, 2022, 5:39 a.m. UTC 
  From: James Hershaw <james.hershaw@corigine.com>

Prepend `0x` to the NFP HWINFO header value that is printed to improve
the readability of the printed statement.

Signed-off-by: James Hershaw <james.hershaw@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
---
 drivers/net/nfp/nfpcore/nfp_hwinfo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Niklas Söderlund Sept. 20, 2022, 9:58 a.m. UTC | #1 
Hi all,

Gentle ping.

On 2022-08-26 13:39:03 +0800, Chaoyong He wrote:
> From: James Hershaw <james.hershaw@corigine.com>
> 
> Prepend `0x` to the NFP HWINFO header value that is printed to improve
> the readability of the printed statement.
> 
> Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> ---
>  drivers/net/nfp/nfpcore/nfp_hwinfo.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/nfp/nfpcore/nfp_hwinfo.c b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
> index c0516bf..9f848bd 100644
> --- a/drivers/net/nfp/nfpcore/nfp_hwinfo.c
> +++ b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
> @@ -108,7 +108,7 @@
>  		goto exit_free;
>  
>  	header = (void *)db;
> -	printf("NFP HWINFO header: %08x\n", *(uint32_t *)header);
> +	printf("NFP HWINFO header: %#08x\n", *(uint32_t *)header);
>  	if (nfp_hwinfo_is_updating(header))
>  		goto exit_free;
>  
> -- 
> 1.8.3.1
>
Ferruh Yigit Sept. 20, 2022, 5:33 p.m. UTC | #2 
On 8/26/2022 6:39 AM, Chaoyong He wrote:
> From: James Hershaw <james.hershaw@corigine.com>
> 
> Prepend `0x` to the NFP HWINFO header value that is printed to improve
> the readability of the printed statement.
> 
> Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> ---
>   drivers/net/nfp/nfpcore/nfp_hwinfo.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/nfp/nfpcore/nfp_hwinfo.c b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
> index c0516bf..9f848bd 100644
> --- a/drivers/net/nfp/nfpcore/nfp_hwinfo.c
> +++ b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
> @@ -108,7 +108,7 @@
>   		goto exit_free;
>   
>   	header = (void *)db;
> -	printf("NFP HWINFO header: %08x\n", *(uint32_t *)header);
> +	printf("NFP HWINFO header: %#08x\n", *(uint32_t *)header);

Why driver is directly using 'printf', but not rte_log APIs?

I can see there are already 'PMD_INIT_LOG' & 'PMD_DRV_LOG' macros for this.
Niklas Söderlund Sept. 20, 2022, 5:51 p.m. UTC | #3 
Hi Ferruh,

Thanks for your feedback.

On 2022-09-20 18:33:02 +0100, Ferruh Yigit wrote:
> On 8/26/2022 6:39 AM, Chaoyong He wrote:
> > From: James Hershaw <james.hershaw@corigine.com>
> > 
> > Prepend `0x` to the NFP HWINFO header value that is printed to improve
> > the readability of the printed statement.
> > 
> > Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> > Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> > ---
> >   drivers/net/nfp/nfpcore/nfp_hwinfo.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/nfp/nfpcore/nfp_hwinfo.c b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
> > index c0516bf..9f848bd 100644
> > --- a/drivers/net/nfp/nfpcore/nfp_hwinfo.c
> > +++ b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
> > @@ -108,7 +108,7 @@
> >   		goto exit_free;
> >   	header = (void *)db;
> > -	printf("NFP HWINFO header: %08x\n", *(uint32_t *)header);
> > +	printf("NFP HWINFO header: %#08x\n", *(uint32_t *)header);
> 
> Why driver is directly using 'printf', but not rte_log APIs?
> 
> I can see there are already 'PMD_INIT_LOG' & 'PMD_DRV_LOG' macros for this.

We have a series ready to convert all printf style logging into rte_log 
APIs as well as fix some other style issues.

We also have a few other things in our internal patch queue waiting to 
be sent out. To reduce conflicts in patchwork we are sending them out in 
the order as some of them depends on each other. And the one cleaning up 
log messages are at the end of the pile unfortunately.

Do you think it's acceptable to take this fix as-is and then a patch 
that convert all printf on one go, or would you prefers we move touch 
this line only once and create a v2 of this fix while also moving it to 
the rte_log APIs?
Ferruh Yigit Sept. 20, 2022, 6:01 p.m. UTC | #4 
On 9/20/2022 6:51 PM, Niklas Söderlund wrote:
> Hi Ferruh,
> 
> Thanks for your feedback.
> 
> On 2022-09-20 18:33:02 +0100, Ferruh Yigit wrote:
>> On 8/26/2022 6:39 AM, Chaoyong He wrote:
>>> From: James Hershaw <james.hershaw@corigine.com>
>>>
>>> Prepend `0x` to the NFP HWINFO header value that is printed to improve
>>> the readability of the printed statement.
>>>
>>> Signed-off-by: James Hershaw <james.hershaw@corigine.com>
>>> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
>>> ---
>>>    drivers/net/nfp/nfpcore/nfp_hwinfo.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/nfp/nfpcore/nfp_hwinfo.c b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
>>> index c0516bf..9f848bd 100644
>>> --- a/drivers/net/nfp/nfpcore/nfp_hwinfo.c
>>> +++ b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
>>> @@ -108,7 +108,7 @@
>>>    		goto exit_free;
>>>    	header = (void *)db;
>>> -	printf("NFP HWINFO header: %08x\n", *(uint32_t *)header);
>>> +	printf("NFP HWINFO header: %#08x\n", *(uint32_t *)header);
>>
>> Why driver is directly using 'printf', but not rte_log APIs?
>>
>> I can see there are already 'PMD_INIT_LOG' & 'PMD_DRV_LOG' macros for this.
> 
> We have a series ready to convert all printf style logging into rte_log
> APIs as well as fix some other style issues.
> 
> We also have a few other things in our internal patch queue waiting to
> be sent out. To reduce conflicts in patchwork we are sending them out in
> the order as some of them depends on each other. And the one cleaning up
> log messages are at the end of the pile unfortunately.
> 
> Do you think it's acceptable to take this fix as-is and then a patch
> that convert all printf on one go, or would you prefers we move touch
> this line only once and create a v2 of this fix while also moving it to
> the rte_log APIs?
> 

Hi Niklas,

Good to hear that you have patch to convert them to rte_log.

Instead of changing the log content and API with same patch, it is 
better to have them separate.

I prefer to convert them to proper log API first, and later fix the 
content of the log (to not update a line with wrong call).
But order of patch preference is a soft one, if somehow other-way around 
(first fix the log content, later the API) makes your life easier, I am 
OK to go with that too (as long as both issues are fixed).
Niklas Söderlund Sept. 21, 2022, 7:19 a.m. UTC | #5 
Hello Ferruh,

On 2022-09-20 19:01:47 +0100, Ferruh Yigit wrote:
> Instead of changing the log content and API with same patch, it is 
> better to have them separate.

I agree.

> 
> I prefer to convert them to proper log API first, and later fix the content
> of the log (to not update a line with wrong call).
> But order of patch preference is a soft one, if somehow other-way around
> (first fix the log content, later the API) makes your life easier, I am OK
> to go with that too (as long as both issues are fixed).

In principle I agree with you here as well. In this case it would make 
our life easier if we could do it the other way around. Reason being the 
patch to convert the NFP PMD to the log API touch most files in the 
driver and is at the tail of the 50+ patches we have in our internal 
queue trying to get out.

While it would be somewhat OK to post that patch separately I fear it 
would create conflicts with the other patches in our queue. So I thin 
either we drop this patch now and we pick it up at the end of our queue,
or we take this as is now. My preference would be to take it now, but 
I'm not feeling strongly about it.
Ferruh Yigit Sept. 21, 2022, 8:09 a.m. UTC | #6 
On 9/21/2022 8:19 AM, Niklas Söderlund wrote:
> Hello Ferruh,
> 
> On 2022-09-20 19:01:47 +0100, Ferruh Yigit wrote:
>> Instead of changing the log content and API with same patch, it is
>> better to have them separate.
> 
> I agree.
> 
>>
>> I prefer to convert them to proper log API first, and later fix the content
>> of the log (to not update a line with wrong call).
>> But order of patch preference is a soft one, if somehow other-way around
>> (first fix the log content, later the API) makes your life easier, I am OK
>> to go with that too (as long as both issues are fixed).
> 
> In principle I agree with you here as well. In this case it would make
> our life easier if we could do it the other way around. Reason being the
> patch to convert the NFP PMD to the log API touch most files in the
> driver and is at the tail of the 50+ patches we have in our internal
> queue trying to get out.
> 
> While it would be somewhat OK to post that patch separately I fear it
> would create conflicts with the other patches in our queue. So I thin
> either we drop this patch now and we pick it up at the end of our queue,
> or we take this as is now. My preference would be to take it now, but
> I'm not feeling strongly about it.
> 

It is very trivial change, lets get it out of way.
For log fix patch, I understand concern on the conflicts, but please 
don't forget to sent it out after functional patches are done.
Ferruh Yigit Sept. 21, 2022, 8:35 a.m. UTC | #7 
On 8/26/2022 6:39 AM, Chaoyong He wrote:
> From: James Hershaw <james.hershaw@corigine.com>
> 
> Prepend `0x` to the NFP HWINFO header value that is printed to improve
> the readability of the printed statement.
> 
> Signed-off-by: James Hershaw <james.hershaw@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>

     Fixes: c7e9729da6b5 ("net/nfp: support CPP")
     Cc: stable@dpdk.org

Applied to dpdk-next-net/main, thanks.
diff mbox series

Patch

diff --git a/drivers/net/nfp/nfpcore/nfp_hwinfo.c b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
index c0516bf..9f848bd 100644
--- a/drivers/net/nfp/nfpcore/nfp_hwinfo.c
+++ b/drivers/net/nfp/nfpcore/nfp_hwinfo.c
@@ -108,7 +108,7 @@ 
 		goto exit_free;
 
 	header = (void *)db;
-	printf("NFP HWINFO header: %08x\n", *(uint32_t *)header);
+	printf("NFP HWINFO header: %#08x\n", *(uint32_t *)header);
 	if (nfp_hwinfo_is_updating(header))
 		goto exit_free;