Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Monday, April 19, 2021 10:06 PM
> To: Bing Zhao <bi...@nvidia.com>
> Cc: Ori Kam <or...@nvidia.com>; ferruh.yi...@intel.com;
> andrew.rybche...@oktetlabs.ru; dev@dpdk.org;
> ajit.khapa...@broadcom.com; xiaoyun...@intel.com
> Subject: Re: [dpdk-dev] [PATCH v3 1/3] ethdev: introduce conntrack
> flow action and item
>
> External email: Use caution opening links or attachments
>
>
> 16/04/2021 19:54, Bing Zhao:
> > +/**
> > + * The packet is valid after conntrack checking.
> > + */
> > +#define RTE_FLOW_CONNTRACK_PKT_STATE_VALID RTE_BIT32(0)
> > +/**
> > + * The state of the connection is changed.
> > + */
> > +#define RTE_FLOW_CONNTRACK_PKT_STATE_CHANGED RTE_BIT32(1)
> > +/**
> > + * Error is detected on this packet for this connection and
> > + * an invalid state is set.
> > + */
> > +#define RTE_FLOW_CONNTRACK_PKT_STATE_INVALID RTE_BIT32(2)
> > +/**
> > + * The HW connection tracking module is disabled.
> > + * It can be due to application command or an invalid state.
> > + */
> > +#define RTE_FLOW_CONNTRACK_PKT_STATE_DISABLED RTE_BIT32(3)
> > +/**
> > + * The packet contains some bad field(s) and cannot continue
> > + * with the conntrack module checking.
> > + */
> > +#define RTE_FLOW_CONNTRACK_PKT_STATE_BAD RTE_BIT32(4)
>
> I like it better now that all bits have the same prefix, thanks.
>
> > +enum rte_flow_conntrack_state {
> > + /**< SYN-ACK packet was seen. */
> > + RTE_FLOW_CONNTRACK_STATE_SYN_RECV,
> > + /**< 3-way handshake was done. */
> > + RTE_FLOW_CONNTRACK_STATE_ESTABLISHED,
> > + /**< First FIN packet was received to close the connection.
> */
> > + RTE_FLOW_CONNTRACK_STATE_FIN_WAIT,
> > + /**< First FIN was ACKed. */
> > + RTE_FLOW_CONNTRACK_STATE_CLOSE_WAIT,
> > + /**< Second FIN was received, waiting for the last ACK. */
> > + RTE_FLOW_CONNTRACK_STATE_LAST_ACK,
> > + /**< Second FIN was ACKed, connection was closed. */
> > + RTE_FLOW_CONNTRACK_STATE_TIME_WAIT,
> > +};
>
> These doxygen comments should not have "<" in them, because they are
> "before".
All "<" are removed, thanks.
>
> [...]
> > + /** No TCP check will be done except the state change. */
> > + uint32_t liberal_mode:1;
> > + /**<The current state of this connection. */
>
> s,/**<,/** ,
>
> > + enum rte_flow_conntrack_state state;
>
> Looks good overrall, thanks.
>
BR. Bing
