Re: [PATCH v2 01/11] ethdev: add flex item modify field support

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 1/19/23 07:58, Rongwei Liu wrote:
> Add flex item as modify field destination.
> Add "struct rte_flow_item_flex_handle *flex_handle" into
> "struct rte_flow_action_modify_data" as union with existed
> "level" member. This new member is dedicated for modifying
> flex item.
> 
> Add flex item modify field cmdline support. Now user can use
> testpmd cli to specify which flex item to be modified, either
> source or destination.
> 
> Syntax is as below:
> modify_field op set dst_type flex_item dst_level 0
> dst_offset 16 src_type value src_value 0x123456781020 width 8
> 
> Signed-off-by: Rongwei Liu <rongweil@nvidia.com>
> Acked-by: Ori Kam <orika@nvidia.com>

[snip]

> diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst
> index b8c5b68d6c..c673205e5e 100644
> --- a/doc/guides/rel_notes/release_23_03.rst
> +++ b/doc/guides/rel_notes/release_23_03.rst
> @@ -56,6 +56,10 @@ New Features
>        =======================================================
>   
>   

It should be just one empty line here

> +* ethdev: added a new field:

"added a new field' is too generic.

> +
> +  - modify flex item: ``rte_flow_action_modify_data.flex_handle``
> +

And two empty lines here.

>   Removed Items
>   -------------
>   
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index b60987db4b..c66a65351d 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -3528,6 +3528,7 @@ enum rte_flow_field_id {
>   	RTE_FLOW_FIELD_IPV6_ECN,	/**< IPv6 ECN. */
>   	RTE_FLOW_FIELD_GTP_PSC_QFI,	/**< GTP QFI. */
>   	RTE_FLOW_FIELD_METER_COLOR,	/**< Meter color marker. */
> +	RTE_FLOW_FIELD_FLEX_ITEM,	/**< Flex item. */
>   };
>   
>   /**
> @@ -3541,8 +3542,11 @@ struct rte_flow_action_modify_data {
>   	RTE_STD_C11
>   	union {
>   		struct {
> -			/** Encapsulation level or tag index. */
> -			uint32_t level;
> +			/**< Encapsulation level or tag index or flex item handle. */

Have you tried to generate documentation? If it is a union documentation 
it should be /**, not /**<.
In general, it is better to document union from overall
point of view. What is it logically? Do not define union
as just a union of its fields.

> +			union {
> +				uint32_t level;
> +				struct rte_flow_item_flex_handle *flex_handle;

Union items documentation missing.

> +			};
>   			/** Number of bits to skip from a field. */
>   			uint32_t offset;
>   		};