> -----Original Message----- > From: Ferruh Yigit <ferruh.yi...@xilinx.com> > Sent: Wednesday, September 7, 2022 19:17 > To: Guo, Junfeng <junfeng....@intel.com>; Zhang, Qi Z > <qi.z.zh...@intel.com>; Wu, Jingjing <jingjing...@intel.com> > Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun...@intel.com>; > awogbem...@google.com; Richardson, Bruce > <bruce.richard...@intel.com>; Wang, Haiyue <haiyue.w...@intel.com> > Subject: Re: [PATCH v2 02/10] net/gve: add logs and OS specific > implementation > > On 9/7/2022 7:58 AM, Guo, Junfeng wrote: > > CAUTION: This message has originated from an External Source. Please > use proper judgment and caution when opening attachments, clicking > links, or responding to this email. > > > > > >> -----Original Message----- > >> From: Ferruh Yigit <ferruh.yi...@xilinx.com> > >> Sent: Friday, September 2, 2022 01:21 > >> To: Guo, Junfeng <junfeng....@intel.com>; Zhang, Qi Z > >> <qi.z.zh...@intel.com>; Wu, Jingjing <jingjing...@intel.com> > >> Cc: dev@dpdk.org; Li, Xiaoyun <xiaoyun...@intel.com>; > >> awogbem...@google.com; Richardson, Bruce > >> <bruce.richard...@intel.com>; Wang, Haiyue > <haiyue.w...@intel.com> > >> Subject: Re: [PATCH v2 02/10] net/gve: add logs and OS specific > >> implementation > >> > >> On 8/29/2022 9:41 AM, Junfeng Guo wrote: > >> > >>> > >>> Add GVE PMD logs. > >>> Add some MACRO definitions and memory operations which are > specific > >>> for DPDK. > >>> > >>> Signed-off-by: Haiyue Wang <haiyue.w...@intel.com> > >>> Signed-off-by: Xiaoyun Li <xiaoyun...@intel.com> > >>> Signed-off-by: Junfeng Guo <junfeng....@intel.com> > >> > >> <...> > >> > >>> diff --git a/drivers/net/gve/gve_logs.h b/drivers/net/gve/gve_logs.h > >>> new file mode 100644 > >>> index 0000000000..a050253f59 > >>> --- /dev/null > >>> +++ b/drivers/net/gve/gve_logs.h > >>> @@ -0,0 +1,22 @@ > >>> +/* SPDX-License-Identifier: BSD-3-Clause > >>> + * Copyright(C) 2022 Intel Corporation > >>> + */ > >>> + > >>> +#ifndef _GVE_LOGS_H_ > >>> +#define _GVE_LOGS_H_ > >>> + > >>> +extern int gve_logtype_init; > >>> +extern int gve_logtype_driver; > >>> + > >>> +#define PMD_INIT_LOG(level, fmt, args...) \ > >>> + rte_log(RTE_LOG_ ## level, gve_logtype_init, "%s(): " fmt "\n", \ > >>> + __func__, ##args) > >>> + > >>> +#define PMD_DRV_LOG_RAW(level, fmt, args...) \ > >>> + rte_log(RTE_LOG_ ## level, gve_logtype_driver, "%s(): " fmt, \ > >>> + __func__, ## args) > >>> + > +#define PMD_DRV_LOG(level, fmt, args...) \ > >>> + PMD_DRV_LOG_RAW(level, fmt "\n", ## args) > >>> + > >> > >> Why 'PMD_DRV_LOG_RAW' is needed, why not directly use > >> 'PMD_DRV_LOG'? > > > > It seems that the _RAW macro was first introduced at i40e driver logs > file. > > Since sometimes the trailing '\n' is added at the end of the log message > in > > the base code, the PMD_DRV_LOG_RAW macro that will not add one is > > used to keep consistent of the new line character. > > > > Well, looks that the macro PMD_DRV_LOG_RAW is somewhat > redundant. > > I think it's ok to remove PMD_DRV_LOG_RAW and keep all the log > messages > > end without the trailing '\n'. Thanks! > > > > Or you can add '\n' to 'PMD_DRV_LOG', to not change all logs. Only > having two macro seems unnecessary.
Yes, already did as this form in the coming version gve pmd code. Thanks! > > >> > >> > >> Do you really need two different log types? How do you differentiate > >> 'init' & 'driver' types? As far as I can see there is mixed usage of them. > > > > The PMD_INIT_LOG is used at the init stage, while the PMD_DRV_LOG > > is used at the driver normal running stage. I agree that there might be > > mixed usage of these two macros. I'll try to check all these usages and > > update them at correct conditions in the coming versions. > > If you insist that only one log type is needed to keep the code clean, > > then I could update them as you expected. Thanks! > > > > I do not insist, but it looks like you are complicating things, is there > really a benefit to have two different log types? Well, these two types may be used to show init/driver logs, respectively. But It seems that there is no such specific need to use two log types in the GVE PMD. Anyway, I think it is good time to keep the code clean and not just inherit from previous drivers. We can add new log type in the future if it's required. Thanks!
