Re: [dpdk-dev] app/testpmd: fix PPPOES flow API

["Zhang, Xiao" <xiao.zhang@intel.com>]

Hi Ori,

> -----Original Message-----
> From: Ori Kam <orika@mellanox.com>
> Sent: Sunday, March 29, 2020 2:28 PM
> To: Zhang, Xiao <xiao.zhang@intel.com>; dev@dpdk.org
> Cc: Wang, Ying A <ying.a.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; stable@dpdk.org
> Subject: RE: app/testpmd: fix PPPOES flow API
> 
> Hi Xiao,
> 
> Is the proto_id part of the basic header or not?

Proto_id is part of PPPOE session header,

> 
> From the spec it looks like a different header.
> 
> If it is part of the original header then all documentations and rte_structs should
> be changed, to reflect this.
> 
> It will be very helpful if the patch message would explain the bug and why it was
> changed.

Okay, will add more message. The next value of the ITEM_PPPOE_PROTO_ID should be unsigned value but not item list.

> 
> Also please see inline other comment.
> 
> Best,
> Ori
> 
> > -----Original Message-----
> > From: Xiao Zhang <xiao.zhang@intel.com>
> > Sent: Friday, March 27, 2020 11:19 AM
> > To: dev@dpdk.org
> > Cc: Ori Kam <orika@mellanox.com>; ying.a.wang@intel.com;
> > qi.z.zhang@intel.com; wei.zhao1@intel.com; Xiao Zhang
> > <xiao.zhang@intel.com>; stable@dpdk.org
> > Subject: app/testpmd: fix PPPOES flow API
> >
> > The command line to create RTE flow for specific proto_id of PPPOES is
> > not correct. This patch is to fix this issue.
> >
> > Fixes: 226c6e60c35b ("ethdev: add PPPoE to flow API")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Xiao Zhang <xiao.zhang@intel.com>
> > ---
> >  app/test-pmd/cmdline_flow.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> > index a78154502..c25a2598d 100644
> > --- a/app/test-pmd/cmdline_flow.c
> > +++ b/app/test-pmd/cmdline_flow.c
> > @@ -768,7 +768,6 @@ static const enum index next_item[] = {
> >  	ITEM_GTP_PSC,
> >  	ITEM_PPPOES,
> >  	ITEM_PPPOED,
> > -	ITEM_PPPOE_PROTO_ID,
> >  	ITEM_HIGIG2,
> >  	ITEM_TAG,
> >  	ITEM_L2TPV3OIP,
> > @@ -1030,11 +1029,6 @@ static const enum index item_pppoed[] = {
> >
> >  static const enum index item_pppoes[] = {
> >  	ITEM_PPPOE_SEID,
> > -	ITEM_NEXT,
> > -	ZERO,
> > -};
> > -
> > -static const enum index item_pppoe_proto_id[] = {
> >  	ITEM_PPPOE_PROTO_ID,
> >  	ITEM_NEXT,
> >  	ZERO,
> > @@ -2643,10 +2637,9 @@ static const struct token token_list[] = {
> >  	[ITEM_PPPOE_PROTO_ID] = {
> >  		.name = "proto_id",
> >  		.help = "match PPPoE session protocol identifier",
> > -		.priv = PRIV_ITEM(PPPOE_PROTO_ID,
> > -				sizeof(struct rte_flow_item_pppoe_proto_id)),
> > -		.next = NEXT(item_pppoe_proto_id),
> > -		.call = parse_vc,
> > +		.next = NEXT(item_pppoes, NEXT_ENTRY(UNSIGNED),
> > item_param),
> > +		.args = ARGS(ARGS_ENTRY_HTON
> > +			     (struct rte_flow_item_pppoe_proto_id, proto_id)),
> 
> Where is the memory for this proto_id is defined?

Do you mean this?
lib/librte_ethdev/rte_flow.h
1360 struct rte_flow_item_pppoe_proto_id {
1361         rte_be16_t proto_id; /**< PPP protocol identifier. */
1362 };

> 
> >  	},
> >  	[ITEM_HIGIG2] = {
> >  		.name = "higig2",
> > --
> > 2.17.1