fix BUG in tg3_tx

G'day,

The tg3 transmit code assumes that tg3_tx() will never have to clean up part of an skb queued for transmit. This assumption is wrong; sometimes tg3_tx() will be called partway through an skb, which trips the first BUG() and drops the machine into kdb. We've seen this happen on 2.4 (rarely) and 2.6 (easy to reproduce, just run about 5 minutes' worth of NFS or Samba load).

This patch against 2.6.6 fixes this problem by changing the shadow transmit logic to put the non-NULL skb pointer in the *last* ring entry instead of the first, so that it is freed only after all parts have been DMAd onto the card, and can handle tg3_tx() being called partway through.

SGI PV 903520 and PV 891640.

Index: linux/drivers/net/tg3.c
===================================================================
@@ -2103,10 +2103,6 @@
 	return err;
 }
 
-/* Tigon3 never reports partial packet sends.  So we do not
- * need special logic to handle SKBs that have not had all
- * of their frags sent yet, like SunGEM does.
- */
 static void tg3_tx(struct tg3 *tp)
 {
 	u32 hw_idx = tp->hw_status->idx[0].tx_consumer;

@@ -2115,37 +2111,26 @@
 	while (sw_idx != hw_idx) {
 		struct tx_ring_info *ri = &tp->tx_buffers[sw_idx];
 		struct sk_buff *skb = ri->skb;
-		int i;
-
-		if (unlikely(skb == NULL))
-			BUG();
-
-		pci_unmap_single(tp->pdev,
-				 pci_unmap_addr(ri, mapping),
-				 skb_headlen(skb),
-				 PCI_DMA_TODEVICE);
 
 		ri->skb = NULL;
-
-		sw_idx = NEXT_TX(sw_idx);
-
-		for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
-			if (unlikely(sw_idx == hw_idx))
-				BUG();
-
-			ri = &tp->tx_buffers[sw_idx];
-			if (unlikely(ri->skb != NULL))
-				BUG();
-
+		
+		if (ri->index == 0) {
+			pci_unmap_single(tp->pdev,
+					 pci_unmap_addr(ri, mapping),
+					 pci_unmap_len(ri, len),
+					 PCI_DMA_TODEVICE);
+		} else {
 			pci_unmap_page(tp->pdev,
 				       pci_unmap_addr(ri, mapping),
-				       skb_shinfo(skb)->frags[i].size,
+				       pci_unmap_len(ri, len),
 				       PCI_DMA_TODEVICE);
-
-			sw_idx = NEXT_TX(sw_idx);
 		}
-
-		dev_kfree_skb_irq(skb);
+		if (skb) {
+			if (unlikely(ri->index != skb_shinfo(skb)->nr_frags))
+				BUG();
+			dev_kfree_skb_irq(skb);
+		}
+		sw_idx = NEXT_TX(sw_idx);
 	}
 
 	tp->tx_cons = sw_idx;

@@ -2605,6 +2590,18 @@
 	schedule_work(&tp->reset_task);
 }
 
+static inline void tg3_set_txri(struct tg3 *tp, int entry, struct sk_buff *skb,
+				int index, dma_addr_t addr, int len)
+{
+	struct tx_ring_info *ri = &tp->tx_buffers[entry];
+
+	ri->skb = (index == skb_shinfo(skb)->nr_frags ? skb : NULL);
+	ri->index = index;
+	pci_unmap_addr_set(ri, mapping, addr);
+	pci_unmap_len_set(ri, len, len);
+}
+
+
 static void tg3_set_txd(struct tg3 *, int, dma_addr_t, int, u32, u32);
 
 static int tigon3_4gb_hwbug_workaround(struct tg3 *tp, struct sk_buff *skb,

@@ -2645,6 +2642,7 @@
 		if (i == 0) {
 			tp->tx_buffers[entry].skb = new_skb;
 			pci_unmap_addr_set(&tp->tx_buffers[entry], mapping, new_addr);
+			pci_unmap_len_set(&tp->tx_buffers[entry], len, new_skb->len);
 		} else {
 			tp->tx_buffers[entry].skb = NULL;
 		}

@@ -2804,8 +2802,7 @@
 	/* Queue skb data, a.k.a. the main skb fragment. */
 	mapping = pci_map_single(tp->pdev, skb->data, len, PCI_DMA_TODEVICE);
 
-	tp->tx_buffers[entry].skb = skb;
-	pci_unmap_addr_set(&tp->tx_buffers[entry], mapping, mapping);
+	tg3_set_txri(tp, entry, skb, 0, mapping, len);
 
 	would_hit_hwbug = 0;
 

@@ -2831,8 +2828,7 @@
 					       frag->page_offset,
 					       len, PCI_DMA_TODEVICE);
 
