Remove the old ATA_*LOCK_CH macros that used atomic ops and use

mutexes instead.
This closes the last (known) race issues in ATA which should fix
the various hangs etc seen on heavy loaded systems.

Change from using timeout functions to using callout functions in
the timeout code. This together with above closes the race that could
happen if timeout and device interrupt occured simultaniously.

Also fix the possible recursion in ata_reinit() on very dodgy
devices that could take us down in the probe.

5 files changed, 258 insertions, 215 deletions

diff --git a/sys/dev/ata/ata-all.c b/sys/dev/ata/ata-all.c
index cf7acb9..1c4b0d6 100644
--- a/sys/dev/ata/ata-all.c
+++ b/sys/dev/ata/ata-all.c
@@ -67,6 +67,7 @@ static struct cdevsw ata_cdevsw = {
 
 /* prototypes */
 static void ata_shutdown(void *, int);
+static void ata_interrupt(void *);
 static int ata_getparam(struct ata_device *, u_int8_t);
 static void ata_identify_devices(struct ata_channel *);
 static void ata_boot_attach(void);
@@ -132,7 +133,9 @@ ata_attach(device_t dev)
     ch->device[SLAVE].unit = ATA_SLAVE;
     ch->device[SLAVE].mode = ATA_PIO;
     ch->dev = dev;
-    ch->lock = ATA_IDLE;
+    ch->state = ATA_IDLE;
+    bzero(&ch->state_mtx, sizeof(struct mtx));
+    mtx_init(&ch->state_mtx, "ATA state lock", NULL, MTX_DEF);
 
     /* initialise device(s) on this channel */
     ch->locking(ch, ATA_LF_LOCK);
@@ -147,7 +150,7 @@ ata_attach(device_t dev)
 	return ENXIO;
     }
     if ((error = bus_setup_intr(dev, ch->r_irq, ATA_INTR_FLAGS,
-				ch->hw.interrupt, ch, &ch->ih))) {
+				ata_interrupt, ch, &ch->ih))) {
 	ata_printf(ch, -1, "unable to setup interrupt\n");
 	return error;
     }
@@ -189,7 +192,9 @@ ata_detach(device_t dev)
     ata_fail_requests(ch, NULL);
 
     /* unlock the channel */
-    ATA_UNLOCK_CH(ch);
+    mtx_lock(&ch->state_mtx);
+    ch->state = ATA_IDLE;
+    mtx_unlock(&ch->state_mtx);
     ch->locking(ch, ATA_LF_UNLOCK);
 
     /* detach devices on this channel */
@@ -238,9 +243,17 @@ ata_reinit(struct ata_channel *ch)
     if (bootverbose)
 	ata_printf(ch, -1, "reiniting channel ..\n");
 
-    ATA_FORCELOCK_CH(ch);
+    /* grap the channel lock no matter what */
+    mtx_lock(&ch->state_mtx);
+    ch->state = ATA_ACTIVE;
+    mtx_unlock(&ch->state_mtx);
+
+    if (ch->flags & ATA_IMMEDIATE_MODE)
+	return EIO;
+    else
+	ch->flags |= ATA_IMMEDIATE_MODE;
+
     ata_catch_inflight(ch);
-    ch->flags |= ATA_IMMEDIATE_MODE;
     devices = ch->devices;
 
     ch->hw.reset(ch);
@@ -266,14 +279,28 @@ ata_reinit(struct ata_channel *ch)
 	}
     }
 
-    /* unlock the channel */
-    ATA_UNLOCK_CH(ch);
-    ch->locking(ch, ATA_LF_UNLOCK);
-
     /* identify what is present on the channel now */
     ata_identify_devices(ch);
 
-    /* attach new devices that appeared during reset */
+    /* detach what left the channel during identify */
+    if ((misdev = devices & ~ch->devices)) {
+	if ((misdev & (ATA_ATA_MASTER | ATA_ATAPI_MASTER)) &&
+	    ch->device[MASTER].detach) {
+	    ch->device[MASTER].detach(&ch->device[MASTER]);
+	    ata_fail_requests(ch, &ch->device[MASTER]);
+	    free(ch->device[MASTER].param, M_ATA);
+	    ch->device[MASTER].param = NULL;
+	}
+	if ((misdev & (ATA_ATA_SLAVE | ATA_ATAPI_SLAVE)) &&
+	    ch->device[SLAVE].detach) {
+	    ch->device[SLAVE].detach(&ch->device[SLAVE]);
+	    ata_fail_requests(ch, &ch->device[SLAVE]);
+	    free(ch->device[SLAVE].param, M_ATA);
+	    ch->device[SLAVE].param = NULL;
+	}
+    }
+
+    /* attach new devices */
     if ((newdev = ~devices & ch->devices)) {
 	if ((newdev & (ATA_ATA_MASTER | ATA_ATAPI_MASTER)) &&
 	    ch->device[MASTER].attach)
@@ -289,7 +316,12 @@ ata_reinit(struct ata_channel *ch)
 
     if (bootverbose)
 	ata_printf(ch, -1, "device config done ..\n");
+
     ch->flags &= ~ATA_IMMEDIATE_MODE;
+    mtx_lock(&ch->state_mtx);
+    ch->state = ATA_IDLE;
+    mtx_unlock(&ch->state_mtx);
+
     ata_start(ch);
     return 0;
 }
