net: sungem: fix rx checksum support

PATCH In review Eric Dumazet 5 participants 7 messages Jun 20, 2018 – Aug 27, 2018

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Eric Dumazet

author

2018-06-20 02:18 UTC #0 lore raw

PATCH 1/1 net: sungem: fix rx checksum support

After commit 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"), sungem owners reported the infamous "eth0: hw csum failure" message.

CHECKSUM_COMPLETE has in fact never worked for this driver, but this was masked by the fact that upper stacks had to strip the FCS, and therefore skb->ip_summed was set back to CHECKSUM_NONE before my recent change.

Driver configures a number of bytes to skip when the chip computes the checksum, and for some reason only half of the Ethernet header was skipped.

Then a second problem is that we should strip the FCS by default, unless the driver is updated to eventually support NETIF_F_RXFCS in the future.

Finally, a driver should check if NETIF_F_RXCSUM feature is enabled or not, so that the admin can turn off rx checksum if wanted.

Many thanks to Andreas Schwab and Mathieu Malaterre for their help in debugging this issue.

Signed-off-by Eric Dumazet <[email protected]>

Reported-by Meelis Roos <[email protected]>

Reported-by Mathieu Malaterre <[email protected]>

Reported-by Andreas Schwab <[email protected]>

Tested-by Andreas Schwab <[email protected]>

---
 drivers/net/ethernet/sun/sungem.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

drivers/net/ethernet/sun/sungem.c +12 ~~-10~~

index 7a16d40a72d13cf1d522e8a3a396c826fe76f9b9..b9221fc1674dfa0ef17a43f8ff86d700a1ae514f 100644
@@ -60,8 +60,7 @@
 #include <linux/sungem_phy.h>
 #include "sungem.h"
 
-/* Stripping FCS is causing problems, disabled for now */
-#undef STRIP_FCS
+#define STRIP_FCS
 
 #define DEFAULT_MSG	(NETIF_MSG_DRV		| \
 			 NETIF_MSG_PROBE	| \

