diff mbox series

[v2,08/12] net/ngbe: fix debug log

Message ID	20220209104213.602728-9-jiawenwu@trustnetic.com (mailing list archive)
State	Rejected, archived
Delegated to:	Ferruh Yigit
Headers	From: Jiawen Wu <jiawenwu@trustnetic.com> To: dev@dpdk.org Cc: Jiawen Wu <jiawenwu@trustnetic.com>, stable@dpdk.org Subject: [PATCH v2 08/12] net/ngbe: fix debug log Date: Wed, 9 Feb 2022 18:42:09 +0800 Message-Id: <20220209104213.602728-9-jiawenwu@trustnetic.com> In-Reply-To: <20220209104213.602728-1-jiawenwu@trustnetic.com> References: <20220209104213.602728-1-jiawenwu@trustnetic.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Feedback-ID: bizesmtp:trustnetic.com:qybgforeign:qybgforeign1 Precedence: list Errors-To: dev-bounces@dpdk.org
Series	Wangxun fixes and supports \| [v2,00/12] Wangxun fixes and supports [v2,01/12] net/ngbe: fix failed to receive packets [v2,02/12] net/ngbe: fix link interrupt sometimes lost [v2,03/12] net/ngbe: fix Tx pending [v2,04/12] net/ngbe: fix RxTx packet statistics [v2,05/12] net/ngbe: optimize the PHY initialization process [v2,06/12] net/ngbe: add support to custom PHY interfaces [v2,07/12] net/ngbe: add LED OEM support [v2,08/12] net/ngbe: fix debug log [v2,09/12] net/txgbe: add LED OEM support [v2,10/12] net/txgbe: fix debug log [v2,11/12] net/txgbe: fix to set link up and down [v2,12/12] net/txgbe: fix KR auto-negotiation

Checks

Context	Check	Description
ci/checkpatch	success	coding style OK

Commit Message

Jiawen Wu Feb. 9, 2022, 10:42 a.m. UTC

  Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
that double line was added by using 'DEBUGOUT'.

Fixes: cc934df178ab ("net/ngbe: add log and error types")
Cc: stable@dpdk.org

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Ferruh Yigit Feb. 9, 2022, 7:07 p.m. UTC | #1

On 2/9/2022 10:42 AM, Jiawen Wu wrote:
> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
> that double line was added by using 'DEBUGOUT'.
> 
> Fixes: cc934df178ab ("net/ngbe: add log and error types")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>   drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ngbe/ngbe_logs.h b/drivers/net/ngbe/ngbe_logs.h
> index fd306419e6..b7d78fb400 100644
> --- a/drivers/net/ngbe/ngbe_logs.h
> +++ b/drivers/net/ngbe/ngbe_logs.h
> @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
>   		"%s(): " fmt "\n", __func__, ##args)
>   
>   extern int ngbe_logtype_driver;
> -#define PMD_DRV_LOG(level, fmt, args...) \
> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
>   	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
> -		"%s(): " fmt "\n", __func__, ##args)
> +		"%s(): " fmt, __func__, ##args)
> +
> +#define PMD_DRV_LOG(level, fmt, args...) \
> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
>   

Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same thing,
but one appends '\n' other doesn't, this is very error prune and there
are already wrong usage in the driver.
I think no need to add complexity for something as simple as this, what do
you think to unify the DEBUG level macros, at least unify the line ending
behavior?


>   #ifdef RTE_ETHDEV_DEBUG_RX
>   extern int ngbe_logtype_rx;
> @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
>   #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
>   #endif
>   
> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt, ##args)
> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt, ##args)
>   
>   #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
>   #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")

What is difference between 'PMD_INIT_FUNC_TRACE' and 'DEBUGFUNC'?
As far as I can see they are for same reason, and both macros are
used. If they are for same reason can you please unify the usage?

> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
> +#define DEBUGFUNC(fmt)            do { } while (0)

If 'DEBUGFUNC' can be removed, why not removing from the code, instead
of above define? This is your driver, you have full control on it.

Jiawen Wu Feb. 10, 2022, 8:03 a.m. UTC | #2

