Bug 9940

Summary: e1000e driver with 82566DM-2 controller doesn't strip crc from frames
Product: Drivers Reporter: Johan Andersson (johan.andersson)
Component: NetworkAssignee: Auke Kok (auke-jan.h.kok)
Status: CLOSED CODE_FIX    
Severity: normal CC: andy, auke-jan.h.kok, kernel
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.24 Tree: Mainline
Regression: ---

Description Johan Andersson 2008-02-12 01:50:48 UTC
Latest working kernel version: n/a
Earliest failing kernel version: 2.6.24
Distribution: Gentoo 2007.0
Hardware Environment: HP Compaq dc7800p
Software Environment: n/a
Problem Description:
The e1000e driver doesn't strip the ethernet frame crc is used with the 82566DM-2 chip.
This will result, among other things, that bridging doesn't work together with this chip.
Reverting commit 140a74802894e9db57e5cd77ccff77e590ece5f3 fixes this issue but has performance implications.

Steps to reproduce:
Send a ping and monitor the responce with tcpdump.
Comment 1 Anonymous Emailer 2008-02-12 02:13:47 UTC
Reply-To: akpm@linux-foundation.org

On Tue, 12 Feb 2008 01:50:49 -0800 (PST) bugme-daemon@bugzilla.kernel.org wrote:

> http://bugzilla.kernel.org/show_bug.cgi?id=9940
> 
>            Summary: e1000e driver with 82566DM-2 controller doesn't strip
>                     crc from frames
>            Product: Drivers
>            Version: 2.5
>      KernelVersion: 2.6.24
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: Network
>         AssignedTo: jgarzik@pobox.com
>         ReportedBy: johan.andersson@transmode.se
> 
> 
> Latest working kernel version: n/a
> Earliest failing kernel version: 2.6.24
> Distribution: Gentoo 2007.0
> Hardware Environment: HP Compaq dc7800p
> Software Environment: n/a
> Problem Description:
> The e1000e driver doesn't strip the ethernet frame crc is used with the
> 82566DM-2 chip.
> This will result, among other things, that bridging doesn't work together
> with
> this chip.
> Reverting commit 140a74802894e9db57e5cd77ccff77e590ece5f3 fixes this issue
> but
> has performance implications.
> 
> Steps to reproduce:
> Send a ping and monitor the responce with tcpdump.
> 
Comment 2 Daniel Drake 2008-02-12 06:02:08 UTC
for my ref: downstream bug report https://bugs.gentoo.org/show_bug.cgi?id=209235
Comment 3 Auke Kok 2008-02-12 09:15:20 UTC
assign to me please, I'm trying to get this issue resolved asap. reverting the commit might be the safest thing to do, but I'd like to take a little bit of time to see if we can't fix this first.
Comment 4 Andy Gospodarek 2008-02-12 09:25:45 UTC
Sounds good, Auke.  You know it's this commit:

commit 140a74802894e9db57e5cd77ccff77e590ece5f3
Author: Auke Kok <auke-jan.h.kok@intel.com>
Date:   Thu Oct 25 13:57:58 2007 -0700

    e1000e: Re-enable SECRC - crc stripping

but I'll add it here for others still searching for a solution.
Comment 5 Auke Kok 2008-02-12 10:08:12 UTC
proposed fix - copy+paste because I'm lazy


commit c07ec0aa5cc541a128ceb283fdba5053572b2e20
Author: Auke Kok <auke-jan.h.kok@intel.com>
Date:   Tue Feb 12 09:23:44 2008 -0800

    [FOR TEST] e1000e: Fix CRC stripping in hardware context bug
    
    CRC stripping was only correctly enabled for packet split recieves
    which is used when receiving jumbo frames. Correctly enable SECRC
    also for normal buffer packet receives.
    
    Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>

diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c
index 329ee32..3031d6d 100644
--- a/drivers/net/e1000e/netdev.c
+++ b/drivers/net/e1000e/netdev.c
@@ -1670,6 +1670,9 @@ static void e1000_setup_rctl(struct e1000_adapter *adapter)
        else
                rctl |= E1000_RCTL_LPE;
 
+       /* Enable hardware CRC frame stripping */
+       rctl |= E1000_RCTL_SECRC;
+
        /* Setup buffer sizes */
        rctl &= ~E1000_RCTL_SZ_4096;
        rctl |= E1000_RCTL_BSEX;
@@ -1735,9 +1738,6 @@ static void e1000_setup_rctl(struct e1000_adapter *adapter)
 
                /* Enable Packet split descriptors */
                rctl |= E1000_RCTL_DTYP_PS;
-               
-               /* Enable hardware CRC frame stripping */
-               rctl |= E1000_RCTL_SECRC;
 
                psrctl |= adapter->rx_ps_bsize0 >>
                        E1000_PSRCTL_BSIZE0_SHIFT;
Comment 6 Johan Andersson 2008-02-12 13:05:24 UTC
Patch confirmed working on my system.

Packet dump without patch:
21:46:27.499326 00:0f:fe:96:d6:7b (oui Unknown) > 00:0d:88:53:3b:07 (oui Unknown), ethertype IPv4 (0x0800), length 1514: (tos 0x0, ttl 64, id 53637, offset 0, flags [+], proto ICMP (1), length 1500) gentoo-vm.transmode.se > 192.168.46.1: ICMP echo request, id 45593, seq 1, length 1480
21:46:27.499329 00:0f:fe:96:d6:7b (oui Unknown) > 00:0d:88:53:3b:07 (oui Unknown), ethertype IPv4 (0x0800), length 42: (tos 0x0, ttl 64, id 53637, offset 1480, flags [none], proto ICMP (1), length 28) gentoo-vm.transmode.se > 192.168.46.1: icmp
21:46:27.500299 00:0d:88:53:3b:07 (oui Unknown) > 00:0f:fe:96:d6:7b (oui Unknown), ethertype IPv4 (0x0800), length 1518: (tos 0x0, ttl 64, id 17057, offset 0, flags [+], proto ICMP (1), length 1500) 192.168.46.1 > gentoo-vm.transmode.se: ICMP echo reply, id 45593, seq 1, length 1480
21:46:27.500306 00:0d:88:53:3b:07 (oui Unknown) > 00:0f:fe:96:d6:7b (oui Unknown), ethertype IPv4 (0x0800), length 64: (tos 0x0, ttl 64, id 17057, offset 1480, flags [none], proto ICMP (1), length 28) 192.168.46.1 > gentoo-vm.transmode.se: icmp

Packet dump with patch:
1:54:49.651239 00:0f:fe:96:d6:7b (oui Unknown) > 00:0d:88:53:3b:07 (oui Unknown), ethertype IPv4 (0x0800), length 1514: (tos 0x0, ttl 64, id 28268, offset 0, flags [+], proto ICMP (1), length 1500) gentoo-vm.transmode.se > 192.168.46.1: ICMP echo request, id 39705, seq 1, length 1480
21:54:49.651244 00:0f:fe:96:d6:7b (oui Unknown) > 00:0d:88:53:3b:07 (oui Unknown), ethertype IPv4 (0x0800), length 42: (tos 0x0, ttl 64, id 28268, offset 1480, flags [none], proto ICMP (1), length 28) gentoo-vm.transmode.se > 192.168.46.1: icmp
21:54:49.652222 00:0d:88:53:3b:07 (oui Unknown) > 00:0f:fe:96:d6:7b (oui Unknown), ethertype IPv4 (0x0800), length 1514: (tos 0x0, ttl 64, id 17058, offset 0, flags [+], proto ICMP (1), length 1500) 192.168.46.1 > gentoo-vm.transmode.se: ICMP echo reply, id 39705, seq 1, length 1480
21:54:49.652228 00:0d:88:53:3b:07 (oui Unknown) > 00:0f:fe:96:d6:7b (oui Unknown), ethertype IPv4 (0x0800), length 60: (tos 0x0, ttl 64, id 17058, offset 1480, flags [none], proto ICMP (1), length 28) 192.168.46.1 > gentoo-vm.transmode.se: icmp
Comment 7 Andy Gospodarek 2008-02-12 13:11:19 UTC
damn mid-air collisions....

I can confirm this patch works fine on my setup (82571EB).  Without this patch
you simply add the device (eth6 in this case) to a bridge group, assign the
bridge group, an ip, and run these commands:

# tcpdump -e -v -p -n -i eth6 icmp&
# ping -s 1472 10.0.2.1

Without this patch, my output looks like this (edited so you can read it):

16:04:09.874119 00:15:17:00:20:10 > 00:16:d4:69:49:0b, ethertype IPv4 (0x0800),
length 1514
16:04:09.874756 00:16:d4:69:49:0b > 00:15:17:00:20:10, ethertype IPv4 (0x0800),
length 1518

with this patch, I now have the same size frames in both directions:

16:06:54.999694 00:15:17:00:20:10 > 00:16:d4:69:49:0b, ethertype IPv4 (0x0800),
length 1514
16:06:55.000149 00:16:d4:69:49:0b > 00:15:17:00:20:10, ethertype IPv4 (0x0800),
length 1514

I'd say this one is fine to post AFAICT, but confirmation from some other chips
(and from the reporter might be good).
Comment 8 Auke Kok 2008-02-15 10:23:15 UTC
Fix merged in jgarzik/netdev-2.6#upstream-fixes.