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

Varghese, Vipin vipin.varghese at intel.com
Thu Dec 6 07:31:13 CET 2018


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.

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


More information about the dev mailing list