-			tp->tx_buffers[entry].skb = NULL;
-			pci_unmap_addr_set(&tp->tx_buffers[entry], mapping, mapping);
+			tg3_set_txri(tp, entry, skb, i+1, mapping, len);
 
 			if (tg3_4g_overflow_test(mapping, len)) {
 				/* Only one should match. */

@@ -3002,8 +2998,7 @@
 	/* Queue skb data, a.k.a. the main skb fragment. */
 	mapping = pci_map_single(tp->pdev, skb->data, len, PCI_DMA_TODEVICE);
 
-	tp->tx_buffers[entry].skb = skb;
-	pci_unmap_addr_set(&tp->tx_buffers[entry], mapping, mapping);
+	tg3_set_txri(tp, entry, skb, 0, mapping, len);
 
 	tg3_set_txd(tp, entry, mapping, len, base_flags,
 		    (skb_shinfo(skb)->nr_frags == 0) | (mss << 1));

@@ -3025,8 +3020,7 @@
 					       frag->page_offset,
 					       len, PCI_DMA_TODEVICE);
 
-			tp->tx_buffers[entry].skb = NULL;
-			pci_unmap_addr_set(&tp->tx_buffers[entry], mapping, mapping);
+			tg3_set_txri(tp, entry, skb, i+1, mapping, len);
 
 			tg3_set_txd(tp, entry, mapping, len,
 				    base_flags, (i == last));

Index: linux/drivers/net/tg3.h
===================================================================
@@ -1774,6 +1774,8 @@
 struct tx_ring_info {
 	struct sk_buff			*skb;
 	DECLARE_PCI_UNMAP_ADDR(mapping)
+	DECLARE_PCI_UNMAP_LEN(len)
+	u32				index;
 	u32				prev_vlan_tag;
 };
 

Greg.

Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

Greg, perhaps my reading of the tg3 chip docs is different from yours. The hardware is NEVER supposed to do this.

Or is there an errata in some chip versions?

I've never triggered that BUG() assertion on any of my hardware, ever.

I'd like to know where you read that, because neither I nor any of the other SGI engineers who have read the Broadcom docs can find any such guarantee. The consensus here is that there is no guarantee about where the transmit ring consumer index is when the card DMAs a status block update.

We've seen that BUG() trip many times both on 2.4 and 2.6. SGI ProPack for Linux has been shipping for a year now with a workaround for this bug (the BUG() is changed to a break).

We've seen this on both 5701 and 5704 hardware. The SGI cards ship have slightly different firmware from stock Broadcom cards, I don't know if that's a factor. It might also be something to do with the Altix IO architecture. I don't know why the card chooses to do this, but it most certainly does.

The IRIX driver guys tells me this same behaviour happens on the Origin hardware, and the IRIX driver had an equivalent fix applied about 18 months ago.

Ok, here's one of several mostly identical stack traces reported by various people inside SGI on 2.6 kernels. If you want to wait for a day or so I can probably make a 2.4 kernel do this also (I did that during testing a coupld of days ago but didn't save the stack trace, doh!)

[root@budgie root]# swapper[0]: bugcheck! 0 [1]

