[PATCH] tg3 intr masking update

[PATCH] tg3 intr masking update

[Jeff Garzik]

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

Here is a better patch, after I mulled it over on the trip to Atlanta.

It is based on the premise, derived from the chip documentation, that

1) intr-mbox-0 register usage is used to indicate to the NIC that the 
driver is "in interrupt handler".
2) (intr-mbox-0 = 1) == pci-inta-mask + pci-inta-clear + stuff
3) the docs seem to be saying "just use intr-mbox-0 and ignore 
pci-inta-mask"

Based on these three observations, the attached patch updates tg3 for 
the following changes:

1) Mask ints in tg3_interrupt and unmask in tg3_poll.   We not only 
eliminate several MMIO writes/reads, but we also eliminate all usage of 
functions tg3_[un]mask_ints(), which bit-bangs the 
MISC_HOST_CTRL_MASK_PCI_INT bit in the TG3PCI_MISC_HOST_CTRL register.

2) If tg3_main_interrupt_work() returns no-work-exists, we [obviously 
must] unmask ints in tg3_interrupt.  But this is an unlikely case.

3) Simply call netif_rx_schedule(dev) in tg3_interrupt_main_work(). 
This was a change previously discussed.  Eliminates the "poll already 
scheduled" message which was often actually a bogus message.

4) Because of #2, tg3_interrupt_main_work returns boolean 'work_exists' now.

5) There are some Bugzilla bug reports for tg3 1.2, where NMI watchdog 
triggers in tg3_timer.  I did s/spin_lock_irq/spin_lock_irqsave/ in 
tg3_poll for safety, because we might be locking against a timer not 
only an interrupt handler.  And just for general paranoia and safety as 
an "obviously more safe" change.

Comments/feedback/corrections/flames requested :)

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 2960 bytes --]

===== drivers/net/tg3.c 1.40 vs edited =====
--- 1.40/drivers/net/tg3.c	Mon Dec  9 21:32:02 2002
+++ edited/drivers/net/tg3.c	Sat Dec 21 15:06:34 2002
@@ -217,22 +217,6 @@
 	tr32(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW);
 }
 
-static inline void tg3_mask_ints(struct tg3 *tp)
-{
-	tw32(TG3PCI_MISC_HOST_CTRL,
-	     (tp->misc_host_ctrl | MISC_HOST_CTRL_MASK_PCI_INT));
-}
-
-static inline void tg3_unmask_ints(struct tg3 *tp)
-{
-	tw32(TG3PCI_MISC_HOST_CTRL,
-	     (tp->misc_host_ctrl & ~MISC_HOST_CTRL_MASK_PCI_INT));
-	if (tp->hw_status->status & SD_STATUS_UPDATED) {
-		tw32(GRC_LOCAL_CTRL,
-		     tp->grc_local_ctrl | GRC_LCLCTRL_SETINT);
-	}
-}
-
 static void tg3_switch_clocks(struct tg3 *tp)
 {
 	if (tr32(TG3PCI_CLOCK_CTRL) & CLOCK_CTRL_44MHZ_CORE) {
@@ -2014,8 +1998,9 @@
 	struct tg3 *tp = netdev->priv;
 	struct tg3_hw_status *sblk = tp->hw_status;
 	int done;
+	unsigned long flags;
 
-	spin_lock_irq(&tp->lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 	if (!(tp->tg3_flags &
 	      (TG3_FLAG_USE_LINKCHG_REG |
@@ -2052,15 +2037,18 @@
 
 	if (done) {
 		netif_rx_complete(netdev);
-		tg3_unmask_ints(tp);
+		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
+		     	     0x00000000);
+		/* don't care about flushing this write immediately */
 	}
 
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
 
 	return (done ? 0 : 1);
 }
 
-static __inline__ void tg3_interrupt_main_work(struct net_device *dev, struct tg3 *tp)
+static inline unsigned int tg3_interrupt_main_work(struct net_device *dev,
+					   	   struct tg3 *tp)
 {
 	struct tg3_hw_status *sblk = tp->hw_status;
 	int work_exists = 0;
@@ -2075,19 +2063,10 @@
 	    sblk->idx[0].rx_producer != tp->rx_rcb_ptr)
 		work_exists = 1;
 
-	if (!work_exists)
-		return;
+	if (work_exists)
+		netif_rx_schedule(dev);
 
-	if (netif_rx_schedule_prep(dev)) {
-		/* NOTE: These writes are posted by the readback of
-		 *       the mailbox register done by our caller.
-		 */
-		tg3_mask_ints(tp);
-		__netif_rx_schedule(dev);
-	} else {
-		printk(KERN_ERR PFX "%s: Error, poll already scheduled\n",
-		       dev->name);
-	}
+	return work_exists;
 }
 
 static void tg3_interrupt(int irq, void *dev_id, struct pt_regs *regs)
@@ -2102,13 +2081,16 @@
 	if (sblk->status & SD_STATUS_UPDATED) {
 		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
 			     0x00000001);
+		/* flush the intr clear-and-mask before updating status block */
+		tr32(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW);
 		sblk->status &= ~SD_STATUS_UPDATED;
 
-		tg3_interrupt_main_work(dev, tp);
-
-		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
-			     0x00000000);
-		tr32(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW);
+		if (!tg3_interrupt_main_work(dev, tp)) {
+			/* if work doesn't exist... */
+			tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
+			     	     0x00000000);
+			tr32(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW);
+		}
 	}
 
 	spin_unlock_irqrestore(&tp->lock, flags);

Re: [PATCH] tg3 intr masking update

[Jeff Garzik]

Additional note about this change,

The NAPI docs mention specific areas where races may occur, and 
coincedentally so do the tg3 docs.  The tg3 docs suggest to me that in 
tg3_poll, after we unmask ints, we should
* flush the MMIO write
* check the status block for updates again
* tell tg3 to deliver to us an artificial interrupt just in case we 
raced [IMO preferred versus simply returning 'not-done' in tg3_poll retval]

But this is just a theory and not tested yet...