Subject: Re: kern/26568: Yesterday's "pciide" `irqack fix' breaks Promise 202xx controllers
To: None <paul@Plectere.com>
From: Manuel Bouyer <bouyer@antioche.lip6.fr>
List: netbsd-bugs
Date: 08/06/2004 17:03:39
--ibTvN161/egqYuK8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Fri, Aug 06, 2004 at 02:24:43AM -0700, paul@Plectere.com wrote:
> >Fix:
> 	
> 	Revert the change for the Promises?  Maybe a further test on the
> wdc state beyond the simple "wdcintr(wdc_cp)"? - Either way, please do not
> write the "IDEDMA_CTL" during the interrupt without acknowledging the interrupt
> to the hardware(i.e. the EOI dance on x86) (If a DMA is really pending, we
> can get into the infinite-loop case described (remember, now the WDC `cause'
> has been cleared) beginning when the outstanding DMA completes or we lose
> the outstanding transaction - neither is a good choice;  The outstanding
> request causes another "bogus" interrupt, etc), or look into a non-zero
> return and doing the EOI dance to prevent redelivery of the same interrupt
> (Note: the case in the Promise returns zero, if we're eating the interrupt,
> we probably should return one -- i.e. "rv = 1;" ? - I didn't test this, but
> it seem like it might be simple enough to work).

Can you try the attached patch, then ?
But now there is something I don't understand. How did it happen we
didn't run in this infinite loop before ? If clearing the interrupt cause
isn't enouth, then we should have seen the same behavior with the old code.
If wdcintr() didn't claim the interrupt, there's no reasons it will claim
it the next time.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--

--ibTvN161/egqYuK8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=diff2

Index: acardide.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/acardide.c,v
retrieving revision 1.9
diff -u -r1.9 acardide.c
--- acardide.c	2 Aug 2004 19:08:16 -0000	1.9
+++ acardide.c	6 Aug 2004 14:48:37 -0000
@@ -316,10 +316,8 @@
 			    sc->sc_wdcdev.sc_dev.dv_xname, i);
 			bus_space_write_1(sc->sc_dma_iot,
 			    cp->dma_iohs[IDEDMA_CTL], 0, dmastat);
-		} else if (crv == 1)
-			rv = 1;
-		else if (rv == 0)
-			rv = crv;
+		}
+		rv = 1;
 	}
 	return rv;
 }
Index: aceride.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/aceride.c,v
retrieving revision 1.7
diff -u -r1.7 aceride.c
--- aceride.c	2 Aug 2004 19:08:16 -0000	1.7
+++ aceride.c	6 Aug 2004 14:48:37 -0000
@@ -285,8 +285,8 @@
 				printf("%s:%d: bogus intr\n",
 				    sc->sc_wdcdev.sc_dev.dv_xname, i);
 				pciide_irqack(wdc_cp);
-			} else
-				rv = 1;
+			}
+			rv = 1;
 		}
 	}
 	return rv;
Index: cmdide.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/cmdide.c,v
retrieving revision 1.11
diff -u -r1.11 cmdide.c
--- cmdide.c	2 Aug 2004 19:08:16 -0000	1.11
+++ cmdide.c	6 Aug 2004 14:48:37 -0000
@@ -221,8 +221,8 @@
 				printf("%s:%d: bogus intr\n",
 				    sc->sc_wdcdev.sc_dev.dv_xname, i);
 				sc->sc_wdcdev.irqack(wdc_cp);
-			} else
-				rv = 1;
+			}
+			rv = 1;
 		}
 	}
 	return rv;
Index: hptide.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/hptide.c,v
retrieving revision 1.10
diff -u -r1.10 hptide.c
--- hptide.c	2 Aug 2004 19:37:33 -0000	1.10
+++ hptide.c	6 Aug 2004 14:48:37 -0000
@@ -398,8 +398,8 @@
 			    sc->sc_wdcdev.sc_dev.dv_xname, i);
 			bus_space_write_1(sc->sc_dma_iot,
 			    cp->dma_iohs[IDEDMA_CTL], 0, dmastat);
-		} else
-			rv = 1;
+		}
+		rv = 1;
 	}
 	return rv;
 }
Index: pdcide.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/pdcide.c,v
retrieving revision 1.12
diff -u -r1.12 pdcide.c
--- pdcide.c	2 Aug 2004 19:08:16 -0000	1.12
+++ pdcide.c	6 Aug 2004 14:48:37 -0000
@@ -509,8 +509,8 @@
 				printf("%s:%d: bogus intr (reg 0x%x)\n",
 				    sc->sc_wdcdev.sc_dev.dv_xname, i, scr);
 				pciide_irqack(wdc_cp);
-			} else
-				rv = 1;
+			}
+			rv = 1;
 		}
 	}
 	return rv;
@@ -548,11 +548,13 @@
 		if((dmastat & IDEDMA_CTL_INTR) == 0)
 			continue;
 		crv = wdcintr(wdc_cp);
-		if (crv == 0)
+		if (crv == 0) {
 			printf("%s:%d: bogus intr\n",
 			    sc->sc_wdcdev.sc_dev.dv_xname, i);
-		else
-			rv = 1;
+			bus_space_write_1(sc->sc_dma_iot,
+			    cp->dma_iohs[IDEDMA_CTL], 0, dmastat);
+		} 
+		rv = 1;
 	}
 	return rv;
 }
Index: rccide.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/rccide.c,v
retrieving revision 1.7
diff -u -r1.7 rccide.c
--- rccide.c	3 Jan 2004 22:56:53 -0000	1.7
+++ rccide.c	6 Aug 2004 14:48:37 -0000
@@ -270,8 +270,8 @@
 			    sc->sc_wdcdev.sc_dev.dv_xname, i);
 			bus_space_write_1(sc->sc_dma_iot,
 			    cp->dma_iohs[IDEDMA_CTL], 0, dmastat);
-		} else
-			rv = 1;
+		} 
+		rv = 1;
 	}
 	return rv;
 }

--ibTvN161/egqYuK8--