HW checksum offload problem on Xilinx Zynq GEM

When working on upgrading the v3.x kernels of our embedded devices to more recent 4.x kernels we noticed some of our proprietary networking stuff is broken. Further investigations brought up an issue with small UDP packets (data payload <= 2), which contained wrong UDP header checksums.
We tracked this down to commit 85ff3d87bf2ef1fadcde8553628c60f79806fdb4 net/macb: add TX checksum offload feature. It turns out that Zynq's GEM is obviously buggy regarding the UDP checksum calculation of such small UDP packets as long as the UDP checksum field is != 0 *BEFORE* the HW calulation. But since udp_send_skb() *ALWAYS* calculates the UDP header checksum (unless disabled via socket option), this is the usual case. Unfortunately it does not respect the net device feature setting which would leave UDP checksum untouched when checksum offloading is enabled.

This can be easily verfied by using the following sample code on a Zynq platform and tracing the relevant traffic on the receiving host:

/* testing Zynq's UDP checksum error
* arm-linux-gnueabihf-gcc -Wall -o udp-chksum-test udp-chksum-test.c */

static void sends(int fd, const char *str, const struct sockaddr_in *addr) {
sendto(fd, str, strlen(str), 0, (struct sockaddr *)addr,
sizeof(*addr));
}

int main(int argc, char *argv[])

{
        int s;
        struct sockaddr_in addr;

        addr.sin_family = AF_INET;
        addr.sin_port = htons(9991);
        addr.sin_addr.s_addr = inet_addr("192.168.1.1");
        s = socket(AF_INET, SOCK_DGRAM, 0);

        if (s < 0) {
                perror("socket");
                exit(1);
        }

        sends(s, "1", &addr);           /* not received, CSUM error */
        sends(s, "22", &addr);          /* not received, CSUM error */
        sends(s, "333", &addr);         /* OK */
        sends(s, "4444", &addr);        /* OK */

        close(s);
        return 0;
}

Before turning off HW checksum offload feature we get 'bad udp cksum':

$ tcpdump -t -c 4 -vv -n -i eth1 port 9991 2> /dev/null | fold -w 75 -s
IP (tos 0x0, ttl 64, id 55613, offset 0, flags [DF], proto UDP (17),
length 29)
    192.168.4.2.55734 > 192.168.1.1.9991: [bad udp cksum 0xc15b ->
0x47ca!] UDP, length 1
IP (tos 0x0, ttl 64, id 55614, offset 0, flags [DF], proto UDP (17),
length 30)
    192.168.4.2.55734 > 192.168.1.1.9991: [bad udp cksum 0xc026 ->
0x4696!] UDP, length 2
IP (tos 0x0, ttl 64, id 55615, offset 0, flags [DF], proto UDP (17),
length 31)
    192.168.4.2.55734 > 192.168.1.1.9991: [udp sum ok] UDP, length 3
IP (tos 0x0, ttl 64, id 55616, offset 0, flags [DF], proto UDP (17),
length 32)
    192.168.4.2.55734 > 192.168.1.1.9991: [udp sum ok] UDP, length 4

Without HW checksum offloading everything is fine:

$ tcpdump -t -c 4 -vv -n -i eth1 port 9991 2> /dev/null | fold -w 75 -s
IP (tos 0x0, ttl 64, id 63227, offset 0, flags [DF], proto UDP (17),
length 29)
    192.168.4.2.44439 > 192.168.1.1.9991: [udp sum ok] UDP, length 1
IP (tos 0x0, ttl 64, id 63228, offset 0, flags [DF], proto UDP (17),
length 30)
    192.168.4.2.44439 > 192.168.1.1.9991: [udp sum ok] UDP, length 2
IP (tos 0x0, ttl 64, id 63229, offset 0, flags [DF], proto UDP (17),
length 31)
    192.168.4.2.44439 > 192.168.1.1.9991: [udp sum ok] UDP, length 3
IP (tos 0x0, ttl 64, id 63230, offset 0, flags [DF], proto UDP (17),
length 32)
    192.168.4.2.44439 > 192.168.1.1.9991: [udp sum ok] UDP, length 4

This issue might also affect other SoCs using Cadence MACB/GEM implementations. Since I cannot verify this due to lack of hardware, this solution is restricted to Xilinx Zynq7000.

Helmut Buchsbaum (1):
net: macb: disable hardware checksum offload for Xilinx Zynq

 drivers/net/ethernet/cadence/macb.c | 6 ++++--
 drivers/net/ethernet/cadence/macb.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

2.1.4

When sending UDP packets with data payload size <= 2, Zynq's GEM implementation for HW checksum offloading calculates wrong checksums.

Adding a MACB_CAPS_NO_CSUM_OFFLOAD capability and turning off HW checksum offloading for Zynq solves it.

---
 drivers/net/ethernet/cadence/macb.c | 6 ++++--
 drivers/net/ethernet/cadence/macb.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

index 89c0cfa..26eba1a 100644
@@ -2430,7 +2430,8 @@ static int macb_init(struct platform_device *pdev)
 	/* Set features */
 	dev->hw_features = NETIF_F_SG;
 	/* Checksum offload is only available on gem with packet buffer */
-	if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE))
+	if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE) &&
+	    !(bp->caps & MACB_CAPS_NO_CSUM_OFFLOAD))
 		dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
 	if (bp->caps & MACB_CAPS_SG_DISABLED)
 		dev->hw_features &= ~NETIF_F_SG;