On February 10, 2022 3:07 AM, Ferruh Yigit wrote:
> On 2/9/2022 10:42 AM, Jiawen Wu wrote:
> > Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
> > that double line was added by using 'DEBUGOUT'.
> >
> > Fixes: cc934df178ab ("net/ngbe: add log and error types")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> > ---
> >   drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ngbe/ngbe_logs.h
> > b/drivers/net/ngbe/ngbe_logs.h index fd306419e6..b7d78fb400 100644
> > --- a/drivers/net/ngbe/ngbe_logs.h
> > +++ b/drivers/net/ngbe/ngbe_logs.h
> > @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
> >   		"%s(): " fmt "\n", __func__, ##args)
> >
> >   extern int ngbe_logtype_driver;
> > -#define PMD_DRV_LOG(level, fmt, args...) \
> > +#define PMD_TLOG_DRIVER(level, fmt, args...) \
> >   	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
> > -		"%s(): " fmt "\n", __func__, ##args)
> > +		"%s(): " fmt, __func__, ##args)
> > +
> > +#define PMD_DRV_LOG(level, fmt, args...) \
> > +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
> >
> 
> Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same thing,
> but one appends '\n' other doesn't, this is very error prune and there are
> already wrong usage in the driver.
> I think no need to add complexity for something as simple as this, what do you
> think to unify the DEBUG level macros, at least unify the line ending behavior?
> 
> 
> >   #ifdef RTE_ETHDEV_DEBUG_RX
> >   extern int ngbe_logtype_rx;
> > @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
> >   #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
> >   #endif
> >
> > -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt, ##args)
> > +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt,
> ##args)
> >
> >   #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
> >   #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
> 
> What is difference between 'PMD_INIT_FUNC_TRACE' and 'DEBUGFUNC'?
> As far as I can see they are for same reason, and both macros are used. If they
> are for same reason can you please unify the usage?
> 
> > -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
> > +#define DEBUGFUNC(fmt)            do { } while (0)
> 
> If 'DEBUGFUNC' can be removed, why not removing from the code, instead of
> above define? This is your driver, you have full control on it.

Okay. I just realize that this is going to be a big change, so I want to keep it to a minimum.
Anyway, 'DEBUGFUNC' should be removed, because 'DEBUGOUT' already contains the function name.

Ferruh Yigit Feb. 10, 2022, 9:02 a.m. UTC | #3

On 2/10/2022 8:03 AM, Jiawen Wu wrote:
> On February 10, 2022 3:07 AM, Ferruh Yigit wrote:
>> On 2/9/2022 10:42 AM, Jiawen Wu wrote:
>>> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
>>> that double line was added by using 'DEBUGOUT'.
>>>
>>> Fixes: cc934df178ab ("net/ngbe: add log and error types")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
>>> ---
>>>    drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
>>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/ngbe/ngbe_logs.h
>>> b/drivers/net/ngbe/ngbe_logs.h index fd306419e6..b7d78fb400 100644
>>> --- a/drivers/net/ngbe/ngbe_logs.h
>>> +++ b/drivers/net/ngbe/ngbe_logs.h
>>> @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
>>>    		"%s(): " fmt "\n", __func__, ##args)
>>>
>>>    extern int ngbe_logtype_driver;
>>> -#define PMD_DRV_LOG(level, fmt, args...) \
>>> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
>>>    	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
>>> -		"%s(): " fmt "\n", __func__, ##args)
>>> +		"%s(): " fmt, __func__, ##args)
>>> +
>>> +#define PMD_DRV_LOG(level, fmt, args...) \
>>> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
>>>
>>
>> Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same thing,
>> but one appends '\n' other doesn't, this is very error prune and there are
>> already wrong usage in the driver.
>> I think no need to add complexity for something as simple as this, what do you
>> think to unify the DEBUG level macros, at least unify the line ending behavior?
>>
>>
>>>    #ifdef RTE_ETHDEV_DEBUG_RX
>>>    extern int ngbe_logtype_rx;
>>> @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
>>>    #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
>>>    #endif
>>>
>>> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt, ##args)
>>> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt,
>> ##args)
>>>
>>>    #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
>>>    #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
>>
>> What is difference between 'PMD_INIT_FUNC_TRACE' and 'DEBUGFUNC'?
>> As far as I can see they are for same reason, and both macros are used. If they
>> are for same reason can you please unify the usage?
>>
>>> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
>>> +#define DEBUGFUNC(fmt)            do { } while (0)
>>
>> If 'DEBUGFUNC' can be removed, why not removing from the code, instead of
>> above define? This is your driver, you have full control on it.
> 
> Okay. I just realize that this is going to be a big change, so I want to keep it to a minimum.
> Anyway, 'DEBUGFUNC' should be removed, because 'DEBUGOUT' already contains the function name.
> 

