>Number:         33445
>Category:       kern
>Synopsis:       These patches fix a number of problems with the pdcsata driver for NetBSD-3.0 and -current..
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon May 08 23:30:00 +0000 2006
>Originator:     Brian Buhrow
>Release:        NetBSD 3.0_STABLE and -current
>Organization:
	Vianet Communications
>Environment:
	
	
System: NetBSD fserv1.via.net 3.0_STABLE NetBSD 3.0_STABLE (NFBNETBSD) #0: Tue Jan 31 14:45:08 PST 2006 buhrow@lothlorien.nfbcal.org:/usr/src/sys/arch/i386/compile/NFBNETBSD i386
Architecture: i386
Machine: i386
>Description:
	
	The driver for the family of Promise SATA controllers,
/usr/src/sys/dev/pci/pdcsata.c is not very robust when it comes to handling
transient drive errors, or interrupt hickups when the card is under load.
Worse, my experience seems to indicate, and the Linux driver confirms,
that these cards tend to fall over rather frequently during high load
operations or if drives unexpectedly reset or go to sleep.  Symptoms
include interupt timeouts during heavy load, the inability to reset drives
if they go to sleep, and a failure of the card to generate interrupts at
all if the interrupt load gets too high.

>How-To-Repeat:
	
	To test to see if you're encountering the problems this driver fixes
before you patch, try the following steps:

1.  Install a card supported by the pdcsata driver, either one of the 203xx
cards, or the 205xx cards.  The Promise PDC40718 and PDC40719 cards are
also supported by the pdcsata driver.

2.  After the card is configured, and you have a disk on it which is
running, perform the command:
atactl /dev/wd3d sleep
Assuming the drive attached to your pdcsata driven card is wd3.  Change the
drive number to match the drive actually attached to your pdcsata card.
Now, run
disklabel -r wd3
Again, making the same assumptions as above.
	If you have the broken verssion of the driver, you won't be able to
revive the drive without a reboot.
>Fix:
	
	These patches solve all the bugs listed above, as well as simplify
the driver.  I have tested these patches on production systems running at
high volume, and they work well.  I have been working with abs@netbsd.org,
who also has one of these cards, and they help him as well, although there
are still some minor issues to work out with his setup.
	These patches apply cleanly against 3.0 sources as of April 21, 2006,
but I believe they'll apply equally cleanly to current 3.0  sources, as
well as -current sources.
	I would like to see these fixes get into 3.0, as well as the 4.0
branch.

-thanks
-Brian


Index: pdcsata.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/pdcsata.c,v
retrieving revision 1.3.2.2
diff -u -r1.3.2.2 pdcsata.c
--- pdcsata.c	5 Feb 2006 17:13:57 -0000	1.3.2.2
+++ pdcsata.c	5 May 2006 17:07:57 -0000
@@ -48,17 +48,19 @@
 
 #define PDC203xx_BAR_IDEREGS 0x1c /* BAR where the IDE registers are mapped */
 
+#define PDC_CHANNELBASE(ch) 0x200 + ((ch) * 0x80)
+#define PDC_ERRMASK 0x00780700
+
 static void pdcsata_chip_map(struct pciide_softc *, struct pci_attach_args *);
 static void pdc203xx_setup_channel(struct ata_channel *);
-static int  pdc203xx_pci_intr(void *);
 static void pdc203xx_irqack(struct ata_channel *);
 static int  pdc203xx_dma_init(void *, int, int, void *, size_t, int);
 static void pdc203xx_dma_start(void *,int ,int);
 static int  pdc203xx_dma_finish(void *, int, int, int);
+static int  pdcsata_pci_intr(void *);
+static void pdcsata_do_reset(struct ata_channel *, int);
 
 /* PDC205xx, PDC405xx and PDC407xx. but tested only pdc40718 */
-static int  pdc205xx_pci_intr(void *);
-static void pdc205xx_do_reset(struct ata_channel *, int);
 static void pdc205xx_drv_probe(struct ata_channel *);
 
 static int  pdcsata_match(struct device *, struct cfdata *, void *);
@@ -183,30 +185,8 @@
 		return;
 	}
 	intrstr = pci_intr_string(pa->pa_pc, intrhandle);
