> -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarg...@6wind.com] > Sent: Thursday, April 5, 2018 10:39 PM > To: Zhang, Qi Z <qi.z.zh...@intel.com> > Cc: Thomas Monjalon <tho...@monjalon.net>; Yigit, Ferruh > <ferruh.yi...@intel.com>; dev@dpdk.org; Lu, Wenzhuo > <wenzhuo...@intel.com>; Wu, Jingjing <jingjing...@intel.com>; Ajit Khaparde > <ajit.khapa...@broadcom.com>; Somnath Kotur > <somnath.ko...@broadcom.com>; John Daley <johnd...@cisco.com>; Hyong > Youb Kim <hyon...@cisco.com>; Xing, Beilei <beilei.x...@intel.com>; Ananyev, > Konstantin <konstantin.anan...@intel.com>; Nelio Laranjeiro > <nelio.laranje...@6wind.com>; Yongseok Koh <ys...@mellanox.com>; Jacek > Siuda <j...@semihalf.com>; Tomasz Duszynski <t...@semihalf.com>; Dmitri > Epshtein <d...@marvell.com>; Natalie Samsonov <nsams...@marvell.com>; > Jianbo Liu <jianbo....@arm.com>; Andrew Rybchenko > <arybche...@solarflare.com>; Pascal Mazon <pascal.ma...@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
