Skip to contentSkip to conversation

Fix crash in tg3 when using irqpoll

PATCH In review Bernhard Walle 3 participants 9 messages Mar 22, 2007 – Mar 25, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Fix crash in tg3 when using irqpoll

When using irqpoll I had a crash when I loaded the tg3 network driver (on IA64). The stack trace was:

        ia64_leave_kernel
        [tg3]tg3_interrupt_tagged
        note_interrupt
        __do_IRQ
        ia64_handle_irq
        ia64_leave_kernel
        _spin_unlock_irqrestore
        pci_bus_read_config_dword
        pci_restore_state
        [tg3]tg3_chip_reset
        [tg3]tg3_reset_hw
        [tg3]tg3_init_hw
        [tg3]tg3_open
        dev_open
        dev_change_flags
        devinet_ioctl
        inet_ioctl
        sock_ioctl
        do_ioctl
        vfs_ioctl
        sys_ioctl
        ia64_ret_from_syscall
        __kernel_syscall_via_break

Also, I had a MCA that ended up in a read from a PCI address that belongs to the tg3 driver.

This patch makes sure that even the tr32() instruction in the interrupt handler is not executed which accesses PCI memory. Accessing PCI memory when pci_restore_state() is called is a bad idea because that function modifies the BARs of the PCI device.

I think the problem could also happen when using shared interrupts, not only irqpoll.

---
 drivers/net/tg3.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
drivers/net/tg3.c +11 -11
Index: mainline-msi-init/drivers/net/tg3.c
===================================================================
@@ -3561,7 +3561,10 @@ static irqreturn_t tg3_interrupt(int irq
 	struct net_device *dev = dev_id;
 	struct tg3 *tp = netdev_priv(dev);
 	struct tg3_hw_status *sblk = tp->hw_status;
-	unsigned int handled = 1;
+	unsigned int handled = 0;
+
+	if (tg3_irq_sync(tp))
+		goto out;
 
 	/* In INTx mode, it is possible for the interrupt to arrive at
 	 * the CPU before the status block posted prior to the interrupt.
@@ -3579,8 +3582,6 @@ static irqreturn_t tg3_interrupt(int irq
 		 */
 		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
 			     0x00000001);
-		if (tg3_irq_sync(tp))
-			goto out;
 		sblk->status &= ~SD_STATUS_UPDATED;
 		if (likely(tg3_has_work(tp))) {
 			prefetch(&tp->rx_rcb[tp->rx_rcb_ptr]);
@@ -3592,8 +3593,7 @@ static irqreturn_t tg3_interrupt(int irq
 			tw32_mailbox_f(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
 			     	0x00000000);
 		}
-	} else {	/* shared interrupt */
-		handled = 0;
+		handled = 1;
 	}
 out:
 	return IRQ_RETVAL(handled);
@@ -3604,7 +3604,10 @@ static irqreturn_t tg3_interrupt_tagged(
 	struct net_device *dev = dev_id;
 	struct tg3 *tp = netdev_priv(dev);
 	struct tg3_hw_status *sblk = tp->hw_status;
-	unsigned int handled = 1;
+	unsigned int handled = 0;
+
+	if (tg3_irq_sync(tp))
+		goto out;
 
 	/* In INTx mode, it is possible for the interrupt to arrive at
 	 * the CPU before the status block posted prior to the interrupt.
@@ -3622,8 +3625,6 @@ static irqreturn_t tg3_interrupt_tagged(
 		 */
 		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
 			     0x00000001);
-		if (tg3_irq_sync(tp))
-			goto out;
 		if (netif_rx_schedule_prep(dev)) {
 			prefetch(&tp->rx_rcb[tp->rx_rcb_ptr]);
 			/* Update last_tag to mark that this status has been
@@ -3634,8 +3635,7 @@ static irqreturn_t tg3_interrupt_tagged(
 			tp->last_tag = sblk->status_tag;
 			__netif_rx_schedule(dev);
 		}
-	} else {	/* shared interrupt */
-		handled = 0;
+		handled = 1;
 	}
 out:
 	return IRQ_RETVAL(handled);
