Re: [dpdk-dev] [PATCH 1/4] net/i40e: support replace filter type

["Xing, Beilei" <beilei.xing@intel.com>]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Wednesday, March 8, 2017 11:50 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: Zhang, Helin <helin.zhang@intel.com>; dev@dpdk.org; Iremonger,
> Bernard <bernard.iremonger@intel.com>; Stroe, Laura
> <laura.stroe@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.iremonger@intel.com>
> > Signed-off-by: Stroe Laura <laura.stroe@intel.com>
> > Signed-off-by: Jingjing Wu <jingjing.wu@intel.com>
> > Signed-off-by: Beilei Xing <beilei.xing@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.

> 
> <...>