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

[Lu, Wenzhuo]

Hi Vipin,

> -----Original Message-----
> From: Varghese, Vipin
> Sent: Thursday, December 6, 2018 1:33 PM
> To: Lu, Wenzhuo <wenzhuo.lu at intel.com>; dev at dpdk.org
> Cc: 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
> > > > +	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.
> Suggestion in current patch if comment is kept it will be easier to understand
> that it is taken care in future patch.
> 
> Example patch 2/20 has comment stating adding support in patch 5/20.
> Then in patch 5/20 it removes the ToDo it is easier to read and understand
> the flow
I mean I made a mistake that put the check code in a later patch. Actually this code should be put in this patch. I plan to correct it.
But currently I think we're running out of time. I prefer not supporting multi process in this release.

> 
> >
> > >
> > > > +
> > > > +	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.
> Since primary is application which reserves the huge page memory (malloc,
> zmalloc, memzone). So when secondary is killed or stop whole huge pages
> are released. I am bit confused what is check suggested affecting?
> 
>  And also
> > to me it looks not a good solution to change every PMD for this feature.
> I am not aware about why other PMD are done in specific way. In my
> humble opinion, if there is a right way let it be used rather than doing other
> way.
> 
> I don't
> > see many PMD support it. Maybe we'd better not support it now and wait
> > for a better whole picture.
> I wait for others to comment to this approach.
> 
> snipped