diff mbox

net/mlx4: && vs & typo

Message ID 20170228120215.GA27947@mwanda (mailing list archive)
State Not Applicable
Headers show

Commit Message

Dan Carpenter Feb. 28, 2017, 12:02 p.m. UTC 
Bitwise & was obviously intended here.

Fixes: 745d8ae4622c ("net/mlx4: Spoofcheck and zero MAC can't coexist")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Applies to net.git.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bart Van Assche Feb. 28, 2017, 3:35 p.m. UTC | #1 
On Tue, 2017-02-28 at 15:02 +0300, Dan Carpenter wrote:
> Bitwise & was obviously intended here.

> 

> Fixes: 745d8ae4622c ("net/mlx4: Spoofcheck and zero MAC can't coexist")

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

> ---

> Applies to net.git.

> 

> diff --git a/include/linux/mlx4/driver.h b/include/linux/mlx4/driver.h

> index e965e5090d96..a858bcb6220b 100644

> --- a/include/linux/mlx4/driver.h

> +++ b/include/linux/mlx4/driver.h

> @@ -109,7 +109,7 @@ static inline void mlx4_u64_to_mac(u8 *addr, u64 mac)

>  	int i;

>  

>  	for (i = ETH_ALEN; i > 0; i--) {

> -		addr[i - 1] = mac && 0xFF;

> +		addr[i - 1] = mac & 0xFF;

>  		mac >>= 8;

>  	}

>  }


Is this the only place where such a loop occurs? Should a put_unaligned_be48()
function be introduced?

Bart.
Tariq Toukan Feb. 28, 2017, 4:53 p.m. UTC | #2 
On 28/02/2017 2:02 PM, Dan Carpenter wrote:
> Bitwise & was obviously intended here.

Sure! Thanks for your patch.

>
> Fixes: 745d8ae4622c ("net/mlx4: Spoofcheck and zero MAC can't coexist")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> Applies to net.git.
>
> diff --git a/include/linux/mlx4/driver.h b/include/linux/mlx4/driver.h
> index e965e5090d96..a858bcb6220b 100644
> --- a/include/linux/mlx4/driver.h
> +++ b/include/linux/mlx4/driver.h
> @@ -109,7 +109,7 @@ static inline void mlx4_u64_to_mac(u8 *addr, u64 mac)
>  	int i;
>
>  	for (i = ETH_ALEN; i > 0; i--) {
> -		addr[i - 1] = mac && 0xFF;
> +		addr[i - 1] = mac & 0xFF;
>  		mac >>= 8;
>  	}
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Reviewed-by: Tariq Toukan <tariqt@mellanox.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches Feb. 28, 2017, 10:23 p.m. UTC | #3 
On Tue, 2017-02-28 at 15:35 +0000, Bart Van Assche wrote:
> On Tue, 2017-02-28 at 15:02 +0300, Dan Carpenter wrote:
> > Bitwise & was obviously intended here.
[]
> > diff --git a/include/linux/mlx4/driver.h b/include/linux/mlx4/driver.h
[]
> > @@ -109,7 +109,7 @@ static inline void 	(u8 *addr, u64 mac)
> >  	int i;
> >  
> >  	for (i = ETH_ALEN; i > 0; i--) {
> > -		addr[i - 1] = mac && 0xFF;
> > +		addr[i - 1] = mac & 0xFF;
> >  		mac >>= 8;
> >  	}
> >  }
> 
> Is this the only place where such a loop occurs?

Seems to be.

> Should a put_unaligned_be48()
> function be introduced?

