[Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR

[Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR

[Junfeng Guo]

Add profile conflict check while adding some FDIR rules to aviod
unexpected flow behavior, rules may have conflict including:
        IPv4 <---> {IPv4_UDP, IPv4_TCP, IPv4_SCTP}
        IPv6 <---> {IPv6_UDP, IPv6_TCP, IPv6_SCTP}

For example, when we create an FDIR rule for IPv4, this rule will work
on packets including IPv4, IPv4_UDP, IPv4_TCP and IPv4_SCTP. But if we
then create an FDIR rule for IPv4_UDP and then destroy it, the first
FDIR rule for IPv4 cannot work on pkt IPv4_UDP then.

To prevent this unexpected behavior, we add restriction in software
when creating FDIR rules by adding necessary profile conflict check.

Fixes: 1f7ea1cd6a37 ("ice: Enable FDIR Configure for AVF")
Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
---
 .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
index e6ef6b303222..1431789c194e 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
@@ -541,6 +541,71 @@ static void ice_vc_fdir_rem_prof_all(struct ice_vf *vf)
 	}
 }
 
+/**
+ * ice_vc_fdir_has_prof_conflict
+ * @vf: pointer to the VF structure
+ * @conf: FDIR configuration for each filter
+ *
+ * Check if @conf has conflicting profile with existing profiles
+ *
+ * Return: true on success, and false on error.
+ */
+static bool
+ice_vc_fdir_has_prof_conflict(struct ice_vf *vf,
+			      struct virtchnl_fdir_fltr_conf *conf)
+{
+	struct ice_fdir_fltr *desc;
+
+	list_for_each_entry(desc, &vf->fdir.fdir_rule_list, fltr_node) {
+		struct virtchnl_fdir_fltr_conf *existing_conf =
+						to_fltr_conf_from_desc(desc);
+		struct ice_fdir_fltr *a = &existing_conf->input;
+		struct ice_fdir_fltr *b = &conf->input;
+
+		enum ice_fltr_ptype flow_type_a = a->flow_type;
+		enum ice_fltr_ptype flow_type_b = b->flow_type;
+
+		/* No need to compare two rules with different tunnel type */
+		if (existing_conf->ttype != conf->ttype)
+			continue;
+
+		/* No need to compare two rules with same protocol */
+		if (flow_type_a == flow_type_b)
+			continue;
+
+		switch (flow_type_a) {
+		case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
+		case ICE_FLTR_PTYPE_NONF_IPV4_TCP:
+		case ICE_FLTR_PTYPE_NONF_IPV4_SCTP:
+			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_OTHER)
+				return true;
+			break;
+		case ICE_FLTR_PTYPE_NONF_IPV4_OTHER:
+			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
+			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_TCP ||
+			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_SCTP)
+				return true;
+			break;
+		case ICE_FLTR_PTYPE_NONF_IPV6_UDP:
+		case ICE_FLTR_PTYPE_NONF_IPV6_TCP:
+		case ICE_FLTR_PTYPE_NONF_IPV6_SCTP:
+			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_OTHER)
+				return true;
+			break;
+		case ICE_FLTR_PTYPE_NONF_IPV6_OTHER:
+			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_UDP ||
+			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_TCP ||
+			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_SCTP)
+				return true;
+			break;
+		default:
+			break;
+		}
+	}
+
+	return false;
+}
+
 /**
  * ice_vc_fdir_write_flow_prof
  * @vf: pointer to the VF structure
@@ -677,6 +742,12 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf, struct virtchnl_fdir_add *fltr,
 	enum ice_fltr_ptype flow;
 	int ret;
 
+	ret = ice_vc_fdir_has_prof_conflict(vf, conf);
+	if (ret) {
+		dev_dbg(dev, "Found flow prof conflict for VF %d\n", vf->vf_id);
+		return ret;
+	}
+
 	flow = input->flow_type;
 	ret = ice_vc_fdir_alloc_prof(vf, flow);
 	if (ret) {
-- 
2.25.1

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

Re: [Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR

[Michal Swiatkowski]

On Thu, Mar 09, 2023 at 01:10:11PM +0800, Junfeng Guo wrote:
> Add profile conflict check while adding some FDIR rules to aviod
> unexpected flow behavior, rules may have conflict including:
>         IPv4 <---> {IPv4_UDP, IPv4_TCP, IPv4_SCTP}
>         IPv6 <---> {IPv6_UDP, IPv6_TCP, IPv6_SCTP}
> 
> For example, when we create an FDIR rule for IPv4, this rule will work
> on packets including IPv4, IPv4_UDP, IPv4_TCP and IPv4_SCTP. But if we
> then create an FDIR rule for IPv4_UDP and then destroy it, the first
> FDIR rule for IPv4 cannot work on pkt IPv4_UDP then.
> 
> To prevent this unexpected behavior, we add restriction in software
> when creating FDIR rules by adding necessary profile conflict check.

What about flow conflict when rule is added from host perspective (by
ethtool)? Do we also need to check for conflict? Maybe it is worth
create common code for this case.
> 
> Fixes: 1f7ea1cd6a37 ("ice: Enable FDIR Configure for AVF")
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> ---
>  .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> index e6ef6b303222..1431789c194e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> @@ -541,6 +541,71 @@ static void ice_vc_fdir_rem_prof_all(struct ice_vf *vf)
>  	}
>  }
>  
> +/**
> + * ice_vc_fdir_has_prof_conflict
> + * @vf: pointer to the VF structure
> + * @conf: FDIR configuration for each filter
> + *
> + * Check if @conf has conflicting profile with existing profiles
> + *
> + * Return: true on success, and false on error.
> + */
> +static bool
> +ice_vc_fdir_has_prof_conflict(struct ice_vf *vf,
> +			      struct virtchnl_fdir_fltr_conf *conf)
It isn't aligned.

