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

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

[Ben Greear]

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

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

[Francois Romieu]

Ben Greear <greearb@candelatech.com> :
[...]
> 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 ?

-- 
Ueimor

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

[Ben Greear]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On 02/10/2012 01:30 AM, Francois Romieu wrote:
> Ben Greear<greearb@candelatech.com>  :
> [...]
>> 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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


[-- Attachment #2: rtl.patch --]
[-- Type: text/x-patch, Size: 2834 bytes --]

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 5eb6858..fbd855b 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -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) {

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

[Ben Greear]

On 02/10/2012 09:09 AM, Ben Greear wrote:
> On 02/10/2012 01:30 AM, Francois Romieu wrote:
>> Ben Greear<greearb@candelatech.com> :
>> [...]
>>> 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...)

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

>
> Thanks,
> Ben
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com