Re: [PATCH] drivers/staging/wilc1000: fix sparse warning: right shift by bigger than source value

[Greg Kroah-Hartman]

On Wed, Jul 12, 2017 at 10:23:02AM +0800, Rui Teng wrote:
> On 12/07/2017 1:04 AM, Greg Kroah-Hartman wrote:
> > On Mon, Jul 10, 2017 at 04:57:31PM +0800, Rui Teng wrote:
> > > This patch sets memory to zero directly to avoid unnecessary shift and
> > > bitwise operations on bool type, which can fix a sparse warning and also
> > > improve performance.
> > 
> > It does?  How did you measure the performance impact?  What was now
> > faster?
> 
> It can avoid 3 times right shift and 3 times bitwise operations.
> And once memory set should also faster than 4 times copy operations.
> And add number 4 once should also faster than 4 times plus plus.

And did you test this to prove that this does matter and is noticable?
How do you know that gcc doesn't just optimize it all away?  Is this on
a code path that actually matters?

Don't ever say "improve performance" without actually being able to
prove it please.

> > thanks,
> > 
> > greg k-h
> > 
> > 
> > > 
> > > Signed-off-by: Rui Teng <rui.teng@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/staging/wilc1000/host_interface.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> > > index 2568dfc15181..036c5c19a016 100644
> > > --- a/drivers/staging/wilc1000/host_interface.c
> > > +++ b/drivers/staging/wilc1000/host_interface.c
> > > @@ -2416,10 +2416,9 @@ static void Handle_SetMulticastFilter(struct wilc_vif *vif,
> > >  		goto ERRORHANDLER;
> > > 
> > >  	pu8CurrByte = wid.val;
> > > -	*pu8CurrByte++ = (strHostIfSetMulti->enabled & 0xFF);
> > > -	*pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 8) & 0xFF);
> > > -	*pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 16) & 0xFF);
> > > -	*pu8CurrByte++ = ((strHostIfSetMulti->enabled >> 24) & 0xFF);
> > > +	memset(pu8CurrByte, 0, 4);
> > > +	*pu8CurrByte = (strHostIfSetMulti->enabled & 0xFF);
> > > +	pu8CurrByte += 4;
> > 
> > Are you sure enabled isn't larger than 8 bits?
> 
> The size of bool may larger than 8 bits. But when we assign any value
> to bool type, the value of bool type will only be 1 or 0.
> For example, the following output will be 1 other than 0x100.
> 	bool b = 0x100;
> 	printf("b: %d\n", b);
> 
> Or I think it should change 'enabled' type from bool to u32, to make sure
> that the right shift operation can take effect.

I think we should ask the authors of this code what is expected here as
it is quite odd...

thanks,

greg k-h