ATA driver patch #2 to test

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

    Patch #2!  Testing would be appreciated.

    I found an ata-4 spec and I have attempted to correct the ATA code
    based on the spec.  My previous patch was not sophisticated enough.
    Wholely different tests are required depending on whether DMA is active
    or not.

						-Matt

Index: ata-all.c
===================================================================
RCS file: /cvs/src/sys/dev/disk/ata/ata-all.c,v
retrieving revision 1.21
diff -u -r1.21 ata-all.c
--- ata-all.c	17 Aug 2004 20:59:39 -0000	1.21
+++ ata-all.c	15 Nov 2004 05:36:57 -0000
@@ -26,7 +26,7 @@
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *
  * $FreeBSD: src/sys/dev/ata/ata-all.c,v 1.50.2.45 2003/03/12 14:47:12 sos Exp $
- * $DragonFly: src/sys/dev/disk/ata/ata-all.c,v 1.21 2004/08/17 20:59:39 dillon Exp $
+ * $DragonFly$
  */
 
 #include "opt_ata.h"
@@ -53,6 +53,7 @@
 #include <machine/bus.h>
 #include <machine/clock.h>
 #include <sys/rman.h>
+#include <sys/thread2.h>
 #ifdef __alpha__
 #include <machine/md_var.h>
 #endif
@@ -609,11 +610,60 @@
     if (ch->intr_func && ch->intr_func(ch))
 	return;
 
-    /* if drive is busy it didn't interrupt */
-    if (ATA_INB(ch->r_altio, ATA_ALTSTAT) & ATA_S_BUSY) {
-	DELAY(100);
-	if (!(ATA_INB(ch->r_altio, ATA_ALTSTAT) & ATA_S_DRQ))
-	    return;
+    /*
+     * Figure out if the interrupt is real.  False interrupts can occur in a
+     * number of situations including:
+     *
+     *	- The device asserts the interrupt prior to clearing BUSY or setting
+     *    DRQ.
+     *
+     *  - The device generated an interrupt before we were able to disable
+     *    it and the interrupt thread was already queued.
+     *
+     *  - Other devices share this interrupt vector.
+     *
+     *  - Upon queueing the device command the device may delay before
+     *    setting BUSY (this is handled by ata_command(), not here).
+     *
+     * To deal with the remaining races if ATA_S_BUSY is set we must wait
+     * a bit and check it again to see if it is still set.
+     *
+     * Having to spin in a delay loop is really bad but there is no way
+     * around it.  The ATA spec is just plain broken.
+     */
+
+    if (ch->flags & ATA_DMA_ACTIVE) {
+	/*
+	 * In the DMA case the operation is not complete until both BUSY and
+	 * DRQ are clear.  If either is found to be set we delay and recheck.
+	 * If either is still set the interrupt was bogus and we return.
+	 */
+	if (ATA_INB(ch->r_altio, ATA_ALTSTAT) & (ATA_S_BUSY | ATA_S_DRQ)) {
+	    DELAY(100);
+	    if (ATA_INB(ch->r_altio, ATA_ALTSTAT) & (ATA_S_BUSY | ATA_S_DRQ))
+		return;
+	}
+    } else {
+	/*
+	 * In the PIO case if the device intends to transfer data it will
+	 * set DRQ but may or may not clear BUSY.  If the device does not
+	 * intend to transfer data (for example, an error occurs or there is
+	 * no data to transfer) then the device will clear both BUSY and DRQ.
+	 *
+	 * What this means is we still need to do the delay+recheck due to
+	 * the races mentioned above, but in the second test the interrupt
+	 * is only false if BUSY is set and DRQ is clear.
+	 *
+	 * It may be possible to reduce the 100uS delay below to 10uS.
+	 */
+	if (ATA_INB(ch->r_altio, ATA_ALTSTAT) & ATA_S_BUSY) {
+	    DELAY(100);
+	    if ((ATA_INB(ch->r_altio, ATA_ALTSTAT) & (ATA_S_BUSY | ATA_S_DRQ))
+		== ATA_S_BUSY
+	    ) {
+		return;
+	    }
+	}
     }
 
     /* clear interrupt and get status */
@@ -1048,13 +1098,13 @@
 	   u_int64_t lba, u_int16_t count, u_int8_t feature, int flags)
 {
     int error = 0;
+
 #ifdef ATA_DEBUG
     ata_prtdev(atadev, "ata_command: addr=%04lx, cmd=%02x, "
 	       "lba=%lld, count=%d, feature=%d, flags=%02x\n",
 	       rman_get_start(atadev->channel->r_io), 
 	       command, lba, count, feature, flags);
 #endif
-
     /* select device */
     ATA_OUTB(atadev->channel->r_io, ATA_DRIVE, ATA_D_IBM | atadev->unit);
 
@@ -1069,6 +1119,15 @@
 	return -1;
     }
 