If it will be big, we can move the fix out of this set, to not block set,
and send a log fix later as a separate patch, what do you think?

Jiawen Wu Feb. 10, 2022, 9:49 a.m. UTC | #4

On February 10, 2022 5:03 PM, Ferruh Yigit wrote:
> On 2/10/2022 8:03 AM, Jiawen Wu wrote:
> > On February 10, 2022 3:07 AM, Ferruh Yigit wrote:
> >> On 2/9/2022 10:42 AM, Jiawen Wu wrote:
> >>> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
> >>> that double line was added by using 'DEBUGOUT'.
> >>>
> >>> Fixes: cc934df178ab ("net/ngbe: add log and error types")
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> >>> ---
> >>>    drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
> >>>    1 file changed, 7 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ngbe/ngbe_logs.h
> >>> b/drivers/net/ngbe/ngbe_logs.h index fd306419e6..b7d78fb400 100644
> >>> --- a/drivers/net/ngbe/ngbe_logs.h
> >>> +++ b/drivers/net/ngbe/ngbe_logs.h
> >>> @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
> >>>    		"%s(): " fmt "\n", __func__, ##args)
> >>>
> >>>    extern int ngbe_logtype_driver;
> >>> -#define PMD_DRV_LOG(level, fmt, args...) \
> >>> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
> >>>    	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
> >>> -		"%s(): " fmt "\n", __func__, ##args)
> >>> +		"%s(): " fmt, __func__, ##args)
> >>> +
> >>> +#define PMD_DRV_LOG(level, fmt, args...) \
> >>> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
> >>>
> >>
> >> Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same
> >> thing, but one appends '\n' other doesn't, this is very error prune
> >> and there are already wrong usage in the driver.
> >> I think no need to add complexity for something as simple as this,
> >> what do you think to unify the DEBUG level macros, at least unify the line
> ending behavior?
> >>
> >>
> >>>    #ifdef RTE_ETHDEV_DEBUG_RX
> >>>    extern int ngbe_logtype_rx;
> >>> @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
> >>>    #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
> >>>    #endif
> >>>
> >>> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt,
> ##args)
> >>> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt,
> >> ##args)
> >>>
> >>>    #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
> >>>    #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
> >>
> >> What is difference between 'PMD_INIT_FUNC_TRACE' and 'DEBUGFUNC'?
> >> As far as I can see they are for same reason, and both macros are
> >> used. If they are for same reason can you please unify the usage?
> >>
> >>> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
> >>> +#define DEBUGFUNC(fmt)            do { } while (0)
> >>
> >> If 'DEBUGFUNC' can be removed, why not removing from the code,
> >> instead of above define? This is your driver, you have full control on it.
> >
> > Okay. I just realize that this is going to be a big change, so I want to keep it to
> a minimum.
> > Anyway, 'DEBUGFUNC' should be removed, because 'DEBUGOUT' already
> contains the function name.
> >
> 
> If it will be big, we can move the fix out of this set, to not block set, and send a
> log fix later as a separate patch, what do you think?

I think it's actually better.

Ferruh Yigit Feb. 10, 2022, 10:16 a.m. UTC | #5