@@ -435,7 +434,7 @@ static int gem_rxmac_reset(struct gem *gp)
 	writel(desc_dma & 0xffffffff, gp->regs + RXDMA_DBLOW);
 	writel(RX_RING_SIZE - 4, gp->regs + RXDMA_KICK);
 	val = (RXDMA_CFG_BASE | (RX_OFFSET << 10) |
-	       ((14 / 2) << 13) | RXDMA_CFG_FTHRESH_128);
+	       (ETH_HLEN << 13) | RXDMA_CFG_FTHRESH_128);
 	writel(val, gp->regs + RXDMA_CFG);
 	if (readl(gp->regs + GREG_BIFCFG) & GREG_BIFCFG_M66EN)
 		writel(((5 & RXDMA_BLANK_IPKTS) |

@@ -760,7 +759,6 @@ static int gem_rx(struct gem *gp, int work_to_do)
 	struct net_device *dev = gp->dev;
 	int entry, drops, work_done = 0;
 	u32 done;
-	__sum16 csum;
 
 	if (netif_msg_rx_status(gp))
 		printk(KERN_DEBUG "%s: rx interrupt, done: %d, rx_new: %d\n",

@@ -855,9 +853,13 @@ static int gem_rx(struct gem *gp, int work_to_do)
 			skb = copy_skb;
 		}
 
-		csum = (__force __sum16)htons((status & RXDCTRL_TCPCSUM) ^ 0xffff);
-		skb->csum = csum_unfold(csum);
-		skb->ip_summed = CHECKSUM_COMPLETE;
+		if (likely(dev->features & NETIF_F_RXCSUM)) {
+			__sum16 csum;
+
+			csum = (__force __sum16)htons((status & RXDCTRL_TCPCSUM) ^ 0xffff);
+			skb->csum = csum_unfold(csum);
+			skb->ip_summed = CHECKSUM_COMPLETE;
+		}
 		skb->protocol = eth_type_trans(skb, gp->dev);
 
 		napi_gro_receive(&gp->napi, skb);

@@ -1761,7 +1763,7 @@ static void gem_init_dma(struct gem *gp)
 	writel(0, gp->regs + TXDMA_KICK);
 
 	val = (RXDMA_CFG_BASE | (RX_OFFSET << 10) |
-	       ((14 / 2) << 13) | RXDMA_CFG_FTHRESH_128);
+	       (ETH_HLEN << 13) | RXDMA_CFG_FTHRESH_128);
 	writel(val, gp->regs + RXDMA_CFG);
 
 	writel(desc_dma >> 32, gp->regs + RXDMA_DBHI);

@@ -2985,8 +2987,8 @@ static int gem_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	pci_set_drvdata(pdev, dev);
 
 	/* We can do scatter/gather and HW checksum */
-	dev->hw_features = NETIF_F_SG | NETIF_F_HW_CSUM;
-	dev->features |= dev->hw_features | NETIF_F_RXCSUM;
+	dev->hw_features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
+	dev->features = dev->hw_features;
 	if (pci_using_dac)
 		dev->features |= NETIF_F_HIGHDMA;

Signature · 1 line

2.18.0.rc1.244.gcf134e6275-goog

David Miller

maintainer

2018-06-20 05:30 UTC #1 lore raw

From: Eric Dumazet <[email protected]>
Date: Tue, 19 Jun 2018 19:18:50 -0700

Quoted 20 lines

After commit 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"), sungem owners reported the infamous "eth0: hw csum failure" message.
CHECKSUM_COMPLETE has in fact never worked for this driver, but this was masked by the fact that upper stacks had to strip the FCS, and therefore skb->ip_summed was set back to CHECKSUM_NONE before my recent change.
Driver configures a number of bytes to skip when the chip computes the checksum, and for some reason only half of the Ethernet header was skipped.
Then a second problem is that we should strip the FCS by default, unless the driver is updated to eventually support NETIF_F_RXFCS in the future.
Finally, a driver should check if NETIF_F_RXCSUM feature is enabled or not, so that the admin can turn off rx checksum if wanted.

Quoted 8 lines

Many thanks to Andreas Schwab and Mathieu Malaterre for their help in debugging this issue.
Signed-off-by Eric Dumazet <[email protected]>
Reported-by Meelis Roos <[email protected]>
Reported-by Mathieu Malaterre <[email protected]>
Reported-by Andreas Schwab <[email protected]>
Tested-by Andreas Schwab <[email protected]>

Applied and queued up for -stable, thanks Eric.

Mathieu Malaterre

2018-06-20 11:22 UTC #2 lore raw

Quoted 27 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Tue, 19 Jun 2018 19:18:50 -0700
Quoted 23 lines
After commit 88078d98d1bb ("net: pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends"), sungem owners reported the infamous "eth0: hw csum failure" message.
CHECKSUM_COMPLETE has in fact never worked for this driver, but this was masked by the fact that upper stacks had to strip the FCS, and therefore skb->ip_summed was set back to CHECKSUM_NONE before my recent change.
Driver configures a number of bytes to skip when the chip computes the checksum, and for some reason only half of the Ethernet header was skipped.
Then a second problem is that we should strip the FCS by default, unless the driver is updated to eventually support NETIF_F_RXFCS in the future.
Finally, a driver should check if NETIF_F_RXCSUM feature is enabled or not, so that the admin can turn off rx checksum if wanted.
Many thanks to Andreas Schwab and Mathieu Malaterre for their help in debugging this issue.

Quoted 7 lines

Quoted 5 lines
Signed-off-by Eric Dumazet <[email protected]>
Reported-by Meelis Roos <[email protected]>
Reported-by Mathieu Malaterre <[email protected]>
Reported-by Andreas Schwab <[email protected]>
Tested-by Andreas Schwab <[email protected]>
Applied and queued up for -stable, thanks Eric.

Thanks for the stable tag.

IMHO the commit message should have also reference commit 7ce5a27f2ef8 ("Revert "net: Handle CHECKSUM_COMPLETE more adequately in pskb_trim_rcsum().""). Which means that commit 88078d98d1bb ("net:
pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends") should be ready for stable also, since it seems that the only driver responsible for the revert was sungem.

my 2cts

Eric Dumazet

2018-06-20 12:49 UTC #3 lore raw

Quoted 11 lines · Mathieu Malaterre

Thanks for the stable tag.
IMHO the commit message should have also reference commit 7ce5a27f2ef8 ("Revert "net: Handle CHECKSUM_COMPLETE more adequately in pskb_trim_rcsum().""). Which means that commit 88078d98d1bb ("net:
pskb_trim_rcsum() and CHECKSUM_COMPLETE are friends") should be ready for stable also, since it seems that the only driver responsible for the revert was sungem.
my 2cts

I disagree.

commit 88078d98d1bb is a performance improvement, and not needed for stable.

We might also discover another buggy driver later.

Thanks.

Eric Dumazet

2018-06-22 04:21 UTC #4 lore raw

In reply to David Miller

Quoted 4 lines · David Miller

Tested-by Andreas Schwab <[email protected]>
Applied and queued up for -stable, thanks Eric.

BTW, removing the FCS also means GRO is going to work, finally on this NIC ;)

GRO does not like packets with padding.

[email protected]

2018-08-26 16:57 UTC #5 lore raw

Quoted 3 lines

BTW, removing the FCS also means GRO is going to work, finally on this NIC ;)
GRO does not like packets with padding.

As a follow-up, I am seeing hw csum failures on Sun V440 that has onboard Sun Cassini with sungem driver. First tested version was 4.18 (it happened there once) and now that I tried 4.18+git, it still happens:

[   21.563282] libphy: Fixed MDIO Bus: probed
[   21.617116] cassini: cassini.c:v1.6 (21 May 2008)
[   21.678962] cassini 0000:00:02.0: enabling device (0144 -> 0146)
[   21.761931] cassini 0000:00:02.0 eth0: Sun Cassini+ (64bit/66MHz PCI/Cu) Ethernet[6] 00:03:ba:6f:14:39
[   21.884952] cassini 0003:00:01.0: enabling device (0144 -> 0146)
[   21.967868] cassini 0003:00:01.0 eth1: Sun Cassini+ (64bit/66MHz PCI/Cu) Ethernet[29] 00:03:ba:6f:14:3a
[...]
[   54.341212] eth0: hw csum failure
[   54.384725] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 4.18.0-12952-g2923b27 #1397
[   54.483167] Call Trace:
[   54.515209]  [000000000077838c] __skb_checksum_complete+0xcc/0xe0
[   54.595272]  [000000000080fc84] igmp_rcv+0x224/0x920
[   54.660475]  [00000000007ca3d0] ip_local_deliver+0xb0/0x240
[   54.733675]  [00000000007ca5c0] ip_rcv+0x60/0xa0
[   54.794304]  [0000000000781a30] __netif_receive_skb_one_core+0x30/0x60
[   54.880094]  [0000000000782914] process_backlog+0x94/0x140
[   54.952161]  [0000000000788f6c] net_rx_action+0x1ec/0x320
[   55.023083]  [0000000000870de8] __do_softirq+0xc8/0x200
[   55.091719]  [000000000042c4cc] do_softirq_own_stack+0x2c/0x40
[   55.168362]  [00000000004662d8] irq_exit+0xb8/0xe0
[   55.231266]  [0000000000870ac0] handler_irq+0xc0/0x100
[   55.298756]  [00000000004208b4] tl0_irq5+0x14/0x20
[   55.361670]  [000000000042cafc] arch_cpu_idle+0x9c/0xa0
[   55.447055]  [000000000048a254] cpu_startup_entry+0x14/0x40
[   55.536998]  [000000000095f4b4] 0x95f4b4
[   55.588471]  [0000000040000000] 0x40000000
[  179.780371] eth0: hw csum failure
[  179.823878] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.18.0-12952-g2923b27 #1397
[  179.922230] Call Trace:
[  179.954267]  [000000000077838c] __skb_checksum_complete+0xcc/0xe0
[  180.034335]  [000000000080fc84] igmp_rcv+0x224/0x920
[  180.099536]  [00000000007ca3d0] ip_local_deliver+0xb0/0x240
[  180.172740]  [00000000007ca5c0] ip_rcv+0x60/0xa0
[  180.233368]  [0000000000781a30] __netif_receive_skb_one_core+0x30/0x60
[  180.319159]  [0000000000782914] process_backlog+0x94/0x140
[  180.391225]  [0000000000788f6c] net_rx_action+0x1ec/0x320
[  180.462148]  [0000000000870de8] __do_softirq+0xc8/0x200
[  180.530782]  [000000000042c4cc] do_softirq_own_stack+0x2c/0x40
[  180.607422]  [00000000004662d8] irq_exit+0xb8/0xe0
[  180.670331]  [0000000000870ac0] handler_irq+0xc0/0x100
[  180.737822]  [00000000004208b4] tl0_irq5+0x14/0x20
[  180.800735]  [000000000042caf8] arch_cpu_idle+0x98/0xa0
[  180.869373]  [0000000000489f60] do_idle+0xe0/0x1c0
[  180.932281]  [000000000048a25c] cpu_startup_entry+0x1c/0x40
[  181.005491]  [000000000098e9b4] start_kernel+0x3b8/0x3c8

Signature · 1 line

Meelis Roos ([email protected])

Eric Dumazet

author

2018-08-27 20:45 UTC #6 lore raw

Quoted 47 lines · On Sun, Aug 26, 2018 at 6:14 AM <[email protected]> wrote:

Quoted 4 lines

BTW, removing the FCS also means GRO is going to work, finally on this NIC ;)
GRO does not like packets with padding.