+    /*
+     * Disabling the interrupt may not be enough if it is shared.  It can
+     * take ATA_S_BUSY up to 10uS to become true after a new command has been
+     * queued, we have to guarentee that we do not falsely assume command
+     * completion when the command hasn't even started yet.  We also may need
+     * to interlock with a later tsleep() call.
+     */
+    crit_enter();
+
     /* only use 48bit addressing if needed because of the overhead */
     if ((lba > 268435455 || count > 256) && atadev->param &&
 	atadev->param->support.address48) {
@@ -1106,10 +1165,10 @@
 	    command = ATA_C_FLUSHCACHE48; break;
 	default:
 	    ata_prtdev(atadev, "can't translate cmd to 48bit version\n");
+	    crit_exit();
 	    return -1;
 	}
-    }
-    else {
+    } else {
 	ATA_OUTB(atadev->channel->r_io, ATA_FEATURE, feature);
 	ATA_OUTB(atadev->channel->r_io, ATA_COUNT, count);
 	ATA_OUTB(atadev->channel->r_io, ATA_SECTOR, lba & 0xff);
@@ -1123,41 +1182,49 @@
 		     ATA_D_IBM | ATA_D_LBA | atadev->unit | ((lba>>24) &0xf));
     }
 
-    switch (flags & ATA_WAIT_MASK) {
-    case ATA_IMMEDIATE:
-	ATA_OUTB(atadev->channel->r_io, ATA_CMD, command);
+    /*
+     * Start the command rolling and wait at least 10uS for the device to
+     * bring up BUSY.  Nasty, unfortunately, but at least the delay should
+     * run in parallel with the target device's processing of the command.
+     */
+    atadev->channel->active |= flags & (ATA_WAIT_INTR | ATA_WAIT_READY);
+    ATA_OUTB(atadev->channel->r_io, ATA_CMD, command);
+    DELAY(10);
 
+    /*
+     * Reenable the ATA interrupt unless we are intending to wait for 
+     * completion synchronously.  Note: tsleep is interlocked with our
+     * critical section, otherwise completion may occur before the timeout
+     * is started.
+     */
+    if ((flags & ATA_WAIT_MASK) != ATA_WAIT_READY) {
 	/* enable interrupt */
 	if (atadev->channel->flags & ATA_QUEUED)
 	    ATA_OUTB(atadev->channel->r_altio, ATA_ALTSTAT, ATA_A_4BIT);
-	break;
+    }
 
+    switch (flags & ATA_WAIT_MASK) {
+    case ATA_IMMEDIATE:
+	break;
     case ATA_WAIT_INTR:
-	atadev->channel->active |= ATA_WAIT_INTR;
-	ATA_OUTB(atadev->channel->r_io, ATA_CMD, command);
-
-	/* enable interrupt */
-	if (atadev->channel->flags & ATA_QUEUED)
-	    ATA_OUTB(atadev->channel->r_altio, ATA_ALTSTAT, ATA_A_4BIT);
-
 	if (tsleep((caddr_t)atadev->channel, 0, "atacmd", 10 * hz)) {
 	    ata_prtdev(atadev, "timeout waiting for interrupt\n");
 	    atadev->channel->active &= ~ATA_WAIT_INTR;
 	    error = -1;
 	}
 	break;
-    
     case ATA_WAIT_READY:
-	atadev->channel->active |= ATA_WAIT_READY;
-	ATA_OUTB(atadev->channel->r_io, ATA_CMD, command);
+	crit_exit();
 	if (ata_wait(atadev, ATA_S_READY) < 0) { 
 	    ata_prtdev(atadev, "timeout waiting for cmd=%02x s=%02x e=%02x\n",
 		       command, atadev->channel->status,atadev->channel->error);
 	    error = -1;
 	}
 	atadev->channel->active &= ~ATA_WAIT_READY;
+	crit_enter();
 	break;
     }
+    crit_exit();
     return error;
 }
 
