Re: [dpdk-dev] [PATCH v1 11/16] ethdev: refine TPID handling in flow API

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com]
> Sent: Thursday, April 5, 2018 10:39 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; dev@dpdk.org; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; John Daley <johndale@cisco.com>; Hyong
> Youb Kim <hyonkim@cisco.com>; Xing, Beilei <beilei.xing@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; Nelio Laranjeiro
> <nelio.laranjeiro@6wind.com>; Yongseok Koh <yskoh@mellanox.com>; Jacek
> Siuda <jck@semihalf.com>; Tomasz Duszynski <tdu@semihalf.com>; Dmitri
> Epshtein <dima@marvell.com>; Natalie Samsonov <nsamsono@marvell.com>;
> Jianbo Liu <jianbo.liu@arm.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; Pascal Mazon <pascal.mazon@6wind.com>
> Subject: Re: [PATCH v1 11/16] ethdev: refine TPID handling in flow API
> 
> On Thu, Apr 05, 2018 at 02:20:34PM +0000, Zhang, Qi Z wrote:
> > Hi Adrien:
> >
> > > Hi PMD maintainers, while I'm pretty confident in these changes, I
> > > could not validate them with all devices.
> > >
> > > It would be great if you could apply this patch, run testpmd, create
> > > VLAN flow rules with/without inner EtherType as described and send
> > > matching traffic while making sure nothing was broken in the process.
> > >
> > > Thanks!
> > > ---
> > > diff --git a/drivers/net/i40e/i40e_flow.c
> > > b/drivers/net/i40e/i40e_flow.c index 1b336df74..c6dd889ad 100644
> > > --- a/drivers/net/i40e/i40e_flow.c
> > > +++ b/drivers/net/i40e/i40e_flow.c
> > > @@ -2490,24 +2490,36 @@ i40e_flow_parse_fdir_pattern(struct
> > > rte_eth_dev *dev,
> > >  						      "Invalid MAC_addr mask.");
> > >  					return -rte_errno;
> > >  				}
> > > +			}
> > > +			if (eth_spec && eth_mask && eth_mask->type) {
> > > +				enum rte_flow_item_type next = (item + 1)->type;
> > >
> > > -				if ((eth_mask->type & UINT16_MAX) ==
> > > -				    UINT16_MAX) {
> > > -					input_set |= I40E_INSET_LAST_ETHER_TYPE;
> > > -					filter->input.flow.l2_flow.ether_type =
> > > -						eth_spec->type;
> > > +				if (eth_mask->type != RTE_BE16(0xffff)) {
> > > +					rte_flow_error_set(error, EINVAL,
> > > +						      RTE_FLOW_ERROR_TYPE_ITEM,
> > > +						      item,
> > > +						      "Invalid type mask.");
> > > +					return -rte_errno;
> > >  				}
> > >
> > >  				ether_type = rte_be_to_cpu_16(eth_spec->type);
> > > -				if (ether_type == ETHER_TYPE_IPv4 ||
> > > -				    ether_type == ETHER_TYPE_IPv6 ||
> > > -				    ether_type == ETHER_TYPE_ARP ||
> > > -				    ether_type == outer_tpid) {
> > > +
> > > +				if ((next == RTE_FLOW_ITEM_TYPE_VLAN &&
> > > +				     ether_type != outer_tpid) ||
> > > +				    (next != RTE_FLOW_ITEM_TYPE_VLAN &&
> > > +				     (ether_type == ETHER_TYPE_IPv4 ||
> > > +				      ether_type == ETHER_TYPE_IPv6 ||
> > > +				      ether_type == ETHER_TYPE_ARP ||
> > > +				      ether_type == outer_tpid))) {
> > >  					rte_flow_error_set(error, EINVAL,
> > >  						     RTE_FLOW_ERROR_TYPE_ITEM,
> > >  						     item,
> > >  						     "Unsupported ether_type.");
> > >  					return -rte_errno;
> > > +				} else if (next != RTE_FLOW_ITEM_TYPE_VLAN) {
> > > +					input_set |= I40E_INSET_LAST_ETHER_TYPE;
> > > +					filter->input.flow.l2_flow.ether_type =
> > > +						eth_spec->type;
> > >  				}
> > >  			}
> > >
> > > @@ -2518,6 +2530,14 @@ i40e_flow_parse_fdir_pattern(struct
> > > rte_eth_dev *dev,
> > >  		case RTE_FLOW_ITEM_TYPE_VLAN:
> > >  			vlan_spec = item->spec;
> > >  			vlan_mask = item->mask;
> > > +
> > > +			if (input_set & I40E_INSET_LAST_ETHER_TYPE) {
> > > +				rte_flow_error_set(error, EINVAL,
> > > +						   RTE_FLOW_ERROR_TYPE_ITEM,
> > > +						   item,
> > > +						   "Unsupported outer TPID.");
> > > +				return -rte_errno;
> > > +			}
> >
> > Please correct me I'm wrong, now I40E_INSET_LAST_ETHER_TYPE will only
> > be set when next != RTE_FLOW_ITEM_TYPE_VLAN while,
> RTE_FLOW_ITEM_TYPE_VLAN will only be the next to
> RTE_FLOW_ITEM_TYPE_ETH for fdir, so above check seems unnecessary ?
> 
> You're right, it's unnecessary. I can remove this change or leave it as a safety
> measure for future contributions, since when parsing a VLAN item one is not
> necessarily aware of what happened in previous iterations.
> 
> How about an assertion check for debugging purposes instead?
> 
>  RTE_ASSERT(!(input_set & I40E_INSET_LAST_ETHER_TYPE));

Agree to use assert.

> 
> --
> Adrien Mazarguil
> 6WIND