@@ -298,12 +330,22 @@ int
 ata_suspend(device_t dev)
 {
     struct ata_channel *ch;
+    int gotit = 0;
 
     if (!dev || !(ch = device_get_softc(dev)))
 	return ENXIO;
 
     ch->locking(ch, ATA_LF_LOCK);
-    ATA_SLEEPLOCK_CH(ch);
+
+    while (!gotit) {
+	mtx_lock(&ch->state_mtx);
+	if (ch->state == ATA_IDLE) {
+	    ch->state = ATA_ACTIVE;
+	    gotit = 1;
+	}
+	tsleep(&gotit, PRIBIO, "atasusp", hz/10);
+	mtx_unlock(&ch->state_mtx);
+    }
     return 0;
 }
 
@@ -342,11 +384,66 @@ ata_shutdown(void *arg, int howto)
     }
 }
 
+static void
+ata_interrupt(void *data)
+{
+    struct ata_channel *ch = (struct ata_channel *)data;
+    struct ata_request *request = ch->running;
+    int gotit = 0;
+
+    /* ignore interrupt if there is no running request */
+    if (!request) 
+	return;
+
+    ATA_DEBUG_RQ(request, "interrupt");
+
+    /* ignore interrupt if device is busy */
+    if (ATA_IDX_INB(ch, ATA_ALTSTAT) & ATA_S_BUSY) {
+    	DELAY(100);
+	if (ATA_IDX_INB(ch, ATA_ALTSTAT) & ATA_S_BUSY)
+	    return;
+    }
+
+    ATA_DEBUG_RQ(request, "interrupt accepted");
+
+    mtx_lock(&ch->state_mtx);
+    if (ch->state == ATA_ACTIVE) {
+	ch->state = ATA_INTERRUPT;
+	gotit = 1;
+    }
+    else
+	ata_printf(ch, -1,
+		   "unexpected state in ata_interrupt 0x%02x\n", ch->state);
+    mtx_unlock(&ch->state_mtx);
+
+    /* if we got our locks finish up this request */
+    if (gotit) {
+	request->flags |= ATA_R_INTR_SEEN;
+	if (ch->hw.end_transaction(request) == ATA_OP_CONTINUES) {
+	    request->flags &= ~ATA_R_INTR_SEEN;
+	    mtx_lock(&ch->state_mtx);
+	    ch->state = ATA_ACTIVE;
+	    mtx_unlock(&ch->state_mtx);
+	}
+	else {
+	    ch->running = NULL;
+	    mtx_lock(&ch->state_mtx);
+	    if (ch->flags & ATA_IMMEDIATE_MODE)
+	        ch->state = ATA_ACTIVE;
+	    else
+		ch->state = ATA_IDLE;
+	    mtx_unlock(&ch->state_mtx);
+	    ata_finish(request);
+	}
+    }
+}
+
 /*
  * device related interfaces
  */
 static int
