Re: [dpdk-dev] [PATCH v2 01/14] net/igc: add igc PMD

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

On 3/20/2020 2:46 AM, alvinx.zhang@intel.com wrote:
> From: Alvin Zhang <alvinx.zhang@intel.com>
> 
> Implement device detection and loading.
> Add igc driver guid docs.
> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> 
> v2: Update release note. Modify codes according to comments

<...>

> @@ -0,0 +1,39 @@
> +..  SPDX-License-Identifier: BSD-3-Clause
> +    Copyright(c) 2016 Intel Corporation.

Is the copyright date '2016' correct? If so you can update it as 2016-2020. This
comment is for all files.

> +
> +IGC Poll Mode Driver
> +======================
> +
> +The IGC PMD (librte_pmd_igc) provides poll mode driver support for
> +Foxville I225 Series Network Adapters.

Can you please provide some official links to the product? As much as possible
information about device is good.

<...>

> @@ -56,11 +56,16 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =========================================================
>  
> -* **Updated Mellanox mlx5 driver.**
> +   * **Updated Mellanox mlx5 driver.**
>  
> -  Updated Mellanox mlx5 driver with new features and improvements, including:
> +     Updated Mellanox mlx5 driver with new features and improvements, including:
>  
> -  * Added support for matching on IPv4 Time To Live and IPv6 Hop Limit.
> +     * Added support for matching on IPv4 Time To Live and IPv6 Hop Limit.

Above looks changed by mistake...

> +
> +   * **Added a new driver for Intel Foxville I225 devices.**
> +
> +     Added the new ``igc`` net driver for Intel Foxville I225 devices. See the
> +     :doc:`../nics/igc` NIC guide for more details on this new driver.
>  
>  
>  Removed Items
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 4a7f155..b57841d 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -61,6 +61,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD) += thunderx
>  DIRS-$(CONFIG_RTE_LIBRTE_VDEV_NETVSC_PMD) += vdev_netvsc
>  DIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio
>  DIRS-$(CONFIG_RTE_LIBRTE_VMXNET3_PMD) += vmxnet3
> +DIRS-$(CONFIG_RTE_LIBRTE_IGC_PMD) += igc

Can you please add it alphabetically sorted?

<...>

> +static int
> +eth_igc_dev_init(struct rte_eth_dev *dev)
> +{
> +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> +
> +	PMD_INIT_FUNC_TRACE();
> +	dev->dev_ops = &eth_igc_ops;
> +
> +	/*
> +	 * for secondary processes, we don't initialize any further as primary
> +	 * has already done this work. Only check we don't need a different
> +	 * RX function.
> +	 */
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return 0;
> +
> +	rte_eth_copy_pci_info(dev, pci_dev);

This shouldn't be required, since it is done by
'rte_eth_dev_pci_generic_probe()' just before this funtion
('eth_igc_dev_init()') called.

> +
> +	dev->data->mac_addrs = rte_zmalloc("igc",
> +		RTE_ETHER_ADDR_LEN, 0);
> +	if (dev->data->mac_addrs == NULL) {
> +		PMD_INIT_LOG(ERR, "Failed to allocate %d bytes needed to "
> +				"store MAC addresses", RTE_ETHER_ADDR_LEN);
> +		return -ENOMEM;
> +	}
> +
> +	/* Pass the information to the rte_eth_dev_close() that it should also
> +	 * release the private port resources.
> +	 */
> +	dev->data->dev_flags |= RTE_ETH_DEV_CLOSE_REMOVE;
> +
> +	PMD_INIT_LOG(DEBUG, "port_id %d vendorID=0x%x deviceID=0x%x",
> +			dev->data->port_id, pci_dev->id.vendor_id,
> +			pci_dev->id.device_id);
> +
> +	return 0;
> +}
> +
> +static int
> +eth_igc_dev_uninit(__rte_unused struct rte_eth_dev *eth_dev)
> +{
> +	PMD_INIT_FUNC_TRACE();
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return -EPERM;

It shouldn't return error for secondary. 'rte_eth_dev_release_port()' has
already process type in it, so returning '0' should work better which will cause
some process specific variables cleared.

<...>

> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Intel Corporation
> + */
> +
> +#include "igc_logs.h"
> +#include "rte_common.h"

'rte_common.h' should be included with '<>', #include <rte_common.h>

> +
> +/* declared as extern in igc_logs.h */
> +int igc_logtype_init = -1;
> +int igc_logtype_driver = -1;

I guess no need to set initial values for these, by default '0' will work fine
for below logic.

<...>

> @@ -0,0 +1,3 @@
> +DPDK_20.0.1 {

This release it become "DPDK_20.0.2", although it doesn't matter for the PMD at
all, good to be consistent.

> +	local: *;
> +};
> diff --git a/drivers/net/meson.build b/drivers/net/meson.build
> index b0ea8fe..7d0ae3b 100644
> --- a/drivers/net/meson.build
> +++ b/drivers/net/meson.build
> @@ -49,6 +49,7 @@ drivers = ['af_packet',
>  	'vhost',
>  	'virtio',
>  	'vmxnet3',
> +	'igc',

Can you please add it alphabetically sorted?