--ReaqsoxgOBHFXBhH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Mar 03, 2006 at 09:53:15PM +0100, Manuel Bouyer wrote:
> 
> BTW, I still don't understand why a driver couldn't claim an interrupt
> even if it didn't generate it. AFAIK in the i386 interrupt code nothing
> uses the return value. This is in i386/vector.S, in INTRSTUB.
> >From what I understand in amd64/vector.S the return value is also ignored.
> 
> I'm probably missing something, but I fail to see what ...

One problem is that it's not at all clear to me what putting the bge
hardware in "in interrupt handler mitigation mode" if it did not
actually interrupt you will do.  That's what the next line of if_bge.c
does.

But I think there is another problem.  See attached diff -- we have
been, according to the Linux tg3 driver, reading the wrong register,
and in fact if you don't read the right one, if you're not using MSI
(which we aren't), it's possible to start processing an interrupt
while the status block for the chip is in an inconsistent state.  That
might also be responsible for some of the chaos.

I cannot test this right now.  I would appreciate it if someone else
would.  The relevant part of the tg3 driver is tg3_interrupt() in
tg3.c.

-- 
  Thor Lancelot Simon	                                     tls@rek.tjls.com

  "We cannot usually in social life pursue a single value or a single moral
   aim, untroubled by the need to compromise with others."      - H.L.A. Hart

--ReaqsoxgOBHFXBhH
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="if_bge.diff"

Index: if_bge.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_bge.c,v
retrieving revision 1.102
diff -u -r1.102 if_bge.c
--- if_bge.c	24 Dec 2005 20:27:42 -0000	1.102
+++ if_bge.c	3 Mar 2006 21:00:42 -0000
@@ -3129,12 +3129,19 @@
 	sc = xsc;
 	ifp = &sc->ethercom.ec_if;
 
-#ifdef notdef
-	/* Avoid this for now -- checking this register is expensive. */
-	/* Make sure this is really our interrupt. */
-	if (!(CSR_READ_4(sc, BGE_MISC_LOCAL_CTL) & BGE_MLC_INTR_STATE))
+	/*
+	 * 0x00000002 is PCISTATE_INT_NOT_ACTIVE from the Linux TG3 driver.
+	 * According to the comments in that driver, an interrupt can arrive
+	 * _before_ the corresponding update of the status block.  So even
+	 * if we are not sharing an interrupt with anyone else, we do want
+	 * to read this register before going any further, because the read
+	 * acts as a barrier to force the status block update across the
+	 * bus.
+	 */
+	  
+	if (!(CSR_READ_4(sc, BGE_PCI_PCISTATE) & 0x00000002))
 		return (0);
-#endif
+
 	/* Ack interrupt and stop others from occuring. */
 	CSR_WRITE_4(sc, BGE_MBX_IRQ0_LO, 1);
 

--ReaqsoxgOBHFXBhH--