-ata_ioctl(struct cdev *dev, u_long cmd, caddr_t addr, int32_t flag, struct thread *td)
+ata_ioctl(struct cdev *dev, u_long cmd, caddr_t addr,
+	  int32_t flag, struct thread *td)
 {
     struct ata_cmd *iocmd = (struct ata_cmd *)addr;
     device_t device = devclass_get_device(ata_devclass, iocmd->channel);
diff --git a/sys/dev/ata/ata-all.h b/sys/dev/ata/ata-all.h
index 7de6d7f..e44ac1a 100644
--- a/sys/dev/ata/ata-all.h
+++ b/sys/dev/ata/ata-all.h
@@ -226,7 +226,7 @@ struct ata_request {
     struct sema			done;		/* request done sema */
     int				retries;	/* retry count */
     int				timeout;	/* timeout for this cmd */
-    struct callout_handle	timeout_handle; /* handle for untimeout */
+    struct callout		callout; 	/* callout management */
     int				result;		/* result error code */
     struct task			task;		/* task management */
     struct bio			*bio;		/* bio for this request */
@@ -239,8 +239,8 @@ struct ata_request {
 #define ATA_DEBUG_RQ(request, string) \
     { \
     if (request->flags & ATA_R_DEBUG) \
-        ata_prtdev(request->device, "req=%08x %s " string "\n", \
-                   (u_int)request, ata_cmd2str(request)); \
+        ata_prtdev(request->device, "req=%p %s " string "\n", \
+                   request, ata_cmd2str(request)); \
     }
 #else
 #define ATA_DEBUG_RQ(request, string)
@@ -318,9 +318,10 @@ struct ata_dma {
 
 /* structure holding lowlevel functions */
 struct ata_lowlevel {
-    void (*reset)(struct ata_channel *ch);
+    int (*begin_transaction)(struct ata_request *request);
+    int (*end_transaction)(struct ata_request *request);
     void (*interrupt)(void *channel);
-    int (*transaction)(struct ata_request *request);
+    void (*reset)(struct ata_channel *ch);
     int (*command)(struct ata_device *atadev, u_int8_t command, u_int64_t lba, u_int16_t count, u_int16_t feature);
 };
 
@@ -358,9 +359,12 @@ struct ata_channel {
 #define		ATA_ATAPI_MASTER	0x04
 #define		ATA_ATAPI_SLAVE		0x08
 
-    int				lock;		/* ATA channel lock */
+    struct mtx			state_mtx;	/* state lock */
+    int				state;		/* ATA channel state */
 #define		ATA_IDLE		0x0000
 #define		ATA_ACTIVE		0x0001
+#define		ATA_INTERRUPT		0x0002
+#define		ATA_TIMEOUT		0x0004
 
     void (*reset)(struct ata_channel *);
     void (*locking)(struct ata_channel *, int);
@@ -433,19 +437,6 @@ extern uma_zone_t ata_zone;
 #define ata_alloc_request() uma_zalloc(ata_zone, M_NOWAIT | M_ZERO)
 #define ata_free_request(request) uma_zfree(ata_zone, request)
 
-/* macros for locking a channel */
-#define ATA_LOCK_CH(ch) \
-	atomic_cmpset_acq_int(&(ch)->lock, ATA_IDLE, ATA_ACTIVE)
-
-#define ATA_SLEEPLOCK_CH(ch) \
-	while (!ATA_LOCK_CH(ch)) tsleep((caddr_t)&(ch), PRIBIO, "atalck", 1);
-
-#define ATA_FORCELOCK_CH(ch) \
-	atomic_store_rel_int(&(ch)->lock, ATA_ACTIVE)
-
-#define ATA_UNLOCK_CH(ch) \
-	atomic_store_rel_int(&(ch)->lock, ATA_IDLE)
-
 /* macros to hide busspace uglyness */
 #define ATA_INB(res, offset) \
 	bus_space_read_1(rman_get_bustag((res)), \
diff --git a/sys/dev/ata/ata-disk.c b/sys/dev/ata/ata-disk.c
index 05f7307..9bbacd2 100644
--- a/sys/dev/ata/ata-disk.c
+++ b/sys/dev/ata/ata-disk.c
@@ -354,7 +354,8 @@ addump(void *arg, void *virtual, vm_offset_t physical,
 	request.flags = ATA_R_CONTROL;
     }
 
-    if (request.device->channel->hw.transaction(&request) == ATA_OP_CONTINUES) {
+    if (request.device->channel->
+	hw.begin_transaction(&request) == ATA_OP_CONTINUES) {
 	while (request.device->channel->running == &request &&
 	       !(request.status & ATA_S_ERROR)) {
 	    DELAY(20);
diff --git a/sys/dev/ata/ata-lowlevel.c b/sys/dev/ata/ata-lowlevel.c
index 3fef4a6..5b38772 100644
--- a/sys/dev/ata/ata-lowlevel.c
+++ b/sys/dev/ata/ata-lowlevel.c
@@ -44,11 +44,10 @@ __FBSDID("$FreeBSD$");
 #include <dev/ata/ata-all.h>
 
 /* prototypes */
-static int ata_generic_transaction(struct ata_request *);
-static void ata_generic_interrupt(void *);
+static int ata_begin_transaction(struct ata_request *);
+static int ata_end_transaction(struct ata_request *);
 static void ata_generic_reset(struct ata_channel *);
 static int ata_wait(struct ata_device *, u_int8_t);
-/*static int ata_command(struct ata_device *, u_int8_t, u_int64_t, u_int16_t, u_int16_t);*/
 static void ata_pio_read(struct ata_request *, int);
 static void ata_pio_write(struct ata_request *, int);
 
@@ -61,15 +60,15 @@ static int atadebug = 0;
 void
 ata_generic_hw(struct ata_channel *ch)
 {
+    ch->hw.begin_transaction = ata_begin_transaction;
+    ch->hw.end_transaction = ata_end_transaction;
     ch->hw.reset = ata_generic_reset;
-    ch->hw.transaction = ata_generic_transaction;
-    ch->hw.interrupt = ata_generic_interrupt;
     ch->hw.command = ata_generic_command;
 }
 
 /* must be called with ATA channel locked */
 static int
-ata_generic_transaction(struct ata_request *request)
+ata_begin_transaction(struct ata_request *request)
 {
     struct ata_channel *ch = request->device->channel;
 
@@ -80,7 +79,7 @@ ata_generic_transaction(struct ata_request *request)
 	return ATA_OP_FINISHED;
     }
 
-    ATA_DEBUG_RQ(request, "transaction");
+    ATA_DEBUG_RQ(request, "begin transaction");
 
     /* disable ATAPI DMA writes if HW doesn't support it */
     if ((ch->flags & ATA_ATAPI_DMA_RO) &&
@@ -113,11 +112,6 @@ ata_generic_transaction(struct ata_request *request)
 		    DELAY(10);
 		    request->status = ATA_IDX_INB(ch, ATA_STATUS);
 		} while (request->status & ATA_S_BUSY && timeout--);
-		if (timeout)
-		    printf("ATAPI_RESET time = %dus\n", (1000000-timeout)*10);
-		else
-		    printf("ATAPI_RESET timeout\n");
-
 		if (request->status & ATA_S_ERROR)
 		    request->error = ATA_IDX_INB(ch, ATA_ERROR);
 		break;
@@ -134,9 +128,6 @@ ata_generic_transaction(struct ata_request *request)
 		ata_pio_write(request, request->transfersize);
 	    }
 	}
-	
-	/* record the request as running and return for interrupt */
-	ch->running = request;
 	return ATA_OP_CONTINUES;
 
     /* ATA DMA data transfer commands */
@@ -165,9 +156,6 @@ ata_generic_transaction(struct ata_request *request)
 	    request->result = EIO;
 	    break;
 	}
-
-	/* record the request as running and return for interrupt */
-	ch->running = request;
 	return ATA_OP_CONTINUES;
 
     /* ATAPI PIO commands */
@@ -191,10 +179,8 @@ ata_generic_transaction(struct ata_request *request)
 	}
 
 	/* command interrupt device ? just return and wait for interrupt */
-	if ((request->device->param->config & ATA_DRQ_MASK) == ATA_DRQ_INTR) {
-	    ch->running = request;
+	if ((request->device->param->config & ATA_DRQ_MASK) == ATA_DRQ_INTR)
 	    return ATA_OP_CONTINUES;
-	}
 
 	/* wait for ready to write ATAPI command block */
 	{
@@ -224,9 +210,6 @@ ata_generic_transaction(struct ata_request *request)
 			   (int16_t *)request->u.atapi.ccb,
 			   (request->device->param->config & ATA_PROTO_MASK) ==
 			   ATA_PROTO_ATAPI_12 ? 6 : 8);
-
-	/* record the request as running and return for interrupt */
-	ch->running = request;
 	return ATA_OP_CONTINUES;
 
     case ATA_R_ATAPI|ATA_R_DMA:
@@ -290,9 +273,6 @@ ata_generic_transaction(struct ata_request *request)
 	    request->result = EIO;
 	    break;
 	}
-
-	/* record the request as running and return for interrupt */
-	ch->running = request;
 	return ATA_OP_CONTINUES;
     }
 
@@ -302,36 +282,17 @@ ata_generic_transaction(struct ata_request *request)
     return ATA_OP_FINISHED;
 }
 
-static void
-ata_generic_interrupt(void *data)
+static int
+ata_end_transaction(struct ata_request *request)
 {
-    struct ata_channel *ch = (struct ata_channel *)data;
-    struct ata_request *request = ch->running;
+    struct ata_channel *ch = request->device->channel;
     int length;
 
-    /* ignore this interrupt if there is no running request */
-    if (!request) 
-	return;
-
-    ATA_DEBUG_RQ(request, "interrupt");
-
-    /* ignore interrupt if device is busy */
-    if (!(request->flags & ATA_R_TIMEOUT) &&
-	ATA_IDX_INB(ch, ATA_ALTSTAT) & ATA_S_BUSY) {
-	DELAY(100);
-	if (!(ATA_IDX_INB(ch, ATA_ALTSTAT) & ATA_S_DRQ))
-	    return;
-    }
-
-    ATA_DEBUG_RQ(request, "interrupt accepted");
+    ATA_DEBUG_RQ(request, "end transaction");
 
     /* clear interrupt and get status */
     request->status = ATA_IDX_INB(ch, ATA_STATUS);
 
-    /* register interrupt */    
-    if (!(request->flags & ATA_R_TIMEOUT))
-	request->flags |= ATA_R_INTR_SEEN;
-
     switch (request->flags & (ATA_R_ATAPI | ATA_R_DMA | ATA_R_CONTROL)) {
 
     /* ATA PIO data transfer and control commands */
@@ -348,7 +309,7 @@ ata_generic_interrupt(void *data)
 	/* if we got an error we are done with the HW */
 	if (request->status & ATA_S_ERROR) {
 	    request->error = ATA_IDX_INB(ch, ATA_ERROR);
-	    break;
+	    return ATA_OP_FINISHED;
 	}
 	
 	/* are we moving data ? */
@@ -381,21 +342,21 @@ ata_generic_interrupt(void *data)
 			ata_prtdev(request->device,
 				   "timeout waiting for write DRQ");
 			request->status = ATA_IDX_INB(ch, ATA_STATUS);
-			break;
+			return ATA_OP_FINISHED;
 		    }
 
 		    /* output data and return waiting for new interrupt */
 		    ata_pio_write(request, request->transfersize);
-		    return;
+		    return ATA_OP_CONTINUES;
 		}
 
 		/* if data read command, return & wait for interrupt */
 		if (request->flags & ATA_R_READ)
-		    return;
+		    return ATA_OP_CONTINUES;
 	    }
 	}
 	/* done with HW */
