| Message ID | 20170228120215.GA27947@mwanda |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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>
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.
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.
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 >
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 >
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.
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; } }
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.