Skip to contentSkip to conversation

tg3 intr masking update

PATCH In review Jeff Garzik 1 participant 2 messages Dec 21, 2002

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Jeff Garzik

author
#0 lore raw

PATCH 1/1 tg3 intr masking update

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 :)

===== drivers/net/tg3.c 1.40 vs edited =====

drivers/net/tg3.c +19 -37
@@ -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);
author
#1 lore raw

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...