[PATCH] ethernet: fm10k: Actually drop 4 bits

[PATCH] ethernet: fm10k: Actually drop 4 bits

[Rasmus Villemoes]

The comment explains the intention, but vid has type u16. Before the
inner shift, it is promoted to int, which has plenty of space for all
vid's bits, so nothing is dropped. Use a simple mask instead.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.c b/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
index 275423d4f777..b1c57d0166a9 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
@@ -335,7 +335,7 @@ static s32 fm10k_update_xc_addr_pf(struct fm10k_hw *hw, u16 glort,
 		return FM10K_ERR_PARAM;
 
 	/* drop upper 4 bits of VLAN ID */
-	vid = (vid << 4) >> 4;
+	vid &= 0x0fff;
 
 	/* record fields */
 	mac_update.mac_lower = cpu_to_le32(((u32)mac[2] << 24) |
-- 
2.1.3

Re: [PATCH] ethernet: fm10k: Actually drop 4 bits

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

On Thu, 2015-01-22 at 23:53 +0100, Rasmus Villemoes wrote:
> The comment explains the intention, but vid has type u16. Before the
> inner shift, it is promoted to int, which has plenty of space for all
> vid's bits, so nothing is dropped. Use a simple mask instead.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks Rasmus, I will add your patch to my queue.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] ethernet: fm10k: Actually drop 4 bits

[Vick, Matthew]

On 1/22/15, 2:53 PM, "Rasmus Villemoes" <linux@rasmusvillemoes.dk> wrote:

>The comment explains the intention, but vid has type u16. Before the
>inner shift, it is promoted to int, which has plenty of space for all
>vid's bits, so nothing is dropped. Use a simple mask instead.
>
>Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>---
> drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
>b/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
>index 275423d4f777..b1c57d0166a9 100644
>--- a/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
>+++ b/drivers/net/ethernet/intel/fm10k/fm10k_pf.c
>@@ -335,7 +335,7 @@ static s32 fm10k_update_xc_addr_pf(struct fm10k_hw
>*hw, u16 glort,
> 		return FM10K_ERR_PARAM;
> 
> 	/* drop upper 4 bits of VLAN ID */
>-	vid = (vid << 4) >> 4;
>+	vid &= 0x0fff;
> 
> 	/* record fields */
> 	mac_update.mac_lower = cpu_to_le32(((u32)mac[2] << 24) |

Good catch! I noticed this too and was getting a patch together to address
this.

The difference is that I was planning on not silently accepting an invalid
VLAN ID to begin with and returning FM10K_ERR_PARAM if the VLAN was
invalid, which I think is a better approach for this situation. If it's
alright with you, I'll generate the patch shortly and credit you via your
Reported-by.

Cheers,
Matthew

Re: [PATCH] ethernet: fm10k: Actually drop 4 bits

[Rasmus Villemoes]

On Sat, Jan 24 2015, "Vick, Matthew" <matthew.vick@intel.com> wrote:

> Good catch! I noticed this too and was getting a patch together to address
> this.
>
> The difference is that I was planning on not silently accepting an invalid
> VLAN ID to begin with and returning FM10K_ERR_PARAM if the VLAN was
> invalid, which I think is a better approach for this situation. If it's
> alright with you, I'll generate the patch shortly and credit you via your
> Reported-by.

Sure, do what you think is best.

Rasmus

Re: [PATCH] ethernet: fm10k: Actually drop 4 bits

[Vick, Matthew]

On 1/23/15, 4:50 PM, "Rasmus Villemoes" <linux@rasmusvillemoes.dk> wrote:

>On Sat, Jan 24 2015, "Vick, Matthew" <matthew.vick@intel.com> wrote:
>
>> Good catch! I noticed this too and was getting a patch together to
>>address
>> this.
>>
>> The difference is that I was planning on not silently accepting an
>>invalid
>> VLAN ID to begin with and returning FM10K_ERR_PARAM if the VLAN was
>> invalid, which I think is a better approach for this situation. If it's
>> alright with you, I'll generate the patch shortly and credit you via
>>your
>> Reported-by.
>
>Sure, do what you think is best.
>
>Rasmus

Sounds good. I'll get the patch created and validated through Jeff's tree.

Cheers,
Matthew