[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 2:31 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.
> Thanks for clarifying the same. It will helpful to add 'to do or future items' in
> cover letter, code comment and release documents which helps reviewers,
> early adopters and later maintainers.
I'd like to suggest focusing on what we have. Sorry, for many reasons it's not appropriate to talk too much about we'll do in the future. Like Internally we have a plan, but it keeps changing. Like something is still under investigation...

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