Skip to contentSkip to conversation

r8169 crash (probably my fault) in 3.3.0-rc1+ (net-next)

DISCUSSION Ben Greear 2 participants 4 messages Feb 9, 2012 – Feb 10, 2012

Subsystems (3)
lore
Share & tools
lore

Ben Greear

author
#0 lore raw

I am hacking on the 8169 code, (adding RX-ALL and RX-FCS support on top of my previously posted patches) so this could easily be my fault..but just in case someone else has seen it, please
let me know....

BUG: unable to handle kernel NULL pointer dereference at 00000c9c
IP: [<c07083f6>] skb_gro_reset_offset+0x3a/0x73
*pde = 00000000
Oops: 0000 [#1] SMP
Modules linked in: bridge 8021q garp stp llc ip_gre gre veth fuse macvlan pktgen coretemp hwmon nfs lockd]

Pid: 7659, comm: tshark Not tainted 3.3.0-rc1+ #25 To Be Filled By O.E.M. To Be Filled By O.E.M./To be fi.
EIP: 0060:[<c07083f6>] EFLAGS: 00210246 CPU: 0
EIP is at skb_gro_reset_offset+0x3a/0x73
EAX: 00000c9c EBX: f16353c0 ECX: f175d030 EDX: f16353c0
ESI: f16353d8 EDI: f4e47c68 EBP: f580bf48 ESP: f580bf3c
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process tshark (pid: 7659, ti=f580a000 task=f14ede60 task.ti=f1444000)

Stack:
 f16353c0 f4d184cc f4e47c40 f580bf58 c070c1ad f4d18000 00000002 f580bfb0
 f88fff68 f4d18bb4 3148c000 00000003 f5959060 00000040 0003dc40 f4d18b88
 00000040 803fbf8c f4d18000 f4d184c0 00000003 f4d184cc 00000000 f16353c0
Call Trace:
 [<c070c1ad>] napi_gro_receive+0x10/0x24
 [<f88fff68>] rtl8169_poll+0x2d6/0x443 [r8169]
 [<c070c2ba>] net_rx_action+0x92/0x1a2
 [<c042ad54>] __do_softirq+0x6d/0xfa
 [<c042ace7>] ? irq_enter+0x4d/0x4d
 <IRQ>
 [<c042aba4>] ? irq_exit+0x32/0x93
 [<c0403a20>] ? do_IRQ+0x7c/0x90
 [<c07a96e9>] ? common_interrupt+0x29/0x30

Thanks,
Ben

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#1 lore raw

Ben Greear <[email protected]> :
[...]

Quoted 3 lines

I am hacking on the 8169 code, (adding RX-ALL and RX-FCS support on top of my previously posted patches) so this could easily be my fault.. but just in case someone else has seen it, please let me know....

I have never met it before. Neither does the web.

Are you hacking against davem's -next branch ?

Signature · 1 line
Ueimor
author
#2 lore raw
Quoted 9 lines · Francois Romieu

Ben Greear<[email protected]> :
[...]

Quoted 3 lines

I am hacking on the 8169 code, (adding RX-ALL and RX-FCS support on top of my previously posted patches) so this could easily be my fault.. but just in case someone else has seen it, please let me know....

I have never met it before. Neither does the web.

Are you hacking against davem's -next branch ?

Yes. I was testing the attached patch, and when I enabled the rx-fcs, it crashed shortly after. So, my changes must be bad somehow...

Probably something to do with pkt_size being 4 bytes larger when rx-fcs is enabled (I was hoping that would grab the FCS, as similar logic seems to work fine in 8139too...)

Thanks,
Ben

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
drivers/net/ethernet/realtek/r8169.c +42 -13
index 5eb6858..fbd855b 100644
@@ -1626,21 +1626,32 @@ static void __rtl8169_set_features(struct net_device *dev,
 				   netdev_features_t features)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
-
+	netdev_features_t changed = features ^ dev->features;
 	void __iomem *ioaddr = tp->mmio_addr;
 
-	if (features & NETIF_F_RXCSUM)
-		tp->cp_cmd |= RxChkSum;
-	else
-		tp->cp_cmd &= ~RxChkSum;
+	if (!(changed & (NETIF_F_RXALL | NETIF_F_RXCSUM | NETIF_F_HW_VLAN_RX)))
+		return;
 
