Hi Vipin,
> -----Original Message-----
> From: Varghese, Vipin
> Sent: Tuesday, December 4, 2018 12:41 PM
> To: Lu, Wenzhuo <wenzhuo...@intel.com>; dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo...@intel.com>; Yang, Qiming
> <qiming.y...@intel.com>; Li, Xiaoyun <xiaoyun...@intel.com>; Wu, Jingjing
> <jingjing...@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 02/20] net/ice: support device
> initialization
>
>
> Snipped
>
> > + /* Set the info.ingress_table and info.egress_table
> > + * for UP translate table. Now just set it to 1:1 map by default
> > + * -- 0b 111 110 101 100 011 010 001 000 == 0xFAC688
> > + */
> > + info->ingress_table = rte_cpu_to_le_32(0x00FAC688);
> > + info->egress_table = rte_cpu_to_le_32(0x00FAC688);
> > + info->outer_up_table = rte_cpu_to_le_32(0x00FAC688);
>
> Can we use MACRO instead of exact values for ingress, egress and outer_up
> table.
Good suggestion. Will update it in the next version.
>
> > + return 0;
> > +}
> > +
>
> snipped
>
> > +static int
> > +ice_dev_init(struct rte_eth_dev *dev) {
> > + struct rte_pci_device *pci_dev;
> > + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> > >dev_private);
> > + struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data-
> >dev_private);
> > + int ret;
> > +
> > + dev->dev_ops = &ice_eth_dev_ops;
> > +
> > + pci_dev = RTE_DEV_TO_PCI(dev->device);
> > +
> > + rte_eth_copy_pci_info(dev, pci_dev);
> > + pf->adapter = ICE_DEV_PRIVATE_TO_ADAPTER(dev->data-
> > >dev_private);
> > + pf->adapter->eth_dev = dev;
> > + pf->dev_data = dev->data;
> > + hw->back = pf->adapter;
> > + hw->hw_addr = (uint8_t *)pci_dev->mem_resource[0].addr;
> > + hw->vendor_id = pci_dev->id.vendor_id;
> > + hw->device_id = pci_dev->id.device_id;
> > + hw->subsystem_vendor_id = pci_dev->id.subsystem_vendor_id;
> > + hw->subsystem_device_id = pci_dev->id.subsystem_device_id;
> > + hw->bus.device = pci_dev->addr.devid;
> > + hw->bus.func = pci_dev->addr.function;
> > +
> > + ice_init_controlq_parameter(hw);
> > +
> > + ret = ice_init_hw(hw);
> > + if (ret) {
> > + PMD_INIT_LOG(ERR, "Failed to initialize HW");
> > + return -EINVAL;
> > + }
>
> Definition for ice_init_hw in patch 01/20 does not check for primary-
> secondary. Are we allowing secondary to invoke ice_init_hw if it is
> initialized
> by primary?
It's a patch split issue. We add the check in later patch. Will put it in this
patch in the new version.
>
> > +
> > + PMD_INIT_LOG(INFO, "FW %d.%d.%05d API %d.%d",
> > + hw->fw_maj_ver, hw->fw_min_ver, hw->fw_build,
> > + hw->api_maj_ver, hw->api_min_ver);
> > +
>
> Snipped
>
> > +
> > +static int
> > +ice_dev_uninit(struct rte_eth_dev *dev) {
> > + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> > >dev_private);
> > + struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data-
> >dev_private);
> > +
> > + ICE_PROC_SECONDARY_CHECK_RET_0;
>
> Should not we check if primary is alive and NIC is used or initialized by
> primary then ' ICE_PROC_SECONDARY_CHECK_RET_0'?
I think it's not a critical issue if the process is terminate abnormally
without uninit.
Comparing with that, I have more concern about this scenario, if the primary
process exit and uninit the resource, the secondary process is left alone. And
also to me it looks not a good solution to change every PMD for this feature. I
don't see many PMD support it. Maybe we'd better not support it now and wait
for a better whole picture.
The same below.
>
> > +
> > + ice_dev_close(dev);
> > +
> > + dev->dev_ops = NULL;
> > + dev->rx_pkt_burst = NULL;
> > + dev->tx_pkt_burst = NULL;
> > +
> > + rte_free(dev->data->mac_addrs);
> > + dev->data->mac_addrs = NULL;
> > +
> > + ice_release_vsi(pf->main_vsi);
> > + ice_sched_cleanup_all(hw);
> > + rte_free(hw->port_info);
> > + ice_shutdown_all_ctrlq(hw);
> > +
> > + return 0;
> > +}
> > +
>
> snipped
>
> > +static void
> > +ice_dev_close(struct rte_eth_dev *dev) {
> > + struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data-
> >dev_private);
> > + struct ice_hw *hw = ICE_DEV_PRIVATE_TO_HW(dev->data-
> > >dev_private);
> > +
> > + ICE_PROC_SECONDARY_CHECK_NO_ERR;
> > +
>
> I am just wondering in a multi process (primary-secondary) if primary is
> killed or exited, then if we try to stop the secondary due to this check the
> vsi,
> pool and shutdown is not called. Should not we check if primary is still
> alive,
> if yes then ICE_PROC_SECONDARY_CHECK_NO_ERR?
>
> > + ice_res_pool_destroy(&pf->msix_pool);
> > + ice_release_vsi(pf->main_vsi);
> > +
> > + ice_shutdown_all_ctrlq(hw);
> > +}
>
> snipped
