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

["Xuanziyang (William, Chip Application Design Logic and Hardware Development Dept IT_Products & Solutions)" <xuanziyang2@huawei.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

OK, thanks.

> 
> >
> > 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?

Yes, we use rte_eth_dev_pci_generic_probe, it can do this. And I will delete it here.

> 
> > +
> > +	/* 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?

I will check it, and delete it if not necessary.

> 
> <...>
> 
> > +/**
> > + * 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.

Yes, I find it. I will fix it to set 'RTE_ETH_DEV_CLOSE_REMOVE' flag when initialize.

Thank you for your precious comments very much, I will fix these problems carefully.