Re: [dpdk-dev] [PATCH v3 01/18] net/axgbe: add minimal dev init and uninit support

["Kumar, Ravi1" <Ravi1.Kumar@amd.com>]

>
>
>-----Original Message-----
>From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] 
>Sent: Friday, March 16, 2018 11:12 PM
>To: Kumar, Ravi1 <Ravi1.Kumar@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.kumar@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