[PATCH] staging: vt6656: Use BIT_ULL() macro instead of bit shift operation

[PATCH] staging: vt6656: Use BIT_ULL() macro instead of bit shift operation

[Oscar Carter]

Replace the bit left shift operation with the BIT_ULL() macro and remove
the unnecessary "and" operation against the bit_nr variable.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
 drivers/staging/vt6656/main_usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index 5e48b3ddb94c..f7ca9e97594d 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -21,6 +21,7 @@
  */
 #undef __NO_VERSION__

+#include <linux/bits.h>
 #include <linux/etherdevice.h>
 #include <linux/file.h>
 #include "device.h"
@@ -802,8 +803,7 @@ static u64 vnt_prepare_multicast(struct ieee80211_hw *hw,

 	netdev_hw_addr_list_for_each(ha, mc_list) {
 		bit_nr = ether_crc(ETH_ALEN, ha->addr) >> 26;
-
-		mc_filter |= 1ULL << (bit_nr & 0x3f);
+		mc_filter |= BIT_ULL(bit_nr);
 	}

 	priv->mc_list_count = mc_list->count;
--
2.20.1

Re: [PATCH] staging: vt6656: Use BIT_ULL() macro instead of bit shift operation

[Greg Kroah-Hartman]

On Sat, Mar 07, 2020 at 11:49:29AM +0100, Oscar Carter wrote:
> Replace the bit left shift operation with the BIT_ULL() macro and remove
> the unnecessary "and" operation against the bit_nr variable.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
>  drivers/staging/vt6656/main_usb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> index 5e48b3ddb94c..f7ca9e97594d 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -21,6 +21,7 @@
>   */
>  #undef __NO_VERSION__
> 
> +#include <linux/bits.h>
>  #include <linux/etherdevice.h>
>  #include <linux/file.h>
>  #include "device.h"
> @@ -802,8 +803,7 @@ static u64 vnt_prepare_multicast(struct ieee80211_hw *hw,
> 
>  	netdev_hw_addr_list_for_each(ha, mc_list) {
>  		bit_nr = ether_crc(ETH_ALEN, ha->addr) >> 26;
> -
> -		mc_filter |= 1ULL << (bit_nr & 0x3f);
> +		mc_filter |= BIT_ULL(bit_nr);

Are you sure this does the same thing?  You are not masking off bit_nr
anymore, why not?

thanks,

greg k-h

Re: [PATCH] staging: vt6656: Use BIT_ULL() macro instead of bit shift operation

[Oscar Carter]

On Sun, Mar 08, 2020 at 07:55:38AM +0100, Greg Kroah-Hartman wrote:
> On Sat, Mar 07, 2020 at 11:49:29AM +0100, Oscar Carter wrote:
> > Replace the bit left shift operation with the BIT_ULL() macro and remove
> > the unnecessary "and" operation against the bit_nr variable.
> >
> > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> > ---
> >  drivers/staging/vt6656/main_usb.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > index 5e48b3ddb94c..f7ca9e97594d 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -21,6 +21,7 @@
> >   */
> >  #undef __NO_VERSION__
> >
> > +#include <linux/bits.h>
> >  #include <linux/etherdevice.h>
> >  #include <linux/file.h>
> >  #include "device.h"
> > @@ -802,8 +803,7 @@ static u64 vnt_prepare_multicast(struct ieee80211_hw *hw,
> >
> >  	netdev_hw_addr_list_for_each(ha, mc_list) {
> >  		bit_nr = ether_crc(ETH_ALEN, ha->addr) >> 26;
> > -
> > -		mc_filter |= 1ULL << (bit_nr & 0x3f);
> > +		mc_filter |= BIT_ULL(bit_nr);
>
> Are you sure this does the same thing?  You are not masking off bit_nr
> anymore, why not?

My reasons are exposed below:

The ether_crc function returns an u32 type (unsigned of 32 bits). Then the right
shift operand discards the 26 lsb bits (the bits shifted off the right side are
discarded). The 6 msb bits of the u32 returned by the ether_crc function are
positioned in bit 5 to bit 0 of the variable bit_nr. Due to the right shift
happens over an unsigned type, the 26 new bits added on the left side will be 0.

In summary, after the right bit shift operation we obtain in the variable bit_nr
(unsigned of 32 bits) the value represented by the 6 msb bits of the value
returned by the ether_crc function. So, only the 6 lsb bits of the variable
bit_nr are important. The 26 msb bits of this variable are 0.

In this situation, the "and" operation with the mask 0x3f (mask of 6 lsb bits)
is unnecessary due to its purpose is to reset (set to 0 value) the 26 msb bits
that are yet 0.
>
> thanks,
>
> greg k-h

thanks,

Oscar

Re: [PATCH] staging: vt6656: Use BIT_ULL() macro instead of bit shift operation

[Malcolm Priestley]

>>>   */
>>>  #undef __NO_VERSION__
>>>
>>> +#include <linux/bits.h>
>>>  #include <linux/etherdevice.h>
>>>  #include <linux/file.h>
>>>  #include "device.h"
>>> @@ -802,8 +803,7 @@ static u64 vnt_prepare_multicast(struct ieee80211_hw *hw,
>>>
>>>  	netdev_hw_addr_list_for_each(ha, mc_list) {
>>>  		bit_nr = ether_crc(ETH_ALEN, ha->addr) >> 26;
>>> -
>>> -		mc_filter |= 1ULL << (bit_nr & 0x3f);
>>> +		mc_filter |= BIT_ULL(bit_nr);
>>
>> Are you sure this does the same thing?  You are not masking off bit_nr
>> anymore, why not?
> 
> My reasons are exposed below:
> 
> The ether_crc function returns an u32 type (unsigned of 32 bits). Then the right
> shift operand discards the 26 lsb bits (the bits shifted off the right side are
> discarded). The 6 msb bits of the u32 returned by the ether_crc function are
> positioned in bit 5 to bit 0 of the variable bit_nr. Due to the right shift
> happens over an unsigned type, the 26 new bits added on the left side will be 0.
> 
> In summary, after the right bit shift operation we obtain in the variable bit_nr
> (unsigned of 32 bits) the value represented by the 6 msb bits of the value
> returned by the ether_crc function. So, only the 6 lsb bits of the variable
> bit_nr are important. The 26 msb bits of this variable are 0.
> 
> In this situation, the "and" operation with the mask 0x3f (mask of 6 lsb bits)
> is unnecessary due to its purpose is to reset (set to 0 value) the 26 msb bits
> that are yet 0.

The mask is only there out of legacy originally it was 31(0x1f) and the
bit_nr spread across two mc_filter u32 arrays.

The mask is not needed now it is u64.

The patch is fine.

Regards

Malcolm

Re: [PATCH] staging: vt6656: Use BIT_ULL() macro instead of bit shift operation

[Greg Kroah-Hartman]

On Sun, Mar 08, 2020 at 07:22:07PM +0000, Malcolm Priestley wrote:
> >>>   */
> >>>  #undef __NO_VERSION__
> >>>
> >>> +#include <linux/bits.h>
> >>>  #include <linux/etherdevice.h>
> >>>  #include <linux/file.h>
> >>>  #include "device.h"
> >>> @@ -802,8 +803,7 @@ static u64 vnt_prepare_multicast(struct ieee80211_hw *hw,
> >>>
> >>>  	netdev_hw_addr_list_for_each(ha, mc_list) {
> >>>  		bit_nr = ether_crc(ETH_ALEN, ha->addr) >> 26;
> >>> -
> >>> -		mc_filter |= 1ULL << (bit_nr & 0x3f);
> >>> +		mc_filter |= BIT_ULL(bit_nr);
> >>
> >> Are you sure this does the same thing?  You are not masking off bit_nr
> >> anymore, why not?
> > 
> > My reasons are exposed below:
> > 
> > The ether_crc function returns an u32 type (unsigned of 32 bits). Then the right
> > shift operand discards the 26 lsb bits (the bits shifted off the right side are
> > discarded). The 6 msb bits of the u32 returned by the ether_crc function are
> > positioned in bit 5 to bit 0 of the variable bit_nr. Due to the right shift
> > happens over an unsigned type, the 26 new bits added on the left side will be 0.
> > 
> > In summary, after the right bit shift operation we obtain in the variable bit_nr
> > (unsigned of 32 bits) the value represented by the 6 msb bits of the value
> > returned by the ether_crc function. So, only the 6 lsb bits of the variable
> > bit_nr are important. The 26 msb bits of this variable are 0.
> > 
> > In this situation, the "and" operation with the mask 0x3f (mask of 6 lsb bits)
> > is unnecessary due to its purpose is to reset (set to 0 value) the 26 msb bits
> > that are yet 0.
> 
> The mask is only there out of legacy originally it was 31(0x1f) and the
> bit_nr spread across two mc_filter u32 arrays.
> 
> The mask is not needed now it is u64.
> 
> The patch is fine.

Ok, then the changelog needs to be fixed up to explain all of this and
resent.

thanks,

greg k-h

Re: [PATCH] staging: vt6656: Use BIT_ULL() macro instead of bit shift operation

[Oscar Carter]

On Tue, Mar 10, 2020 at 10:50:11AM +0100, Greg Kroah-Hartman wrote:
> On Sun, Mar 08, 2020 at 07:22:07PM +0000, Malcolm Priestley wrote:
> > >>>   */
> > >>>  #undef __NO_VERSION__
> > >>>
> > >>> +#include <linux/bits.h>
> > >>>  #include <linux/etherdevice.h>
> > >>>  #include <linux/file.h>
> > >>>  #include "device.h"
> > >>> @@ -802,8 +803,7 @@ static u64 vnt_prepare_multicast(struct ieee80211_hw *hw,
> > >>>
> > >>>  	netdev_hw_addr_list_for_each(ha, mc_list) {
> > >>>  		bit_nr = ether_crc(ETH_ALEN, ha->addr) >> 26;
> > >>> -
> > >>> -		mc_filter |= 1ULL << (bit_nr & 0x3f);
> > >>> +		mc_filter |= BIT_ULL(bit_nr);
> > >>
> > >> Are you sure this does the same thing?  You are not masking off bit_nr
> > >> anymore, why not?
> > >
> > > My reasons are exposed below:
> > >
> > > The ether_crc function returns an u32 type (unsigned of 32 bits). Then the right
> > > shift operand discards the 26 lsb bits (the bits shifted off the right side are
> > > discarded). The 6 msb bits of the u32 returned by the ether_crc function are
> > > positioned in bit 5 to bit 0 of the variable bit_nr. Due to the right shift
> > > happens over an unsigned type, the 26 new bits added on the left side will be 0.
> > >
> > > In summary, after the right bit shift operation we obtain in the variable bit_nr
> > > (unsigned of 32 bits) the value represented by the 6 msb bits of the value
> > > returned by the ether_crc function. So, only the 6 lsb bits of the variable
> > > bit_nr are important. The 26 msb bits of this variable are 0.
> > >
> > > In this situation, the "and" operation with the mask 0x3f (mask of 6 lsb bits)
> > > is unnecessary due to its purpose is to reset (set to 0 value) the 26 msb bits
> > > that are yet 0.
> >
> > The mask is only there out of legacy originally it was 31(0x1f) and the
> > bit_nr spread across two mc_filter u32 arrays.
> >
> > The mask is not needed now it is u64.
> >
> > The patch is fine.
>
> Ok, then the changelog needs to be fixed up to explain all of this and
> resent.

Ok, I will create a new version patch with all of this information and I will
resend it.

>
> thanks,
>
> greg k-h

thanks,

Oscar