Pid: 0, CPU 0, comm: swapper
psr : 0000101009022018 ifs : 8000000000000d1e ip : [<a000000200165980>] Not tainted
ip is at tg3_tx+0x5a0/0x5c0 [tg3]
unat: 0000000000000000 pfs : 0000000000000d1e rsc : 0000000000000003
rnat: 800000025da66955 bsps: a0000001000fdec0 pr : 80000000ff7669a5
ldrs: 0000000000000000 ccv : 0000000000000000 fpsr: 0009804c8a70033f csd : 0000000000000000 ssd : 0000000000000000
b0 : a000000200165980 b6 : a000000100003320 b7 : a0000001000cc200 f6 : 1003e0fc0fc0fc0fc0fc1 f7 : 0ffdaa200000000000000
f8 : 1003e0000000000000240 f9 : 1003e0000000000002490
f10 : 1003e000000000ea00000 f11 : 1003e00000000367b7ad0
r1 : a0000001009ec7f0 r2 : 0000000000000000 r3 : 0000000000004000 r8 : 0000000000000026 r9 : 0000000000000002 r10 : 0000000000000001 r11 : 0000000000000004 r12 : e0000030146a3c50 r13 : e00000301469c000 r14 : 0000000000004000 r15 : a000000100734fb0 r16 : e00000b004a147a8 r17 : e00000b07ba88060 r18 : e000003007ba0000 r19 : 0000000000180000 r20 : 0000000000000014 r21 : 0000000000080000 r22 : 0000000000100000 r23 : e00000b004a150e4 r24 : e00000b004a150d8 r25 : e0000030146a3bf0 r26 : e00000b004a15810 r27 : 0000000000000074 r28 : 0000000000000074 r29 : e000003007ba002c r30 : e00000b07ba8802c r31 : 0000000000000002

Call Trace:
 [<a000000100015180>] show_stack+0x80/0xa0
                                sp=e0000030146a3820 bsp=e00000301469d2c8
 [<a0000001000384f0>] die+0x1b0/0x280
                                sp=e0000030146a39f0 bsp=e00000301469d2a0
 [<a000000100038960>] ia64_bad_break+0x340/0x480
                                sp=e0000030146a39f0 bsp=e00000301469d280
 [<a00000010000de20>] ia64_leave_kernel+0x0/0x260
                                sp=e0000030146a3a80 bsp=e00000301469d280
 [<a000000200165980>] tg3_tx+0x5a0/0x5c0 [tg3]
                                sp=e0000030146a3c50 bsp=e00000301469d188
 [<a000000200166cf0>] tg3_interrupt_main_work+0x150/0x280 [tg3]
                                sp=e0000030146a3c50 bsp=e00000301469d158
 [<a000000200166f20>] tg3_interrupt+0x100/0x1c0 [tg3]
                                sp=e0000030146a3c50 bsp=e00000301469d110
 [<a000000100011640>] handle_IRQ_event+0xa0/0x120
                                sp=e0000030146a3c50 bsp=e00000301469d0c8
 [<a000000100012170>] do_IRQ+0x390/0x4a0
                                sp=e0000030146a3c50 bsp=e00000301469d078
 [<a000000100014160>] ia64_handle_irq+0xc0/0x1a0
                                sp=e0000030146a3c50 bsp=e00000301469d040
 [<a00000010000de20>] ia64_leave_kernel+0x0/0x260
                                sp=e0000030146a3c50 bsp=e00000301469d040
 [<a000000100084010>] snidle+0xb0/0x180
                                sp=e0000030146a3e20 bsp=e00000301469d038
 [<a000000100015cb0>] cpu_idle+0x130/0x220
                                sp=e0000030146a3e20 bsp=e00000301469cfa8
 [<a000000100630fa0>] start_kernel+0x460/0x4e0
                                sp=e0000030146a3e20 bsp=e00000301469cf50
 [<a000000100008600>] _start+0x2c0/0x0
                                sp=e0000030146a3e30 bsp=e00000301469cf50
 3 out of 4 cpus in kdb, waiting for the rest

1 cpu are not in kdb, their state is unknown