> +{
> +	struct ice_fdir_fltr *desc;
> +
> +	list_for_each_entry(desc, &vf->fdir.fdir_rule_list, fltr_node) {
> +		struct virtchnl_fdir_fltr_conf *existing_conf =
> +						to_fltr_conf_from_desc(desc);
> +		struct ice_fdir_fltr *a = &existing_conf->input;
> +		struct ice_fdir_fltr *b = &conf->input;
> +
> +		enum ice_fltr_ptype flow_type_a = a->flow_type;
> +		enum ice_fltr_ptype flow_type_b = b->flow_type;
I think You should folow RCT variable declaration here, and remove empty
line.

> +
> +		/* No need to compare two rules with different tunnel type */
> +		if (existing_conf->ttype != conf->ttype)
> +			continue;
> +
> +		/* No need to compare two rules with same protocol */
> +		if (flow_type_a == flow_type_b)
> +			continue;
This two ifs can be combined into one.

> +
> +		switch (flow_type_a) {
> +		case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
> +		case ICE_FLTR_PTYPE_NONF_IPV4_TCP:
> +		case ICE_FLTR_PTYPE_NONF_IPV4_SCTP:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_OTHER)
> +				return true;
> +			break;
> +		case ICE_FLTR_PTYPE_NONF_IPV4_OTHER:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_TCP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_SCTP)
> +				return true;
> +			break;
> +		case ICE_FLTR_PTYPE_NONF_IPV6_UDP:
> +		case ICE_FLTR_PTYPE_NONF_IPV6_TCP:
> +		case ICE_FLTR_PTYPE_NONF_IPV6_SCTP:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_OTHER)
> +				return true;
> +			break;
> +		case ICE_FLTR_PTYPE_NONF_IPV6_OTHER:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_UDP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_TCP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_SCTP)
> +				return true;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  /**
>   * ice_vc_fdir_write_flow_prof
>   * @vf: pointer to the VF structure
> @@ -677,6 +742,12 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf, struct virtchnl_fdir_add *fltr,
>  	enum ice_fltr_ptype flow;
>  	int ret;
>  
> +	ret = ice_vc_fdir_has_prof_conflict(vf, conf);
> +	if (ret) {
> +		dev_dbg(dev, "Found flow prof conflict for VF %d\n", vf->vf_id);
> +		return ret;
> +	}
> +
>  	flow = input->flow_type;
>  	ret = ice_vc_fdir_alloc_prof(vf, flow);
>  	if (ret) {
> -- 
> 2.25.1
> 
> _______________________________________________
> 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

Re: [Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR

[Guo, Junfeng]

Thanks for the review! Comments inline.

> -----Original Message-----
> From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Sent: Thursday, March 9, 2023 22:02
> To: Guo, Junfeng <junfeng.guo@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net] ice: add profile conflict check
> for AVF FDIR
> 
> On Thu, Mar 09, 2023 at 01:10:11PM +0800, Junfeng Guo wrote:
> > Add profile conflict check while adding some FDIR rules to aviod
> > unexpected flow behavior, rules may have conflict including:
> >         IPv4 <---> {IPv4_UDP, IPv4_TCP, IPv4_SCTP}
> >         IPv6 <---> {IPv6_UDP, IPv6_TCP, IPv6_SCTP}
> >
> > For example, when we create an FDIR rule for IPv4, this rule will work
> > on packets including IPv4, IPv4_UDP, IPv4_TCP and IPv4_SCTP. But if we
> > then create an FDIR rule for IPv4_UDP and then destroy it, the first
> > FDIR rule for IPv4 cannot work on pkt IPv4_UDP then.
> >
> > To prevent this unexpected behavior, we add restriction in software
> > when creating FDIR rules by adding necessary profile conflict check.
> 
> What about flow conflict when rule is added from host perspective (by
> ethtool)? Do we also need to check for conflict? Maybe it is worth
> create common code for this case.
> >
> > Fixes: 1f7ea1cd6a37 ("ice: Enable FDIR Configure for AVF")
> > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> > ---
> >  .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 71 +++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > index e6ef6b303222..1431789c194e 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > @@ -541,6 +541,71 @@ static void ice_vc_fdir_rem_prof_all(struct
> ice_vf *vf)
> >  	}
> >  }
> >
> > +/**
> > + * ice_vc_fdir_has_prof_conflict
> > + * @vf: pointer to the VF structure
> > + * @conf: FDIR configuration for each filter
> > + *
> > + * Check if @conf has conflicting profile with existing profiles
> > + *
> > + * Return: true on success, and false on error.
> > + */
> > +static bool
> > +ice_vc_fdir_has_prof_conflict(struct ice_vf *vf,
> > +			      struct virtchnl_fdir_fltr_conf *conf)
> It isn't aligned.