Index: ata-disk.c
===================================================================
RCS file: /cvs/src/sys/dev/disk/ata/ata-disk.c,v
retrieving revision 1.23
diff -u -r1.23 ata-disk.c
--- ata-disk.c	23 Sep 2004 11:50:03 -0000	1.23
+++ ata-disk.c	15 Nov 2004 05:08:37 -0000
@@ -462,6 +462,9 @@
     u_int64_t lba;
     u_int32_t count, max_count;
     u_int8_t cmd;
+#if 0
+    u_int8_t status;
+#endif
     int flags = ATA_IMMEDIATE;
 
     /* get request params */
@@ -503,7 +506,20 @@
 
 	devstat_start_transaction(&adp->stats);
 
-	/* does this drive & transfer work with DMA ? */
+	/*
+	 * DMA OPERATION.  A DMA based ATA command issues the command and
+	 * then sets up and initiates DMA.  Once the command is issued the
+	 * device will set ATA_S_BUSY.  When the device is ready to transfer
+	 * actual data it will clear ATA_S_BUSY and set ATA_S_DRQ.  However,
+	 * because DRQ represents actual data ready to roll this can take
+	 * a while (e.g. for a read the disk has to get to the data).  We
+	 * don't want to spin that long.
+	 *
+	 * It is unclear whether DMA can simply be started without waiting
+	 * for ATA_S_BUSY to clear but that seems to be what everyone else
+	 * does.  It might be prudent to wait for at least ATA_S_READY but
+	 * we don't (yet) do that.
+	 */
 	request->flags &= ~ADR_F_DMA_USED;
 	if (adp->device->mode >= ATA_DMA &&
 	    !ata_dmasetup(adp->device, request->data, request->bytecount)) {
@@ -531,8 +547,7 @@
 		    ATA_INB(adp->device->channel->r_io, ATA_IREASON) &
 		    ATA_I_RELEASE)
 		    return ad_service(adp, 1);
-	    }
-	    else {
+	    } else {
 		cmd = (request->flags & ADR_F_READ) ?
 		    ATA_C_READ_DMA : ATA_C_WRITE_DMA;
 
@@ -540,18 +555,6 @@
 		    ata_prtdev(adp->device, "error executing command");
 		    goto transfer_failed;
 		}
-#if 0
-		/*
-		 * wait for data transfer phase
-		 *
-		 * well this should be here acording to specs, but older
-		 * promise controllers doesn't like it, they lockup!
-		 */
-		if (ata_wait(adp->device, ATA_S_READY | ATA_S_DRQ)) {
-		    ata_prtdev(adp->device, "timeout waiting for data phase\n");
-		    goto transfer_failed;
-		}
-#endif
 	    }
 
 	    /* start transfer, return and wait for interrupt */
@@ -899,11 +902,14 @@
 ad_timeout(struct ad_request *request)
 {
     struct ad_softc *adp = request->softc;
+    u_int8_t status;
 
+    status = ATA_INB(adp->device->channel->r_io, ATA_STATUS);
     adp->device->channel->running = NULL;
-    ata_prtdev(adp->device, "%s command timeout tag=%d serv=%d - resetting\n",
+    ata_prtdev(adp->device, "%s command timeout tag=%d serv=%d status %02x"
+			    " - resetting\n",
 	       (request->flags & ADR_F_READ) ? "READ" : "WRITE",
-	       request->tag, request->serv);
+	       request->tag, request->serv, status);
 
     if (request->flags & ADR_F_DMA_USED) {
 	ata_dmadone(adp->device);