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

Message ID	1415587563-11512-2-git-send-email-huawei.xie@intel.com (mailing list archive)
State	Changes Requested, archived
Headers	From: Huawei Xie <huawei.xie@intel.com> To: dev@dpdk.org Date: Mon, 10 Nov 2014 10:46:02 +0800 Message-Id: <1415587563-11512-2-git-send-email-huawei.xie@intel.com> In-Reply-To: <1415587563-11512-1-git-send-email-huawei.xie@intel.com> References: <1415587563-11512-1-git-send-email-huawei.xie@intel.com> Subject: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix Precedence: list Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org>

Message ID

1415587563-11512-2-git-send-email-huawei.xie@intel.com (mailing list archive)

State

Changes Requested, archived

Headers

From: Huawei Xie <huawei.xie@intel.com>
To: dev@dpdk.org
Date: Mon, 10 Nov 2014 10:46:02 +0800
Message-Id: <1415587563-11512-2-git-send-email-huawei.xie@intel.com>
In-Reply-To: <1415587563-11512-1-git-send-email-huawei.xie@intel.com>
References: <1415587563-11512-1-git-send-email-huawei.xie@intel.com>
Subject: [dpdk-dev] [PATCH v2 1/2] lib/librte_pmd_i40e: set vlan filter fix
Precedence: list
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Commit Message

Huawei Xie Nov. 10, 2014, 2:46 a.m. UTC

  ">> 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(-)

Comments

Zhang, Helin Nov. 10, 2014, 5:08 a.m. UTC | #1

> -----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

Huawei Xie Nov. 10, 2014, 6:42 a.m. UTC | #2

> -----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

Michael Qiu Nov. 24, 2014, 8:32 a.m. UTC | #3

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

Huawei Xie Nov. 24, 2014, 9:41 p.m. UTC | #4

> -----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

diff mbox

Patch

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;