I think here is just the display issue, the "+" at the beginning will occupy
a character place. Once applied, it will show correctly.

> 
> > +{
> > +	struct ice_fdir_fltr *desc;
> > +
> > +	list_for_each_entry(desc, &vf->fdir.fdir_rule_list, fltr_node) {
> > +		struct virtchnl_fdir_fltr_conf *existing_conf =
> > +
> 	to_fltr_conf_from_desc(desc);
> > +		struct ice_fdir_fltr *a = &existing_conf->input;
> > +		struct ice_fdir_fltr *b = &conf->input;
> > +
> > +		enum ice_fltr_ptype flow_type_a = a->flow_type;
> > +		enum ice_fltr_ptype flow_type_b = b->flow_type;
> I think You should folow RCT variable declaration here, and remove
> empty
> line.

Thanks for the advice!

Do you mean update the code order like this?
	{
	enum ice_fltr_ptype flow_type_a, flow_type_b;
	struct ice_fdir_fltr *a = &existing_conf->input;
	struct ice_fdir_fltr *b = &conf->input;

	flow_type_a = a->flow_type;
	flow_type_b = b->flow_type;
	}
Or like this?
	{
	enum ice_fltr_ptype flow_type_a, flow_type_b;
	struct ice_fdir_fltr *a, *b;

	a = &existing_conf->input;
	b = &conf->input;
	flow_type_a = a->flow_type;
	flow_type_b = b->flow_type;
	}

> 
> > +
> > +		/* No need to compare two rules with different tunnel
> type */
> > +		if (existing_conf->ttype != conf->ttype)
> > +			continue;
> > +
> > +		/* No need to compare two rules with same protocol */
> > +		if (flow_type_a == flow_type_b)
> > +			continue;
> This two ifs can be combined into one.

Sure, it could be updated in the coming version. Thanks!

> 
> > +
> > +		switch (flow_type_a) {
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_TCP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_SCTP:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_OTHER)
> > +				return true;
> > +			break;
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_OTHER:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_TCP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_SCTP)
> > +				return true;
> > +			break;
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_UDP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_TCP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_SCTP:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_OTHER)
> > +				return true;
> > +			break;
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_OTHER:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_UDP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_TCP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_SCTP)
> > +				return true;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  /**
> >   * ice_vc_fdir_write_flow_prof
> >   * @vf: pointer to the VF structure
> > @@ -677,6 +742,12 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf,
> struct virtchnl_fdir_add *fltr,
> >  	enum ice_fltr_ptype flow;
> >  	int ret;
> >
> > +	ret = ice_vc_fdir_has_prof_conflict(vf, conf);
> > +	if (ret) {
> > +		dev_dbg(dev, "Found flow prof conflict for VF %d\n", vf-
> >vf_id);
> > +		return ret;
> > +	}
> > +
> >  	flow = input->flow_type;
> >  	ret = ice_vc_fdir_alloc_prof(vf, flow);
> >  	if (ret) {
> > --
> > 2.25.1
> >
> > _______________________________________________
> > 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

Re: [Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR

[Michal Swiatkowski]

On Fri, Mar 10, 2023 at 05:16:22AM +0000, Guo, Junfeng wrote:
> Thanks for the review! Comments inline.
> 
> > -----Original Message-----
> > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > Sent: Thursday, March 9, 2023 22:02
> > To: Guo, Junfeng <junfeng.guo@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org
> > Subject: Re: [Intel-wired-lan] [PATCH net] ice: add profile conflict check
> > for AVF FDIR
> > 
> > On Thu, Mar 09, 2023 at 01:10:11PM +0800, Junfeng Guo wrote:
> > > Add profile conflict check while adding some FDIR rules to aviod
> > > unexpected flow behavior, rules may have conflict including:
> > >         IPv4 <---> {IPv4_UDP, IPv4_TCP, IPv4_SCTP}
> > >         IPv6 <---> {IPv6_UDP, IPv6_TCP, IPv6_SCTP}
> > >
> > > For example, when we create an FDIR rule for IPv4, this rule will work
> > > on packets including IPv4, IPv4_UDP, IPv4_TCP and IPv4_SCTP. But if we
> > > then create an FDIR rule for IPv4_UDP and then destroy it, the first
> > > FDIR rule for IPv4 cannot work on pkt IPv4_UDP then.
> > >
> > > To prevent this unexpected behavior, we add restriction in software
> > > when creating FDIR rules by adding necessary profile conflict check.
> > 
> > What about flow conflict when rule is added from host perspective (by
> > ethtool)? Do we also need to check for conflict? Maybe it is worth
> > create common code for this case.
> > >
> > > Fixes: 1f7ea1cd6a37 ("ice: Enable FDIR Configure for AVF")
> > > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> > > ---
> > >  .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 71 +++++++++++++++++++
> > >  1 file changed, 71 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > > index e6ef6b303222..1431789c194e 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > > @@ -541,6 +541,71 @@ static void ice_vc_fdir_rem_prof_all(struct
> > ice_vf *vf)
> > >  	}
> > >  }
> > >
> > > +/**
> > > + * ice_vc_fdir_has_prof_conflict
> > > + * @vf: pointer to the VF structure
> > > + * @conf: FDIR configuration for each filter
> > > + *
> > > + * Check if @conf has conflicting profile with existing profiles
> > > + *
> > > + * Return: true on success, and false on error.
> > > + */
> > > +static bool
> > > +ice_vc_fdir_has_prof_conflict(struct ice_vf *vf,
> > > +			      struct virtchnl_fdir_fltr_conf *conf)
> > It isn't aligned.
> 
> I think here is just the display issue, the "+" at the beginning will occupy
> a character place. Once applied, it will show correctly.
> 

