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

[Rongwei Liu]

Hi Andrew

BR
Rongwei

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
> Sent: Friday, January 20, 2023 17:08
> To: Rongwei Liu <rongweil at nvidia.com>; Matan Azrad <matan at nvidia.com>;
> Slava Ovsiienko <viacheslavo at nvidia.com>; Ori Kam <orika at nvidia.com>;
> NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas at monjalon.net>; Aman
> Singh <aman.deep.singh at intel.com>; Yuying Zhang
> <yuying.zhang at intel.com>; Ferruh Yigit <ferruh.yigit at amd.com>
> Cc: dev at dpdk.org; Raslan Darawsheh <rasland at 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 <rongweil at nvidia.com>
> > Acked-by: Ori Kam <orika at 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.
Added it in the "rte_flow.rst" following ``level`` segment.
> 
> > +                     };
> >                       /** Number of bits to skip from a field. */
> >                       uint32_t offset;
> >               };