Why?  This is used exactly once.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Feb. 28, 2017, 10:45 p.m. UTC | #4 
On 02/28/2017 02:23 PM, Joe Perches wrote:
> On Tue, 2017-02-28 at 15:35 +0000, Bart Van Assche wrote:
>> On Tue, 2017-02-28 at 15:02 +0300, Dan Carpenter wrote:
>>> Bitwise & was obviously intended here.
> []
>>> diff --git a/include/linux/mlx4/driver.h b/include/linux/mlx4/driver.h
> []
>>> @@ -109,7 +109,7 @@ static inline void 	(u8 *addr, u64 mac)
>>>  	int i;
>>>  
>>>  	for (i = ETH_ALEN; i > 0; i--) {
>>> -		addr[i - 1] = mac && 0xFF;
>>> +		addr[i - 1] = mac & 0xFF;
>>>  		mac >>= 8;
>>>  	}
>>>  }
>>
>> Is this the only place where such a loop occurs?
> 
> Seems to be.
> 
>> Should a put_unaligned_be48()
>> function be introduced?
> 
> Why?  This is used exactly once.

Really? Here is an example of another open-coded version of
put_unaligned_be48() from arch/mips/cavium-octeon/octeon-platform.c:

	new_mac[0] = (mac >> 40) & 0xff;
	new_mac[1] = (mac >> 32) & 0xff;
	new_mac[2] = (mac >> 24) & 0xff;
	new_mac[3] = (mac >> 16) & 0xff;
	new_mac[4] = (mac >> 8) & 0xff;
	new_mac[5] = mac & 0xff;

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julia Lawall March 1, 2017, 6:52 a.m. UTC | #5 
On Tue, 28 Feb 2017, Bart Van Assche wrote:

> On 02/28/2017 02:23 PM, Joe Perches wrote:
> > On Tue, 2017-02-28 at 15:35 +0000, Bart Van Assche wrote:
> >> On Tue, 2017-02-28 at 15:02 +0300, Dan Carpenter wrote:
> >>> Bitwise & was obviously intended here.
> > []
> >>> diff --git a/include/linux/mlx4/driver.h b/include/linux/mlx4/driver.h
> > []
> >>> @@ -109,7 +109,7 @@ static inline void 	(u8 *addr, u64 mac)
> >>>  	int i;
> >>>
> >>>  	for (i = ETH_ALEN; i > 0; i--) {
> >>> -		addr[i - 1] = mac && 0xFF;
> >>> +		addr[i - 1] = mac & 0xFF;
> >>>  		mac >>= 8;
> >>>  	}
> >>>  }
> >>
> >> Is this the only place where such a loop occurs?
> >
> > Seems to be.
> >
> >> Should a put_unaligned_be48()
> >> function be introduced?
> >
> > Why?  This is used exactly once.
>
> Really? Here is an example of another open-coded version of
> put_unaligned_be48() from arch/mips/cavium-octeon/octeon-platform.c:
>
> 	new_mac[0] = (mac >> 40) & 0xff;
> 	new_mac[1] = (mac >> 32) & 0xff;
> 	new_mac[2] = (mac >> 24) & 0xff;
> 	new_mac[3] = (mac >> 16) & 0xff;
> 	new_mac[4] = (mac >> 8) & 0xff;
> 	new_mac[5] = mac & 0xff;

drivers/media/radio/radio-shark2.c:
        for (i = 0; i < 6; i++)
               shark->transfer_buffer[i + 1] = (reg >> (40 - i * 8)) & 0xff;

drivers/rtc/rtc-ab3100.c
       buf[0] = (hw_counter) & 0xFF;
       buf[1] = (hw_counter >> 8) & 0xFF;
       buf[2] = (hw_counter >> 16) & 0xFF;
       buf[3] = (hw_counter >> 24) & 0xFF;
       buf[4] = (hw_counter >> 32) & 0xFF;
       buf[5] = (hw_counter >> 40) & 0xFF;

drivers/net/ethernet/sun/ldmvsw.c
        for (i = 0; i < ETH_ALEN; i++)
               port->raddr[i] = (*rmac >> (5 - i) * 8) & 0xff;

drivers/net/ethernet/sun/sunvnet.c
        for (i = 0; i < ETH_ALEN; i++)
               dev->dev_addr[i] = (*local_mac >> (5 - i) * 8) & 0xff;

        for (i = 0; i < ETH_ALEN; i++)
               port->raddr[i] = (*rmac >> (5 - i) * 8) & 0xff;

julia

