Re: [dpdk-dev] [PATCH v4 11/21] ethdev: define structures for getting flow director information

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2014-10-22 09:01, Jingjing Wu:
> +/**
> + * A structure used to report the status of the flow director filters in use.
> + */
> +struct rte_eth_fdir {
> +	/** Number of filters with collision indication. */
> +	uint16_t collision;
> +	/** Number of free (non programmed) filters. */
> +	uint16_t free;
> +	/** The Lookup hash value of the added filter that updated the value
> +	   of the MAXLEN field */
> +	uint16_t maxhash;
> +	/** Longest linked list of filters in the table. */
> +	uint8_t maxlen;
> +	/** Number of added filters. */
> +	uint64_t add;
> +	/** Number of removed filters. */
> +	uint64_t remove;
> +	/** Number of failed added filters (no more space in device). */
> +	uint64_t f_add;
> +	/** Number of failed removed filters. */
> +	uint64_t f_remove;
> +};

rte_eth_fdir is a name which doesn't say what it really is.
This structure looks like a collection of statistics.
Why not rte_eth_fdir_stats?

> +struct rte_eth_fdir_ext {
> +	uint16_t guarant_spc;  /**< guaranteed spaces.*/
> +	uint16_t guarant_cnt;  /**< Number of filters in guaranteed spaces. */
> +	uint16_t best_spc;     /**< best effort spaces.*/
> +	uint16_t best_cnt;     /**< Number of filters in best effort spaces. */
> +};

I don't understand why this "extended" structure is not merged in the first one.
Adding new fields don't break old API.

> +/**
> + * A structure used to get the status information of flow director filter.
> + * to support RTE_ETH_FILTER_FDIR with RTE_ETH_FILTER_INFO operation.
> + */

OK content of this comment is good.
But the second sentence has no start.
Please try to have an uppercase letter at the beginning of your sentences,
and a subject followed by a verb.
(side note: this is also true for commit logs)

> +struct rte_eth_fdir_info {
> +	int mode;                 /**< if 0 disbale, if 1 enable*/

Typo: disbale

-- 
Thomas