Oh, sure, sorry for pointing it.

> > 
> > > +{
> > > +	struct ice_fdir_fltr *desc;
> > > +
> > > +	list_for_each_entry(desc, &vf->fdir.fdir_rule_list, fltr_node) {
> > > +		struct virtchnl_fdir_fltr_conf *existing_conf =
> > > +
> > 	to_fltr_conf_from_desc(desc);
> > > +		struct ice_fdir_fltr *a = &existing_conf->input;
> > > +		struct ice_fdir_fltr *b = &conf->input;
> > > +
> > > +		enum ice_fltr_ptype flow_type_a = a->flow_type;
> > > +		enum ice_fltr_ptype flow_type_b = b->flow_type;
> > I think You should folow RCT variable declaration here, and remove
> > empty
> > line.
> 
> Thanks for the advice!
> 
> Do you mean update the code order like this?
> 	{
> 
To follow RCT:
struct ice_fdir_fltr *a = &existing_conf->input;
enum ice_fltr_ptype flow_type_a, flow_type_b;
struct ice_fdir_fltr *b = &conf->input;

> 	flow_type_a = a->flow_type;
> 	flow_type_b = b->flow_type;
> 	}
> Or like this?
> 	{
> 	enum ice_fltr_ptype flow_type_a, flow_type_b;
> 	struct ice_fdir_fltr *a, *b;
This is also fine

Also fine will be:
struct ice_fdir_fltr *a = &existing_conf->input;
enum ice_fltr_ptype flow_type_a = a->flow_type;
enum ice_fltr_ptype flow_type_b = b->flow_type;
struct ice_fdir_fltr *b = &conf->input;

And it's look the best in my opinion, but it is only cosmetic.

> 
> 	a = &existing_conf->input;
> 	b = &conf->input;
> 	flow_type_a = a->flow_type;
> 	flow_type_b = b->flow_type;
> 	}
> 
> > 
> > > +
> > > +		/* No need to compare two rules with different tunnel
> > type */
> > > +		if (existing_conf->ttype != conf->ttype)
> > > +			continue;
> > > +
> > > +		/* No need to compare two rules with same protocol */
> > > +		if (flow_type_a == flow_type_b)
> > > +			continue;
> > This two ifs can be combined into one.
> 
> Sure, it could be updated in the coming version. Thanks!
> 
> > 
> > > +
> > > +		switch (flow_type_a) {
> > > +		case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
> > > +		case ICE_FLTR_PTYPE_NONF_IPV4_TCP:
> > > +		case ICE_FLTR_PTYPE_NONF_IPV4_SCTP:
> > > +			if (flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV4_OTHER)
> > > +				return true;
> > > +			break;
> > > +		case ICE_FLTR_PTYPE_NONF_IPV4_OTHER:
> > > +			if (flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
> > > +			    flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV4_TCP ||
> > > +			    flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV4_SCTP)
> > > +				return true;
> > > +			break;
> > > +		case ICE_FLTR_PTYPE_NONF_IPV6_UDP:
> > > +		case ICE_FLTR_PTYPE_NONF_IPV6_TCP:
> > > +		case ICE_FLTR_PTYPE_NONF_IPV6_SCTP:
> > > +			if (flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV6_OTHER)
> > > +				return true;
> > > +			break;
> > > +		case ICE_FLTR_PTYPE_NONF_IPV6_OTHER:
> > > +			if (flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV6_UDP ||
> > > +			    flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV6_TCP ||
> > > +			    flow_type_b ==
> > ICE_FLTR_PTYPE_NONF_IPV6_SCTP)
> > > +				return true;
> > > +			break;
> > > +		default:
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	return false;
> > > +}
> > > +
> > >  /**
> > >   * ice_vc_fdir_write_flow_prof
> > >   * @vf: pointer to the VF structure
> > > @@ -677,6 +742,12 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf,
> > struct virtchnl_fdir_add *fltr,
> > >  	enum ice_fltr_ptype flow;
> > >  	int ret;
> > >
> > > +	ret = ice_vc_fdir_has_prof_conflict(vf, conf);
> > > +	if (ret) {
> > > +		dev_dbg(dev, "Found flow prof conflict for VF %d\n", vf-
> > >vf_id);
> > > +		return ret;
> > > +	}
> > > +
> > >  	flow = input->flow_type;
> > >  	ret = ice_vc_fdir_alloc_prof(vf, flow);
> > >  	if (ret) {
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > 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

Re: [Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR

[Tony Nguyen]

On 3/10/2023 2:15 AM, Michal Swiatkowski wrote:
> On Fri, Mar 10, 2023 at 05:16:22AM +0000, Guo, Junfeng wrote:

[...]

>>>> +
>>> 	to_fltr_conf_from_desc(desc);
>>>> +		struct ice_fdir_fltr *a = &existing_conf->input;
>>>> +		struct ice_fdir_fltr *b = &conf->input;
>>>> +
>>>> +		enum ice_fltr_ptype flow_type_a = a->flow_type;
>>>> +		enum ice_fltr_ptype flow_type_b = b->flow_type;
>>> I think You should folow RCT variable declaration here, and remove
>>> empty
>>> line.
>>
>> Thanks for the advice!
>>
>> Do you mean update the code order like this?
>> 	{
>>
> To follow RCT:
> struct ice_fdir_fltr *a = &existing_conf->input;
> enum ice_fltr_ptype flow_type_a, flow_type_b;
> struct ice_fdir_fltr *b = &conf->input;
> 
>> 	flow_type_a = a->flow_type;
>> 	flow_type_b = b->flow_type;
>> 	}
>> Or like this?
>> 	{
>> 	enum ice_fltr_ptype flow_type_a, flow_type_b;
>> 	struct ice_fdir_fltr *a, *b;
> This is also fine
> 
> Also fine will be:
> struct ice_fdir_fltr *a = &existing_conf->input;
> enum ice_fltr_ptype flow_type_a = a->flow_type;
> enum ice_fltr_ptype flow_type_b = b->flow_type;
> struct ice_fdir_fltr *b = &conf->input;
> 
> And it's look the best in my opinion, but it is only cosmetic.

Looks like flow type b has a dependency on fltr b so I don't think this 
will work.

Either of the suggestions previously mentioned should work:

	struct ice_fdir_fltr *a = &existing_conf->input;
	enum ice_fltr_ptype flow_type_a, flow_type_b;
	struct ice_fdir_fltr *b = &conf->input;

	flow_type_a = a->flow_type;
	flow_type_b = b->flow_type;
or:

	enum ice_fltr_ptype flow_type_a, flow_type_b;
	struct ice_fdir_fltr *a, *b;

	a = &existing_conf->input;
	b = &conf->input;
	flow_type_a = a->flow_type;
	flow_type_b = b->flow_type;
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR

[Guo, Junfeng]

> -----Original Message-----
> From: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Sent: Saturday, March 11, 2023 02:03
> To: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>; Guo,
> Junfeng <junfeng.guo@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net] ice: add profile conflict check
> for AVF FDIR
> 
> On 3/10/2023 2:15 AM, Michal Swiatkowski wrote:
> > On Fri, Mar 10, 2023 at 05:16:22AM +0000, Guo, Junfeng wrote:
> 
> [...]
> 
> >>>> +
> >>> 	to_fltr_conf_from_desc(desc);
> >>>> +		struct ice_fdir_fltr *a = &existing_conf->input;
> >>>> +		struct ice_fdir_fltr *b = &conf->input;
> >>>> +
> >>>> +		enum ice_fltr_ptype flow_type_a = a->flow_type;
> >>>> +		enum ice_fltr_ptype flow_type_b = b->flow_type;
> >>> I think You should folow RCT variable declaration here, and remove
> >>> empty
> >>> line.
> >>
> >> Thanks for the advice!
> >>
> >> Do you mean update the code order like this?
> >> 	{
> >>
> > To follow RCT:
> > struct ice_fdir_fltr *a = &existing_conf->input;
> > enum ice_fltr_ptype flow_type_a, flow_type_b;
> > struct ice_fdir_fltr *b = &conf->input;
> >
> >> 	flow_type_a = a->flow_type;
> >> 	flow_type_b = b->flow_type;
> >> 	}
> >> Or like this?
> >> 	{
> >> 	enum ice_fltr_ptype flow_type_a, flow_type_b;
> >> 	struct ice_fdir_fltr *a, *b;
> > This is also fine
> >
> > Also fine will be:
> > struct ice_fdir_fltr *a = &existing_conf->input;
> > enum ice_fltr_ptype flow_type_a = a->flow_type;
> > enum ice_fltr_ptype flow_type_b = b->flow_type;
> > struct ice_fdir_fltr *b = &conf->input;
> >
> > And it's look the best in my opinion, but it is only cosmetic.
> 
> Looks like flow type b has a dependency on fltr b so I don't think this
> will work.
> 
> Either of the suggestions previously mentioned should work:
> 
> 	struct ice_fdir_fltr *a = &existing_conf->input;
> 	enum ice_fltr_ptype flow_type_a, flow_type_b;
> 	struct ice_fdir_fltr *b = &conf->input;
> 
> 	flow_type_a = a->flow_type;
> 	flow_type_b = b->flow_type;
> or:
> 
> 	enum ice_fltr_ptype flow_type_a, flow_type_b;
> 	struct ice_fdir_fltr *a, *b;
> 
> 	a = &existing_conf->input;
> 	b = &conf->input;
> 	flow_type_a = a->flow_type;
> 	flow_type_b = b->flow_type;

LGTM, thank you all for the careful review!
Will update this in the coming version patch.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH net v2] ice: add profile conflict check for AVF FDIR

[Junfeng Guo]

Add profile conflict check while adding some FDIR rules to aviod
unexpected flow behavior, rules may have conflict including:
        IPv4 <---> {IPv4_UDP, IPv4_TCP, IPv4_SCTP}
        IPv6 <---> {IPv6_UDP, IPv6_TCP, IPv6_SCTP}

For example, when we create an FDIR rule for IPv4, this rule will work
on packets including IPv4, IPv4_UDP, IPv4_TCP and IPv4_SCTP. But if we
then create an FDIR rule for IPv4_UDP and then destroy it, the first
FDIR rule for IPv4 cannot work on pkt IPv4_UDP then.

To prevent this unexpected behavior, we add restriction in software
when creating FDIR rules by adding necessary profile conflict check.

Fixes: 1f7ea1cd6a37 ("ice: Enable FDIR Configure for AVF")
Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
---
 .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
index e6ef6b303222..06296ffc3608 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
@@ -541,6 +541,73 @@ static void ice_vc_fdir_rem_prof_all(struct ice_vf *vf)
 	}
 }
 
+/**
+ * ice_vc_fdir_has_prof_conflict
+ * @vf: pointer to the VF structure
+ * @conf: FDIR configuration for each filter
+ *
+ * Check if @conf has conflicting profile with existing profiles
+ *
+ * Return: true on success, and false on error.
+ */
+static bool
+ice_vc_fdir_has_prof_conflict(struct ice_vf *vf,
+			      struct virtchnl_fdir_fltr_conf *conf)
+{
+	struct ice_fdir_fltr *desc;
+
+	list_for_each_entry(desc, &vf->fdir.fdir_rule_list, fltr_node) {
+		struct virtchnl_fdir_fltr_conf *existing_conf =
+						to_fltr_conf_from_desc(desc);
+		enum ice_fltr_ptype flow_type_a, flow_type_b;
+		struct ice_fdir_fltr *a, *b;
+
+		a = &existing_conf->input;
+		b = &conf->input;
+		flow_type_a = a->flow_type;
+		flow_type_b = b->flow_type;
+
+		/**
+		 * No need to compare two rules with different tunnel types or
+		 * with the same protocol type
+		 */
+		if (existing_conf->ttype != conf->ttype ||
+		    flow_type_a == flow_type_b)
+			continue;
+
+		switch (flow_type_a) {
+		case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
+		case ICE_FLTR_PTYPE_NONF_IPV4_TCP:
+		case ICE_FLTR_PTYPE_NONF_IPV4_SCTP:
+			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_OTHER)
+				return true;
+			break;
+		case ICE_FLTR_PTYPE_NONF_IPV4_OTHER:
+			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
+			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_TCP ||
+			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_SCTP)
+				return true;
+			break;
+		case ICE_FLTR_PTYPE_NONF_IPV6_UDP:
+		case ICE_FLTR_PTYPE_NONF_IPV6_TCP:
+		case ICE_FLTR_PTYPE_NONF_IPV6_SCTP:
+			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_OTHER)
+				return true;
+			break;
+		case ICE_FLTR_PTYPE_NONF_IPV6_OTHER:
+			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_UDP ||
+			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_TCP ||
+			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_SCTP)
+				return true;
+			break;
+		default:
+			break;
+		}
+	}
+
+	return false;
+}
+
 /**
  * ice_vc_fdir_write_flow_prof
  * @vf: pointer to the VF structure
@@ -677,6 +744,12 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf, struct virtchnl_fdir_add *fltr,
 	enum ice_fltr_ptype flow;
 	int ret;
 
+	ret = ice_vc_fdir_has_prof_conflict(vf, conf);
+	if (ret) {
+		dev_dbg(dev, "Found flow prof conflict for VF %d\n", vf->vf_id);
+		return ret;
+	}
+
 	flow = input->flow_type;
 	ret = ice_vc_fdir_alloc_prof(vf, flow);
 	if (ret) {
-- 
2.25.1

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

Re: [Intel-wired-lan] [PATCH net v2] ice: add profile conflict check for AVF FDIR

[Jesse Brandeburg]

On 3/12/2023 7:17 PM, Junfeng Guo wrote:
> Add profile conflict check while adding some FDIR rules to aviod
> unexpected flow behavior, rules may have conflict including:
>         IPv4 <---> {IPv4_UDP, IPv4_TCP, IPv4_SCTP}
>         IPv6 <---> {IPv6_UDP, IPv6_TCP, IPv6_SCTP}
> 
> For example, when we create an FDIR rule for IPv4, this rule will work
> on packets including IPv4, IPv4_UDP, IPv4_TCP and IPv4_SCTP. But if we
> then create an FDIR rule for IPv4_UDP and then destroy it, the first
> FDIR rule for IPv4 cannot work on pkt IPv4_UDP then.
> 
> To prevent this unexpected behavior, we add restriction in software
> when creating FDIR rules by adding necessary profile conflict check.
> 
> Fixes: 1f7ea1cd6a37 ("ice: Enable FDIR Configure for AVF")
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> ---
>  .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 73 +++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> index e6ef6b303222..06296ffc3608 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> @@ -541,6 +541,73 @@ static void ice_vc_fdir_rem_prof_all(struct ice_vf *vf)
>  	}
>  }
>  
> +/**
> + * ice_vc_fdir_has_prof_conflict
> + * @vf: pointer to the VF structure
> + * @conf: FDIR configuration for each filter
> + *
> + * Check if @conf has conflicting profile with existing profiles
> + *
> + * Return: true on success, and false on error.
> + */
> +static bool
> +ice_vc_fdir_has_prof_conflict(struct ice_vf *vf,
> +			      struct virtchnl_fdir_fltr_conf *conf)
> +{
> +	struct ice_fdir_fltr *desc;
> +
> +	list_for_each_entry(desc, &vf->fdir.fdir_rule_list, fltr_node) {
> +		struct virtchnl_fdir_fltr_conf *existing_conf =
> +						to_fltr_conf_from_desc(desc);
> +		enum ice_fltr_ptype flow_type_a, flow_type_b;
> +		struct ice_fdir_fltr *a, *b;
> +
> +		a = &existing_conf->input;
> +		b = &conf->input;
> +		flow_type_a = a->flow_type;
> +		flow_type_b = b->flow_type;
> +
> +		/**
> +		 * No need to compare two rules with different tunnel types or

"/**" is special magic for declaring documentation in kernel and
doxygen. Just use:

/* multi-line
 * comment
 */

which is netdev style.

> +		 * with the same protocol type
> +		 */
> +		if (existing_conf->ttype != conf->ttype ||
> +		    flow_type_a == flow_type_b)
> +			continue;
> +
> +		switch (flow_type_a) {
> +		case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
> +		case ICE_FLTR_PTYPE_NONF_IPV4_TCP:
> +		case ICE_FLTR_PTYPE_NONF_IPV4_SCTP:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_OTHER)
> +				return true;
> +			break;
> +		case ICE_FLTR_PTYPE_NONF_IPV4_OTHER:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_TCP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_SCTP)
> +				return true;
> +			break;
> +		case ICE_FLTR_PTYPE_NONF_IPV6_UDP:
> +		case ICE_FLTR_PTYPE_NONF_IPV6_TCP:
> +		case ICE_FLTR_PTYPE_NONF_IPV6_SCTP:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_OTHER)
> +				return true;
> +			break;
> +		case ICE_FLTR_PTYPE_NONF_IPV6_OTHER:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_UDP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_TCP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_SCTP)
> +				return true;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  /**
>   * ice_vc_fdir_write_flow_prof
>   * @vf: pointer to the VF structure
> @@ -677,6 +744,12 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf, struct virtchnl_fdir_add *fltr,
>  	enum ice_fltr_ptype flow;
>  	int ret;
>  
> +	ret = ice_vc_fdir_has_prof_conflict(vf, conf);
> +	if (ret) {
> +		dev_dbg(dev, "Found flow prof conflict for VF %d\n", vf->vf_id);

This is just a nit, but generally messages to the log should always
contain full words and not abbreviations.

"Found flow profile conflict for VF %d\n"


> +		return ret;
> +	}
> +
>  	flow = input->flow_type;
>  	ret = ice_vc_fdir_alloc_prof(vf, flow);
>  	if (ret) {

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

Re: [Intel-wired-lan] [PATCH net v2] ice: add profile conflict check for AVF FDIR

[Guo, Junfeng]

> -----Original Message-----
> From: Brandeburg, Jesse <jesse.brandeburg@intel.com>
> Sent: Monday, March 13, 2023 23:51
> To: Guo, Junfeng <junfeng.guo@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net v2] ice: add profile conflict
> check for AVF FDIR
> 
> On 3/12/2023 7:17 PM, Junfeng Guo wrote:
> > Add profile conflict check while adding some FDIR rules to aviod
> > unexpected flow behavior, rules may have conflict including:
> >         IPv4 <---> {IPv4_UDP, IPv4_TCP, IPv4_SCTP}
> >         IPv6 <---> {IPv6_UDP, IPv6_TCP, IPv6_SCTP}
> >
> > For example, when we create an FDIR rule for IPv4, this rule will work
> > on packets including IPv4, IPv4_UDP, IPv4_TCP and IPv4_SCTP. But if we
> > then create an FDIR rule for IPv4_UDP and then destroy it, the first
> > FDIR rule for IPv4 cannot work on pkt IPv4_UDP then.
> >
> > To prevent this unexpected behavior, we add restriction in software
> > when creating FDIR rules by adding necessary profile conflict check.
> >
> > Fixes: 1f7ea1cd6a37 ("ice: Enable FDIR Configure for AVF")
> > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> > ---
> >  .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 73 +++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > index e6ef6b303222..06296ffc3608 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> > @@ -541,6 +541,73 @@ static void ice_vc_fdir_rem_prof_all(struct
> ice_vf *vf)
> >  	}
> >  }
> >
> > +/**
> > + * ice_vc_fdir_has_prof_conflict
> > + * @vf: pointer to the VF structure
> > + * @conf: FDIR configuration for each filter
> > + *
> > + * Check if @conf has conflicting profile with existing profiles
> > + *
> > + * Return: true on success, and false on error.
> > + */
> > +static bool
> > +ice_vc_fdir_has_prof_conflict(struct ice_vf *vf,
> > +			      struct virtchnl_fdir_fltr_conf *conf)
> > +{
> > +	struct ice_fdir_fltr *desc;
> > +
> > +	list_for_each_entry(desc, &vf->fdir.fdir_rule_list, fltr_node) {
> > +		struct virtchnl_fdir_fltr_conf *existing_conf =
> > +
> 	to_fltr_conf_from_desc(desc);
> > +		enum ice_fltr_ptype flow_type_a, flow_type_b;
> > +		struct ice_fdir_fltr *a, *b;
> > +
> > +		a = &existing_conf->input;
> > +		b = &conf->input;
> > +		flow_type_a = a->flow_type;
> > +		flow_type_b = b->flow_type;
> > +
> > +		/**
> > +		 * No need to compare two rules with different tunnel
> types or
> 
> "/**" is special magic for declaring documentation in kernel and
> doxygen. Just use:
> 
> /* multi-line
>  * comment
>  */
> 
> which is netdev style.

Sure, will follow this style. Thanks for the reminding!

> 
> > +		 * with the same protocol type
> > +		 */
> > +		if (existing_conf->ttype != conf->ttype ||
> > +		    flow_type_a == flow_type_b)
> > +			continue;
> > +
> > +		switch (flow_type_a) {
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_TCP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_SCTP:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_OTHER)
> > +				return true;
> > +			break;
> > +		case ICE_FLTR_PTYPE_NONF_IPV4_OTHER:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_TCP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV4_SCTP)
> > +				return true;
> > +			break;
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_UDP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_TCP:
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_SCTP:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_OTHER)
> > +				return true;
> > +			break;
> > +		case ICE_FLTR_PTYPE_NONF_IPV6_OTHER:
> > +			if (flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_UDP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_TCP ||
> > +			    flow_type_b ==
> ICE_FLTR_PTYPE_NONF_IPV6_SCTP)
> > +				return true;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  /**
> >   * ice_vc_fdir_write_flow_prof
> >   * @vf: pointer to the VF structure
> > @@ -677,6 +744,12 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf,
> struct virtchnl_fdir_add *fltr,
> >  	enum ice_fltr_ptype flow;
> >  	int ret;
> >
> > +	ret = ice_vc_fdir_has_prof_conflict(vf, conf);
> > +	if (ret) {
> > +		dev_dbg(dev, "Found flow prof conflict for VF %d\n", vf-
> >vf_id);
> 
> This is just a nit, but generally messages to the log should always
> contain full words and not abbreviations.
> 
> "Found flow profile conflict for VF %d\n"

Will update this, thanks!

> 
> 
> > +		return ret;
> > +	}
> > +
> >  	flow = input->flow_type;
> >  	ret = ice_vc_fdir_alloc_prof(vf, flow);
> >  	if (ret) {

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