[dpdk-dev] [PATCH] i40e: Fix a vlan bug

[dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Chen Jing D(Mark)]

From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>

i40e uses an bitmap array to store those vlan tags that are set by
application. In function i40e_set_vlan_filter, it stores vlan tag
to wrong place. This change will fix it.

Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index 87e750a..cebc21d 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -4163,8 +4163,8 @@ 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));
+	vid_idx = (uint32_t)(vlan_id / I40E_UINT32_BIT_SIZE);
+	vid_bit = (uint32_t)(1 << (vlan_id % I40E_UINT32_BIT_SIZE));
 
 	if (vsi->vfta[vid_idx] & vid_bit)
 		return 1;
@@ -4178,14 +4178,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 / I40E_UINT32_BIT_SIZE);
+	vid_bit = (uint32_t)(1 << (vlan_id % I40E_UINT32_BIT_SIZE));
 
 	if (on)
 		vsi->vfta[vid_idx] |= vid_bit;
-- 
1.7.7.6

Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Qiu, Michael]

Hi Mark,

I think Huawei (huawei.xie@intel.com) has one patch set to fix this issue.

If your patch is totally different with him:

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

please ignore my comments :)

But you both calculation are different.

Thanks,
Michael
On 12/4/2014 5:51 PM, Chen Jing D(Mark) wrote:
> From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>
>
> i40e uses an bitmap array to store those vlan tags that are set by
> application. In function i40e_set_vlan_filter, it stores vlan tag
> to wrong place. This change will fix it.
>
> Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
> ---
>  lib/librte_pmd_i40e/i40e_ethdev.c |   11 ++++-------
>  1 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
> index 87e750a..cebc21d 100644
> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> @@ -4163,8 +4163,8 @@ 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));
> +	vid_idx = (uint32_t)(vlan_id / I40E_UINT32_BIT_SIZE);
> +	vid_bit = (uint32_t)(1 << (vlan_id % I40E_UINT32_BIT_SIZE));
>  
>  	if (vsi->vfta[vid_idx] & vid_bit)
>  		return 1;
> @@ -4178,14 +4178,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 / I40E_UINT32_BIT_SIZE);
> +	vid_bit = (uint32_t)(1 << (vlan_id % I40E_UINT32_BIT_SIZE));
>  
>  	if (on)
>  		vsi->vfta[vid_idx] |= vid_bit;

Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Chen, Jing D]

Yes, the same to his. As he is in vacation, I'd like to send out patch for him.

> -----Original Message-----
> From: Qiu, Michael
> Sent: Thursday, December 04, 2014 6:19 PM
> To: Chen, Jing D; dev@dpdk.org
> Cc: Xie, Huawei
> Subject: Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug
> 
> Hi Mark,
> 
> I think Huawei (huawei.xie@intel.com) has one patch set to fix this issue.
> 
> If your patch is totally different with him:
> 
> [dpdk-dev] [PATCH v4 0/2] lib/librte_pmd_i40e: set vlan filter fix
> 
> please ignore my comments :)
> 
> But you both calculation are different.
> 
> Thanks,
> Michael
> On 12/4/2014 5:51 PM, Chen Jing D(Mark) wrote:
> > From: "Chen Jing D(Mark)" <jing.d.chen@intel.com>
> >
> > i40e uses an bitmap array to store those vlan tags that are set by
> > application. In function i40e_set_vlan_filter, it stores vlan tag
> > to wrong place. This change will fix it.
> >
> > Signed-off-by: Chen Jing D(Mark) <jing.d.chen@intel.com>
> > ---
> >  lib/librte_pmd_i40e/i40e_ethdev.c |   11 ++++-------
> >  1 files changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> b/lib/librte_pmd_i40e/i40e_ethdev.c
> > index 87e750a..cebc21d 100644
> > --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> > +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> > @@ -4163,8 +4163,8 @@ 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));
> > +	vid_idx = (uint32_t)(vlan_id / I40E_UINT32_BIT_SIZE);
> > +	vid_bit = (uint32_t)(1 << (vlan_id % I40E_UINT32_BIT_SIZE));
> >
> >  	if (vsi->vfta[vid_idx] & vid_bit)
> >  		return 1;
> > @@ -4178,14 +4178,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 / I40E_UINT32_BIT_SIZE);
> > +	vid_bit = (uint32_t)(1 << (vlan_id % I40E_UINT32_BIT_SIZE));
> >
> >  	if (on)
> >  		vsi->vfta[vid_idx] |= vid_bit;

Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Thomas Monjalon]

