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

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

[Rui Teng]

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.

Signed-off-by: Rui Teng <rui.teng@linux.vnet.ibm.com>
---
 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;
 
 	*pu8CurrByte++ = (strHostIfSetMulti->cnt & 0xFF);
 	*pu8CurrByte++ = ((strHostIfSetMulti->cnt >> 8) & 0xFF);
-- 
2.11.0

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

[Greg Kroah-Hartman]

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?

thanks,

greg k-h


> 
> Signed-off-by: Rui Teng <rui.teng@linux.vnet.ibm.com>
> ---
>  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?

thanks,

greg k-h

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

[Rui Teng]

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.

>
> thanks,
>
> greg k-h
>
>
>>
>> Signed-off-by: Rui Teng <rui.teng@linux.vnet.ibm.com>
>> ---
>>  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.

>
> thanks,
>
> greg k-h
>

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@linux.vnet.ibm.com>
> > > ---
> > >  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

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

[Joe Perches]

On Wed, 2017-07-12 at 08:12 +0200, Greg Kroah-Hartman wrote:
> 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.

Using __be32 would be more intelligible.

Maybe something like this: (in any number of patches)

o convert u8 *pu8CurrByte to be32 *buf
o convert strHostIfSetMulti to smulti
o remove useless initialization of result
o remove unnecessary parentheses
o return directly on kalloc failure
o remove label

Ending up with: (uncompiled/untested)
---
static void Handle_SetMulticastFilter(struct wilc_vif *vif,
				      struct set_multicast *smulti)
{
	s32 result;
	struct wid wid;
	__be32 *buf;

	wid.id = (u16)WID_SETUP_MULTICAST_FILTER;
	wid.type = WID_BIN;
	wid.size = sizeof(struct set_multicast) + smulti->cnt * ETH_ALEN;
	wid.val = kmalloc(wid.size, GFP_KERNEL);
	if (!wid.val)
		return;

	buf = (__force __be32 *)wid.val;
	*buf++ = cpu_to_be32(smulti->enabled);
	*buf++ = cpu_to_be32(smulti->cnt);

	memcpy(buf, wilc_multicast_mac_addr_list, smulti->cnt * ETH_ALEN);

	result = wilc_send_config_pkt(vif, SET_CFG, &wid, 1,
				      wilc_get_vif_idx(vif));
	if (result)
		netdev_err(vif->ndev, "Failed to send setup multicast\n");

	kfree(wid.val);
}