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

[Jacob Keller <jacob.e.keller@intel.com>]

On 1/16/2023 2:47 AM, Michal Swiatkowski wrote:
> On Fri, Jan 13, 2023 at 02:37:25PM -0800, Jacob Keller wrote:
>> The ice_vsi_alloc and ice_vsi_cfg functions are used together to allocate
>> and configure a new VSI, called as part of the ice_vsi_setup function.
>>
>> In the future with the addition of the subfunction code the ice driver
>> will want to be able to allocate a VSI while delaying the configuration to
>> a later point of the port activation.
>>
>> Currently this requires that the port code know what type of VSI should
>> be allocated. This is required because ice_vsi_alloc assigns the VSI type.
>>
>> Refactor the ice_vsi_alloc and ice_vsi_cfg functions so that VSI type
>> assignment isn't done until the configuration stage. This will allow the
>> devlink port addition logic to reserve a VSI as early as possible before
>> the type of the port is known. In this way, the port add can fail in the
>> event that all hardware VSI resources are exhausted.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_lib.c  | 65 ++++++++++++-----------
>>  drivers/net/ethernet/intel/ice/ice_lib.h  |  3 +-
>>  drivers/net/ethernet/intel/ice/ice_main.c |  3 +-
>>  3 files changed, 37 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
>> index 0bf99f0e3faa..f89279ede9a1 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>> @@ -640,28 +640,18 @@ ice_vsi_alloc_def(struct ice_vsi *vsi, struct ice_channel *ch)
>>  /**
>>   * ice_vsi_alloc - Allocates the next available struct VSI in the PF
>>   * @pf: board private structure
>> - * @pi: pointer to the port_info instance
>> - * @vsi_type: type of VSI
>> - * @ch: ptr to channel
>> - * @vf: VF for ICE_VSI_VF and ICE_VSI_CTRL
>>   *
>> - * The VF pointer is used for ICE_VSI_VF and ICE_VSI_CTRL. For ICE_VSI_CTRL,
>> - * it may be NULL in the case there is no association with a VF. For
>> - * ICE_VSI_VF the VF pointer *must not* be NULL.
>> + * Reserves a VSI index from the PF and allocates an empty VSI structure
>> + * without a type. The VSI structure must later be initialized by calling
>> + * ice_vsi_cfg().
>>   *
>>   * returns a pointer to a VSI on success, NULL on failure.
>>   */
>> -static struct ice_vsi *
>> -ice_vsi_alloc(struct ice_pf *pf, struct ice_port_info *pi,
>> -	      enum ice_vsi_type vsi_type, struct ice_channel *ch,
>> -	      struct ice_vf *vf)
>> +static struct ice_vsi *ice_vsi_alloc(struct ice_pf *pf)
>>  {
>>  	struct device *dev = ice_pf_to_dev(pf);
>>  	struct ice_vsi *vsi = NULL;
>>  
>> -	if (WARN_ON(vsi_type == ICE_VSI_VF && !vf))
>> -		return NULL;
>> -
>>  	/* Need to protect the allocation of the VSIs at the PF level */
>>  	mutex_lock(&pf->sw_mutex);
>>  
>> @@ -678,11 +668,7 @@ ice_vsi_alloc(struct ice_pf *pf, struct ice_port_info *pi,
>>  	if (!vsi)
>>  		goto unlock_pf;
>>  
>> -	vsi->type = vsi_type;
>>  	vsi->back = pf;
>> -	vsi->port_info = pi;
>> -	/* For VSIs which don't have a connected VF, this will be NULL */
>> -	vsi->vf = vf;
>>  	set_bit(ICE_VSI_DOWN, vsi->state);
>>  
>>  	/* fill slot and make note of the index */
>> @@ -693,15 +679,6 @@ ice_vsi_alloc(struct ice_pf *pf, struct ice_port_info *pi,
>>  	pf->next_vsi = ice_get_free_slot(pf->vsi, pf->num_alloc_vsi,
>>  					 pf->next_vsi);
>>  
>> -	if (vsi->type == ICE_VSI_CTRL) {
>> -		if (vf) {
>> -			vf->ctrl_vsi_idx = vsi->idx;
>> -		} else {
>> -			WARN_ON(pf->ctrl_vsi_idx != ICE_NO_VSI);
>> -			pf->ctrl_vsi_idx = vsi->idx;
>> -		}
>> -	}
>> -
>>  unlock_pf:
>>  	mutex_unlock(&pf->sw_mutex);
>>  	return vsi;
>> @@ -2859,18 +2836,33 @@ ice_vsi_cfg_def(struct ice_vsi *vsi, struct ice_channel *ch, int init_vsi)
>>  }
>>  
>>  /**
>> - * 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?

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

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