Re: [dpdk-dev] [PATCH v5 2/5] ethdev: add enum type and relevant structures for hash filter control

["Zhang, Helin" <helin.zhang@intel.com>]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, November 3, 2014 3:57 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5 2/5] ethdev: add enum type and relevant
> structures for hash filter control
> 
> 2014-10-21 11:14, Helin Zhang:
> > +enum rte_eth_hash_filter_info_type {
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_UNKNOWN = 0,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PCTYPE,
> 
> PCTYPE is an unknown word in the API layer.
> Could you replace it by something more generic?
In ethdev layer, the idea of 'flow type' will be used for generic purpose.

> 
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_SYM_HASH_ENA_PER_PORT,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_HASH_FUNCTION,
> > +	RTE_ETH_HASH_FILTER_INFO_TYPE_MAX,
> > +};
> 
> You should comment each constant.
Yes, agree.

> 
> > +struct rte_eth_sym_hash_ena_info {
> > +	/**< packet classification type, defined in rte_ethdev.h */
> > +	uint8_t pctype;
> 
> No, PCTYPE is not anymore defined in ethdev.
No pctype will be there on ethdev layer.

> 
> > +/**
> > + * A structure used to set or get filter swap information, to support
> > + * 'RTE_ETH_FILTER_HASH', 'RTE_ETH_FILTER_GET/RTE_ETH_FILTER_SET',
> > + * with information type 'RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP'.
> > + */
> > +struct rte_eth_filter_swap_info {
> > +	/**< Packet classification type, defined in rte_ethdev.h */
> > +	uint8_t pctype;
> > +	/**< Offset of the 1st field of the 1st couple to be swapped. */
> > +	uint8_t off0_src0;
> > +	/**< Offset of the 2nd field of the 1st couple to be swapped. */
> > +	uint8_t off0_src1;
> > +	/**< Field length of the first couple. */
> > +	uint8_t len0;
> > +	/**< Offset of the 1st field of the 2nd couple to be swapped. */
> > +	uint8_t off1_src0;
> > +	/**< Offset of the 2nd field of the 2nd couple to be swapped. */
> > +	uint8_t off1_src1;
> > +	/**< Field length of the second couple. */
> > +	uint8_t len1;
> > +};
> 
> I guess it would be easier to understand if
> RTE_ETH_HASH_FILTER_INFO_TYPE_FILTER_SWAP was defined previously.
> 
> --
> Thomas

Regards,
Helin