@@ -7052,7 +7052,7 @@ static int tg3_open(struct net_device *d
 		return err;
 	}
 
-	tg3_full_lock(tp, 0);
+	tg3_full_lock(tp, 1);
 
 	err = tg3_init_hw(tp, 1);
 	if (err) {
#1 lore raw
Quoted 5 lines · Bernhard Walle

This patch makes sure that even the tr32() instruction in the interrupt handler is not executed which accesses PCI memory. Accessing PCI memory when pci_restore_state() is called is a bad idea because that function modifies the BARs of the PCI device.

It is not caused by the BAR as it doesn't get changed in this case. The pci_restore_state() call is to restore the memory enable bit in the PCI command register. The tr32() call in tg3_interrupt() will cause a master abort if it is called before the memory enable bit has been restored.

drivers/net/tg3.c static irqreturn_t tg3_interrupt(int irq · line 3567

Quoted 11 lines
drivers/net/tg3.c +4 -1
@@ -3561,7 +3561,10 @@ static irqreturn_t tg3_interrupt(int irq
 	struct net_device *dev = dev_id;
 	struct tg3 *tp = netdev_priv(dev);
 	struct tg3_hw_status *sblk = tp->hw_status;
-	unsigned int handled = 1;
+	unsigned int handled = 0;
+
+	if (tg3_irq_sync(tp))
+		goto out;

This will break other things. When we disable interrupts, we set the irq_sync flag but allow one more interrupt to be generated. The irq handler will simply mask off the interrupt when it sees the irq_sync flag. With the above change, the irq handler can no longer mask off this trailing interrupt and you may get screaming interrupts as a result.

Thanks for reporting this. I'll try to come up with a good solution.

#2 lore raw

Hello Michael,

Quoted 7 lines · Michael Chan
Quoted 4 lines · Bernhard Walle

This patch makes sure that even the tr32() instruction in the interrupt handler is not executed which accesses PCI memory. Accessing PCI memory when pci_restore_state() is called is a bad idea because that function modifies the BARs of the PCI device.

Quoted 5 lines

It is not caused by the BAR as it doesn't get changed in this case. The pci_restore_state() call is to restore the memory enable bit in the PCI command register. The tr32() call in tg3_interrupt() will cause a master abort if it is called before the memory enable bit has been restored.

Ok, thanks for the explanation. I wondered why you call pci_restore_state() here, normally that's only called from .resume handlers.

Quoted 13 lines

drivers/net/tg3.c static irqreturn_t tg3_interrupt(int irq · line 3567

Quoted 11 lines
drivers/net/tg3.c +4 -1
@@ -3561,7 +3561,10 @@ static irqreturn_t tg3_interrupt(int irq
 	struct net_device *dev = dev_id;
 	struct tg3 *tp = netdev_priv(dev);
 	struct tg3_hw_status *sblk = tp->hw_status;
-	unsigned int handled = 1;
+	unsigned int handled = 0;
+
+	if (tg3_irq_sync(tp))
+		goto out;
Quoted 6 lines

This will break other things. When we disable interrupts, we set the irq_sync flag but allow one more interrupt to be generated. The irq handler will simply mask off the interrupt when it sees the irq_sync flag. With the above change, the irq handler can no longer mask off this trailing interrupt and you may get screaming interrupts as a result.

You're right, I had only the case in mind where the network device doesn't generate any interrupts (in initialisation phase and in shutdown phase) because it's disabled in the device, and the interrupt handler is only called because of IRQ-sharing/irqpoll.

Quoted 1 line

Thanks for reporting this. I'll try to come up with a good solution.

Could you please CC me, I'd like to test it here.

Thanks,
Bernhard

#3 lore raw

PATCH 1/3 : Eliminate the unused TG3_FLAG_SPLIT_MODE flag.

[TG3]: Eliminate the unused TG3_FLAG_SPLIT_MODE flag.

This flag to support multiple PCIX split completions was never used because of hardware bugs. This will make room for a new flag.

drivers/net/tg3.c +1 -16
index 8c8f9f4..ab87bb1 100644
@@ -6321,8 +6321,6 @@ static int tg3_reset_hw(struct tg3 *tp, int reset_phy)
 		      RDMAC_MODE_ADDROFLOW_ENAB | RDMAC_MODE_FIFOOFLOW_ENAB |
 		      RDMAC_MODE_FIFOURUN_ENAB | RDMAC_MODE_FIFOOREAD_ENAB |
 		      RDMAC_MODE_LNGREAD_ENAB);
-	if (tp->tg3_flags & TG3_FLAG_SPLIT_MODE)
-		rdmac_mode |= RDMAC_MODE_SPLIT_ENABLE;
 
 	/* If statement applies to 5705 and 5750 PCI devices only */
 	if ((GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705 &&
@@ -6495,9 +6493,6 @@ static int tg3_reset_hw(struct tg3 *tp, int reset_phy)
 		} else if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5704) {
 			val &= ~(PCIX_CAPS_SPLIT_MASK | PCIX_CAPS_BURST_MASK);
 			val |= (PCIX_CAPS_MAX_BURST_CPIOB << PCIX_CAPS_BURST_SHIFT);
-			if (tp->tg3_flags & TG3_FLAG_SPLIT_MODE)
-				val |= (tp->split_mode_max_reqs <<
-					PCIX_CAPS_SPLIT_SHIFT);
 		}
 		tw32(TG3PCI_X_CAPS, val);
 	}
@@ -10863,14 +10858,6 @@ static int __devinit tg3_get_invariants(struct tg3 *tp)
 	grc_misc_cfg = tr32(GRC_MISC_CFG);
 	grc_misc_cfg &= GRC_MISC_CFG_BOARD_ID_MASK;
 
-	/* Broadcom's driver says that CIOBE multisplit has a bug */
-#if 0
-	if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5704 &&
-	    grc_misc_cfg == GRC_MISC_CFG_BOARD_ID_5704CIOBE) {
-		tp->tg3_flags |= TG3_FLAG_SPLIT_MODE;
-		tp->split_mode_max_reqs = SPLIT_MODE_5704_MAX_REQ;
-	}
-#endif
 	if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705 &&
 	    (grc_misc_cfg == GRC_MISC_CFG_BOARD_ID_5788 ||
 	     grc_misc_cfg == GRC_MISC_CFG_BOARD_ID_5788M))
@@ -11968,14 +11955,12 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
 		       i == 5 ? '\n' : ':');
 
 	printk(KERN_INFO "%s: RXcsums[%d] LinkChgREG[%d] "
-	       "MIirq[%d] ASF[%d] Split[%d] WireSpeed[%d] "
-	       "TSOcap[%d] \n",
+	       "MIirq[%d] ASF[%d] WireSpeed[%d] TSOcap[%d]\n",
 	       dev->name,
 	       (tp->tg3_flags & TG3_FLAG_RX_CHECKSUMS) != 0,
 	       (tp->tg3_flags & TG3_FLAG_USE_LINKCHG_REG) != 0,
 	       (tp->tg3_flags & TG3_FLAG_USE_MI_INTERRUPT) != 0,
 	       (tp->tg3_flags & TG3_FLAG_ENABLE_ASF) != 0,
-	       (tp->tg3_flags & TG3_FLAG_SPLIT_MODE) != 0,
 	       (tp->tg3_flags2 & TG3_FLG2_NO_ETH_WIRE_SPEED) == 0,
 	       (tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE) != 0);
 	printk(KERN_INFO "%s: dma_rwctrl[%08x] dma_mask[%d-bit]\n",
drivers/net/tg3.h +0 -4
index 086892d..5df8f76 100644
@@ -2223,7 +2223,6 @@ struct tg3 {
 #define TG3_FLAG_40BIT_DMA_BUG		0x08000000
 #define TG3_FLAG_BROKEN_CHECKSUMS	0x10000000
 #define TG3_FLAG_GOT_SERDES_FLOWCTL	0x20000000
-#define TG3_FLAG_SPLIT_MODE		0x40000000
 #define TG3_FLAG_INIT_COMPLETE		0x80000000
 	u32				tg3_flags2;
 #define TG3_FLG2_RESTART_TIMER		0x00000001
@@ -2262,9 +2261,6 @@ struct tg3 {
 #define TG3_FLG2_NO_FWARE_REPORTED	0x40000000
 #define TG3_FLG2_PHY_ADJUST_TRIM	0x80000000
 
-	u32				split_mode_max_reqs;
-#define SPLIT_MODE_5704_MAX_REQ		3
-
 	struct timer_list		timer;
 	u16				timer_counter;
 	u16				timer_multiplier;
maintainer Applied
#4 lore raw

Re: [TG3 1/3]: Eliminate the unused TG3_FLAG_SPLIT_MODE flag.

From: "Michael Chan" <[email protected]>
Date: Fri, 23 Mar 2007 16:12:57 -0800

Quoted 6 lines

[TG3]: Eliminate the unused TG3_FLAG_SPLIT_MODE flag.

This flag to support multiple PCIX split completions was never used because of hardware bugs. This will make room for a new flag.

Applied.

#5 lore raw

PATCH 2/3 : Exit irq handler during chip reset.

In reply to Bernhard Walle

[TG3]: Exit irq handler during chip reset.

On most tg3 chips, the memory enable bit in the PCI command register gets cleared during chip reset and must be restored before accessing PCI registers using memory cycles. The chip does not generate interrupt during chip reset, but the irq handler can still be called because of irq sharing or irqpoll. Reading a register in the irq handler can cause a master abort in this scenario and may result in a crash on some architectures.

Use the TG3_FLAG_CHIP_RESETTING flag to tell the irq handler to exit without touching any registers. The checking of the flag is in the "slow" path of the irq handler and will not affect normal performance. The msi handler is not shared and therefore does not require checking the flag.

Thanks to Bernhard Walle <[email protected]> for reporting the problem.

drivers/net/tg3.c +66 -47
index ab87bb1..9aca100 100644
@@ -3568,32 +3568,34 @@ static irqreturn_t tg3_interrupt(int irq, void *dev_id)
 	 * Reading the PCI State register will confirm whether the
 	 * interrupt is ours and will flush the status block.
 	 */
-	if ((sblk->status & SD_STATUS_UPDATED) ||
-	    !(tr32(TG3PCI_PCISTATE) & PCISTATE_INT_NOT_ACTIVE)) {
-		/*
-		 * Writing any value to intr-mbox-0 clears PCI INTA# and
-		 * chip-internal interrupt pending events.
-		 * Writing non-zero to intr-mbox-0 additional tells the
-		 * NIC to stop sending us irqs, engaging "in-intr-handler"
-		 * event coalescing.
-		 */
-		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
-			     0x00000001);
-		if (tg3_irq_sync(tp))
+	if (unlikely(!(sblk->status & SD_STATUS_UPDATED))) {
+		if ((tp->tg3_flags & TG3_FLAG_CHIP_RESETTING) ||
+		    (tr32(TG3PCI_PCISTATE) & PCISTATE_INT_NOT_ACTIVE)) {
+			handled = 0;
 			goto out;
-		sblk->status &= ~SD_STATUS_UPDATED;
-		if (likely(tg3_has_work(tp))) {
-			prefetch(&tp->rx_rcb[tp->rx_rcb_ptr]);
-			netif_rx_schedule(dev);		/* schedule NAPI poll */
-		} else {
-			/* No work, shared interrupt perhaps?  re-enable
-			 * interrupts, and flush that PCI write
-			 */
-			tw32_mailbox_f(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
-			     	0x00000000);
 		}
-	} else {	/* shared interrupt */
-		handled = 0;
+	}
+
+	/*
+	 * Writing any value to intr-mbox-0 clears PCI INTA# and
+	 * chip-internal interrupt pending events.
+	 * Writing non-zero to intr-mbox-0 additional tells the
+	 * NIC to stop sending us irqs, engaging "in-intr-handler"
+	 * event coalescing.
+	 */
+	tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW, 0x00000001);
+	if (tg3_irq_sync(tp))
+		goto out;
+	sblk->status &= ~SD_STATUS_UPDATED;
+	if (likely(tg3_has_work(tp))) {
+		prefetch(&tp->rx_rcb[tp->rx_rcb_ptr]);
+		netif_rx_schedule(dev);		/* schedule NAPI poll */
+	} else {
+		/* No work, shared interrupt perhaps?  re-enable
+		 * interrupts, and flush that PCI write
+		 */
+		tw32_mailbox_f(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
+		     	0x00000000);
 	}
 out:
 	return IRQ_RETVAL(handled);
@@ -3611,31 +3613,33 @@ static irqreturn_t tg3_interrupt_tagged(int irq, void *dev_id)
 	 * Reading the PCI State register will confirm whether the
 	 * interrupt is ours and will flush the status block.
 	 */
-	if ((sblk->status_tag != tp->last_tag) ||
-	    !(tr32(TG3PCI_PCISTATE) & PCISTATE_INT_NOT_ACTIVE)) {
-		/*
-		 * writing any value to intr-mbox-0 clears PCI INTA# and
-		 * chip-internal interrupt pending events.
-		 * writing non-zero to intr-mbox-0 additional tells the
-		 * NIC to stop sending us irqs, engaging "in-intr-handler"
-		 * event coalescing.
-		 */
-		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
-			     0x00000001);
-		if (tg3_irq_sync(tp))
+	if (unlikely(sblk->status_tag == tp->last_tag)) {
+		if ((tp->tg3_flags & TG3_FLAG_CHIP_RESETTING) ||
+		    (tr32(TG3PCI_PCISTATE) & PCISTATE_INT_NOT_ACTIVE)) {
+			handled = 0;
 			goto out;
-		if (netif_rx_schedule_prep(dev)) {
-			prefetch(&tp->rx_rcb[tp->rx_rcb_ptr]);
-			/* Update last_tag to mark that this status has been
-			 * seen. Because interrupt may be shared, we may be
-			 * racing with tg3_poll(), so only update last_tag
-			 * if tg3_poll() is not scheduled.
-			 */
-			tp->last_tag = sblk->status_tag;
-			__netif_rx_schedule(dev);
 		}
-	} else {	/* shared interrupt */
-		handled = 0;
+	}
+
+	/*
+	 * writing any value to intr-mbox-0 clears PCI INTA# and
+	 * chip-internal interrupt pending events.
+	 * writing non-zero to intr-mbox-0 additional tells the
+	 * NIC to stop sending us irqs, engaging "in-intr-handler"
+	 * event coalescing.
+	 */
+	tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW, 0x00000001);
+	if (tg3_irq_sync(tp))
+		goto out;
+	if (netif_rx_schedule_prep(dev)) {
+		prefetch(&tp->rx_rcb[tp->rx_rcb_ptr]);
+		/* Update last_tag to mark that this status has been
+		 * seen. Because interrupt may be shared, we may be
+		 * racing with tg3_poll(), so only update last_tag
+		 * if tg3_poll() is not scheduled.
+		 */
+		tp->last_tag = sblk->status_tag;
+		__netif_rx_schedule(dev);
 	}
 out:
 	return IRQ_RETVAL(handled);
@@ -4823,6 +4827,19 @@ static int tg3_chip_reset(struct tg3 *tp)
 	if (write_op == tg3_write_flush_reg32)
 		tp->write32 = tg3_write32;
 
+	/* Prevent the irq handler from reading or writing PCI registers
+	 * during chip reset when the memory enable bit in the PCI command
+	 * register may be cleared.  The chip does not generate interrupt
+	 * at this time, but the irq handler may still be called due to irq
+	 * sharing or irqpoll.
+	 */
+	tp->tg3_flags |= TG3_FLAG_CHIP_RESETTING;
+	tp->hw_status->status = 0;
+	tp->hw_status->status_tag = 0;
+	tp->last_tag = 0;
+	smp_mb();
+	synchronize_irq(tp->pdev->irq);
+
 	/* do the reset */
 	val = GRC_MISC_CFG_CORECLK_RESET;
 
@@ -4904,6 +4921,8 @@ static int tg3_chip_reset(struct tg3 *tp)
 
 	pci_restore_state(tp->pdev);
 
+	tp->tg3_flags &= ~TG3_FLAG_CHIP_RESETTING;
+
 	/* Make sure PCI-X relaxed ordering bit is clear. */
 	pci_read_config_dword(tp->pdev, TG3PCI_X_CAPS, &val);
 	val &= ~PCIX_CAPS_RELAXED_ORDERING;
drivers/net/tg3.h +1 -0
index 5df8f76..d515ed2 100644
@@ -2223,6 +2223,7 @@ struct tg3 {
 #define TG3_FLAG_40BIT_DMA_BUG		0x08000000
 #define TG3_FLAG_BROKEN_CHECKSUMS	0x10000000
 #define TG3_FLAG_GOT_SERDES_FLOWCTL	0x20000000
+#define TG3_FLAG_CHIP_RESETTING		0x40000000
 #define TG3_FLAG_INIT_COMPLETE		0x80000000
 	u32				tg3_flags2;
 #define TG3_FLG2_RESTART_TIMER		0x00000001
maintainer Applied
#6 lore raw

Re: [TG3 2/3]: Exit irq handler during chip reset.

From: "Michael Chan" <[email protected]>
Date: Fri, 23 Mar 2007 16:13:07 -0800

Quoted 16 lines

[TG3]: Exit irq handler during chip reset.

On most tg3 chips, the memory enable bit in the PCI command register gets cleared during chip reset and must be restored before accessing PCI registers using memory cycles. The chip does not generate interrupt during chip reset, but the irq handler can still be called because of irq sharing or irqpoll. Reading a register in the irq handler can cause a master abort in this scenario and may result in a crash on some architectures.

Use the TG3_FLAG_CHIP_RESETTING flag to tell the irq handler to exit without touching any registers. The checking of the flag is in the "slow" path of the irq handler and will not affect normal performance. The msi handler is not shared and therefore does not require checking the flag.

Quoted 3 lines

Thanks to Bernhard Walle <[email protected]> for reporting the problem.

Applied, except I fixed up some weird tabbing on this line:

Quoted 1 line
(quoted diff) +0 -0
@@ -3568,32 +3568,34 @@ static irqreturn_t tg3_interrupt(int irq, void *dev_id)

...

drivers/net/tg3.c static irqreturn_t tg3_interrupt(int irq, void *dev_id) · line 3598

Quoted 6 lines
drivers/net/tg3.c +6 -0
+	} else {
+		/* No work, shared interrupt perhaps?  re-enable
+		 * interrupts, and flush that PCI write
+		 */
+		tw32_mailbox_f(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
+		     	0x00000000);
                ^^^^^^^^

There are two tabs, some space characters, then a final tab, and it doesn't even make the argument match up to the openning parenthesis on the previous line :-)

#7 lore raw

PATCH 3/3 : Update version and reldate.

In reply to Bernhard Walle

[TG3]: Update version and reldate.

Update version to 3.75.

drivers/net/tg3.c +2 -2
index 9aca100..e682f90 100644
@@ -64,8 +64,8 @@
 
 #define DRV_MODULE_NAME		"tg3"
 #define PFX DRV_MODULE_NAME	": "
-#define DRV_MODULE_VERSION	"3.74"
-#define DRV_MODULE_RELDATE	"February 20, 2007"
+#define DRV_MODULE_VERSION	"3.75"
+#define DRV_MODULE_RELDATE	"March 23, 2007"
 
 #define TG3_DEF_MAC_MODE	0
 #define TG3_DEF_RX_MODE		0