2014-12-04 10:18, Qiu, Michael:
> Hi Mark,
> 
> I think Huawei (huawei.xie@intel.com) has one patch set to fix this issue.
> 
> If your patch is totally different with him:
> 
> [dpdk-dev] [PATCH v4 0/2] lib/librte_pmd_i40e: set vlan filter fix
> 
> please ignore my comments :)
> 
> But you both calculation are different.

Yes, please Jing (Mark), if you reworked the v4 patch, it would clearer
to have a changelog, to name it v5 and to insert it in the previous
thread with --in-reply-to.
At the moment, both patches block each other.

-- 
Thomas

Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Chen, Jing D]

As I don't know what commit he is based on, I'd like to generate a new patch with latest dpdk repo.

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, December 04, 2014 6:26 PM
> To: Chen, Jing D
> Cc: dev@dpdk.org; Qiu, Michael
> Subject: Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug
> 
> 2014-12-04 10:18, Qiu, Michael:
> > Hi Mark,
> >
> > I think Huawei (huawei.xie@intel.com) has one patch set to fix this issue.
> >
> > If your patch is totally different with him:
> >
> > [dpdk-dev] [PATCH v4 0/2] lib/librte_pmd_i40e: set vlan filter fix
> >
> > please ignore my comments :)
> >
> > But you both calculation are different.
> 
> Yes, please Jing (Mark), if you reworked the v4 patch, it would clearer
> to have a changelog, to name it v5 and to insert it in the previous
> thread with --in-reply-to.
> At the moment, both patches block each other.
> 
> --
> Thomas

Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Thomas Monjalon]

2014-12-04 10:30, Chen, Jing D:
> As I don't know what commit he is based on, I'd like to generate a new patch with latest dpdk repo.

There's something wrong here. You rework a patch and you don't know
what was the current status but you expect that the reviewers can understand
it better than you?
You are breaking all the elementary rules of patch management.
We have currently 2 fixes pending for the same bug. 

PS: please don't top post.

-- 
Thomas

> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Thursday, December 04, 2014 6:26 PM
> > To: Chen, Jing D
> > Cc: dev@dpdk.org; Qiu, Michael
> > Subject: Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug
> > 
> > 2014-12-04 10:18, Qiu, Michael:
> > > Hi Mark,
> > >
> > > I think Huawei (huawei.xie@intel.com) has one patch set to fix this issue.
> > >
> > > If your patch is totally different with him:
> > >
> > > [dpdk-dev] [PATCH v4 0/2] lib/librte_pmd_i40e: set vlan filter fix
> > >
> > > please ignore my comments :)
> > >
> > > But you both calculation are different.
> > 
> > Yes, please Jing (Mark), if you reworked the v4 patch, it would clearer
> > to have a changelog, to name it v5 and to insert it in the previous
> > thread with --in-reply-to.
> > At the moment, both patches block each other.
> > 
> > --
> > Thomas

Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Chen, Jing D]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, December 4, 2014 6:39 PM
> To: Chen, Jing D
> Cc: dev@dpdk.org; Qiu, Michael
> Subject: Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug
> 
> 2014-12-04 10:30, Chen, Jing D:
> > As I don't know what commit he is based on, I'd like to generate a new
> patch with latest dpdk repo.
> 
> There's something wrong here. You rework a patch and you don't know what
> was the current status but you expect that the reviewers can understand it
> better than you?

You don't understand me. Please read my above words again.
As I said, he is in vacation, I came to fix problem. I know exactly what's the problem. So, I used simple way. 

> You are breaking all the elementary rules of patch management.

