Patchwork [net-next,07/11] e1000e: cleanup Rx checksum offload code

login
register
mail settings
Submitter Jeff Kirsher
Date Jan. 3, 2012, 7:19 p.m.
Message ID <1325618356-2655-8-git-send-email-jeffrey.t.kirsher@intel.com>
Download mbox | patch
Permalink /patch/134063/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Jeff Kirsher - Jan. 3, 2012, 7:19 p.m.
From: Bruce Allan <bruce.w.allan@intel.com>

1) cleanup whitespace in e1000_rx_checksum() function header comment
2) do not check hardware checksum when Rx checksum is disabled
3) reduce duplicated calls to le16_to_cpu() by just using it within
   e1000_rx_checksum() instead of in each call to the function

Signed-off-by: Bruce Allan <bruce.w.allan@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/e1000e/netdev.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)
David Miller - Jan. 3, 2012, 8:08 p.m.
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Tue,  3 Jan 2012 11:19:12 -0800

> -		__sum16 sum = (__force __sum16)htons(csum);
> +		__sum16 sum = (__force __sum16)htons(le16_to_cpu(csum));

Looks like a NOP.  It's essentially "cpu_to_le16(le16_to_cpu(csum)" as
far as I can tell.
--
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
Allan, Bruce W - Jan. 3, 2012, 8:17 p.m.
>-----Original Message-----
>From: David Miller [mailto:davem@davemloft.net]
>Sent: Tuesday, January 03, 2012 12:09 PM
>To: Kirsher, Jeffrey T
>Cc: Allan, Bruce W; netdev@vger.kernel.org; gospo@redhat.com;
>sassmann@redhat.com
>Subject: Re: [net-next 07/11] e1000e: cleanup Rx checksum offload code
>
>From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>Date: Tue,  3 Jan 2012 11:19:12 -0800
>
>> -		__sum16 sum = (__force __sum16)htons(csum);
>> +		__sum16 sum = (__force __sum16)htons(le16_to_cpu(csum));
>
>Looks like a NOP.  It's essentially "cpu_to_le16(le16_to_cpu(csum)" as
>far as I can tell.

OK, I'll look into fixing this.

Thanks,
Bruce.
--
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
Ben Hutchings - Jan. 3, 2012, 9 p.m.
On Tue, 2012-01-03 at 15:08 -0500, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Tue,  3 Jan 2012 11:19:12 -0800
> 
> > -		__sum16 sum = (__force __sum16)htons(csum);
> > +		__sum16 sum = (__force __sum16)htons(le16_to_cpu(csum));
> 
> Looks like a NOP.  It's essentially "cpu_to_le16(le16_to_cpu(csum)" as
> far as I can tell.

Looks like a swab() to me...

Ben.
David Miller - Jan. 3, 2012, 9:15 p.m.
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 3 Jan 2012 21:00:12 +0000

> On Tue, 2012-01-03 at 15:08 -0500, David Miller wrote:
>> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> Date: Tue,  3 Jan 2012 11:19:12 -0800
>> 
>> > -		__sum16 sum = (__force __sum16)htons(csum);
>> > +		__sum16 sum = (__force __sum16)htons(le16_to_cpu(csum));
>> 
>> Looks like a NOP.  It's essentially "cpu_to_le16(le16_to_cpu(csum)" as
>> far as I can tell.
> 
> Looks like a swab() to me...

I don't see how it can be.  It's effectively doing a 16-bit swap and
then a 16-bit swap again, which is a NOP.

It's the same as "(__force __sum16) csum" and I bet if the code above
works, then this expression I'm suggesting will work too.
--
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
Ben Hutchings - Jan. 3, 2012, 9:24 p.m.
On Tue, 2012-01-03 at 16:15 -0500, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Tue, 3 Jan 2012 21:00:12 +0000
> 
> > On Tue, 2012-01-03 at 15:08 -0500, David Miller wrote:
> >> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> >> Date: Tue,  3 Jan 2012 11:19:12 -0800
> >> 
> >> > -		__sum16 sum = (__force __sum16)htons(csum);
> >> > +		__sum16 sum = (__force __sum16)htons(le16_to_cpu(csum));
> >> 
> >> Looks like a NOP.  It's essentially "cpu_to_le16(le16_to_cpu(csum)" as
> >> far as I can tell.
> > 
> > Looks like a swab() to me...
> 
> I don't see how it can be.  It's effectively doing a 16-bit swap and
> then a 16-bit swap again, which is a NOP.
> 
> It's the same as "(__force __sum16) csum" and I bet if the code above
> works, then this expression I'm suggesting will work too.

In this part of the universe, network order is big-endian.

Ben.
David Miller - Jan. 3, 2012, 10:02 p.m.
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 3 Jan 2012 21:24:02 +0000