-	break;
+	return ATA_OP_FINISHED;
 
     /* ATA DMA data transfer commands */
     case ATA_R_DMA:
@@ -416,7 +377,7 @@ ata_generic_interrupt(void *data)
 	ch->dma->unload(ch);
 
 	/* done with HW */
-	break;
+	return ATA_OP_FINISHED;
 
     /* ATAPI PIO commands */
     case ATA_R_ATAPI:
@@ -432,13 +393,13 @@ ata_generic_interrupt(void *data)
 	    if (!(request->status & ATA_S_DRQ)) {
 		ata_prtdev(request->device, "command interrupt without DRQ\n");
 		request->status = ATA_S_ERROR;
-		break;
+		return ATA_OP_FINISHED;
 	    }
 	    ATA_IDX_OUTSW_STRM(ch, ATA_DATA, (int16_t *)request->u.atapi.ccb,
 			       (request->device->param->config &
 				ATA_PROTO_MASK)== ATA_PROTO_ATAPI_12 ? 6 : 8);
 	    /* return wait for interrupt */
-	    return;
+	    return ATA_OP_CONTINUES;
 
 	case ATAPI_P_WRITE:
 	    if (request->flags & ATA_R_READ) {
@@ -446,7 +407,7 @@ ata_generic_interrupt(void *data)
 		ata_prtdev(request->device,
 			   "%s trying to write on read buffer\n",
 			   ata_cmd2str(request));
-		break;
+		return ATA_OP_FINISHED;
 	    }
 	    ata_pio_write(request, length);
 	    request->donecount += length;
