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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 3/19/2018 12:33 PM, Kumar, Ravi1 wrote:
>>
>>
>> -----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. 

Hi Ravi,

Is there any update on PMD, will it able to make before integration deadline?

Thanks,
ferruh

> 
> Regards,
> Ravi
>