Re: [dpdk-dev] [PATCH] ethdev: fix byte order inconsistence between fdir flow and mask

["Wu, Jingjing" <jingjing.wu@intel.com>]

> 
> 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.
> 
> Please be more precise. Which one is big endian?
> Wasn't it tested before?
> 
It is tested, it works. But byte order fields in the structures for fdir are 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")
> 
> Please put Fixes: on the two lines.
> 
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.
> 
> John, comment on release notes formatting?
> It's important to have the first items well formatted.
> 
> > @@ -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.
> 
> Please take care of uppercase typo here.
Ok. Thanks for point.
> 
> > -	/* 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)
> > +	*/
> 
> Spacing typo here.
> Sorry for the nits ;)
> 
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. */
> 
> 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