@@ -455,7 +416,7 @@ ata_generic_interrupt(void *data)
 	    request->transfersize = min((request->bytecount-request->donecount),
 					request->transfersize);
 	    /* return wait for interrupt */
-	    return;
+	    return ATA_OP_CONTINUES;
 
 	case ATAPI_P_READ:
 	    if (request->flags & ATA_R_WRITE) {
@@ -463,7 +424,7 @@ ata_generic_interrupt(void *data)
 		ata_prtdev(request->device,
 			   "%s trying to read on write buffer\n",
 			   ata_cmd2str(request));
-		break;
+		return ATA_OP_FINISHED;
 	    }
 	    ata_pio_read(request, length);
 	    request->donecount += length;
@@ -472,7 +433,7 @@ ata_generic_interrupt(void *data)
 	    request->transfersize = min((request->bytecount-request->donecount),
 					request->transfersize);
 	    /* return wait for interrupt */
-	    return;
+	    return ATA_OP_CONTINUES;
 
 	case ATAPI_P_DONEDRQ:
 	    ata_prtdev(request->device,
@@ -494,7 +455,7 @@ ata_generic_interrupt(void *data)
 	case ATAPI_P_DONE:
 	    if (request->status & (ATA_S_ERROR | ATA_S_DWF))
 		request->error = ATA_IDX_INB(ch, ATA_ERROR);
-	    break;
+	    return ATA_OP_FINISHED;
 
 	default:
 	    ata_prtdev(request->device, "unknown transfer phase\n");
@@ -502,7 +463,7 @@ ata_generic_interrupt(void *data)
 	}
 
 	/* done with HW */
-	break;
+	return ATA_OP_FINISHED;
 
     /* ATAPI DMA commands */
     case ATA_R_ATAPI|ATA_R_DMA:
@@ -523,12 +484,8 @@ ata_generic_interrupt(void *data)
 	ch->dma->unload(ch);
 
 	/* done with HW */
-	break;
+	return ATA_OP_FINISHED;
     }
-
-    /* finished running this request schedule completition */
-    ch->running = NULL;
-    ata_finish(request);
 }
 
 /* must be called with ATA channel locked */
