linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drivers/staging/wilc1000: fix sparse warning: right shift by bigger than source value
@ 2017-07-10  8:57 Rui Teng
  2017-07-11 17:04 ` Greg Kroah-Hartman
  0 siblings, 1 reply; 5+ messages in thread
From: Rui Teng @ 2017-07-10  8:57 UTC (permalink / raw)
  To: Aditya Shankar, Ganesh Krishna, Greg Kroah-Hartman
  Cc: linux-wireless, devel, linux-kernel, 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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] drivers/staging/wilc1000: fix sparse warning: right shift by bigger than source value
  2017-07-10  8:57 [PATCH] drivers/staging/wilc1000: fix sparse warning: right shift by bigger than source value Rui Teng
@ 2017-07-11 17:04 ` Greg Kroah-Hartman
  2017-07-12  2:23   ` Rui Teng
  0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2017-07-11 17:04 UTC (permalink / raw)
  To: Rui Teng
  Cc: Aditya Shankar, Ganesh Krishna, devel, linux-wireless, linux-kernel

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drivers/staging/wilc1000: fix sparse warning: right shift by bigger than source value
  2017-07-11 17:04 ` Greg Kroah-Hartman
@ 2017-07-12  2:23   ` Rui Teng
  2017-07-12  6:12     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 5+ messages in thread
From: Rui Teng @ 2017-07-12  2:23 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Aditya Shankar, Ganesh Krishna, devel, linux-wireless, linux-kernel

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
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drivers/staging/wilc1000: fix sparse warning: right shift by bigger than source value
  2017-07-12  2:23   ` Rui Teng
@ 2017-07-12  6:12     ` Greg Kroah-Hartman
  2017-07-12  8:11       ` Joe Perches
  0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2017-07-12  6:12 UTC (permalink / raw)
  To: Rui Teng
  Cc: devel, Aditya Shankar, linux-wireless, linux-kernel, Ganesh Krishna

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] drivers/staging/wilc1000: fix sparse warning: right shift by bigger than source value
  2017-07-12  6:12     ` Greg Kroah-Hartman
@ 2017-07-12  8:11       ` Joe Perches
  0 siblings, 0 replies; 5+ messages in thread
From: Joe Perches @ 2017-07-12  8:11 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Rui Teng
  Cc: devel, Aditya Shankar, linux-wireless, linux-kernel, Ganesh Krishna

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);
}

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-07-12  8:11 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-10  8:57 [PATCH] drivers/staging/wilc1000: fix sparse warning: right shift by bigger than source value Rui Teng
2017-07-11 17:04 ` Greg Kroah-Hartman
2017-07-12  2:23   ` Rui Teng
2017-07-12  6:12     ` Greg Kroah-Hartman
2017-07-12  8:11       ` Joe Perches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).