Hi Andrew
BR
Rongwei
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> Sent: Friday, January 20, 2023 17:08
> To: Rongwei Liu <rongw...@nvidia.com>; Matan Azrad <ma...@nvidia.com>;
> Slava Ovsiienko <viachesl...@nvidia.com>; Ori Kam <or...@nvidia.com>;
> NBU-Contact-Thomas Monjalon (EXTERNAL) <tho...@monjalon.net>; Aman
> Singh <aman.deep.si...@intel.com>; Yuying Zhang
> <yuying.zh...@intel.com>; Ferruh Yigit <ferruh.yi...@amd.com>
> Cc: dev@dpdk.org; Raslan Darawsheh <rasl...@nvidia.com>
> Subject: Re: [PATCH v2 01/11] ethdev: add flex item modify field support
>
> External email: Use caution opening links or attachments
>
>
> 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 <rongw...@nvidia.com>
> > Acked-by: Ori Kam <or...@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
>
Sure.
> > +* 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.
>
Sure.
> > 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 /**<.
Sure. Sorry, I followed wrong existed examples.
> 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.
Currently, 'flex_handle' is documents in rte_flow.rst file " table::
destination/source field definition" as a new row.
From API aspect, when modifying flex item, user should specify the pointer of
the flex item instead of ID.
That' why it was added as a union.
>
> > + union {
> > + uint32_t level;
> > + struct rte_flow_item_flex_handle
> > + *flex_handle;
>
> Union items documentation missing.
See above. Do we need another place to document the union again?
>
> > + };
> > /** Number of bits to skip from a field. */
> > uint32_t offset;
> > };