diff --git a/sys/dev/ata/ata-queue.c b/sys/dev/ata/ata-queue.c
index 8f5d19c..fb77e83 100644
--- a/sys/dev/ata/ata-queue.c
+++ b/sys/dev/ata/ata-queue.c
@@ -52,25 +52,30 @@ static char *ata_skey2str(u_int8_t);
 void
 ata_queue_request(struct ata_request *request)
 {
+    struct ata_channel *ch = request->device->channel;
+
     /* mark request as virgin */
     request->result = request->status = request->error = 0;
+    callout_init(&request->callout, 1);
+
     if (!request->callback && !(request->flags & ATA_R_REQUEUE))
 	sema_init(&request->done, 0, "ATA request done");
 
     /* in IMMEDIATE_MODE we dont queue but call HW directly */
     /* used only during reinit for getparm and config */
-    if ((request->device->channel->flags & ATA_IMMEDIATE_MODE) &&
+    if ((ch->flags & ATA_IMMEDIATE_MODE) &&
 	(request->flags & (ATA_R_CONTROL | ATA_R_IMMEDIATE))) {
 
 	/* arm timeout */
-	if (!request->timeout_handle.callout && !dumping) {
-	    request->timeout_handle =
-		timeout((timeout_t*)ata_timeout, request, request->timeout*hz);
-	}
-
+	if (!dumping)
+	    callout_reset(&request->callout, request->timeout * hz,
+			  (timeout_t*)ata_timeout, request);
+	
 	/* kick HW into action */
-	if (request->device->channel->hw.transaction(request)==ATA_OP_FINISHED){
-	    untimeout((timeout_t *)ata_timeout,request,request->timeout_handle);
+	ch->running = request;
+	if (ch->hw.begin_transaction(request) == ATA_OP_FINISHED) {
+	    ch->running = NULL;
+	    callout_drain(&request->callout);
 	    if (!request->callback) 
 		sema_destroy(&request->done);
 	    return;
@@ -78,18 +83,14 @@ ata_queue_request(struct ata_request *request)
     }
     else  {
 	/* put request on the locked queue at the specified location */
-	mtx_lock(&request->device->channel->queue_mtx);
+	mtx_lock(&ch->queue_mtx);
 	if (request->flags & ATA_R_IMMEDIATE)
-	    TAILQ_INSERT_HEAD(&request->device->channel->ata_queue,
-			      request, chain);
+	    TAILQ_INSERT_HEAD(&ch->ata_queue, request, chain);
 	else
-	    TAILQ_INSERT_TAIL(&request->device->channel->ata_queue,
-			      request, chain);
-	mtx_unlock(&request->device->channel->queue_mtx);
-
+	    TAILQ_INSERT_TAIL(&ch->ata_queue, request, chain);
+	mtx_unlock(&ch->queue_mtx);
 	ATA_DEBUG_RQ(request, "queued");
-
-	ata_start(request->device->channel);
+	ata_start(ch);
     }
 
     /* if this is a requeued request callback/sleep has been setup */
@@ -156,6 +157,7 @@ void
 ata_start(struct ata_channel *ch)
 {
     struct ata_request *request;
+    int gotit = 0;
 
     /* if in immediate mode, just skip start requests (stall queue) */
     if (ch->flags & ATA_IMMEDIATE_MODE)
@@ -172,31 +174,38 @@ ata_start(struct ata_channel *ch)
 	mtx_lock(&ch->queue_mtx);
     }
 
-    /* if we have work todo, try to lock the ATA HW and start transaction */
+    /* if we have work todo, try to grap the ATA HW and start transaction */
     if ((request = TAILQ_FIRST(&ch->ata_queue))) {
 	ch->locking(ch, ATA_LF_LOCK);
-	if (!ATA_LOCK_CH(ch)) {
+	mtx_lock(&ch->state_mtx);
+	if (ch->state == ATA_IDLE) {
+	    ch->state = ATA_ACTIVE;
+	    gotit = 1;
+	}
+	mtx_unlock(&ch->state_mtx);
+	if (!gotit) {
 	    mtx_unlock(&ch->queue_mtx);
 	    return;
 	}
-
 	TAILQ_REMOVE(&ch->ata_queue, request, chain);
 	mtx_unlock(&ch->queue_mtx);
-
 	ATA_DEBUG_RQ(request, "starting");
 
 	/* arm timeout */
-	if (!request->timeout_handle.callout && !dumping) {
-	    request->timeout_handle =
-		timeout((timeout_t*)ata_timeout, request, request->timeout*hz);
-	}
+	if (!dumping)
+	    callout_reset(&request->callout, request->timeout * hz,
+			  (timeout_t*)ata_timeout, request);
 
-	/* kick HW into action and wait for interrupt if it flies*/
-	if (ch->hw.transaction(request) == ATA_OP_CONTINUES)
-	    return;
-
-	/* finish up this (failed) request */
-	ata_finish(request);
+	/* kick HW into action */
+	ch->running = request;
+	if (ch->hw.begin_transaction(request) == ATA_OP_FINISHED) {
+	    ch->running = NULL;
+	    mtx_lock(&ch->state_mtx);
+	    ch->state = ATA_IDLE;
+	    mtx_unlock(&ch->state_mtx);
+            ch->locking(ch, ATA_LF_UNLOCK);
+	    ata_finish(request);
+	}
     }
     else
 	mtx_unlock(&ch->queue_mtx);
@@ -206,80 +215,56 @@ void
 ata_finish(struct ata_request *request)
 {
     struct ata_channel *ch = request->device->channel;
-    ATA_DEBUG_RQ(request, "taskqueue completition");
-
-    /* if we timed out the unlocking of the ATA channel is done later */
-    if (!(request->flags & ATA_R_TIMEOUT)) {
-        ATA_UNLOCK_CH(ch);
-        ch->locking(ch, ATA_LF_UNLOCK);
-    }
 
-    /* request is done schedule it for completition */
+    /* schedule it for completition */
     if (ch->flags & ATA_IMMEDIATE_MODE) {
+	ATA_DEBUG_RQ(request, "finish directly");
 	ata_completed(request, 0);
     }
     else {
-	if (request->bio && !(request->flags & ATA_R_TIMEOUT))
+	if (request->bio && !(request->flags & ATA_R_TIMEOUT)) {
+	    ATA_DEBUG_RQ(request, "finish via bio_taskqueue");
 	    bio_taskqueue(request->bio, (bio_task_t *)ata_completed, request);
+	}
 	else {
 	    TASK_INIT(&request->task, 0, ata_completed, request);
+	    ATA_DEBUG_RQ(request, "finish via taskqueue_thread");
 	    taskqueue_enqueue(taskqueue_thread, &request->task);
 	}
     }
 }
 
-/* current command finished, clean up and return result */
 static void
 ata_completed(void *context, int dummy)
 {
     struct ata_request *request = (struct ata_request *)context;
-    struct ata_channel *channel = request->device->channel;
+    struct ata_channel *ch = request->device->channel;
 
-    ATA_DEBUG_RQ(request, "completed called");
+    ATA_DEBUG_RQ(request, "completed entered");
 
+    /* did everything go according to plan ? */
     if (request->flags & ATA_R_TIMEOUT) {
-	/* workarounds for devices failing to interrupt */
-	if (!request->status) {
-		ata_prtdev(request->device,
-			   "FAILURE - %s no interrupt\n",
-			   ata_cmd2str(request));
-	    request->result = ENXIO;
-	    ATA_UNLOCK_CH(channel);
-	    channel->locking(channel, ATA_LF_UNLOCK);
-	}
-	else if (request->status == (ATA_S_READY | ATA_S_DSC)) {
-		ata_prtdev(request->device,
-			   "WARNING - %s no interrupt but good status\n",
-			   ata_cmd2str(request));
-	    ATA_UNLOCK_CH(channel);
-	    channel->locking(channel, ATA_LF_UNLOCK);
-	}
-	else {
-	    /* reset controller and devices */
-	    ata_reinit(channel);
-
-	    /* if retries still permit, reinject this request */
-	    if (request->retries-- > 0) {
-		request->flags &= ~ATA_R_TIMEOUT;
-		request->flags |= (ATA_R_IMMEDIATE | ATA_R_REQUEUE);
-		ata_queue_request(request);
-		return;
-	    }
 
-	    /* otherwise just finish with error */
-	    else {
-		if (!(request->flags & ATA_R_QUIET))
-		    ata_prtdev(request->device,
-			       "FAILURE - %s timed out\n",
-			       ata_cmd2str(request));
-		if (!request->result)
-		    request->result = EIO;
-	    }
+	/* if reinit succeeds and retries still permit, reinject request */
+	if (!ata_reinit(ch) && request->retries-- > 0) {
+	    request->flags &= ~(ATA_R_TIMEOUT | ATA_R_DEBUG);
+	    request->flags |= (ATA_R_IMMEDIATE | ATA_R_REQUEUE);
+	    ATA_DEBUG_RQ(request, "completed reinjecting");
+	    ata_queue_request(request);
+	    return;
 	}
+
+	/* finish with error */
+	if (!(request->flags & ATA_R_QUIET))
+	    ata_prtdev(request->device,
+		       "FAILURE - %s timed out\n",
+		       ata_cmd2str(request));
+	if (!request->result)
+	    request->result = EIO;
     }
     else {
 	/* untimeout request now we have control back */
-	untimeout((timeout_t *)ata_timeout, request, request->timeout_handle);
+	callout_drain(&request->callout);
 
 	/* do the all the magic for completition evt retry etc etc */
 	if ((request->status & (ATA_S_CORR | ATA_S_ERROR)) == ATA_S_CORR) {
@@ -356,6 +341,7 @@ ata_completed(void *context, int dummy)
 	    request->timeout = 5;
 	    request->flags &= (ATA_R_ATAPI | ATA_R_QUIET);
 	    request->flags |= (ATA_R_READ | ATA_R_IMMEDIATE | ATA_R_REQUEUE);
+	    ATA_DEBUG_RQ(request, "autoissue request sense");
 	    ata_queue_request(request);
 	    return;
 	}
@@ -408,51 +394,45 @@ ata_completed(void *context, int dummy)
 
     ATA_DEBUG_RQ(request, "completed callback/wakeup");
 
+    /* get results back to the initiator */
     if (request->callback)
 	(request->callback)(request);
     else
 	sema_post(&request->done);
 
-    ata_start(channel);
+    ata_start(ch);
 }
 
 static void
 ata_timeout(struct ata_request *request)
 {
-    ATA_DEBUG_RQ(request, "timeout");
+    struct ata_channel *ch = request->device->channel;
+    int gotit = 0;
+
+    /* mark request as no longer running we'll shoot it down shortly */
+    ch->running = NULL;
 
-    /* clear timeout etc */
-    request->timeout_handle.callout = NULL;
+    /* debug on */
+    request->flags |= ATA_R_DEBUG;
+    ATA_DEBUG_RQ(request, "timeout");
 
+    /* if we saw an interrupt before the timeout, shout and re_arm timeout */
     if (request->flags & ATA_R_INTR_SEEN) {
-	if (request->retries-- > 0) {
-            ata_prtdev(request->device,
-		       "WARNING - %s interrupt was seen but timeout fired",
-		       ata_cmd2str(request));
-	    if (!(request->flags & (ATA_R_ATAPI | ATA_R_CONTROL)))
-		printf(" LBA=%llu", (unsigned long long)request->u.ata.lba);
-	    printf("\n");
+        ata_prtdev(request->device,
+		   "WARNING - %s interrupt was seen but timeout fired",
+		   ata_cmd2str(request));
+	if (!(request->flags & (ATA_R_ATAPI | ATA_R_CONTROL)))
+	    printf(" LBA=%llu", (unsigned long long)request->u.ata.lba);
+	printf("\n");
 
-	    /* re-arm timeout */
-	    if (!request->timeout_handle.callout && !dumping) {
-		request->timeout_handle =
-		    timeout((timeout_t*)ata_timeout, request,
-			    request->timeout * hz);
-	    }
-	}
-	else {
-            ata_prtdev(request->device,
-		       "WARNING - %s interrupt was seen but taskqueue stalled",
-		       ata_cmd2str(request));
-	    if (!(request->flags & (ATA_R_ATAPI | ATA_R_CONTROL)))
-		printf(" LBA=%llu", (unsigned long long)request->u.ata.lba);
-	    printf("\n");
-	    ata_completed(request, 0);
-	}
+	/* re-arm timeout */
+	if (!dumping)
+	    callout_reset(&request->callout, request->timeout * hz,
+			  (timeout_t*)ata_timeout, request);
 	return;
     }
 
-    /* report that we timed out */
+    /* report that we timed out if we have any retries left */
     if (!(request->flags & ATA_R_QUIET) && request->retries > 0) {
 	ata_prtdev(request->device,
 		   "TIMEOUT - %s retrying (%d retr%s left)",
@@ -463,9 +443,26 @@ ata_timeout(struct ata_request *request)
 	printf("\n");
     }
 
-    /* now simulate the missing interrupt */
-    request->flags |= ATA_R_TIMEOUT;
-    request->device->channel->hw.interrupt(request->device->channel);
+    /*
+     * if we are waiting for a commend to complete set ATA_TIMEOUT so
+     * we wont loose the race with an eventual interrupt arriving late
+     */
+    mtx_lock(&ch->state_mtx);
+    if (ch->state == ATA_ACTIVE) {
+        ch->state = ATA_TIMEOUT;
+	gotit = 1;
+    }
+    else
+	ata_printf(ch, -1,
+		   "unexpected state in ata_timeout 0x%02x\n", ch->state);
+    mtx_unlock(&ch->state_mtx);
+    
+    /* we got our locks now try to clean up the situation */
+    if (gotit) {
+	request->flags |= ATA_R_TIMEOUT;
+	ch->hw.end_transaction(request);
+	ata_finish(request);
+    }
 }
 
 void
@@ -475,7 +472,7 @@ ata_catch_inflight(struct ata_channel *ch)
 
     ch->running = NULL;
     if (request) {
-        untimeout((timeout_t *)ata_timeout, request, request->timeout_handle);
+        callout_drain(&request->callout);
         ata_prtdev(request->device,
                    "WARNING - %s requeued due to channel reset",
                    ata_cmd2str(request));
@@ -507,8 +504,8 @@ ata_fail_requests(struct ata_channel *ch, struct ata_device *device)
     mtx_unlock(&ch->queue_mtx);
 
     /* if we have a request "in flight" fail it as well */
-    if ((request = ch->running) && (!device || request->device == device)) {
-	untimeout((timeout_t *)ata_timeout, request, request->timeout_handle);
+    if ((request = ch->running) && (!device || request->device == device)){
+	callout_drain(&request->callout);
 	ch->running = NULL;
 	request->result = ENXIO;
 	if (request->callback)