Please kindly list all the elementary rules of patch management, please. If possible, can you post it somewhere so other new guys can find and follow?

> We have currently 2 fixes pending for the same bug.
> 
> PS: please don't top post.

I apologized for top post. 

> 
> --
> Thomas
> 
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Sent: Thursday, December 04, 2014 6:26 PM
> > > To: Chen, Jing D
> > > Cc: dev@dpdk.org; Qiu, Michael
> > > Subject: Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug
> > >
> > > 2014-12-04 10:18, Qiu, Michael:
> > > > Hi Mark,
> > > >
> > > > I think Huawei (huawei.xie@intel.com) has one patch set to fix this
> issue.
> > > >
> > > > If your patch is totally different with him:
> > > >
> > > > [dpdk-dev] [PATCH v4 0/2] lib/librte_pmd_i40e: set vlan filter fix
> > > >
> > > > please ignore my comments :)
> > > >
> > > > But you both calculation are different.
> > >
> > > Yes, please Jing (Mark), if you reworked the v4 patch, it would
> > > clearer to have a changelog, to name it v5 and to insert it in the
> > > previous thread with --in-reply-to.
> > > At the moment, both patches block each other.
> > >
> > > --
> > > Thomas

Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Thomas Monjalon]

2014-12-04 14:29, Chen, Jing D:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2014-12-04 10:30, Chen, Jing D:
> > > As I don't know what commit he is based on, I'd like to generate a new
> > patch with latest dpdk repo.
> > 
> > There's something wrong here. You rework a patch and you don't know what
> > was the current status but you expect that the reviewers can understand it
> > better than you?
> 
> You don't understand me. Please read my above words again.

Yes there probably is a misunderstanding.

> As I said, he is in vacation, I came to fix problem. I know exactly what's the problem. So, I used simple way.

So Huawei was trying to fix the bug and you suggest another way to fix it.
But you didn't explain why your fix is better than the previous one.
And we don't know if it's the continuation of his work or not.
If you are trying to fix exactly the same problem, incrementing the version
number of the patch makes clear that previous version doesn't need to be
reviewed, reworked or applied. In patchwork language, it supersedes the
previous patch which won't appear anymore.

> > You are breaking all the elementary rules of patch management.
> 
> Please kindly list all the elementary rules of patch management, please.
> If possible, can you post it somewhere so other new guys can find and follow?

They are explained in http://dpdk.org/dev#send.
That's the ones I've enumerated in my first email:
- changelog
- increment version number (v5 here)
- use --in-reply-to

> > We have currently 2 fixes pending for the same bug.

To sum it up, we need:
1) a review
2) an agreement that the Huawei's fix is superseded by this one

Thank you
-- 
Thomas

> > PS: please don't top post.
> 
> I apologized for top post. 
> 
> > 
> > --
> > Thomas
> > 
> > > > -----Original Message-----
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > Sent: Thursday, December 04, 2014 6:26 PM
> > > > To: Chen, Jing D
> > > > Cc: dev@dpdk.org; Qiu, Michael
> > > > Subject: Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug
> > > >
> > > > 2014-12-04 10:18, Qiu, Michael:
> > > > > Hi Mark,
> > > > >
> > > > > I think Huawei (huawei.xie@intel.com) has one patch set to fix this
> > issue.
> > > > >
> > > > > If your patch is totally different with him:
> > > > >
> > > > > [dpdk-dev] [PATCH v4 0/2] lib/librte_pmd_i40e: set vlan filter fix
> > > > >
> > > > > please ignore my comments :)
> > > > >
> > > > > But you both calculation are different.
> > > >
> > > > Yes, please Jing (Mark), if you reworked the v4 patch, it would
> > > > clearer to have a changelog, to name it v5 and to insert it in the
> > > > previous thread with --in-reply-to.
> > > > At the moment, both patches block each other.
> > > >
> > > > --
> > > > Thomas
> 

Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Xie, Huawei]

Hi Thomas:
I will continue work on this fix.
Do you have comments to the v4 patch?
For Bruce's comment, I add some descriptive commit message for the commit.
For the constant number, I define a macro as the wrapper for the VFA array index and value.