On 2/10/2022 9:49 AM, Jiawen Wu wrote:
> On February 10, 2022 5:03 PM, Ferruh Yigit wrote:
>> On 2/10/2022 8:03 AM, Jiawen Wu wrote:
>>> On February 10, 2022 3:07 AM, Ferruh Yigit wrote:
>>>> On 2/9/2022 10:42 AM, Jiawen Wu wrote:
>>>>> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And fix
>>>>> that double line was added by using 'DEBUGOUT'.
>>>>>
>>>>> Fixes: cc934df178ab ("net/ngbe: add log and error types")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
>>>>> ---
>>>>>     drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
>>>>>     1 file changed, 7 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ngbe/ngbe_logs.h
>>>>> b/drivers/net/ngbe/ngbe_logs.h index fd306419e6..b7d78fb400 100644
>>>>> --- a/drivers/net/ngbe/ngbe_logs.h
>>>>> +++ b/drivers/net/ngbe/ngbe_logs.h
>>>>> @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
>>>>>     		"%s(): " fmt "\n", __func__, ##args)
>>>>>
>>>>>     extern int ngbe_logtype_driver;
>>>>> -#define PMD_DRV_LOG(level, fmt, args...) \
>>>>> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
>>>>>     	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
>>>>> -		"%s(): " fmt "\n", __func__, ##args)
>>>>> +		"%s(): " fmt, __func__, ##args)
>>>>> +
>>>>> +#define PMD_DRV_LOG(level, fmt, args...) \
>>>>> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
>>>>>
>>>>
>>>> Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same
>>>> thing, but one appends '\n' other doesn't, this is very error prune
>>>> and there are already wrong usage in the driver.
>>>> I think no need to add complexity for something as simple as this,
>>>> what do you think to unify the DEBUG level macros, at least unify the line
>> ending behavior?
>>>>
>>>>
>>>>>     #ifdef RTE_ETHDEV_DEBUG_RX
>>>>>     extern int ngbe_logtype_rx;
>>>>> @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
>>>>>     #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
>>>>>     #endif
>>>>>
>>>>> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt,
>> ##args)
>>>>> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt,
>>>> ##args)
>>>>>
>>>>>     #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
>>>>>     #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
>>>>
>>>> What is difference between 'PMD_INIT_FUNC_TRACE' and 'DEBUGFUNC'?
>>>> As far as I can see they are for same reason, and both macros are
>>>> used. If they are for same reason can you please unify the usage?
>>>>
>>>>> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
>>>>> +#define DEBUGFUNC(fmt)            do { } while (0)
>>>>
>>>> If 'DEBUGFUNC' can be removed, why not removing from the code,
>>>> instead of above define? This is your driver, you have full control on it.
>>>
>>> Okay. I just realize that this is going to be a big change, so I want to keep it to
>> a minimum.
>>> Anyway, 'DEBUGFUNC' should be removed, because 'DEBUGOUT' already
>> contains the function name.
>>>
>>
>> If it will be big, we can move the fix out of this set, to not block set, and send a
>> log fix later as a separate patch, what do you think?
> 
> I think it's actually better.
> 

As long as you commit to send log fix after -rc1, I am OK.

Are you planning to send a new version of this set, or will it work
if I just drop the patch 8/12 & 10/12 of this set and continue with this version?

Jiawen Wu Feb. 11, 2022, 2:02 a.m. UTC | #6