Entering kdb (current=0xe00000301469c000, pid 0) on processor 0 Oops: <NULL> due to oops @ 0xa000000200165980
psr: 0x0000101009022018 ifs: 0x8000000000000d1e ip: 0xa000000200165980
unat: 0x0000000000000000 pfs: 0x0000000000000d1e rsc: 0x0000000000000003
rnat: 0x800000025da66955 bsps: 0xa0000001000fdec0 pr: 0x80000000ff7669a5
ldrs: 0x0000000000000000 ccv: 0x0000000000000000 fpsr: 0x0009804c8a70033f

  b0: 0xa000000200165980    b6: 0xa000000100003320    b7: 0xa0000001000cc200  
  r1: 0xa0000001009ec7f0    r2: 0x0000000000000000    r3: 0x0000000000004000  
  r8: 0x0000000000000026    r9: 0x0000000000000002   r10: 0x0000000000000001  
 r11: 0x0000000000000004   r12: 0xe0000030146a3c50   r13: 0xe00000301469c000  
 r14: 0x0000000000004000   r15: 0xa000000100734fb0   r16: 0xe00000b004a147a8  
 r17: 0xe00000b07ba88060   r18: 0xe000003007ba0000   r19: 0x0000000000180000  
 r20: 0x0000000000000014   r21: 0x0000000000080000   r22: 0x0000000000100000  
 r23: 0xe00000b004a150e4   r24: 0xe00000b004a150d8   r25: 0xe0000030146a3bf0  
 r26: 0xe00000b004a15810   r27: 0x0000000000000074   r28: 0x0000000000000074  
 r29: 0xe000003007ba002c   r30: 0xe00000b07ba8802c   r31: 0x0000000000000002  

&regs = e0000030146a3a90
[0]kdb>

Greg.

Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

