> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, March 8, 2017 11:50 PM
> To: Xing, Beilei <beilei.x...@intel.com>; Wu, Jingjing <jingjing...@intel.com>
> Cc: Zhang, Helin <helin.zh...@intel.com>; dev@dpdk.org; Iremonger,
> Bernard <bernard.iremon...@intel.com>; Stroe, Laura
> <laura.st...@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 1/4] net/i40e: support replace filter type
>
> On 3/3/2017 9:31 AM, Beilei Xing wrote:
> > Add new admin queue function and extended fields in DCR 288:
> > - Add admin queue function for Replace filter
> > command (Opcode: 0x025F)
> > - Add General fields for Add/Remove Cloud filters
> > command
> >
> > This patch will be removed to base driver in future.
> >
> > Signed-off-by: Bernard Iremonger <bernard.iremon...@intel.com>
> > Signed-off-by: Stroe Laura <laura.st...@intel.com>
> > Signed-off-by: Jingjing Wu <jingjing...@intel.com>
> > Signed-off-by: Beilei Xing <beilei.x...@intel.com>
> > ---
> > drivers/net/i40e/i40e_ethdev.h | 106 ++++++++++++++++++++++++++++
> > drivers/net/i40e/i40e_flow.c | 152
> +++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 258 insertions(+)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.h
> > b/drivers/net/i40e/i40e_ethdev.h index f545850..3a49865 100644
> > --- a/drivers/net/i40e/i40e_ethdev.h
> > +++ b/drivers/net/i40e/i40e_ethdev.h
> > @@ -729,6 +729,100 @@ struct i40e_valid_pattern {
> > parse_filter_t parse_filter;
> > };
> >
> > +/* Support replace filter */
> > +
> > +/* i40e_aqc_add_remove_cloud_filters_element_big_data is used when
> > + * I40E_AQC_ADD_REM_CLOUD_CMD_BIG_BUFFER flag is set. refer to
> > + * DCR288
>
> Please do not refer to DCR, unless you can provide a public link for it.
OK, got it.
>
> > + */
> > +struct i40e_aqc_add_remove_cloud_filters_element_big_data {
> > + struct i40e_aqc_add_remove_cloud_filters_element_data element;
>
> What is the difference between
> "i40e_aqc_add_remove_cloud_filters_element_big_data" and
> "i40e_aqc_add_remove_cloud_filters_element_data", why need big_data
> one?
As ' Add/Remove Cloud filters -command buffer ' is changed in the DCR288,
'general fields' exists only when big_buffer is set.
But we don't want to change the "
i40e_aqc_add_remove_cloud_filters_element_data " as it will cause ABI/API
change in kernel driver.
>
> > + uint16_t general_fields[32];
>
> Not very useful variable name.
It's the name from DCR.
>
> <...>
>
> > +/* Replace filter Command 0x025F
> > + * uses the i40e_aqc_replace_cloud_filters,
> > + * and the generic indirect completion structure */ struct
> > +i40e_filter_data {
> > + uint8_t filter_type;
> > + uint8_t input[3];
> > +};
> > +
> > +struct i40e_aqc_replace_cloud_filters_cmd {
>
> Is replace does something different than remove old and add new cloud
> filter?
It's just like remove an old filter and add a new filter.
It can replace both l1 filter and cloud filter.
>
> <...>
>
> > +enum i40e_status_code i40e_aq_add_cloud_filters_big_buffer(struct
> i40e_hw *hw,
> > + uint16_t seid,
> > + struct i40e_aqc_add_remove_cloud_filters_element_big_data
> *filters,
> > + uint8_t filter_count);
> > +enum i40e_status_code i40e_aq_remove_cloud_filters_big_buffer(
> > + struct i40e_hw *hw, uint16_t seid,
> > + struct i40e_aqc_add_remove_cloud_filters_element_big_data
> *filters,
> > + uint8_t filter_count);
> > +enum i40e_status_code i40e_aq_replace_cloud_filters(struct i40e_hw
> *hw,
> > + struct i40e_aqc_replace_cloud_filters_cmd *filters,
> > + struct i40e_aqc_replace_cloud_filters_cmd_buf
> *cmd_buf);
> > +
>
> Do you need these function declarations?
We can remove it if we define them with "static".
>
> > #define I40E_DEV_TO_PCI(eth_dev) \
> > RTE_DEV_TO_PCI((eth_dev)->device)
> >
> > diff --git a/drivers/net/i40e/i40e_flow.c
> > b/drivers/net/i40e/i40e_flow.c index f163ce5..3c49228 100644
> > --- a/drivers/net/i40e/i40e_flow.c
> > +++ b/drivers/net/i40e/i40e_flow.c
> > @@ -1874,3 +1874,155 @@ i40e_flow_flush_tunnel_filter(struct i40e_pf
> > *pf)
> >
> > return ret;
> > }
> > +
> > +#define i40e_aqc_opc_replace_cloud_filters 0x025F #define
> > +I40E_AQC_ADD_REM_CLOUD_CMD_BIG_BUFFER 1
> > +/**
> > + * i40e_aq_add_cloud_filters_big_buffer
> > + * @hw: pointer to the hardware structure
> > + * @seid: VSI seid to add cloud filters from
> > + * @filters: Buffer which contains the filters in big buffer to be
> > +added
> > + * @filter_count: number of filters contained in the buffer
> > + *
> > + * Set the cloud filters for a given VSI. The contents of the
> > + * i40e_aqc_add_remove_cloud_filters_element_big_data are filled
> > + * in by the caller of the function.
> > + *
> > + **/
> > +enum i40e_status_code i40e_aq_add_cloud_filters_big_buffer(
>
> There are already non big_buffer versions of these functions, like
> "i40e_aq_add_cloud_filters()" why big_data version required, what it does
> differently?
Parameters are different.
We add i40e_aq_add_cloud_filters_big_buffer to handle structure "
i40e_aqc_add_remove_cloud_filters_element_data " which includes general_fields.
>
> And is there a reason that these functions are not static? (For this patch
> they
> are not used at all and will cause build error, but my question is after they
> started to be used)
No.. same with the patch for Pipeline Personalization Profile, it's designed
according to base code style.
>
> <...>
