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

[Oscar Carter <oscar.carter@gmx.com>]

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