[ Michael, the discussion here is about whether the tigon3 hardware
ever partially ACK's completion of a multi-frag TX frame. I believe it never does, but Greg claims he can trigger such a case and has proposed a patch to the tg3 driver which attempts to handle that. ]

The most relevant (and accurate) piece of documentation for the chip is Broadcom's own driver :-) And they do not account at all for such a case of partial-packet TX completion indication. If the first frag is ACK'd they assume the whole packet has been taken. Here is the relevant code from the bcm5700 driver in LM_ServiceTxInterrupt():

    while(SwConIdx != HwConIdx)
    {
        pPacket = pDevice->SendRing[SwConIdx];
        pDevice->SendRing[SwConIdx] = 0;

        /* Set the return status. */
        pPacket->PacketStatus = LM_STATUS_SUCCESS;

        /* Put the packet in the TxPacketXmittedQ for indication later. */
        QQ_PushTail(&pDevice->TxPacketXmittedQ.Container, pPacket);

        /* Move to the next packet's BD. */
        SwConIdx = (SwConIdx + pPacket->u.Tx.FragCount) & 
            T3_SEND_RCB_ENTRY_COUNT_MASK;

        /* Update the number of unused BDs. */
        MM_ATOMIC_ADD(&pDevice->SendBdLeft, pPacket->u.Tx.FragCount);

        /* Get the new updated HwConIdx. */
        HwConIdx = pDevice->pStatusBlkVirt->Idx[0].SendConIdx;
    } /* while */

Imagine how badly this piece of code would fail if partial ACK'ing of TX packets actually occurred. It would loop past HwConIdx and thus ACK really-not-completed packets, potentially colliding with what the chip is transmitting and thus causing massive data corruption and likely a crash. Actually, it would most likely loop past all valid TX packets and dereference a pPacket NULL pointer.

The Windows-flavoured driver you decided to replace in Linux?

I agree that this code appears to implictly rely on always getting complete send ring updates.

So has this driver been tested in 2.6? I did notice that the bug occurs far more frequently in 2.6 because better zero-copy code means that the driver actually sees skbs with frags instead of just the header. The driver might be accidentally working because pPacket->u.Tx.FragCount=1 during all its testing.

Also, I'm not familiar with this driver's source (I haven't looked at it for a long time), but I can see that there are behaviour differences between this driver and the tg3 driver which might affect the visibility of the bug.

For one thing, this code fetches a new sample of the hardware consumer index on every loop iteration. This may result in accidentally bumping up HwConIdx to avoid apparent partial completions. Also note that the queuing step (QQ_PushTail) might add enough delay to make a second status block update more likely.

Without looking at the remainder of the driver, I can't say if this code is called more or less frequently than tg3_tx(). If it's using any interrupt coalescing at all it will be called much less frequently and thus have a smaller window to see a partial complete. On my hardware I see interrupt rates of 10000-30000 per second per card, and tracing shows that tg3_tx() is called on most of these interrupts. It could be the case that the huge interrupt rate in the tg3 driver is banging hard on a race condition which the bcom driver avoids.

Also, the NAPI code in the tg3 driver will presumably set up the card's interrupt coalescing engine with different parameters, which might have an effect on the timing of status block updates.

Finally, the tg3 driver departs from the recommended ISR flow control diagram and handles a status block update during the ISR by using the SETINT bit to tell the card to force a new interrupt (instead of restarting the ISR). This might also have an effect.

In short, even if the implicit assumptions in the bcom driver are correct in that driver, I don't see how you can argue that they can be carried across to the tg3 driver.

BTW, at least one other person has reported what appears to be the exactly this bug:

http://marc.theaimsgroup.com/?l=linux-kernel&m=102822850329939&w=2

He found the same bug in both bcom and tg3 drivers, and his hardware has little in common with mine (apart from having >1 fast CPUs).

Greg.

Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

Greg, did you see Micahel Chan's response? A Broadcom engineer is telling us "the hardware does not ACK partial TX packets."

I can't think of a more reliable source for this kind of information, can you? Given this, it doesn't matter all of the difference you mention between the tg3 and bcm5700 driver, the hardware simply is never supposed to do this as stated by somehow who has access to the actual hardware engineers. :-)

I don't argue that you aren't seeing something strange, but perhaps that is due to corruption occuring elsewhere, or perhaps something peculiar about your system hardware (perhaps the PCI controller mis-orders PCI transactions or something silly like that)?

Have you reproduced this on some system other than these huge SGI ones?

So, BTW, I'm not putting in a workaround for the partial-TX-packet ACK in tg3_tx() until we figure out why this is happening for Greg.

We found that packets were mysteriously being discarded by our Broadcom 570X hardware under heavy receive load. This occurred especially when multiple NICs were in use.

The problem turned out to be that h/w flow control wasn't being used, even after that should have been autonegotiated. The attached patch (against 2.6.6) fixes that.

--

Arthur

@@ -1027,6 +1027,8 @@
 static void tg3_setup_flow_control(struct tg3 *tp, u32 local_adv, u32 remote_adv)
 {
 	u32 new_tg3_flags = 0;
+	u32 old_rx_mode = tp->rx_mode;
+	u32 old_tx_mode = tp->tx_mode;
 
 	if (local_adv & ADVERTISE_PAUSE_CAP) {
 		if (local_adv & ADVERTISE_PAUSE_ASYM) {

@@ -1057,10 +1059,18 @@
 	else
 		tp->rx_mode &= ~RX_MODE_FLOW_CTRL_ENABLE;
 
+	if (old_rx_mode != tp->rx_mode) {
+		tw32_f(MAC_RX_MODE, tp->rx_mode);
+	}
+	
 	if (new_tg3_flags & TG3_FLAG_TX_PAUSE)
 		tp->tx_mode |= TX_MODE_FLOW_CTRL_ENABLE;
 	else
 		tp->tx_mode &= ~TX_MODE_FLOW_CTRL_ENABLE;
+
+	if (old_tx_mode != tp->tx_mode) {
+		tw32_f(MAC_TX_MODE, tp->tx_mode);
+	}
 }
 
 static void tg3_aux_stat_to_speed_duplex(struct tg3 *tp, u32 val, u16 *speed, u8 *duplex)

What an embarassing bug. Patch applied, good spotting Arthur.

Yes I did. I've been working towards gathering data for a reply.

I can think of one: actual observation of the card in action in the field. Experiment trumps theory.

To this end, I instrumented the driver + my patch to BUG() out if the tx_ring_info.index is not a predicted value, i.e. if the tg3_tx() ever starts partway through a packet. It's been running overnight under >200 MB/s of NFS read load, nothing yet.

There are many things peculiar about our hardware. Otherwise we'd be "the world stops at 4 processors" Dell.

I haven't tried; my job is first and foremost to make SGI hardware work. However I did point you to a report on lkml where someone on non-SGI hardware has seen what appears to be the same problem. I'm not yet willing to consign this to the "wacky SGI PCI hardware" bucket.

Greg.

Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.