diff mbox

[net-next,01/13] ixgb: eliminate checkstack warnings

Message ID 1316246677-8830-2-git-send-email-jeffrey.t.kirsher@intel.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Kirsher, Jeffrey T Sept. 17, 2011, 8:04 a.m. UTC 
From: Jesse Brandeburg <jesse.brandeburg@intel.com>

Really trivial fix, use kzalloc/kree instead of stack space.

before:
[jbrandeb@jbrandeb-mobl2 linux-2.6]$ make checkstack|grep ixgb_
0x0210 ixgb_set_multi [ixgb]:				768
0x04f8 ixgb_check_options [ixgb]:			220
0x04334 ixgb_set_ringparam [ixgb]:			124
0x04516 ixgb_set_ringparam [ixgb]:			124

after:
0x04f8 ixgb_check_options [ixgb]:			220
0x04354 ixgb_set_ringparam [ixgb]:			124
0x04536 ixgb_set_ringparam [ixgb]:			124

Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by:  Aaron Brown  <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgb/ixgb_main.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

Comments

Joe Perches Sept. 17, 2011, 8:43 a.m. UTC | #1 
On Sat, 2011-09-17 at 01:04 -0700, Jeff Kirsher wrote:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Really trivial fix, use kzalloc/kree instead of stack space.

Some more trivialities...

> diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c b/drivers/net/ethernet/intel/ixgb/ixgb_main.c\
[]
> @@ -1120,8 +1120,12 @@ ixgb_set_multi(struct net_device *netdev)
>  		rctl |= IXGB_RCTL_MPE;
>  		IXGB_WRITE_REG(hw, RCTL, rctl);
>  	} else {
> -		u8 mta[IXGB_MAX_NUM_MULTICAST_ADDRESSES *
> -			    IXGB_ETH_LENGTH_OF_ADDRESS];
> +		u8 *mta = kzalloc(IXGB_MAX_NUM_MULTICAST_ADDRESSES *
> +			      IXGB_ETH_LENGTH_OF_ADDRESS, GFP_KERNEL);

This doesn't need to be kzalloc as every byte is overwritten.
It should be kmalloc.

Maybe delete the #define IXGB_ETH_LENGTH_OF_ADDRESS and
sed 's/\bIXGB_ETH_LENGTH_OF_ADDRESS\b/ETH_ALEN/g' ?

Perhaps this loop could be clearer without the multiply:

		i = 0;
		netdev_for_each_mc_addr(ha, netdev)
			memcpy(&mta[i++ * IXGB_ETH_LENGTH_OF_ADDRESS],
			       ha->addr, IXGB_ETH_LENGTH_OF_ADDRESS);

Perhaps:

		u8 *addr = mta;
		netdev_for_each_mc_addr(ha, netdev) {
			memcpy(addr, ha->addr, ETH_ALEN);
			addr += ETH_ALEN;
		}


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Brandeburg Sept. 19, 2011, 9:36 p.m. UTC | #2 
On Sat, 17 Sep 2011 01:43:26 -0700
Joe Perches <joe@perches.com> wrote:

> On Sat, 2011-09-17 at 01:04 -0700, Jeff Kirsher wrote:
> > From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> > Really trivial fix, use kzalloc/kree instead of stack space.
> 
> Some more trivialities...
> 
> > diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
> > b/drivers/net/ethernet/intel/ixgb/ixgb_main.c\
> []
> > @@ -1120,8 +1120,12 @@ ixgb_set_multi(struct net_device *netdev)
> >  		rctl |= IXGB_RCTL_MPE;
> >  		IXGB_WRITE_REG(hw, RCTL, rctl);
> >  	} else {
> > -		u8 mta[IXGB_MAX_NUM_MULTICAST_ADDRESSES *
> > -			    IXGB_ETH_LENGTH_OF_ADDRESS];
> > +		u8 *mta = kzalloc(IXGB_MAX_NUM_MULTICAST_ADDRESSES
> > *
> > +			      IXGB_ETH_LENGTH_OF_ADDRESS,
> > GFP_KERNEL);
> 
> This doesn't need to be kzalloc as every byte is overwritten.
> It should be kmalloc.

done, V2 on its way

> Maybe delete the #define IXGB_ETH_LENGTH_OF_ADDRESS and
> sed 's/\bIXGB_ETH_LENGTH_OF_ADDRESS\b/ETH_ALEN/g' ?

done

 
> Perhaps this loop could be clearer without the multiply:
> 
> 		i = 0;
> 		netdev_for_each_mc_addr(ha, netdev)
> 			memcpy(&mta[i++ * IXGB_ETH_LENGTH_OF_ADDRESS],
> 			       ha->addr, IXGB_ETH_LENGTH_OF_ADDRESS);
> 
> Perhaps:
> 
> 		u8 *addr = mta;
> 		netdev_for_each_mc_addr(ha, netdev) {
> 			memcpy(addr, ha->addr, ETH_ALEN);
> 			addr += ETH_ALEN;
> 		}

done, but because of the nature of the changes being code flow, I'm
going to retest through our lab.  V2 will hopefully be at the list
shortly.

Thanks for the feedback,
 Jesse
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/intel/ixgb/ixgb_main.c b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
index b8fb163..500823b 100644
--- a/drivers/net/ethernet/intel/ixgb/ixgb_main.c
+++ b/drivers/net/ethernet/intel/ixgb/ixgb_main.c
@@ -1120,8 +1120,12 @@  ixgb_set_multi(struct net_device *netdev)
 		rctl |= IXGB_RCTL_MPE;
 		IXGB_WRITE_REG(hw, RCTL, rctl);
 	} else {
-		u8 mta[IXGB_MAX_NUM_MULTICAST_ADDRESSES *
-			    IXGB_ETH_LENGTH_OF_ADDRESS];
+		u8 *mta = kzalloc(IXGB_MAX_NUM_MULTICAST_ADDRESSES *
+			      IXGB_ETH_LENGTH_OF_ADDRESS, GFP_KERNEL);
+		if (!mta) {
+			pr_err("allocation of multicast memory failed\n");
+			goto alloc_failed;
+		}
 
 		IXGB_WRITE_REG(hw, RCTL, rctl);
 
@@ -1131,8 +1135,10 @@  ixgb_set_multi(struct net_device *netdev)
 			       ha->addr, IXGB_ETH_LENGTH_OF_ADDRESS);
 
 		ixgb_mc_addr_list_update(hw, mta, netdev_mc_count(netdev), 0);
+		kfree(mta);
 	}
 
+alloc_failed:
 	if (netdev->features & NETIF_F_HW_VLAN_RX)
 		ixgb_vlan_strip_enable(adapter);
 	else