Message ID | 1417686605-6778-1-git-send-email-jing.d.chen@intel.com (mailing list archive) |
---|---|
State | Rejected, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 6B1438032; Thu, 4 Dec 2014 10:50:16 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 058667E75 for <dev@dpdk.org>; Thu, 4 Dec 2014 10:50:13 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP; 04 Dec 2014 01:50:12 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,862,1389772800"; d="scan'208";a="424962678" Received: from shvmail01.sh.intel.com ([10.239.29.42]) by FMSMGA003.fm.intel.com with ESMTP; 04 Dec 2014 01:39:53 -0800 Received: from shecgisg003.sh.intel.com (shecgisg003.sh.intel.com [10.239.29.90]) by shvmail01.sh.intel.com with ESMTP id sB49o8ob021598; Thu, 4 Dec 2014 17:50:09 +0800 Received: from shecgisg003.sh.intel.com (localhost [127.0.0.1]) by shecgisg003.sh.intel.com (8.13.6/8.13.6/SuSE Linux 0.8) with ESMTP id sB49o67B006815; Thu, 4 Dec 2014 17:50:08 +0800 Received: (from jingche2@localhost) by shecgisg003.sh.intel.com (8.13.6/8.13.6/Submit) id sB49o6P9006811; Thu, 4 Dec 2014 17:50:06 +0800 From: "Chen Jing D(Mark)" <jing.d.chen@intel.com> To: dev@dpdk.org Date: Thu, 4 Dec 2014 17:50:05 +0800 Message-Id: <1417686605-6778-1-git-send-email-jing.d.chen@intel.com> X-Mailer: git-send-email 1.7.12.2 Subject: [dpdk-dev] [PATCH] i40e: Fix a vlan bug X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Chen, Jing D
Dec. 4, 2014, 9:50 a.m. UTC
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(-)
Comments
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;
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;
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.
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
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.
> -----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
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
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)
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 > >
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 :)
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;