[dpdk-dev] [PATCH v2 0/2] lib/librte_pmd_i40e: set vlan filter fix

[dpdk-dev] [PATCH v2 0/2] lib/librte_pmd_i40e: set vlan filter fix

[Huawei Xie]

This patchset fixes "set vlan filter" issue.

v2 changes:
* add two macros I40E_VFTA_IDX and I40E_VFTA_BIT for VFTA array operation.

Huawei Xie (2):
  vlan id set fix
  add I40E_VFTA_IDX and I40E_VFTA_BIT macros for VFTA related operation

 lib/librte_pmd_i40e/i40e_ethdev.c | 20 ++++++++++----------
 lib/librte_pmd_i40e/i40e_ethdev.h |  9 +++++++++
 2 files changed, 19 insertions(+), 10 deletions(-)

-- 
1.8.1.4

[dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix

[Huawei Xie]

">> 5" rather than ">> 4"

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index 5074262..c0cf3cf 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -4048,14 +4048,11 @@ i40e_set_vlan_filter(struct i40e_vsi *vsi,
 {
 	uint32_t vid_idx, vid_bit;
 
-#define UINT32_BIT_MASK      0x1F
-#define VALID_VLAN_BIT_MASK  0xFFF
 	/* VFTA is 32-bits size array, each element contains 32 vlan bits, Find the
 	 *  element first, then find the bits it belongs to
 	 */
-	vid_idx = (uint32_t) ((vlan_id & VALID_VLAN_BIT_MASK) >>
-		  sizeof(uint32_t));
-	vid_bit = (uint32_t) (1 << (vlan_id & UINT32_BIT_MASK));
+	vid_idx = (uint32_t) ((vlan_id >> 5 ) & 0x7F);
+	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
 
 	if (on)
 		vsi->vfta[vid_idx] |= vid_bit;
-- 
1.8.1.4

[dpdk-dev] [PATCH v2 2/2] lib/librte_pmd_i40e: add I40E_VFTA_IDX and I40E_VFTA_BIT macros for VFTA related operation

[Huawei Xie]

Add two macros I40E_VFTA_IDX and I40E_VFTA_BIT for VFTA manipulation.
Add vlan_id check in vlan filter search and set function.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 17 ++++++++++-------
 lib/librte_pmd_i40e/i40e_ethdev.h |  9 +++++++++
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index c0cf3cf..245460f 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -4033,8 +4033,11 @@ i40e_find_vlan_filter(struct i40e_vsi *vsi,
 {
 	uint32_t vid_idx, vid_bit;
 
-	vid_idx = (uint32_t) ((vlan_id >> 5) & 0x7F);
-	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
+	if (vlan_id > ETH_VLAN_ID_MAX)
+		return 0;
+
+	vid_idx = I40E_VFTA_IDX(vlan_id);
+	vid_bit = I40E_VFTA_BIT(vlan_id);
 
 	if (vsi->vfta[vid_idx] & vid_bit)
 		return 1;
@@ -4048,11 +4051,11 @@ i40e_set_vlan_filter(struct i40e_vsi *vsi,
 {
 	uint32_t vid_idx, vid_bit;
 
-	/* VFTA is 32-bits size array, each element contains 32 vlan bits, Find the
-	 *  element first, then find the bits it belongs to
-	 */
-	vid_idx = (uint32_t) ((vlan_id >> 5 ) & 0x7F);
-	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
+	if (vlan_id > ETH_VLAN_ID_MAX)
+		return;
+
+	vid_idx = I40E_VFTA_IDX(vlan_id);
+	vid_bit = I40E_VFTA_BIT(vlan_id);
 
 	if (on)
 		vsi->vfta[vid_idx] |= vid_bit;
diff --git a/lib/librte_pmd_i40e/i40e_ethdev.h b/lib/librte_pmd_i40e/i40e_ethdev.h
index 96361c2..4f2c16a 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.h
+++ b/lib/librte_pmd_i40e/i40e_ethdev.h
@@ -50,6 +50,15 @@
 #define I40E_DEFAULT_QP_NUM_FDIR  64
 #define I40E_UINT32_BIT_SIZE      (CHAR_BIT * sizeof(uint32_t))
 #define I40E_VFTA_SIZE            (4096 / I40E_UINT32_BIT_SIZE)
+/*
+ * vlan_id is a 12 bit number.
+ * The VFTA array is actually a 4096 bit array, 128 of 32bit elements.
+ * 2^5 = 32. The val of lower 5 bits specifies the bit in the 32bit element.
+ * The higher 7 bit val specifies VFTA array index.
+ */
+#define I40E_VFTA_BIT(vlan_id)    (1 << ((vlan_id) & 0x1F))
+#define I40E_VFTA_IDX(vlan_id)    ((vlan_id) >> 5)
+
 /* Default TC traffic in case DCB is not enabled */
 #define I40E_DEFAULT_TCMAP        0x1
 
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH v2 2/2] lib/librte_pmd_i40e: add I40E_VFTA_IDX and I40E_VFTA_BIT macros for VFTA related operation

[Zhang, Helin]

Hi Huawei

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> Sent: Monday, November 10, 2014 10:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 2/2] lib/librte_pmd_i40e: add I40E_VFTA_IDX and
> I40E_VFTA_BIT macros for VFTA related operation
> 
> Add two macros I40E_VFTA_IDX and I40E_VFTA_BIT for VFTA manipulation.
> Add vlan_id check in vlan filter search and set function.
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> ---
>  lib/librte_pmd_i40e/i40e_ethdev.c | 17 ++++++++++-------
> lib/librte_pmd_i40e/i40e_ethdev.h |  9 +++++++++
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> b/lib/librte_pmd_i40e/i40e_ethdev.c
> index c0cf3cf..245460f 100644
> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> @@ -4033,8 +4033,11 @@ i40e_find_vlan_filter(struct i40e_vsi *vsi,  {
>  	uint32_t vid_idx, vid_bit;
> 
> -	vid_idx = (uint32_t) ((vlan_id >> 5) & 0x7F);
> -	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
> +	if (vlan_id > ETH_VLAN_ID_MAX)
> +		return 0;
> +
> +	vid_idx = I40E_VFTA_IDX(vlan_id);
> +	vid_bit = I40E_VFTA_BIT(vlan_id);
> 
>  	if (vsi->vfta[vid_idx] & vid_bit)
>  		return 1;
> @@ -4048,11 +4051,11 @@ i40e_set_vlan_filter(struct i40e_vsi *vsi,  {
>  	uint32_t vid_idx, vid_bit;
> 
> -	/* VFTA is 32-bits size array, each element contains 32 vlan bits, Find the
> -	 *  element first, then find the bits it belongs to
> -	 */
> -	vid_idx = (uint32_t) ((vlan_id >> 5 ) & 0x7F);
> -	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
> +	if (vlan_id > ETH_VLAN_ID_MAX)
> +		return;
> +
> +	vid_idx = I40E_VFTA_IDX(vlan_id);
> +	vid_bit = I40E_VFTA_BIT(vlan_id);
> 
>  	if (on)
>  		vsi->vfta[vid_idx] |= vid_bit;
> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.h
> b/lib/librte_pmd_i40e/i40e_ethdev.h
> index 96361c2..4f2c16a 100644
> --- a/lib/librte_pmd_i40e/i40e_ethdev.h
> +++ b/lib/librte_pmd_i40e/i40e_ethdev.h
> @@ -50,6 +50,15 @@
>  #define I40E_DEFAULT_QP_NUM_FDIR  64
>  #define I40E_UINT32_BIT_SIZE      (CHAR_BIT * sizeof(uint32_t))
>  #define I40E_VFTA_SIZE            (4096 / I40E_UINT32_BIT_SIZE)
> +/*
> + * vlan_id is a 12 bit number.
> + * The VFTA array is actually a 4096 bit array, 128 of 32bit elements.
> + * 2^5 = 32. The val of lower 5 bits specifies the bit in the 32bit element.
> + * The higher 7 bit val specifies VFTA array index.
> + */
> +#define I40E_VFTA_BIT(vlan_id)    (1 << ((vlan_id) & 0x1F))
> +#define I40E_VFTA_IDX(vlan_id)    ((vlan_id) >> 5)
Why not define the 0x1f and 5 more meaningful in macros?

Why define it in this header file? It seems that only used in i40e_ethdev.c.

> +
>  /* Default TC traffic in case DCB is not enabled */
>  #define I40E_DEFAULT_TCMAP        0x1
> 
> --
> 1.8.1.4

Regards,
Helin

Re: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix

[Zhang, Helin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> Sent: Monday, November 10, 2014 10:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix
> 
> ">> 5" rather than ">> 4"
> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> ---
>  lib/librte_pmd_i40e/i40e_ethdev.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> b/lib/librte_pmd_i40e/i40e_ethdev.c
> index 5074262..c0cf3cf 100644
> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> @@ -4048,14 +4048,11 @@ i40e_set_vlan_filter(struct i40e_vsi *vsi,  {
>  	uint32_t vid_idx, vid_bit;
> 
> -#define UINT32_BIT_MASK      0x1F
> -#define VALID_VLAN_BIT_MASK  0xFFF
>  	/* VFTA is 32-bits size array, each element contains 32 vlan bits, Find the
>  	 *  element first, then find the bits it belongs to
>  	 */
> -	vid_idx = (uint32_t) ((vlan_id & VALID_VLAN_BIT_MASK) >>
> -		  sizeof(uint32_t));
> -	vid_bit = (uint32_t) (1 << (vlan_id & UINT32_BIT_MASK));
> +	vid_idx = (uint32_t) ((vlan_id >> 5 ) & 0x7F);
> +	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
I don't understand why remove macros and use numeric instead?

> 
>  	if (on)
>  		vsi->vfta[vid_idx] |= vid_bit;
> --
> 1.8.1.4

Regards,
Helin

Re: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix

[Xie, Huawei]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Sunday, November 09, 2014 10:09 PM
> To: Xie, Huawei; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> > Sent: Monday, November 10, 2014 10:46 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix
> >
> > ">> 5" rather than ">> 4"
> >
> > Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> > ---
> >  lib/librte_pmd_i40e/i40e_ethdev.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> > b/lib/librte_pmd_i40e/i40e_ethdev.c
> > index 5074262..c0cf3cf 100644
> > --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> > +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> > @@ -4048,14 +4048,11 @@ i40e_set_vlan_filter(struct i40e_vsi *vsi,  {
> >  	uint32_t vid_idx, vid_bit;
> >
> > -#define UINT32_BIT_MASK      0x1F
> > -#define VALID_VLAN_BIT_MASK  0xFFF
> >  	/* VFTA is 32-bits size array, each element contains 32 vlan bits, Find the
> >  	 *  element first, then find the bits it belongs to
> >  	 */
> > -	vid_idx = (uint32_t) ((vlan_id & VALID_VLAN_BIT_MASK) >>
> > -		  sizeof(uint32_t));
> > -	vid_bit = (uint32_t) (1 << (vlan_id & UINT32_BIT_MASK));
> > +	vid_idx = (uint32_t) ((vlan_id >> 5 ) & 0x7F);
> > +	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
> I don't understand why remove macros and use numeric instead?
Those macros are wrongly defined. Correct macros are defined in second patch.
> 
> >
> >  	if (on)
> >  		vsi->vfta[vid_idx] |= vid_bit;
> > --
> > 1.8.1.4
> 
> Regards,
> Helin

Re: [dpdk-dev] [PATCH v2 2/2] lib/librte_pmd_i40e: add I40E_VFTA_IDX and I40E_VFTA_BIT macros for VFTA related operation

[Xie, Huawei]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Sunday, November 09, 2014 10:08 PM
> To: Xie, Huawei; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 2/2] lib/librte_pmd_i40e: add I40E_VFTA_IDX
> and I40E_VFTA_BIT macros for VFTA related operation
> 
> Hi Huawei
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> > Sent: Monday, November 10, 2014 10:46 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2 2/2] lib/librte_pmd_i40e: add I40E_VFTA_IDX
> and
> > I40E_VFTA_BIT macros for VFTA related operation
> >
> > Add two macros I40E_VFTA_IDX and I40E_VFTA_BIT for VFTA manipulation.
> > Add vlan_id check in vlan filter search and set function.
> >
> > Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> > ---
> >  lib/librte_pmd_i40e/i40e_ethdev.c | 17 ++++++++++-------
> > lib/librte_pmd_i40e/i40e_ethdev.h |  9 +++++++++
> >  2 files changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> > b/lib/librte_pmd_i40e/i40e_ethdev.c
> > index c0cf3cf..245460f 100644
> > --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> > +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> > @@ -4033,8 +4033,11 @@ i40e_find_vlan_filter(struct i40e_vsi *vsi,  {
> >  	uint32_t vid_idx, vid_bit;
> >
> > -	vid_idx = (uint32_t) ((vlan_id >> 5) & 0x7F);
> > -	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
> > +	if (vlan_id > ETH_VLAN_ID_MAX)
> > +		return 0;
> > +
> > +	vid_idx = I40E_VFTA_IDX(vlan_id);
> > +	vid_bit = I40E_VFTA_BIT(vlan_id);
> >
> >  	if (vsi->vfta[vid_idx] & vid_bit)
> >  		return 1;
> > @@ -4048,11 +4051,11 @@ i40e_set_vlan_filter(struct i40e_vsi *vsi,  {
> >  	uint32_t vid_idx, vid_bit;
> >
> > -	/* VFTA is 32-bits size array, each element contains 32 vlan bits, Find the
> > -	 *  element first, then find the bits it belongs to
> > -	 */
> > -	vid_idx = (uint32_t) ((vlan_id >> 5 ) & 0x7F);
> > -	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
> > +	if (vlan_id > ETH_VLAN_ID_MAX)
> > +		return;
> > +
> > +	vid_idx = I40E_VFTA_IDX(vlan_id);
> > +	vid_bit = I40E_VFTA_BIT(vlan_id);
> >
> >  	if (on)
> >  		vsi->vfta[vid_idx] |= vid_bit;
> > diff --git a/lib/librte_pmd_i40e/i40e_ethdev.h
> > b/lib/librte_pmd_i40e/i40e_ethdev.h
> > index 96361c2..4f2c16a 100644
> > --- a/lib/librte_pmd_i40e/i40e_ethdev.h
> > +++ b/lib/librte_pmd_i40e/i40e_ethdev.h
> > @@ -50,6 +50,15 @@
> >  #define I40E_DEFAULT_QP_NUM_FDIR  64
> >  #define I40E_UINT32_BIT_SIZE      (CHAR_BIT * sizeof(uint32_t))
> >  #define I40E_VFTA_SIZE            (4096 / I40E_UINT32_BIT_SIZE)
> > +/*
> > + * vlan_id is a 12 bit number.
> > + * The VFTA array is actually a 4096 bit array, 128 of 32bit elements.
> > + * 2^5 = 32. The val of lower 5 bits specifies the bit in the 32bit element.
> > + * The higher 7 bit val specifies VFTA array index.
> > + */
> > +#define I40E_VFTA_BIT(vlan_id)    (1 << ((vlan_id) & 0x1F))
> > +#define I40E_VFTA_IDX(vlan_id)    ((vlan_id) >> 5)
> Why not define the 0x1f and 5 more meaningful in macros?
> 
> Why define it in this header file? It seems that only used in i40e_ethdev.c.
> 
It is a macro for i40e common functionality, as I40E_VFTA_SIZE macro.
> > +
> >  /* Default TC traffic in case DCB is not enabled */
> >  #define I40E_DEFAULT_TCMAP        0x1
> >
> > --
> > 1.8.1.4
> 
> Regards,
> Helin

Re: [dpdk-dev] [PATCH v2 0/2] lib/librte_pmd_i40e: set vlan filter fix

[Chen, Jing D]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> Sent: Monday, November 10, 2014 10:46 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2 0/2] lib/librte_pmd_i40e: set vlan filter fix
> 
> This patchset fixes "set vlan filter" issue.
> 
> v2 changes:
> * add two macros I40E_VFTA_IDX and I40E_VFTA_BIT for VFTA array
> operation.
> 
> Huawei Xie (2):
>   vlan id set fix
>   add I40E_VFTA_IDX and I40E_VFTA_BIT macros for VFTA related operation
> 
>  lib/librte_pmd_i40e/i40e_ethdev.c | 20 ++++++++++----------
>  lib/librte_pmd_i40e/i40e_ethdev.h |  9 +++++++++
>  2 files changed, 19 insertions(+), 10 deletions(-)
> 
> --
> 1.8.1.4

Acked-by : Jing Chen <jing.d.chen@intel.com>

Re: [dpdk-dev] [PATCH v2 0/2] lib/librte_pmd_i40e: set vlan filter fix

[Xie, Huawei]

Hi Thomas:
Do you have issues applying this patch and vhost VMDQ patch?
Without this fix,  the vlan filter set willn't work.

-Huawei

> -----Original Message-----
> From: Chen, Jing D
> Sent: Monday, November 10, 2014 1:26 AM
> To: Xie, Huawei; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v2 0/2] lib/librte_pmd_i40e: set vlan filter fix
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> > Sent: Monday, November 10, 2014 10:46 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2 0/2] lib/librte_pmd_i40e: set vlan filter fix
> >
> > This patchset fixes "set vlan filter" issue.
> >
> > v2 changes:
> > * add two macros I40E_VFTA_IDX and I40E_VFTA_BIT for VFTA array
> > operation.
> >
> > Huawei Xie (2):
> >   vlan id set fix
> >   add I40E_VFTA_IDX and I40E_VFTA_BIT macros for VFTA related operation
> >
> >  lib/librte_pmd_i40e/i40e_ethdev.c | 20 ++++++++++----------
> >  lib/librte_pmd_i40e/i40e_ethdev.h |  9 +++++++++
> >  2 files changed, 19 insertions(+), 10 deletions(-)
> >
> > --
> > 1.8.1.4
> 
> Acked-by : Jing Chen <jing.d.chen@intel.com>

Re: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix

[Qiu, Michael]

On 11/10/2014 2:42 PM, Xie, Huawei wrote:
>
>> -----Original Message-----
>> From: Zhang, Helin
>> Sent: Sunday, November 09, 2014 10:09 PM
>> To: Xie, Huawei; dev@dpdk.org
>> Subject: RE: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix
>>
>>
>>
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
>>> Sent: Monday, November 10, 2014 10:46 AM
>>> To: dev@dpdk.org
>>> Subject: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix
>>>
>>> ">> 5" rather than ">> 4"
>>>
>>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
>>> ---
>>>  lib/librte_pmd_i40e/i40e_ethdev.c | 7 ++-----
>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
>>> b/lib/librte_pmd_i40e/i40e_ethdev.c
>>> index 5074262..c0cf3cf 100644
>>> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
>>> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
>>> @@ -4048,14 +4048,11 @@ i40e_set_vlan_filter(struct i40e_vsi *vsi,  {
>>>  	uint32_t vid_idx, vid_bit;
>>>
>>> -#define UINT32_BIT_MASK      0x1F
>>> -#define VALID_VLAN_BIT_MASK  0xFFF
>>>  	/* VFTA is 32-bits size array, each element contains 32 vlan bits, Find the
>>>  	 *  element first, then find the bits it belongs to
>>>  	 */
>>> -	vid_idx = (uint32_t) ((vlan_id & VALID_VLAN_BIT_MASK) >>
>>> -		  sizeof(uint32_t));
>>> -	vid_bit = (uint32_t) (1 << (vlan_id & UINT32_BIT_MASK));
>>> +	vid_idx = (uint32_t) ((vlan_id >> 5 ) & 0x7F);

                                                                              
^^^^^^^^^^^^
                                                                       
       No need additional space after '5'
>>> +	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
>> I don't understand why remove macros and use numeric instead?
> Those macros are wrongly defined. Correct macros are defined in second patch.

Is there any issue to swap your patch order, and use your defined macros
here? That would be much better if no other issue.

Thanks,
Michael
>>>  	if (on)
>>>  		vsi->vfta[vid_idx] |= vid_bit;
>>> --
>>> 1.8.1.4
>> Regards,
>> Helin

Re: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix

[Xie, Huawei]

> -----Original Message-----
> From: Qiu, Michael
> Sent: Monday, November 24, 2014 1:33 AM
> To: Xie, Huawei; Zhang, Helin; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix
> 
> On 11/10/2014 2:42 PM, Xie, Huawei wrote:
> >
> >> -----Original Message-----
> >> From: Zhang, Helin
> >> Sent: Sunday, November 09, 2014 10:09 PM
> >> To: Xie, Huawei; dev@dpdk.org
> >> Subject: RE: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Huawei Xie
> >>> Sent: Monday, November 10, 2014 10:46 AM
> >>> To: dev@dpdk.org
> >>> Subject: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix
> >>>
> >>> ">> 5" rather than ">> 4"
> >>>
> >>> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> >>> ---
> >>>  lib/librte_pmd_i40e/i40e_ethdev.c | 7 ++-----
> >>>  1 file changed, 2 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> >>> b/lib/librte_pmd_i40e/i40e_ethdev.c
> >>> index 5074262..c0cf3cf 100644
> >>> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> >>> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> >>> @@ -4048,14 +4048,11 @@ i40e_set_vlan_filter(struct i40e_vsi *vsi,  {
> >>>  	uint32_t vid_idx, vid_bit;
> >>>
> >>> -#define UINT32_BIT_MASK      0x1F
> >>> -#define VALID_VLAN_BIT_MASK  0xFFF
> >>>  	/* VFTA is 32-bits size array, each element contains 32 vlan bits, Find the
> >>>  	 *  element first, then find the bits it belongs to
> >>>  	 */
> >>> -	vid_idx = (uint32_t) ((vlan_id & VALID_VLAN_BIT_MASK) >>
> >>> -		  sizeof(uint32_t));
> >>> -	vid_bit = (uint32_t) (1 << (vlan_id & UINT32_BIT_MASK));
> >>> +	vid_idx = (uint32_t) ((vlan_id >> 5 ) & 0x7F);
> 
> 
> ^^^^^^^^^^^^
> 
>        No need additional space after '5'
> >>> +	vid_bit = (uint32_t) (1 << (vlan_id & 0x1F));
> >> I don't understand why remove macros and use numeric instead?
> > Those macros are wrongly defined. Correct macros are defined in second
> patch.
> 
> Is there any issue to swap your patch order, and use your defined macros
> here? That would be much better if no other issue.

The first patch shows clearly what we fixes.
The second patch use MACROs for better code.
> 
> Thanks,
> Michael
> >>>  	if (on)
> >>>  		vsi->vfta[vid_idx] |= vid_bit;
> >>> --
> >>> 1.8.1.4
> >> Regards,
> >> Helin