>
> Bart.
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tariq Toukan March 1, 2017, 8:34 a.m. UTC | #6 
On 01/03/2017 8:52 AM, Julia Lawall wrote:
> On Tue, 28 Feb 2017, Bart Van Assche wrote:
>
>> On 02/28/2017 02:23 PM, Joe Perches wrote:
>>> On Tue, 2017-02-28 at 15:35 +0000, Bart Van Assche wrote:
>>>> On Tue, 2017-02-28 at 15:02 +0300, Dan Carpenter wrote:
>>>>> Bitwise & was obviously intended here.
>>> []
>>>>> diff --git a/include/linux/mlx4/driver.h b/include/linux/mlx4/driver.h
>>> []
>>>>> @@ -109,7 +109,7 @@ static inline void 	(u8 *addr, u64 mac)
>>>>>  	int i;
>>>>>
>>>>>  	for (i = ETH_ALEN; i > 0; i--) {
>>>>> -		addr[i - 1] = mac && 0xFF;
>>>>> +		addr[i - 1] = mac & 0xFF;
>>>>>  		mac >>= 8;
>>>>>  	}
>>>>>  }
>>>>
>>>> Is this the only place where such a loop occurs?
>>>
>>> Seems to be.
>>>
>>>> Should a put_unaligned_be48()
>>>> function be introduced?
>>>
>>> Why?  This is used exactly once.
>>
>> Really? Here is an example of another open-coded version of
>> put_unaligned_be48() from arch/mips/cavium-octeon/octeon-platform.c:
>>
>> 	new_mac[0] = (mac >> 40) & 0xff;
>> 	new_mac[1] = (mac >> 32) & 0xff;
>> 	new_mac[2] = (mac >> 24) & 0xff;
>> 	new_mac[3] = (mac >> 16) & 0xff;
>> 	new_mac[4] = (mac >> 8) & 0xff;
>> 	new_mac[5] = mac & 0xff;
>
> drivers/media/radio/radio-shark2.c:
>         for (i = 0; i < 6; i++)
>                shark->transfer_buffer[i + 1] = (reg >> (40 - i * 8)) & 0xff;
>
> drivers/rtc/rtc-ab3100.c
>        buf[0] = (hw_counter) & 0xFF;
>        buf[1] = (hw_counter >> 8) & 0xFF;
>        buf[2] = (hw_counter >> 16) & 0xFF;
>        buf[3] = (hw_counter >> 24) & 0xFF;
>        buf[4] = (hw_counter >> 32) & 0xFF;
>        buf[5] = (hw_counter >> 40) & 0xFF;
>
> drivers/net/ethernet/sun/ldmvsw.c
>         for (i = 0; i < ETH_ALEN; i++)
>                port->raddr[i] = (*rmac >> (5 - i) * 8) & 0xff;
>
> drivers/net/ethernet/sun/sunvnet.c
>         for (i = 0; i < ETH_ALEN; i++)
>                dev->dev_addr[i] = (*local_mac >> (5 - i) * 8) & 0xff;
>
>         for (i = 0; i < ETH_ALEN; i++)
>                port->raddr[i] = (*rmac >> (5 - i) * 8) & 0xff;
>
> julia
>

With these code replication examples, I agree that a function should be 
introduced.

>>
>> Bart.
>> --
>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller March 1, 2017, 5:52 p.m. UTC | #7 
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Tue, 28 Feb 2017 15:02:15 +0300

> Bitwise & was obviously intended here.
> 
> Fixes: 745d8ae4622c ("net/mlx4: Spoofcheck and zero MAC can't coexist")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/mlx4/driver.h b/include/linux/mlx4/driver.h
index e965e5090d96..a858bcb6220b 100644
--- a/include/linux/mlx4/driver.h
+++ b/include/linux/mlx4/driver.h
@@ -109,7 +109,7 @@  static inline void mlx4_u64_to_mac(u8 *addr, u64 mac)
 	int i;
 
 	for (i = ETH_ALEN; i > 0; i--) {
-		addr[i - 1] = mac && 0xFF;
+		addr[i - 1] = mac & 0xFF;
 		mac >>= 8;
 	}
 }