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

["Jiawen Wu" <jiawenwu@trustnetic.com>]

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.