From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Wu, Jingjing" <jingjing.wu@intel.com>
Subject: Re: [PATCH] ethdev: fix byte order inconsistence between
 fdir flow and mask
Date: Thu, 28 Jan 2016 05:55:16 +0000
Message-ID: <9BB6961774997848B5B42BEC655768F8D8EA55@SHSMSX104.ccr.corp.intel.com>
References: <1453883856-31246-1-git-send-email-jingjing.wu@intel.com>
 <2497810.HTMk7hhgUj@xps13>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Cc: "dev@dpdk.org" <dev@dpdk.org>
To: Thomas Monjalon <thomas.monjalon@6wind.com>, "Mcnamara, John"
 <john.mcnamara@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
 by dpdk.org (Postfix) with ESMTP id 17246C1BC
 for <dev@dpdk.org>; Thu, 28 Jan 2016 06:55:23 +0100 (CET)
In-Reply-To: <2497810.HTMk7hhgUj@xps13>
Content-Language: en-US
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

>=20
> 2016-01-27 16:37, Jingjing Wu:
> > Fixed issue of byte order in ethdev library that the structure for
> > setting fdir's mask and flow entry is inconsist and made inputs of
> > mask be in big endian.
>=20
> Please be more precise. Which one is big endian?
> Wasn't it tested before?
>=20
It is tested, it works. But byte order fields in the structures for fdir ar=
e not
consist, few are in host endian. It will make user confused, so this patch
is to make all fields in the same byte order.

> > fixes: 76c6f89e80d4 ("ixgbe: support new flow director masks")
> >        2d4c1a9ea2ac ("ethdev: add new flow director masks")
>=20
> Please put Fixes: on the two lines.
>=20
OK. Will add in later version.

> > --- a/doc/guides/rel_notes/release_2_3.rst
> > +++ b/doc/guides/rel_notes/release_2_3.rst
> > @@ -19,6 +19,10 @@ Drivers
> >  Libraries
> >  ~~~~~~~~~
> >
> > +* ** fix byte order inconsistence between fdir flow and mask **
> > +
> > +  Fixed issue in ethdev library that the structure for setting
> > + fdir's mask and flow entry is inconsist in byte order.
>=20
> John, comment on release notes formatting?
> It's important to have the first items well formatted.
>=20
> > @@ -39,6 +43,8 @@ API Changes
> >  ABI Changes
> >  -----------
> >
> > +* The fields in  The ethdev structures ``rte_eth_fdir_masks`` were
> > +  changed to be in big endian.
>=20
> Please take care of uppercase typo here.
Ok. Thanks for point.
>=20
> > -	/* write all the same so that UDP, TCP and SCTP use the same mask
> */
> > +	/* write all the same so that UDP, TCP and SCTP use the same mask
> > +	 * (little-endian)
> > +	*/
>=20
> Spacing typo here.
> Sorry for the nits ;)
>=20
Will fix this in later version.

> > -	uint8_t mac_addr_byte_mask;  /** Per byte MAC address mask */
> > +	uint8_t mac_addr_byte_mask;  /** Bit mask for associated byte */
> >  	uint32_t tunnel_id_mask;  /** tunnel ID mask */
> > -	uint8_t tunnel_type_mask;
> > +	uint8_t tunnel_type_mask; /**< 1 - Match tunnel type,
> > +				       0 - Ignore tunnel type. */
>=20
> These changes seem unrelated with the patch.
> It's good to improve doc of this API but it's maybe not enough.
> Example:
> 	uint8_t mac_addr_byte_mask;  /** Bit mask for associated byte */
> Are we sure everybody understand how to fill it?

OK. Will improve this.

Thanks
JIngjing