@@ -2829,7 +2830,8 @@ static const struct macb_config zynqmp_config = {
 };
 
 static const struct macb_config zynq_config = {
-	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF,
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_NO_GIGABIT_HALF |
+		MACB_CAPS_NO_CSUM_OFFLOAD,
 	.dma_burst_length = 16,
 	.clk_init = macb_clk_init,
 	.init = macb_init,

index b6fcf10..79193db 100644
@@ -404,6 +404,7 @@
 #define MACB_CAPS_NO_GIGABIT_HALF		0x00000008
 #define MACB_CAPS_USRIO_DISABLED		0x00000010
 #define MACB_CAPS_JUMBO				0x00000020
+#define MACB_CAPS_NO_CSUM_OFFLOAD		0x08000000
 #define MACB_CAPS_FIFO_MODE			0x10000000
 #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
 #define MACB_CAPS_SG_DISABLED			0x40000000

2.1.4

When working on upgrading the v3.x kernels of our embedded devices to more recent 4.x kernels we noticed some of our proprietary networking stuff is broken. Further investigations brought up an issue with small UDP packets (data payload <= 2), which contained wrong UDP header checksums.
We tracked this down to commit 85ff3d87bf2ef1fadcde8553628c60f79806fdb4 net/macb: add TX checksum offload feature.

It turns out that (at least) Zynq's GEM needs the checksum field set to 0 to correctly calculate the checksum.

Changes since v1:
- dropped disabling HW checksum offload for Zynq
- initialize checksum similar to net/ethernet/freescale/fec_main.c

Helmut Buchsbaum (1):
net: macb: initialize checksum when using checksum offloading

 drivers/net/ethernet/cadence/macb.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

2.1.4

MACB/GEM needs the checksum field initialized to 0 to get correct results on transmit in all cases, e.g. on Zynq, UDP packets with payload <= 2 otherwise contain a wrong checksums.

---
 drivers/net/ethernet/cadence/macb.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

index 89c0cfa..de2f791 100644
@@ -1323,6 +1323,19 @@ dma_error:
 	return 0;
 }
 
+static inline void macb_clear_csum(struct sk_buff *skb)
+{
+	/* no change for packets without checksum offloading */
+	if (skb->ip_summed != CHECKSUM_PARTIAL)
+		return;
+
+	/* initialize checksum field
+	 * This is required - at least for Zynq, which otherwise calculates
+	 * wrong UDP header checksums for UDP packets with UDP data len <=2
+	 */
+	*(__sum16 *)(skb->head + skb->csum_start + skb->csum_offset) = 0;
+}
+
 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	u16 queue_index = skb_get_queue_mapping(skb);

@@ -1362,6 +1375,8 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_BUSY;
 	}
 
+	macb_clear_csum(skb);
+
 	/* Map socket buffer for DMA transfer */
 	if (!macb_tx_map(bp, queue, skb)) {
 		dev_kfree_skb_any(skb);

2.1.4

From: Helmut Buchsbaum <[email protected]>
Date: Mon, 29 Aug 2016 15:57:25 +0200

I'm still struggling to get this fix right..

Changes since v2:
- do not blindly modify SKB contents according to Dave's legitimate objection

Changes since v1:
- dropped disabling HW checksum offload for Zynq
- initialize checksum similar to net/ethernet/freescale/fec_main.c

-- >8 --
MACB/GEM needs the checksum field initialized to 0 to get correct results on transmit in all cases, e.g. on Zynq, UDP packets with payload <= 2 otherwise contain a wrong checksums.

---
 drivers/net/ethernet/cadence/macb.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

index 89c0cfa..d954a97 100644
@@ -1323,6 +1323,24 @@ dma_error:
 	return 0;
 }
 
+static inline int macb_clear_csum(struct sk_buff *skb)
+{
+	/* no change for packets without checksum offloading */
+	if (skb->ip_summed != CHECKSUM_PARTIAL)
+		return 0;
+
+	/* make sure we can modify the header */
+	if (unlikely(skb_cow_head(skb, 0)))
+		return -1;
+
+	/* initialize checksum field
+	 * This is required - at least for Zynq, which otherwise calculates
+	 * wrong UDP header checksums for UDP packets with UDP data len <=2
+	 */
+	*(__sum16 *)(skb_checksum_start(skb) + skb->csum_offset) = 0;
+	return 0;
+}
+
 static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	u16 queue_index = skb_get_queue_mapping(skb);

@@ -1362,6 +1380,11 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_BUSY;
 	}
 
+	if (macb_clear_csum(skb)) {
+		dev_kfree_skb_any(skb);
+		return NETDEV_TX_OK;
+	}
+
 	/* Map socket buffer for DMA transfer */
 	if (!macb_tx_map(bp, queue, skb)) {
 		dev_kfree_skb_any(skb);

2.1.4

From: Helmut Buchsbaum <[email protected]>
Date: Sun, 4 Sep 2016 18:09:47 +0200

Applied.

From: Helmut Buchsbaum <[email protected]>
Date: Fri, 19 Aug 2016 15:04:57 +0200

Then simply clear the checksum field in the driver, or fix udp_send_skb() to do what you claim it is supposed to.

That seems like a much more appropriate fix to me.

Fix the driver to do that. When offloading a UDP checksum the pseudo header checksum is written into the checksum field-- that's the interface that everyone else has been using without issue.

Thanks for pointing me in the right direction. I'm just verifying a patch update I'll provide soon!