Re: [Intel-wired-lan] [PATCH net-next 04/13] ice: add helper function for checking VSI VF requirement

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

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.

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan