| Message ID | 1325618356-2655-8-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
>-----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
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.
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
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.
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
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;