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

[Thomas Monjalon <thomas.monjalon@6wind.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?

> fixes: 76c6f89e80d4 ("ixgbe: support new flow director masks")
>        2d4c1a9ea2ac ("ethdev: add new flow director masks")

Please put Fixes: on the two lines.

> --- 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.

> -	/* 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 ;)

> -	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?