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

["Xie, Huawei" <huawei.xie@intel.com>]

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