[dpdk-dev] [PATCH v2 02/20] net/ice: support device initialization

Lu, Wenzhuo wenzhuo.lu at intel.com
Thu Dec 6 06:01:14 CET 2018


Hi Vipin,

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Tuesday, December 4, 2018 12:41 PM
> To: Lu, Wenzhuo <wenzhuo.lu at intel.com>; dev at dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>; Yang, Qiming
> <qiming.yang at intel.com>; Li, Xiaoyun <xiaoyun.li at intel.com>; Wu, Jingjing
> <jingjing.wu at 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


More information about the dev mailing list