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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1325635974.24257.20.camel@joe2Laptop>
Date:	Tue, 03 Jan 2012 16:12:54 -0800
From:	Joe Perches <joe@...ches.com>
To:	"Allan, Bruce W" <bruce.w.allan@...el.com>
Cc:	David Miller <davem@...emloft.net>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>
Subject: RE: [net-next 08/11] e1000e: add Receive Packet Steering (RPS)
 support

On Tue, 2012-01-03 at 20:28 +0000, Allan, Bruce W wrote:
> >From: David Miller [mailto:davem@...emloft.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@...el.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");


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html