Re: [dpdk-dev] [PATCH v4 11/11] net/hinic: add support for basic device operations

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

On 6/6/2019 12:07 PM, Ziyang Xuan wrote:
> Add hinic PMD initialization and ethernet operatioins code.

Hi Xuan,

Previous patches puts the code without enabling them, this last patch registers
the PMD with lots of new code, it is hard to review this PMD.

I think "OCTEON TX2" which also submitted this release [1] is good sample of how
building the PMD incrementally, feature by feature, can you please check it?
[1] https://patches.dpdk.org/user/todo/dpdk/?series=4848

> 
> Signed-off-by: Ziyang Xuan <xuanziyang2@huawei.com>
> ---
>  drivers/net/hinic/hinic_pmd_ethdev.c        | 2125 +++++++++++++++++++
>  drivers/net/hinic/rte_pmd_hinic_version.map |    4 +

.map file needs to be added in the patch that adds "hinic/Makefile", otherwise
shared build will fail for those patches in between.

<...>

> +
> +/* Hinic PMD parameters */
> +#define ETH_HINIC_FW_VER	"check_fw_version"
> +
> +static const char *const valid_params[] = {
> +	ETH_HINIC_FW_VER,
> +	NULL};


Can you please document this devargs in hinic documentation, describe what it
does, and perhaps provide a sample command line to use it.

<...>
<...>

> +	snprintf(nic_dev->proc_dev_name,
> +		 sizeof(nic_dev->proc_dev_name),
> +		 "hinic-%.4x:%.2x:%.2x.%x",
> +		 pci_dev->addr.domain, pci_dev->addr.bus,
> +		 pci_dev->addr.devid, pci_dev->addr.function);
> +
> +	rte_eth_copy_pci_info(eth_dev, pci_dev);

You may not need this, can you please double check?

> +
> +	/* clear RX ring mbuf allocated failed */
> +	eth_dev->data->rx_mbuf_alloc_failed = 0;

At this stage all ethdev->data should be 0, is this assignment required?

<...>

> +/**
> + * DPDK callback to close the device.
> + *
> + * @param dev
> + *   Pointer to Ethernet device structure.
> + */
> +void hinic_dev_close(struct rte_eth_dev *dev)
> +{

You may want to 'RTE_ETH_DEV_CLOSE_REMOVE' flag to cause 'rte_eth_dev_close()'
clean ethdev resources clean, please check other PMDs and ethdev API for sample
usage.