[   21.563282] libphy: Fixed MDIO Bus: probed
[   21.617116] cassini: cassini.c:v1.6 (21 May 2008)
[   21.678962] cassini 0000:00:02.0: enabling device (0144 -> 0146)
[   21.761931] cassini 0000:00:02.0 eth0: Sun Cassini+ (64bit/66MHz PCI/Cu) Ethernet[6] 00:03:ba:6f:14:39
[   21.884952] cassini 0003:00:01.0: enabling device (0144 -> 0146)
[   21.967868] cassini 0003:00:01.0 eth1: Sun Cassini+ (64bit/66MHz PCI/Cu) Ethernet[29] 00:03:ba:6f:14:3a
[...]
[   54.341212] eth0: hw csum failure
[   54.384725] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 4.18.0-12952-g2923b27 #1397
[   54.483167] Call Trace:
[   54.515209]  [000000000077838c] __skb_checksum_complete+0xcc/0xe0
[   54.595272]  [000000000080fc84] igmp_rcv+0x224/0x920
[   54.660475]  [00000000007ca3d0] ip_local_deliver+0xb0/0x240
[   54.733675]  [00000000007ca5c0] ip_rcv+0x60/0xa0
[   54.794304]  [0000000000781a30] __netif_receive_skb_one_core+0x30/0x60
[   54.880094]  [0000000000782914] process_backlog+0x94/0x140
[   54.952161]  [0000000000788f6c] net_rx_action+0x1ec/0x320
[   55.023083]  [0000000000870de8] __do_softirq+0xc8/0x200
[   55.091719]  [000000000042c4cc] do_softirq_own_stack+0x2c/0x40
[   55.168362]  [00000000004662d8] irq_exit+0xb8/0xe0
[   55.231266]  [0000000000870ac0] handler_irq+0xc0/0x100
[   55.298756]  [00000000004208b4] tl0_irq5+0x14/0x20
[   55.361670]  [000000000042cafc] arch_cpu_idle+0x9c/0xa0
[   55.447055]  [000000000048a254] cpu_startup_entry+0x14/0x40
[   55.536998]  [000000000095f4b4] 0x95f4b4
[   55.588471]  [0000000040000000] 0x40000000
[  179.780371] eth0: hw csum failure
[  179.823878] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.18.0-12952-g2923b27 #1397
[  179.922230] Call Trace:
[  179.954267]  [000000000077838c] __skb_checksum_complete+0xcc/0xe0
[  180.034335]  [000000000080fc84] igmp_rcv+0x224/0x920
[  180.099536]  [00000000007ca3d0] ip_local_deliver+0xb0/0x240
[  180.172740]  [00000000007ca5c0] ip_rcv+0x60/0xa0
[  180.233368]  [0000000000781a30] __netif_receive_skb_one_core+0x30/0x60
[  180.319159]  [0000000000782914] process_backlog+0x94/0x140
[  180.391225]  [0000000000788f6c] net_rx_action+0x1ec/0x320
[  180.462148]  [0000000000870de8] __do_softirq+0xc8/0x200

Quoted 8 lines

[  180.530782]  [000000000042c4cc] do_softirq_own_stack+0x2c/0x40
[  180.607422]  [00000000004662d8] irq_exit+0xb8/0xe0
[  180.670331]  [0000000000870ac0] handler_irq+0xc0/0x100
[  180.737822]  [00000000004208b4] tl0_irq5+0x14/0x20
[  180.800735]  [000000000042caf8] arch_cpu_idle+0x98/0xa0
[  180.869373]  [0000000000489f60] do_idle+0xe0/0x1c0
[  180.932281]  [000000000048a25c] cpu_startup_entry+0x1c/0x40
[  181.005491]  [000000000098e9b4] start_kernel+0x3b8/0x3c8

Note that these traces are for non TCP packets.

I suspect the driver should not provide CHECKSUM_COMPLETE for non TCP packets, maybe the NIC is mishandling this case.