-
-	switch (sc->sc_pp->ide_product) {
-	case PCI_PRODUCT_PROMISE_PDC20318:
-	case PCI_PRODUCT_PROMISE_PDC20319:
-	case PCI_PRODUCT_PROMISE_PDC20371:
-	case PCI_PRODUCT_PROMISE_PDC20375:
-	case PCI_PRODUCT_PROMISE_PDC20376:
-	case PCI_PRODUCT_PROMISE_PDC20377:
-	case PCI_PRODUCT_PROMISE_PDC20378:
-	case PCI_PRODUCT_PROMISE_PDC20379:
-	default:
-		sc->sc_pci_ih = pci_intr_establish(pa->pa_pc,
-		    intrhandle, IPL_BIO, pdc203xx_pci_intr, sc);
-		break;
-
-	case PCI_PRODUCT_PROMISE_PDC40718:
-	case PCI_PRODUCT_PROMISE_PDC40719:
-	case PCI_PRODUCT_PROMISE_PDC20571:
-	case PCI_PRODUCT_PROMISE_PDC20575:
-	case PCI_PRODUCT_PROMISE_PDC20579:
-		sc->sc_pci_ih = pci_intr_establish(pa->pa_pc,
-		    intrhandle, IPL_BIO, pdc205xx_pci_intr, sc);
-		break;
-	}
+	sc->sc_pci_ih = pci_intr_establish(pa->pa_pc,
+	    intrhandle, IPL_BIO, pdcsata_pci_intr, sc);
 
 	if (sc->sc_pci_ih == NULL) {
 		aprint_error("%s: couldn't establish native-PCI interrupt",
@@ -258,6 +238,8 @@
 	sc->sc_wdcdev.sc_atac.atac_set_modes = pdc203xx_setup_channel;
 	sc->sc_wdcdev.sc_atac.atac_channels = sc->wdc_chanarray;
 
+	sc->sc_wdcdev.reset = pdcsata_do_reset;
+
 	switch (sc->sc_pp->ide_product) {
 	case PCI_PRODUCT_PROMISE_PDC20318:
 	case PCI_PRODUCT_PROMISE_PDC20319:
@@ -281,7 +263,6 @@
 		bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x60, 0x00ff00ff);
 		sc->sc_wdcdev.sc_atac.atac_nchannels = PDC40718_NCHANNELS;
 
-		sc->sc_wdcdev.reset = pdc205xx_do_reset;
 		sc->sc_wdcdev.sc_atac.atac_probe = pdc205xx_drv_probe;
 
 		break;
@@ -290,7 +271,6 @@
 		bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x60, 0x00ff00ff);
 		sc->sc_wdcdev.sc_atac.atac_nchannels = PDC20575_NCHANNELS;
 
-		sc->sc_wdcdev.reset = pdc205xx_do_reset;
 		sc->sc_wdcdev.sc_atac.atac_probe = pdc205xx_drv_probe;
 
 		break;
@@ -403,53 +383,37 @@
 }
 
 static int
-pdc203xx_pci_intr(void *arg)
+pdcsata_pci_intr(void *arg)
 {
 	struct pciide_softc *sc = arg;
 	struct pciide_channel *cp;
 	struct ata_channel *wdc_cp;
 	int i, rv, crv;
-	u_int32_t scr;
-
-	rv = 0;
-	scr = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x00040);
-
-	for (i = 0; i < sc->sc_wdcdev.sc_atac.atac_nchannels; i++) {
-		cp = &sc->pciide_channels[i];
-		wdc_cp = &cp->ata_channel;
-		if (scr & (1 << (i + 1))) {
-			crv = wdcintr(wdc_cp);
-			if (crv == 0) {
-				printf("%s:%d: bogus intr (reg 0x%x)\n",
-				    sc->sc_wdcdev.sc_atac.atac_dev.dv_xname,
-				    i, scr);
-			} else
-				rv = 1;
-		}
-	}
-	return rv;
-}
-
-static int
-pdc205xx_pci_intr(void *arg)
-{
-	struct pciide_softc *sc = arg;
-	struct pciide_channel *cp;
-	struct ata_channel *wdc_cp;
-	int i, rv, crv;
-	u_int32_t scr, status;
+	u_int32_t scr, status, chanbase;
 
 	rv = 0;
 	scr = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x40);
+	if (scr == 0xffffffff) return(1);
 	bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x40, scr & 0x0000ffff);
-
-	status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x60);
-	bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, 0x60, status & 0x000000ff);
+	scr = scr & 0x0000ffff;
+	if (!scr) return(1);
 
 	for (i = 0; i < sc->sc_wdcdev.sc_atac.atac_nchannels; i++) {
 		cp = &sc->pciide_channels[i];
 		wdc_cp = &cp->ata_channel;
 		if (scr & (1 << (i + 1))) {
+			chanbase = PDC_CHANNELBASE(i) + 0x48;
+			status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
+			if (status & PDC_ERRMASK) {
+				chanbase = PDC_CHANNELBASE(i) + 0x60;
+				status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
+				status |= 0x800;
+				bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase, status);
+				status &= ~0x800;
+				bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase, status);
+				status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
+				continue;
+			}
 			crv = wdcintr(wdc_cp);
 			if (crv == 0) {
 				printf("%s:%d: bogus intr (reg 0x%x)\n",
@@ -541,24 +505,29 @@
 
 
 static void
-pdc205xx_do_reset(struct ata_channel *chp, int poll)
+pdcsata_do_reset(struct ata_channel *chp, int poll)
 {
 	struct pciide_softc *sc = CHAN_TO_PCIIDE(chp);
-	u_int32_t scontrol;
-
-	wdc_do_reset(chp, poll);
+	int reset, status, i, chanbase;
 
 	/* reset SATA */
-	scontrol = SControl_DET_INIT | SControl_SPD_ANY | SControl_IPM_NONE;
-	SCONTROL_WRITE(sc, chp->ch_channel, scontrol);
-	delay(50*1000);
-
-	scontrol &= ~SControl_DET_INIT;
-	SCONTROL_WRITE(sc, chp->ch_channel, scontrol);
-	delay(50*1000);
-}
+	reset = (1 << 11);
+	chanbase = PDC_CHANNELBASE(chp->ch_channel) + 0x60;
+	for (i = 0; i < 11;i ++) {
+		status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
+		if (status & reset) break;
+		delay(100);
+		status |= reset;
+		bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase, status);
+	}
+	status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
+	status &= ~reset;
+	bus_space_write_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase, status);
+	status = bus_space_read_4(sc->sc_ba5_st, sc->sc_ba5_sh, chanbase);
 
+	wdc_do_reset(chp, poll);
 
+}
 
 static void
 pdc205xx_drv_probe(struct ata_channel *chp)

>Unformatted: