Re: [Intel-wired-lan] [PATCH net-next 03/13] ice: move vsi_type assignment from ice_vsi_alloc to ice_vsi_cfg

From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: Anthony Nguyen <anthony.l.nguyen@intel.com>,
	Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
Subject: Re: [Intel-wired-lan] [PATCH net-next 03/13] ice: move vsi_type assignment from ice_vsi_alloc to ice_vsi_cfg
Date: Wed, 18 Jan 2023 06:37:22 +0100	[thread overview]
Message-ID: <Y8eFkqSXDbTOAaL4@localhost.localdomain> (raw)
In-Reply-To: <a16975c6-55d7-1945-063e-6e0ccf5a10af@intel.com>

On Tue, Jan 17, 2023 at 11:20:43AM -0800, Jacob Keller wrote:
[...]
> >>  /**
> >> - * ice_vsi_cfg - configure VSI and tc on it
> >> + * ice_vsi_cfg - configure a previously allocated VSI
> >>   * @vsi: pointer to VSI
> >> + * @vsi_type: the type of VSI to configure as
> >> + * @pi: the port info for this VSI
> >>   * @vf: pointer to VF to which this VSI connects. This field is used primarily
> >>   *      for the ICE_VSI_VF type. Other VSI types should pass NULL.
> >>   * @ch: ptr to channel
> >>   * @init_vsi: is this an initialization or a reconfigure of the VSI
> >> + *
> >> + * Configure a VSI allocated with ice_vsi_alloc.
> >>   */
> >> -int ice_vsi_cfg(struct ice_vsi *vsi, struct ice_vf *vf, struct ice_channel *ch,
> >> -		int init_vsi)
> >> +int ice_vsi_cfg(struct ice_vsi *vsi, enum ice_vsi_type vsi_type,
> >> +		struct ice_port_info *pi, struct ice_vf *vf,
> >> +		struct ice_channel *ch, int init_vsi)
> >>  {
> >> +	struct ice_pf *pf = vsi->back;
> >>  	int ret;
> >>  
> >> +	if (WARN_ON(vsi_type == ICE_VSI_VF && !vf))
> >> +		return -EINVAL;
> >> +
> >> +	vsi->type = vsi_type;
> >> +	vsi->port_info = pi;
> >> +
> >> +	/* For VSIs which don't have a connected VF, this will be NULL */
> >> +	vsi->vf = vf;
> >> +
> >>  	ret = ice_vsi_cfg_def(vsi, ch, init_vsi);
> >>  	if (ret)
> >>  		return ret;
> >> @@ -2879,6 +2871,15 @@ int ice_vsi_cfg(struct ice_vsi *vsi, struct ice_vf *vf, struct ice_channel *ch,
> >>  	if (ret)
> >>  		ice_vsi_decfg(vsi);
> >>  
> >> +	if (vsi->type == ICE_VSI_CTRL) {
> >> +		if (vf) {
> > I know that it is only copy paste, but shouldn't we also check if
> > vsi_idx isn't already set like in PF case?
> > 
> Not sure what you mean? make sure idx is valid? but a memset of vsi will
> initialize that to 0 which will be "valid"?
> 
> or do you want to like confirm that the vsi->idx points to the vsi in
> the array with the right pointer?
> 

Maybe I am missing something. I assume that ther can be only one VF
control VSI on VF. If we set vf->ctrl_vsi_idx to the new value we will lost
previous one without removing the vsi index from hw. In case of PF
control VSI we have WARN_ON for this case:
WARN_ON(pf->ctr_idx != ICE_NO_VSI)

Shouldn't we have
WARN_ON(vf->ctrl_idx != ICE_NO_VSI)
to be sure that there is no more VF control VSIs on this VF?

When VF control VSI is removed the vf->ctr_vsi_idx is set to ICE_NO_VSI,
so it should work as expected.

Probably this case never happen (calling ice_vsi_setup() for VF control
VSI second time on the same VF), but if we have the security check for
PF it will be nice to have the same check for VF.

> >> +			vf->ctrl_vsi_idx = vsi->idx;
> >> +		} else {
> >> +			WARN_ON(pf->ctrl_vsi_idx != ICE_NO_VSI);
> >> +			pf->ctrl_vsi_idx = vsi->idx;
> >> +		}
> >> +	}
> >> +
> >>  	return ret;
> >>  }
> >>  
> >> @@ -2962,13 +2963,13 @@ ice_vsi_setup(struct ice_pf *pf, struct ice_port_info *pi,
> >>  	struct ice_vsi *vsi;
> >>  	int ret;
> >>  
> >> -	vsi = ice_vsi_alloc(pf, pi, vsi_type, ch, vf);
> >> +	vsi = ice_vsi_alloc(pf);
> > Looks nicer :)
> > 
> 
> :) we just moved the mess a bit.
> 
> >>  	if (!vsi) {
> >>  		dev_err(dev, "could not allocate VSI\n");
> >>  		return NULL;
> >>  	}
> >>  
> >> -	ret = ice_vsi_cfg(vsi, vf, ch, ICE_VSI_FLAG_INIT);
> >> +	ret = ice_vsi_cfg(vsi, vsi_type, pi, vf, ch, ICE_VSI_FLAG_INIT);
> > Maybe it is good patchset to implement Your idea about having vsi init
> > params structure?
> > 
> 
> 
> Probably.. I was sending what I had in our internal subfunction +
> scalable tree, so I don't have that developed yet.
> 

Sure

> >>  	if (ret)
> >>  		goto err_vsi_cfg;
> >>  
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h b/drivers/net/ethernet/intel/ice/ice_lib.h
> >> index b76f05e1f8a3..fb785d8cde9a 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice_lib.h
> >> +++ b/drivers/net/ethernet/intel/ice/ice_lib.h
> >> @@ -73,7 +73,8 @@ ice_get_res(struct ice_pf *pf, struct ice_res_tracker *res, u16 needed, u16 id);
> >>  #define ICE_VSI_FLAG_INIT	BIT(0)
> >>  #define ICE_VSI_FLAG_NO_INIT	0
> >>  int ice_vsi_rebuild(struct ice_vsi *vsi, int init_vsi);
> >> -int ice_vsi_cfg(struct ice_vsi *vsi, struct ice_vf *vf,
> >> +int ice_vsi_cfg(struct ice_vsi *vsi, enum ice_vsi_type vsi_type,
> >> +		struct ice_port_info *pi, struct ice_vf *vf,
> >>  		struct ice_channel *ch, int init_vsi);
> >>  
> >>  bool ice_is_reset_in_progress(unsigned long *state);
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> >> index 8fd9c87f30e2..29cd77dd3812 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> >> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> >> @@ -5010,7 +5010,8 @@ int ice_load(struct ice_pf *pf)
> >>  		return err;
> >>  
> >>  	vsi = ice_get_main_vsi(pf);
> >> -	err = ice_vsi_cfg(vsi, NULL, NULL, ICE_VSI_FLAG_INIT);
> >> +	err = ice_vsi_cfg(vsi, ICE_VSI_PF, pf->hw.port_info, NULL, NULL,
> >> +			  ICE_VSI_FLAG_INIT);
> >>  	if (err)
> >>  		goto err_vsi_cfg;
> >>  
> >> -- 
> >> 2.38.1.420.g319605f8f00e
> >>
> >> _______________________________________________
> >> Intel-wired-lan mailing list
> >> Intel-wired-lan@osuosl.org
> >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan