Gianfar TCP checksumming broken in 2.6.35+

Gianfar TCP checksumming broken in 2.6.35+

[Matthew L. Creech]

Hi,

An upgrade from 2.6.34 to 2.6.35 caused networking to stop working on
my MPC8313-based board.  It turned out that TCP checksums were
invalid, so I dug through the .35 changelog to try and isolate the
reason.  The change "tcp: Set CHECKSUM_UNNECESSARY in
tcp_init_nondata_skb" seems to be the specific one that causes
breakage - if I revert this one-liner, things work again:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=2e8e18ef52e7dd1af0a3bd1f7d990a1d0b249586

However, I also noticed that one of my boards was broken while a newer
prototype (which is very similar, hardware-wise) was not.  It turns
out they're using 2 different revisions of silicon, so the broken
board still has a 1.0 version microcontroller.

Therefore I'm guessing (just a hunch) that the root cause of the
problem is MPC8313 errata eTSEC12:

========
eTSEC12: Tx IP and TCP/UDP Checksum Generation not supported for some Tx FCB
         offsets
Description:
	If the Tx FCB (Frame Control Block) 32-byte offset is 0x19, 0x1A, 0x1B,
	0x1C, 0x1D, 0x1E or 0x1F, IP and TCP/UDP header checksum generation do
	not function properly. The checksum value may be inserted in the wrong
	location or not inserted at all.
	IP and TCP/UDP header checksum generation is not supported in LINUX
	and other systems in which headers are prepended to pre-aligned packet
	data, or where the alignment of the Tx FCB cannot be controlled.
	This behavior applies to pseudo-header checksum insertion as well as
	checksum generation.
Workaround:
	Align Tx FCB to a 16 or 32-byte boundary.
	If the alignment of TxFCB is not controllable, set TCTRL[TUCSEN]=0 and
	TCTRL[IPCSEN]=0 to disable IP and TCP/UDP header checksum generation.
Fix plan:
	Fixed in Rev 2.0
========

This appears to have been working previously, but doesn't work any
more.  I'm not familiar enough with Dave's checksum/sk_buff changes to
figure out whether this errata is to blame, though, or how I should
fix it if it is.  Presumably there's some alignment magic needed in
the sk_buff or gfar_add_fcb() to make sure that the microcontroller is
happy with the FCB offset?

Any tip on how I can solve this, or at least verify that this errata
is at fault?  Thanks in advance

-- 
Matthew L. Creech

Re: Gianfar TCP checksumming broken in 2.6.35+

[David Miller]

From: "Matthew L. Creech" <mlcreech@gmail.com>
Date: Tue, 2 Nov 2010 18:29:08 -0400

> An upgrade from 2.6.34 to 2.6.35 caused networking to stop working on
> my MPC8313-based board.  It turned out that TCP checksums were
> invalid, so I dug through the .35 changelog to try and isolate the
> reason.  The change "tcp: Set CHECKSUM_UNNECESSARY in
> tcp_init_nondata_skb" seems to be the specific one that causes
> breakage - if I revert this one-liner, things work again:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=2e8e18ef52e7dd1af0a3bd1f7d990a1d0b249586
> 
> However, I also noticed that one of my boards was broken while a newer
> prototype (which is very similar, hardware-wise) was not.  It turns
> out they're using 2 different revisions of silicon, so the broken
> board still has a 1.0 version microcontroller.
> 
> Therefore I'm guessing (just a hunch) that the root cause of the
> problem is MPC8313 errata eTSEC12:
> 
> ========
> eTSEC12: Tx IP and TCP/UDP Checksum Generation not supported for some Tx FCB
>          offsets
> Description:
> 	If the Tx FCB (Frame Control Block) 32-byte offset is 0x19, 0x1A, 0x1B,
> 	0x1C, 0x1D, 0x1E or 0x1F, IP and TCP/UDP header checksum generation do
> 	not function properly. The checksum value may be inserted in the wrong
> 	location or not inserted at all.
> 	IP and TCP/UDP header checksum generation is not supported in LINUX
> 	and other systems in which headers are prepended to pre-aligned packet
> 	data, or where the alignment of the Tx FCB cannot be controlled.
> 	This behavior applies to pseudo-header checksum insertion as well as
> 	checksum generation.
> Workaround:
> 	Align Tx FCB to a 16 or 32-byte boundary.
> 	If the alignment of TxFCB is not controllable, set TCTRL[TUCSEN]=0 and
> 	TCTRL[IPCSEN]=0 to disable IP and TCP/UDP header checksum generation.
> Fix plan:
> 	Fixed in Rev 2.0
> ========
> 
> This appears to have been working previously, but doesn't work any
> more.  I'm not familiar enough with Dave's checksum/sk_buff changes to
> figure out whether this errata is to blame, though, or how I should
> fix it if it is.  Presumably there's some alignment magic needed in
> the sk_buff or gfar_add_fcb() to make sure that the microcontroller is
> happy with the FCB offset?
> 
> Any tip on how I can solve this, or at least verify that this errata
> is at fault?  Thanks in advance

Can someone please follow up Matthew to get this bug resolved?  It has
been sitting around for a long time.

I suspect the gianfar driver, for these chip revisions, will need to
do a software checksum when the offset matches the criteria mentioned
in the errata above.

Re: Gianfar TCP checksumming broken in 2.6.35+

[Matthew L. Creech]

On Thu, Nov 18, 2010 at 12:06 PM, David Miller <davem@davemloft.net> wrote:
>
> Can someone please follow up Matthew to get this bug resolved? =A0It has
> been sitting around for a long time.
>
> I suspect the gianfar driver, for these chip revisions, will need to
> do a software checksum when the offset matches the criteria mentioned
> in the errata above.
>

I added a patch for this which fixes our affected systems; however, I
don't know if this is a good way to perform checksum offloading, I
just kind of dug around until I found a function that seemed like it
worked.  :)  Patch against 2.6.36 is below for reference.


---
 gianfar.c |   21 +++++++++++++++++++--
 gianfar.h |    1 +
 2 files changed, 20 insertions(+), 2 deletions(-)

diff -purN orig/drivers/net/gianfar.c linux-2.6.36/drivers/net/gianfar.c
--- orig/drivers/net/gianfar.c	2010-11-03 15:10:29.287140651 -0400
+++ linux-2.6.36/drivers/net/gianfar.c	2010-11-03 16:01:03.754321896 -0400
@@ -937,6 +937,10 @@ static void gfar_detect_errata(struct gf
 	unsigned int mod =3D (svr >> 16) & 0xfff6; /* w/o E suffix */
 	unsigned int rev =3D svr & 0xffff;

+	/* MPC8313 Rev < 2.0 */
+	if (pvr =3D=3D 0x80850010 && mod =3D=3D 0x80b0 && rev < 0x0020)
+		priv->errata |=3D GFAR_ERRATA_12;
+
 	/* MPC8313 Rev 2.0 and higher; All MPC837x */
 	if ((pvr =3D=3D 0x80850010 && mod =3D=3D 0x80b0 && rev >=3D 0x0020) ||
 			(pvr =3D=3D 0x80861010 && (mod & 0xfff9) =3D=3D 0x80c0))
@@ -1984,7 +1988,8 @@ static inline struct txfcb *gfar_add_fcb
 	return fcb;
 }

-static inline void gfar_tx_checksum(struct sk_buff *skb, struct txfcb *fcb=
)
+static inline void gfar_tx_checksum(struct sk_buff *skb, struct txfcb *fcb=
,
+				int has_csum_bug)
 {
 	u8 flags =3D 0;

@@ -1994,6 +1999,17 @@ static inline void gfar_tx_checksum(stru
 	 */
 	flags =3D TXFCB_DEFAULT;

+	/* If using old-rev silicon, the alignment of the TXFCB may be off,
+	 * causing TCP checksumming to fail (errata eTSEC12).  In that case,
+	 * we compute the checksum manually.
+	 */
+	if (has_csum_bug) {
+		/* Disable handling of TCP/UDP header (checksumming) */
+		flags &=3D ~TXFCB_TUP;
+		/* Manually add checksum */
+		skb_checksum_help(skb);
+	}
+
 	/* Tell the controller what the protocol is */
 	/* And provide the already calculated phcs */
 	if (ip_hdr(skb)->protocol =3D=3D IPPROTO_UDP) {
@@ -2159,7 +2175,8 @@ static int gfar_start_xmit(struct sk_buf
 	if (CHECKSUM_PARTIAL =3D=3D skb->ip_summed) {
 		fcb =3D gfar_add_fcb(skb);
 		lstatus |=3D BD_LFLAG(TXBD_TOE);
-		gfar_tx_checksum(skb, fcb);
+		gfar_tx_checksum(skb, fcb,
+				gfar_has_errata(priv, GFAR_ERRATA_12));
 	}

 	if (priv->vlgrp && vlan_tx_tag_present(skb)) {
diff -purN orig/drivers/net/gianfar.h linux-2.6.36/drivers/net/gianfar.h
--- orig/drivers/net/gianfar.h	2010-11-03 15:10:29.257142194 -0400
+++ linux-2.6.36/drivers/net/gianfar.h	2010-11-03 15:48:10.117134959 -0400
@@ -1029,6 +1029,7 @@ enum gfar_errata {
 	GFAR_ERRATA_74		=3D 0x01,
 	GFAR_ERRATA_76		=3D 0x02,
 	GFAR_ERRATA_A002	=3D 0x04,
+	GFAR_ERRATA_12		=3D 0x08,
 };

 /* Struct stolen almost completely (and shamelessly) from the FCC enet sou=
rce

--=20
Matthew L. Creech

Re: Gianfar TCP checksumming broken in 2.6.35+

[David Miller]

From: "Matthew L. Creech" <mlcreech@gmail.com>
Date: Thu, 18 Nov 2010 14:31:46 -0500

> On Thu, Nov 18, 2010 at 12:06 PM, David Miller <davem@davemloft.net> =
wrote:
>>
>> Can someone please follow up Matthew to get this bug resolved? =A0It=
 has
>> been sitting around for a long time.
>>
>> I suspect the gianfar driver, for these chip revisions, will need to=

>> do a software checksum when the offset matches the criteria mentione=
d
>> in the errata above.
>>
> =

> I added a patch for this which fixes our affected systems; however, I=

> don't know if this is a good way to perform checksum offloading, I
> just kind of dug around until I found a function that seemed like it
> worked.  :)  Patch against 2.6.36 is below for reference.

It looks fine except I would limit the software checksum to the
exact conditions listed in the errate.

Otherwise this is going to hurt performance and cpu utilization
quite a bit.

Re: Gianfar TCP checksumming broken in 2.6.35+

[Alex Dubov]

It appears that I'm hitting an exactly the same problem with my MPC8548=0Ar=
ev2.0 (errata number eTSEC 49).=0A=0AConsidering that it's close to 3 month=
 now since this most unfortunate=0Abug was reported, was there any resoluti=
on/patch that can fix it for good?=0A=0A=0A=0A