>
> 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 <xuanziya...@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.
