[net-next,04/13] ice: add helper function for checking VSI VF requirement

A few places in ice_lib.c WARN if the VSI type is VF and the VF pointer is
NULL. This helps protect against accidentally creating a ICE_VSI_VF without
providing a VF pointer.

A future change is going to introduce another type of VSI which has the
same requirement, ICE_VSI_ADI. Instead of expanding each WARN_ON check to
include both types, introduce a helper function to do this check. The
ice_vsi_requires_vf function returns true if the VSI *must* have a VF, and
returns false otherwise.

Of specific note is that some VSI types may optionally have a VF but do not
require them, such as the ICE_VSI_CTRL type. These return false.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lib.c | 26 +++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

On Fri, Jan 13, 2023 at 02:37:26PM -0800, Jacob Keller wrote:
> A few places in ice_lib.c WARN if the VSI type is VF and the VF pointer is
> NULL. This helps protect against accidentally creating a ICE_VSI_VF without
> providing a VF pointer.
> 
> A future change is going to introduce another type of VSI which has the
> same requirement, ICE_VSI_ADI. Instead of expanding each WARN_ON check to
> include both types, introduce a helper function to do this check. The
> ice_vsi_requires_vf function returns true if the VSI *must* have a VF, and
> returns false otherwise.
> 
> Of specific note is that some VSI types may optionally have a VF but do not
> require them, such as the ICE_VSI_CTRL type. These return false.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_lib.c | 26 +++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index f89279ede9a1..79555e22a9be 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -34,6 +34,26 @@ const char *ice_vsi_type_str(enum ice_vsi_type vsi_type)
>  	}
>  }
>  
> +/**
> + * ice_vsi_requires_vf - Does this VSI type always require a VF?
> + * @vsi_type: the VSI type
> + *
> + * Returns true if the VSI type *must* have a VF pointer. Returns false
> + * otherwise. In particular, VSI types which may *optionally* have a VF
> + * pointer return false.
> + *
> + * Used to WARN in cases where we always expect a VF pointer to be non-NULL.
> + */
> +static int ice_vsi_requires_vf(enum ice_vsi_type vsi_type)
> +{
> +	switch (vsi_type) {
> +	case ICE_VSI_VF:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
Looks a little strange right now. Maybe send this whole patch with SIOV
series?

> +
>  /**
>   * ice_vsi_ctrl_all_rx_rings - Start or stop a VSI's Rx rings
>   * @vsi: the VSI being configured
> @@ -175,7 +195,7 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi)
>  	struct ice_pf *pf = vsi->back;
>  	struct ice_vf *vf = vsi->vf;
>  
> -	if (WARN_ON(vsi_type == ICE_VSI_VF && !vf))
> +	if (WARN_ON(ice_vsi_requires_vf(vsi_type) && !vf))
>  		return;
>  
>  	switch (vsi_type) {
> @@ -2854,7 +2874,7 @@ int ice_vsi_cfg(struct ice_vsi *vsi, enum ice_vsi_type vsi_type,
>  	struct ice_pf *pf = vsi->back;
>  	int ret;
>  
> -	if (WARN_ON(vsi_type == ICE_VSI_VF && !vf))
> +	if (WARN_ON(ice_vsi_requires_vf(vsi_type) && !vf))
>  		return -EINVAL;
>  
>  	vsi->type = vsi_type;
> @@ -3487,7 +3507,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, int init_vsi)
>  		return -EINVAL;
>  
>  	pf = vsi->back;
> -	if (WARN_ON(vsi->type == ICE_VSI_VF && !vsi->vf))
> +	if (WARN_ON(ice_vsi_requires_vf(vsi->type) && !vsi->vf))
>  		return -EINVAL;
>  
>  	coalesce = kcalloc(vsi->num_q_vectors,
> -- 
> 2.38.1.420.g319605f8f00e
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

On 1/16/2023 2:56 AM, Michal Swiatkowski wrote:
> On Fri, Jan 13, 2023 at 02:37:26PM -0800, Jacob Keller wrote:
>> A few places in ice_lib.c WARN if the VSI type is VF and the VF pointer is
>> NULL. This helps protect against accidentally creating a ICE_VSI_VF without
>> providing a VF pointer.
>>
>> A future change is going to introduce another type of VSI which has the
>> same requirement, ICE_VSI_ADI. Instead of expanding each WARN_ON check to
>> include both types, introduce a helper function to do this check. The
>> ice_vsi_requires_vf function returns true if the VSI *must* have a VF, and
>> returns false otherwise.
>>
>> Of specific note is that some VSI types may optionally have a VF but do not
>> require them, such as the ICE_VSI_CTRL type. These return false.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>>  drivers/net/ethernet/intel/ice/ice_lib.c | 26 +++++++++++++++++++++---
>>  1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
>> index f89279ede9a1..79555e22a9be 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>> @@ -34,6 +34,26 @@ const char *ice_vsi_type_str(enum ice_vsi_type vsi_type)
>>  	}
>>  }
>>  
>> +/**
>> + * ice_vsi_requires_vf - Does this VSI type always require a VF?
>> + * @vsi_type: the VSI type
>> + *
>> + * Returns true if the VSI type *must* have a VF pointer. Returns false
>> + * otherwise. In particular, VSI types which may *optionally* have a VF
>> + * pointer return false.
>> + *
>> + * Used to WARN in cases where we always expect a VF pointer to be non-NULL.
>> + */
>> +static int ice_vsi_requires_vf(enum ice_vsi_type vsi_type)
>> +{
>> +	switch (vsi_type) {
>> +	case ICE_VSI_VF:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
> Looks a little strange right now. Maybe send this whole patch with SIOV
> series?
> 

I can do that, but the main challenge is the number of patches. We're
going to have to send the ice driver implementation as well as the
inet_vdcm driver implementation in the VFIO subfolder. This could easily
hit 15 patches, so I've been trying to find as many places where we can
split patches to other series as possible.

It is definitely odd now, and I could refactor this part to not be a
switch/case and make it switch/case later with the changes that
introduce scalable? But I don't really want the changes that switch to
using ice_vsi_requires_vf to be part of that patch...

On Tue, Jan 17, 2023 at 11:23:00AM -0800, Jacob Keller wrote:
> 
> 
> On 1/16/2023 2:56 AM, Michal Swiatkowski wrote:
> > On Fri, Jan 13, 2023 at 02:37:26PM -0800, Jacob Keller wrote:
> >> A few places in ice_lib.c WARN if the VSI type is VF and the VF pointer is
> >> NULL. This helps protect against accidentally creating a ICE_VSI_VF without
> >> providing a VF pointer.
> >>
> >> A future change is going to introduce another type of VSI which has the
> >> same requirement, ICE_VSI_ADI. Instead of expanding each WARN_ON check to
> >> include both types, introduce a helper function to do this check. The
> >> ice_vsi_requires_vf function returns true if the VSI *must* have a VF, and
> >> returns false otherwise.
> >>
> >> Of specific note is that some VSI types may optionally have a VF but do not
> >> require them, such as the ICE_VSI_CTRL type. These return false.
> >>
> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> >> ---
> >>  drivers/net/ethernet/intel/ice/ice_lib.c | 26 +++++++++++++++++++++---
> >>  1 file changed, 23 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> >> index f89279ede9a1..79555e22a9be 100644
> >> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> >> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> >> @@ -34,6 +34,26 @@ const char *ice_vsi_type_str(enum ice_vsi_type vsi_type)
> >>  	}
> >>  }
> >>  
> >> +/**
> >> + * ice_vsi_requires_vf - Does this VSI type always require a VF?
> >> + * @vsi_type: the VSI type
> >> + *
> >> + * Returns true if the VSI type *must* have a VF pointer. Returns false
> >> + * otherwise. In particular, VSI types which may *optionally* have a VF
> >> + * pointer return false.
> >> + *
> >> + * Used to WARN in cases where we always expect a VF pointer to be non-NULL.
> >> + */
> >> +static int ice_vsi_requires_vf(enum ice_vsi_type vsi_type)
> >> +{
> >> +	switch (vsi_type) {
> >> +	case ICE_VSI_VF:
> >> +		return true;
> >> +	default:
> >> +		return false;
> >> +	}
> >> +}
> > Looks a little strange right now. Maybe send this whole patch with SIOV
> > series?
> > 
> 
> I can do that, but the main challenge is the number of patches. We're
> going to have to send the ice driver implementation as well as the
> inet_vdcm driver implementation in the VFIO subfolder. This could easily
> hit 15 patches, so I've been trying to find as many places where we can
> split patches to other series as possible.
> 
> It is definitely odd now, and I could refactor this part to not be a
> switch/case and make it switch/case later with the changes that
> introduce scalable? But I don't really want the changes that switch to
> using ice_vsi_requires_vf to be part of that patch...

Understand, it is fine for me, changing it to switch/case later seems
like good solution. I think You can also send it as it is right now, it
isn't big thing. I pointed it out, because I didn't know the reasone.

On 1/17/2023 9:24 PM, Michal Swiatkowski wrote:
> On Tue, Jan 17, 2023 at 11:23:00AM -0800, Jacob Keller wrote:
>> On 1/16/2023 2:56 AM, Michal Swiatkowski wrote:
>> I can do that, but the main challenge is the number of patches. We're
>> going to have to send the ice driver implementation as well as the
>> inet_vdcm driver implementation in the VFIO subfolder. This could easily
>> hit 15 patches, so I've been trying to find as many places where we can
>> split patches to other series as possible.
>>
>> It is definitely odd now, and I could refactor this part to not be a
>> switch/case and make it switch/case later with the changes that
>> introduce scalable? But I don't really want the changes that switch to
>> using ice_vsi_requires_vf to be part of that patch...
> 
> Understand, it is fine for me, changing it to switch/case later seems
> like good solution. I think You can also send it as it is right now, it
> isn't big thing. I pointed it out, because I didn't know the reasone.
> 

I dropped it from the v2 of this series and I'll figure out what to do
with it later, thanks!