One question is it isn't based on latest commit, but I tried applying the patch, there is no problem.
+/*
+ * 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)

Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Thomas Monjalon]

Hi Huawei,

2014-12-05 04:56, Xie, Huawei:
> Hi Thomas:
> I will continue work on this fix.
> Do you have comments to the v4 patch?
> For Bruce's comment, I add some descriptive commit message for the commit.
> For the constant number, I define a macro as the wrapper for the VFA array index and value.
> 
> One question is it isn't based on latest commit, but I tried applying the patch, there is no problem.
> +/*
> + * 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)

If you replace the values by constants, it's ok for me.
Note that I won't check the i40e datasheet so you need a reviewer
who will do :)

-- 
Thomas

Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug

[Chen, Jing D]

Hi,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Thursday, December 4, 2014 11:33 PM
> To: Chen, Jing D
> Cc: dev@dpdk.org; Qiu, Michael
> Subject: Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug
> 
> 2014-12-04 14:29, Chen, Jing D:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2014-12-04 10:30, Chen, Jing D:
> > > > As I don't know what commit he is based on, I'd like to generate a
> > > > new
> > > patch with latest dpdk repo.
> > >
> > > There's something wrong here. You rework a patch and you don't know
> > > what was the current status but you expect that the reviewers can
> > > understand it better than you?
> >
> > You don't understand me. Please read my above words again.
> 
> Yes there probably is a misunderstanding.
> 
> > As I said, he is in vacation, I came to fix problem. I know exactly what's the
> problem. So, I used simple way.
> 
> So Huawei was trying to fix the bug and you suggest another way to fix it.
> But you didn't explain why your fix is better than the previous one.
> And we don't know if it's the continuation of his work or not.
> If you are trying to fix exactly the same problem, incrementing the version
> number of the patch makes clear that previous version doesn't need to be
> reviewed, reworked or applied. In patchwork language, it supersedes the
> previous patch which won't appear anymore.
> 

OK, I prefer to follow Huawei's patch set and drop my commit.

> > > You are breaking all the elementary rules of patch management.
> >
> > Please kindly list all the elementary rules of patch management, please.
> > If possible, can you post it somewhere so other new guys can find and
> follow?
> 
> They are explained in http://dpdk.org/dev#send.
> That's the ones I've enumerated in my first email:
> - changelog
> - increment version number (v5 here)
> - use --in-reply-to
>

Thanks for explanation. 
 
> > > We have currently 2 fixes pending for the same bug.
> 
> To sum it up, we need:
> 1) a review
> 2) an agreement that the Huawei's fix is superseded by this one
> 
> Thank you
> --
> Thomas
> 
> > > PS: please don't top post.
> >
> > I apologized for top post.
> >
> > >
> > > --
> > > Thomas
> > >
> > > > > -----Original Message-----
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > Sent: Thursday, December 04, 2014 6:26 PM
> > > > > To: Chen, Jing D
> > > > > Cc: dev@dpdk.org; Qiu, Michael
> > > > > Subject: Re: [dpdk-dev] [PATCH] i40e: Fix a vlan bug
> > > > >
> > > > > 2014-12-04 10:18, Qiu, Michael:
> > > > > > Hi Mark,
> > > > > >
> > > > > > I think Huawei (huawei.xie@intel.com) has one patch set to fix
> > > > > > this
> > > issue.
> > > > > >
> > > > > > If your patch is totally different with him:
> > > > > >
> > > > > > [dpdk-dev] [PATCH v4 0/2] lib/librte_pmd_i40e: set vlan filter
> > > > > > fix
> > > > > >
> > > > > > please ignore my comments :)
> > > > > >
> > > > > > But you both calculation are different.
> > > > >
> > > > > Yes, please Jing (Mark), if you reworked the v4 patch, it would
> > > > > clearer to have a changelog, to name it v5 and to insert it in
> > > > > the previous thread with --in-reply-to.
> > > > > At the moment, both patches block each other.
> > > > >
> > > > > --
> > > > > Thomas
> >