RE: [net-next 08/11] e1000e: add Receive Packet Steering (RPS) support

[Joe Perches <joe@perches.com>]

On Tue, 2012-01-03 at 20:28 +0000, Allan, Bruce W wrote:
> >From: David Miller [mailto:davem@davemloft.net]
> >Fair enough, I rescind my request and this patch is fine as-is.
> Okay, thanks Dave.

A couple of other comments though.

On Tue, 2012-01-03 at 11:19 -0800, Jeff Kirsher wrote:
> From: Bruce Allan <bruce.w.allan@intel.com>
> 
> Enable RPS by default.  Disallow jumbo frames when both receive checksum
> and receive hashing are enabled because the hardware cannot do both IP
> payload checksum (enabled when receive checksum is enabled when using
> packet split which is used for jumbo frames) and provide RSS hash at the
> same time.
[]
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
[]
> +static void e1000e_setup_rss_hash(struct e1000_adapter *adapter)
[]
> +	static const u8 rsshash[40] = {
[]
+	};
> +
> +	/* Fill out hash function seeds */
> +	for (j = 0; j < 10; j++) {
> +		u32 rsskey = rsshash[(j * 4)];
> +		rsskey |= rsshash[(j * 4) + 1] << 8;
> +		rsskey |= rsshash[(j * 4) + 2] << 16;
> +		rsskey |= rsshash[(j * 4) + 3] << 24;

Strictly, don't these shifts first need a cast to u32?

	u32 rsskey = rsshash[j * 4];
	rsskey |= ((u32)rsshash[j * 4 + 1]) << 8;
	rsskey |= ((u32)rsshash[j * 4 + 2]) << 16;
	rsskey |= ((u32)rsshash[j * 4 + 3]) << 24;

[]

> +	if (adapter->rx_ps_pages &&
> +	    (features & NETIF_F_RXCSUM) && (features & NETIF_F_RXHASH)) {
> +		e_err("Enabling both receive checksum offload and receive hashing is not possible with jumbo frames.  Disable jumbos or enable only one of the receive offload features.\n");
> +		return -EINVAL;
> +	}
 
This is a very long output string.
I think keeping individual dmesg lines shorter is preferred.
It might be better to use 2 e_err lines like:

		e_err("Enabling both receive checksum offload and receive hashing is not possible with jumbo frames\n");
		e_err("Disable jumbos or enable only one of the receive offload features\n");

or

		e_err("Enabling both receive checksum offload and receive hashing is not possible with jumbo frames\n"
		      "Disable jumbos or enable only one of the receive offload features\n");