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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 12 Jul 2017 01:11:26 -0700
From:   Joe Perches <joe@...ches.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rui Teng <rui.teng@...ux.vnet.ibm.com>
Cc:     devel@...verdev.osuosl.org,
        Aditya Shankar <aditya.shankar@...rochip.com>,
        linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
        Ganesh Krishna <ganesh.krishna@...rochip.com>
Subject: Re: [PATCH] drivers/staging/wilc1000: fix sparse warning: right
 shift by bigger than source value

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