> > >-----Original Message----- >From: Ferruh Yigit [mailto:ferruh.yi...@intel.com] >Sent: Friday, March 16, 2018 11:12 PM >To: Kumar, Ravi1 <ravi1.ku...@amd.com>; dev@dpdk.org >Subject: Re: [PATCH v3 01/18] net/axgbe: add minimal dev init and uninit >support > >On 3/9/2018 8:42 AM, Ravi Kumar wrote: >> Signed-off-by: Ravi Kumar <ravi1.ku...@amd.com> > ><...> > >> @@ -412,6 +412,12 @@ CONFIG_RTE_PMD_RING_MAX_TX_RINGS=16 >> CONFIG_RTE_LIBRTE_PMD_SOFTNIC=y >> >> # >> +# Compile AMD PMD >> +# >> +CONFIG_RTE_LIBRTE_AXGBE_PMD=y >> +CONFIG_RTE_LIBRTE_AXGBE_DEBUG_INIT=n > >Config file recently updated to have PMD options sorted alphabetically, can >you please move config option according. > ><...> > >> +/* The set of PCI devices this driver supports */ >> +#define AMD_PCI_VENDOR_ID 0x1022 >> +#define AMD_PCI_AXGBE_DEVICE_ID1 0x1458 #define >> +AMD_PCI_AXGBE_DEVICE_ID2 0x1459 > >More descriptive name can help others to find which devices are supported, >instead of id1 and id2. > ><...> > >> +static int >> +eth_axgbe_dev_init(struct rte_eth_dev *eth_dev) { >> + PMD_INIT_FUNC_TRACE(); >> + struct axgbe_port *pdata; >> + struct rte_pci_device *pci_dev; >> + >> + pdata = (struct axgbe_port *)eth_dev->data->dev_private; >> + pdata->eth_dev = eth_dev; > >Since there is already a RTE_PROC_PRIMARY check below, these should be below >that check because secondary doesn't need to update pdata->eth_dev > ><...> > >> +RTE_PMD_REGISTER_PCI(net_axgbe, rte_axgbe_pmd); >> +RTE_PMD_REGISTER_PCI_TABLE(net_axgbe, pci_id_axgbe_map); >> +RTE_PMD_REGISTER_KMOD_DEP(net_axgbe, "* igb_uio | uio_pci_generic"); > >Is vfio-pci intentionally not included as dependency? > >> + >> +RTE_INIT(axgbe_init_log); >> +static void >> +axgbe_init_log(void) >> +{ >> + axgbe_logtype_init = rte_log_register("pmd.axgbe.init"); > >The log string syntax updated, now it should be "pmd.net.axgbe.*" > ><...> > >> +#ifdef RTE_LIBRTE_AXGBE_DEBUG_INIT >> +#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>") #else >> +#define PMD_INIT_FUNC_TRACE() do { } while (0) #endif > >Do we need this config option? The idea of dynamic logging is get rid of them. >If you want to control trace logs seperately, you can create a new logtype for >it can control dynamically. As long as it is not in datapath we can remove log >config options. > >> +extern int axgbe_logtype_driver; >> +#define PMD_DRV_LOG_RAW(level, fmt, args...) \ >> + rte_log(RTE_LOG_ ## level, axgbe_logtype_driver, "%s(): " fmt, \ >> + __func__, ## args) >> + >> +#define PMD_DRV_LOG(level, fmt, args...) \ >> + PMD_DRV_LOG_RAW(level, fmt "\n", ## args) > >Do you need interim PMD_DRV_LOG_RAW? Why not directly define PMD_DRV_LOG? > ><...>
Hi Ferruh, Thanks a lot for the review comments. We are working on these and will upload a patch-set that will address these and the previous SPDX licensing comments. Regards, Ravi
