ATA driver patch #5 to test

Matthew Dillon dillon at apollo.backplane.com
Mon Nov 15 18:37:49 PST 2004


     Here's another one.

					-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 22:36:43 -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,14 +610,72 @@
     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.
+	 *
+	 * The ATA spec says that DMA data may be transfered with BUSY=1 and 
+	 * DRQ=0, or BUSY=0 and DRQ=1.
+	 */
+	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 */
+    /*
+     * As per ATA spec we must throw away data read above and re-read from
+     * the main status register to get good data and to clear the interrupt.
+     * Otherwise the other bits may have still been changing state when we
+     * determined that BUSY and DRQ were golden (the old device was writing to
+     * the status register at the same time we were reading from it problem).
+     */
     ch->status = ATA_INB(ch->r_io, ATA_STATUS);
 
     if (ch->status & ATA_S_ERROR)
@@ -990,13 +1049,26 @@
     return ATA_OP_FINISHED;
 }
 
+/*
+ * Wait for BUSY to clear and then for all bits specified by the mask
+ * to be set.  The ATA spec says that we have to re-read the status register
+ * after we detect that BUSY has cleared to deal with garbage that may occur
+ * when the device is writing to the status register simultanious with our
+ * reading from it.
+ */
 int
 ata_wait(struct ata_device *atadev, u_int8_t mask)
 {
-    int timeout = 0;
-    
-    DELAY(1);
-    while (timeout < 5000000) { /* timeout 5 secs */
+    sysclock_t last;
+    int timeout;
+
+    /*
+     * 5-second timeout
+     */
+    timeout = 5;
+    last = cputimer_count();
+
+    while (timeout) {
 	atadev->channel->status = ATA_INB(atadev->channel->r_io, ATA_STATUS);
 
 	/* if drive fails status, reselect the drive just to be sure */
@@ -1009,38 +1081,70 @@
 		return -1;
 	}
 
-	/* are we done ? */
-	if (!(atadev->channel->status & ATA_S_BUSY))
-	    break;	      
-
-	if (timeout > 1000) {
-	    timeout += 1000;
-	    DELAY(1000);
+	/*
+	 * When BUSY clears we have to re-read the status register to get
+	 * non-junky data in case we raced the device writing to the status
+	 * register.
+	 */
+	if ((atadev->channel->status & ATA_S_BUSY) == 0) {
+	    atadev->channel->status = ATA_INB(atadev->channel->r_io, ATA_STATUS);
+	    if ((atadev->channel->status & ATA_S_BUSY) == 0)
+		break;	      
 	}
-	else {
-	    timeout += 10;
-	    DELAY(10);
+
+	/*
+	 * It is unclear whether we need to delay here or whether we can
+	 * just poll status as fast as possible.  DELAY a little.
+	 */
+	DELAY(5);
+	if (cputimer_count() - last > cputimer_freq) {
+		--timeout;
+		last += cputimer_freq;
 	}
     }	 
+
+    /*
+     * If status indicates error save the error register for future use,
+     * fail on timeout.
+     */
     if (atadev->channel->status & ATA_S_ERROR)
 	atadev->channel->error = ATA_INB(atadev->channel->r_io, ATA_ERROR);
-    if (timeout >= 5000000)	 
+
+    if (timeout == 0)
 	return -1;	    
-    if (!mask)	   
+
+    /*
+     * If we are not waiting for any bits we were just waiting for BUSY to
+     * clear and return the error status.
+     */
+    if (mask == 0)	   
 	return (atadev->channel->status & ATA_S_ERROR);	 
-    
-    /* Wait 50 msec for bits wanted. */	   
-    timeout = 5000;
-    while (timeout--) {	  
-	atadev->channel->status = ATA_INB(atadev->channel->r_io, ATA_STATUS);
-	if ((atadev->channel->status & mask) == mask) {
-	    if (atadev->channel->status & ATA_S_ERROR)
-		atadev->channel->error=ATA_INB(atadev->channel->r_io,ATA_ERROR);
-	    return (atadev->channel->status & ATA_S_ERROR);	      
-	}
-	DELAY (10);	   
-    }	  
-    return -1;	    
+
+    /*
+     * After BUSY has cleared we wait up to 100ms for the desired bits.
+     * The bits should theoretically have already been loaded in.  If
+     * we already have the data we do not have to reload since we already did
+     * that above.
+     */
+    if ((atadev->channel->status & mask) != mask) {
+	timeout = 100;
+	last = cputimer_count();
+	while (timeout) {
+	    atadev->channel->status = ATA_INB(atadev->channel->r_io, ATA_STATUS);
+	    if ((atadev->channel->status & mask) == mask)
+		break;
+	    if (cputimer_count() - last > cputimer_freq / (1000 / 50)) {
+		    --timeout;
+		    last += cputimer_freq / (1000 / 50);
+	    }
+	    DELAY(5);
+	}
+	if (timeout == 0)
+	    return(-1);
+    }
+    if (atadev->channel->status & ATA_S_ERROR)
+	atadev->channel->error = ATA_INB(atadev->channel->r_io,ATA_ERROR);
+    return (atadev->channel->status & ATA_S_ERROR);	      
 }   
 
 int
@@ -1048,15 +1152,16 @@
 	   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);
+    DELAY(10);	/* XXX */
 
     /* disable interrupt from device */
     if (atadev->channel->flags & ATA_QUEUED)
@@ -1069,6 +1174,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 +1220,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 +1237,52 @@
 		     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.
+     *
+     * NOTE: The ATA spec says that the status register is invalid for 400ns
+     * after writing the command byte.
+     */
+    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 22:05:48 -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 */
@@ -694,8 +697,7 @@
 	if (ata_wait(adp->device, (ATA_S_READY | ATA_S_DSC | ATA_S_DRQ)) != 0) {
 	    ata_prtdev(adp->device, "read error detected (too) late");
 	    request->flags |= ADR_F_ERROR;
-	}
-	else {
+	} else {
 	    /* data ready, read in */
 	    if (adp->device->channel->flags & ATA_USE_16BIT)
 		ATA_INSW(adp->device->channel->r_io, ATA_DATA,
@@ -899,11 +901,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);





More information about the Kernel mailing list