On February 10, 2022 6:16 PM, Ferruh Yigit wrote:
> On 2/10/2022 9:49 AM, Jiawen Wu wrote:
> > On February 10, 2022 5:03 PM, Ferruh Yigit wrote:
> >> On 2/10/2022 8:03 AM, Jiawen Wu wrote:
> >>> On February 10, 2022 3:07 AM, Ferruh Yigit wrote:
> >>>> On 2/9/2022 10:42 AM, Jiawen Wu wrote:
> >>>>> Remove 'DEBUGFUNC' due to too many invalid debug log prints. And
> >>>>> fix that double line was added by using 'DEBUGOUT'.
> >>>>>
> >>>>> Fixes: cc934df178ab ("net/ngbe: add log and error types")
> >>>>> Cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> >>>>> ---
> >>>>>     drivers/net/ngbe/ngbe_logs.h | 11 +++++++----
> >>>>>     1 file changed, 7 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/net/ngbe/ngbe_logs.h
> >>>>> b/drivers/net/ngbe/ngbe_logs.h index fd306419e6..b7d78fb400 100644
> >>>>> --- a/drivers/net/ngbe/ngbe_logs.h
> >>>>> +++ b/drivers/net/ngbe/ngbe_logs.h
> >>>>> @@ -15,9 +15,12 @@ extern int ngbe_logtype_init;
> >>>>>     		"%s(): " fmt "\n", __func__, ##args)
> >>>>>
> >>>>>     extern int ngbe_logtype_driver; -#define PMD_DRV_LOG(level,
> >>>>> fmt, args...) \
> >>>>> +#define PMD_TLOG_DRIVER(level, fmt, args...) \
> >>>>>     	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
> >>>>> -		"%s(): " fmt "\n", __func__, ##args)
> >>>>> +		"%s(): " fmt, __func__, ##args)
> >>>>> +
> >>>>> +#define PMD_DRV_LOG(level, fmt, args...) \
> >>>>> +	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
> >>>>>
> >>>>
> >>>> Both 'DEBUGOUT' and 'PMD_DRV_LOG(DEBUG, ..' are in use for same
> >>>> thing, but one appends '\n' other doesn't, this is very error prune
> >>>> and there are already wrong usage in the driver.
> >>>> I think no need to add complexity for something as simple as this,
> >>>> what do you think to unify the DEBUG level macros, at least unify
> >>>> the line
> >> ending behavior?
> >>>>
> >>>>
> >>>>>     #ifdef RTE_ETHDEV_DEBUG_RX
> >>>>>     extern int ngbe_logtype_rx;
> >>>>> @@ -37,10 +40,10 @@ extern int ngbe_logtype_tx;
> >>>>>     #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
> >>>>>     #endif
> >>>>>
> >>>>> -#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt,
> >> ##args)
> >>>>> +#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG,
> fmt,
> >>>> ##args)
> >>>>>
> >>>>>     #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
> >>>>>     #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
> >>>>
> >>>> What is difference between 'PMD_INIT_FUNC_TRACE' and
> 'DEBUGFUNC'?
> >>>> As far as I can see they are for same reason, and both macros are
> >>>> used. If they are for same reason can you please unify the usage?
> >>>>
> >>>>> -#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
> >>>>> +#define DEBUGFUNC(fmt)            do { } while (0)
> >>>>
> >>>> If 'DEBUGFUNC' can be removed, why not removing from the code,
> >>>> instead of above define? This is your driver, you have full control on it.
> >>>
> >>> Okay. I just realize that this is going to be a big change, so I
> >>> want to keep it to
> >> a minimum.
> >>> Anyway, 'DEBUGFUNC' should be removed, because 'DEBUGOUT' already
> >> contains the function name.
> >>>
> >>
> >> If it will be big, we can move the fix out of this set, to not block
> >> set, and send a log fix later as a separate patch, what do you think?
> >
> > I think it's actually better.
> >
> 
> As long as you commit to send log fix after -rc1, I am OK.
> 
> Are you planning to send a new version of this set, or will it work if I just drop
> the patch 8/12 & 10/12 of this set and continue with this version?

It works, you can continue with this version. Thanks Ferruh.

diff mbox series

Patch

diff --git a/drivers/net/ngbe/ngbe_logs.h b/drivers/net/ngbe/ngbe_logs.h
index fd306419e6..b7d78fb400 100644
--- a/drivers/net/ngbe/ngbe_logs.h
+++ b/drivers/net/ngbe/ngbe_logs.h
@@ -15,9 +15,12 @@  extern int ngbe_logtype_init;
 		"%s(): " fmt "\n", __func__, ##args)
 
 extern int ngbe_logtype_driver;
-#define PMD_DRV_LOG(level, fmt, args...) \
+#define PMD_TLOG_DRIVER(level, fmt, args...) \
 	rte_log(RTE_LOG_ ## level, ngbe_logtype_driver, \
-		"%s(): " fmt "\n", __func__, ##args)
+		"%s(): " fmt, __func__, ##args)
+
+#define PMD_DRV_LOG(level, fmt, args...) \
+	PMD_TLOG_DRIVER(level, fmt "\n", ## args)
 
 #ifdef RTE_ETHDEV_DEBUG_RX
 extern int ngbe_logtype_rx;
@@ -37,10 +40,10 @@  extern int ngbe_logtype_tx;
 #define PMD_TX_LOG(level, fmt, args...) do { } while (0)
 #endif
 
-#define TLOG_DEBUG(fmt, args...)  PMD_DRV_LOG(DEBUG, fmt, ##args)
+#define TLOG_DEBUG(fmt, args...)  PMD_TLOG_DRIVER(DEBUG, fmt, ##args)
 
 #define DEBUGOUT(fmt, args...)    TLOG_DEBUG(fmt, ##args)
 #define PMD_INIT_FUNC_TRACE()     TLOG_DEBUG(" >>")
-#define DEBUGFUNC(fmt)            TLOG_DEBUG(fmt)
+#define DEBUGFUNC(fmt)            do { } while (0)
 
 #endif /* _NGBE_LOGS_H_ */