-	if (dev->features & NETIF_F_HW_VLAN_RX)
-		tp->cp_cmd |= RxVlan;
-	else
-		tp->cp_cmd &= ~RxVlan;
+	if (changed & (NETIF_F_RXCSUM | NETIF_F_HW_VLAN_RX)) {
+		if (features & NETIF_F_RXCSUM)
+			tp->cp_cmd |= RxChkSum;
+		else
+			tp->cp_cmd &= ~RxChkSum;
 
-	RTL_W16(CPlusCmd, tp->cp_cmd);
-	RTL_R16(CPlusCmd);
+		if (dev->features & NETIF_F_HW_VLAN_RX)
+			tp->cp_cmd |= RxVlan;
+		else
+			tp->cp_cmd &= ~RxVlan;
+
+		RTL_W16(CPlusCmd, tp->cp_cmd);
+		RTL_R16(CPlusCmd);
+	}
+	if (changed & NETIF_F_RXALL) {
+		int tmp = (RTL_R32(RxConfig) & ~(AcceptErr | AcceptRunt));
+		if (features & NETIF_F_RXALL)
+			tmp |= (AcceptErr | AcceptRunt);
+		RTL_W32(RxConfig, tmp);
+	}
 }
 
 static int rtl8169_set_features(struct net_device *dev,
@@ -4174,6 +4185,9 @@ rtl8169_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		/* 8110SCd requires hardware Rx VLAN - disallow toggling */
 		dev->hw_features &= ~NETIF_F_HW_VLAN_RX;
 
+	dev->hw_features |= NETIF_F_RXALL;
+	dev->hw_features |= NETIF_F_RXFCS;
+
 	tp->hw_start = cfg->hw_start;
 	tp->event_slow = cfg->event_slow;
 
@@ -5747,11 +5761,23 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, u32 budget
 				rtl_schedule_task(tp, RTL_FLAG_TASK_RESET_PENDING);
 				dev->stats.rx_fifo_errors++;
 			}
+			if ((status & (RxRUNT | RxCRC)) &&
+			    !(status & (RxRWT | RxFOVF)) &&
+			    (dev->features & NETIF_F_RXALL))
+				goto process_pkt;
+
 			rtl8169_mark_to_asic(desc, rx_buf_sz);
 		} else {
 			struct sk_buff *skb;
-			dma_addr_t addr = le64_to_cpu(desc->addr);
-			int pkt_size = (status & 0x00003fff) - 4;
+			dma_addr_t addr;
+			int pkt_size;
+
+process_pkt:
+			addr = le64_to_cpu(desc->addr);
+			if (likely(!(dev->features & NETIF_F_RXFCS)))
+				pkt_size = (status & 0x00003fff) - 4;
+			else
+				pkt_size = status & 0x00003fff;
 
 			/*
 			 * The driver does not support incoming fragmented
@@ -6025,6 +6051,9 @@ static void rtl_set_rx_mode(struct net_device *dev)
 		}
 	}
 
+	if (dev->features & NETIF_F_RXALL)
+		rx_mode |= (AcceptErr | AcceptRunt);
+
 	tmp = (RTL_R32(RxConfig) & ~RX_CONFIG_ACCEPT_MASK) | rx_mode;
 
 	if (tp->mac_version > RTL_GIGA_MAC_VER_06) {
author
#3 lore raw
Quoted 9 lines · Ben Greear
Quoted 8 lines · Francois Romieu

Ben Greear<[email protected]> :
[...]

Quoted 3 lines

I am hacking on the 8169 code, (adding RX-ALL and RX-FCS support on top of my previously posted patches) so this could easily be my fault.. but just in case someone else has seen it, please let me know....

I have never met it before. Neither does the web.

Quoted 8 lines
Quoted 1 line

Are you hacking against davem's -next branch ?

Yes. I was testing the attached patch, and when I enabled the rx-fcs, it crashed shortly after. So, my changes must be bad somehow...

Probably something to do with pkt_size being 4 bytes larger when rx-fcs is enabled (I was hoping that would grab the FCS, as similar logic seems to work fine in 8139too...)

I cannot reproduce the problem, so maybe I had a funky build or just got unlucky with some extraneous bug.

With that patch applied, rtl8169 seems to support rx-all and rx-fcs just fine (tested with frames with bad FCS).

If/when the framework patches get accepted I'll post the 8139too and rtl8169 patches for formal review.

Thanks,
Ben

Quoted 4 lines

Thanks,
Ben

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com