ATA driver patch to test

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

    I would appreciate testing of this patch.  It does a couple of things:

    (1) It corrects an incorrect DRQ test that should have been a re-test
	of ATA_S_BUSY (FreeBSD-5 uses the new method).  Testing DRQ is
	insufficient because the race that was meant to be checked was
	whether the interrupt was meant for us or not.  Since the target
	may generate an interrupt and not clear BUSY for up to 100uS, if
	we find BUSY still set we have to wait and check again before 
	assuming that the interrupt was not for us.

    (2) It emplaces a critical section around the command queueing code
	through to the tsleep().  While the code is supposed to be in an
	SPLBIO it may be possible, through a number of circumstances, for
	the interrupt handler to be called anyway just before the tsleep
	is entered, resulting in no wakeup occuring and the tsleep timing
	out.

	Note also that an ATA interrupt may have been queued prior to ata-all's
	disablement of the interrupt.  That is, disabling the hardware bit does
	not necessarily prevent an interrupt from being dispatched (though it
	will prevent more then one, at least from the ATA device).  If the
	interrupt is shared other interrupts may occur anyhow and execute
	that ata's interrupt routine.

    (3) Impose a mandatory 10uS delay after a new ATA command is queued,
	within the critical section.  The target device may take that long
	to set ATA_S_BUSY and if we interrupt before it manages to do so
	we may wind up believing that the command has completed when in fact
	it has not yet started running.

	The BUSY timing is one of the biggest flaws in the ATA specification.
	Since BUSY is controlled by the target device, each combination of
	controller and target device may yield radically different timings
	for the bit.

						-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	13 Nov 2004 08:24:51 -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,10 +610,20 @@
     if (ch->intr_func && ch->intr_func(ch))
 	return;
 
-    /* if drive is busy it didn't interrupt */
+    /*
+     * If the drive is still busy then it did not generate an interrupt.
+     * However, there are two latency problems with ATA.  First, ATA may
+     * generate an interrupt before it clears the BUSY bit, so we have to
+     * DELAY and check again.   Second, ATA may not *set* the BUSY bit after
+     * a command is sent for upwards of 10uS or longer, leading us to believe
+     * that a command has completed when it has not.  #2 is handled
+     * elsewhere, in the command queueing code.
+     *
+     * Pretty dumb, eh?
+     */
     if (ATA_INB(ch->r_altio, ATA_ALTSTAT) & ATA_S_BUSY) {
 	DELAY(100);
-	if (!(ATA_INB(ch->r_altio, ATA_ALTSTAT) & ATA_S_DRQ))
+	if (ATA_INB(ch->r_altio, ATA_ALTSTAT) & ATA_S_BUSY)
 	    return;
     }
 
@@ -1048,13 +1059,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 +1080,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 +1126,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 +1143,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	14 Nov 2004 20:53:39 -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);