> On Tue, 2012-01-03 at 16:15 -0500, David Miller wrote:
>> From: Ben Hutchings <bhutchings@solarflare.com>
>> Date: Tue, 3 Jan 2012 21:00:12 +0000
>> 
>> > On Tue, 2012-01-03 at 15:08 -0500, David Miller wrote:
>> >> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> >> Date: Tue,  3 Jan 2012 11:19:12 -0800
>> >> 
>> >> > -		__sum16 sum = (__force __sum16)htons(csum);
>> >> > +		__sum16 sum = (__force __sum16)htons(le16_to_cpu(csum));
>> >> 
>> >> Looks like a NOP.  It's essentially "cpu_to_le16(le16_to_cpu(csum)" as
>> >> far as I can tell.
>> > 
>> > Looks like a swab() to me...
>> 
>> I don't see how it can be.  It's effectively doing a 16-bit swap and
>> then a 16-bit swap again, which is a NOP.
>> 
>> It's the same as "(__force __sum16) csum" and I bet if the code above
>> works, then this expression I'm suggesting will work too.
> 
> In this part of the universe, network order is big-endian.

Indeed, you're right, therefore this is something like swab().
:-)
--
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

Patch

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 3911401..e01ffce 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -487,10 +487,10 @@  static void e1000_receive_skb(struct e1000_adapter *adapter,
 
 /**
  * e1000_rx_checksum - Receive Checksum Offload
- * @adapter:     board private structure
- * @status_err:  receive descriptor status and error fields
- * @csum:	receive descriptor csum field
- * @sk_buff:     socket buffer with received data
+ * @adapter: board private structure
+ * @status_err: receive descriptor status and error fields
+ * @csum: receive descriptor csum field
+ * @sk_buff: socket buffer with received data
  **/
 static void e1000_rx_checksum(struct e1000_adapter *adapter, u32 status_err,
 			      u32 csum, struct sk_buff *skb)
@@ -500,9 +500,14 @@  static void e1000_rx_checksum(struct e1000_adapter *adapter, u32 status_err,
 
 	skb_checksum_none_assert(skb);
 
+	/* Rx checksum disabled */
+	if (!(adapter->netdev->features & NETIF_F_RXCSUM))
+		return;
+
 	/* Ignore Checksum bit is set */
 	if (status & E1000_RXD_STAT_IXSM)
 		return;
+
 	/* TCP/UDP checksum error bit is set */
 	if (errors & E1000_RXD_ERR_TCPE) {
 		/* let the stack verify checksum errors */
@@ -524,7 +529,7 @@  static void e1000_rx_checksum(struct e1000_adapter *adapter, u32 status_err,
 		 * Hardware complements the payload checksum, so we undo it
 		 * and then put the value in host order for further stack use.
 		 */
-		__sum16 sum = (__force __sum16)htons(csum);
+		__sum16 sum = (__force __sum16)htons(le16_to_cpu(csum));
 		skb->csum = csum_unfold(~sum);
 		skb->ip_summed = CHECKSUM_COMPLETE;
 	}
@@ -957,8 +962,7 @@  static bool e1000_clean_rx_irq(struct e1000_adapter *adapter,
 
 		/* Receive Checksum Offload */
 		e1000_rx_checksum(adapter, staterr,
-				  le16_to_cpu(rx_desc->wb.lower.hi_dword.
-					      csum_ip.csum), skb);
+				  rx_desc->wb.lower.hi_dword.csum_ip.csum, skb);
 
 		e1000_receive_skb(adapter, netdev, skb, staterr,
 				  rx_desc->wb.upper.vlan);
@@ -1318,8 +1322,8 @@  copydone:
 		total_rx_bytes += skb->len;
 		total_rx_packets++;
 
-		e1000_rx_checksum(adapter, staterr, le16_to_cpu(
-			rx_desc->wb.lower.hi_dword.csum_ip.csum), skb);
+		e1000_rx_checksum(adapter, staterr,
+				  rx_desc->wb.lower.hi_dword.csum_ip.csum, skb);
 
 		if (rx_desc->wb.upper.header_status &
 			   cpu_to_le16(E1000_RXDPS_HDRSTAT_HDRSP))
@@ -1491,8 +1495,7 @@  static bool e1000_clean_jumbo_rx_irq(struct e1000_adapter *adapter,
 
 		/* Receive Checksum Offload XXX recompute due to CRC strip? */
 		e1000_rx_checksum(adapter, staterr,
-				  le16_to_cpu(rx_desc->wb.lower.hi_dword.
-					      csum_ip.csum), skb);
+				  rx_desc->wb.lower.hi_dword.csum_ip.csum, skb);
 
 		/* probably a little skewed due to removing CRC */
 		total_rx_bytes += skb->len;