[PATCH 3/3] Use correct IDE error recovery

[Suleiman Souhlal <ssouhlal@FreeBSD.org>]

IDE error recovery is using WIN_IDLEIMMEDIATE which was only valid for
IDE V1 and IDE V2.  Modern drives will not be able to recover using
this error handling.  The correct thing to do is issue a SRST followed
by a SET_FEATURES.

Signed-off-by:	Suleiman Souhlal <suleiman@google.com>

---
 drivers/ide/ide-io.c   |   35 +++++++++++-----
 drivers/ide/ide-iops.c |  105 ++++++++++++++++++++++++++++--------------------
 include/linux/ide.h    |    2 +
 3 files changed, 88 insertions(+), 54 deletions(-)

diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index c193553..2f05b4d 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -519,21 +519,21 @@ static ide_startstop_t ide_ata_error(ide
 	if ((stat & DRQ_STAT) && rq_data_dir(rq) == READ && hwif->err_stops_fifo == 0)
 		try_to_flush_leftover_data(drive);
 
+	if (rq->errors >= ERROR_MAX || blk_noretry_request(rq)) {
+		ide_kill_rq(drive, rq);
+		return ide_stopped;
+	}
+
 	if (hwif->INB(IDE_STATUS_REG) & (BUSY_STAT|DRQ_STAT))
-		/* force an abort */
-		hwif->OUTB(WIN_IDLEIMMEDIATE, IDE_COMMAND_REG);
+		rq->errors |= ERROR_RESET;
 
-	if (rq->errors >= ERROR_MAX || blk_noretry_request(rq))
-		ide_kill_rq(drive, rq);
-	else {
-		if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
-			++rq->errors;
-			return ide_do_reset(drive);
-		}
-		if ((rq->errors & ERROR_RECAL) == ERROR_RECAL)
-			drive->special.b.recalibrate = 1;
+	if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
 		++rq->errors;
+		return ide_do_reset(drive);
 	}
+
+	++rq->errors;
+
 	return ide_stopped;
 }
 
@@ -586,6 +586,13 @@ EXPORT_SYMBOL_GPL(__ide_error);
  *	both new-style (taskfile) and old style command handling here.
  *	In the case of taskfile command handling there is work left to
  *	do
+ *	This used to send a idle immediate to the drive if the drive was
+ *	busy or had drq set.  This violates the ATA spec (can only send IDLE
+ *	immediate when drive is not busy) and really hoses up some drives.
+ *	We've changed it to just do a SRST followed by a set features (set
+ *	udma mode) it those cases.  This is what Western Digital recommends
+ *	for error recovery and what Western Digital says Windows does.  It
+ *	also does not violate the ATA spec as far as I can tell.
  */
  
 ide_startstop_t ide_error (ide_drive_t *drive, const char *msg, u8 stat)
@@ -1004,6 +1011,12 @@ #endif
 		goto kill_rq;
 	}
 
+	/* We reset the drive so we need to issue a SETFEATURES. */
+	if ((drive->current_speed == 0xff) &&
+	    ((rq->cmd_type == REQ_TYPE_ATA_CMD) ||
+	    (rq->cmd_type == REQ_TYPE_ATA_TASK)))
+		ide_config_drive_speed_irq(drive, drive->desired_speed);
+
 	block    = rq->sector;
 	if (blk_fs_request(rq) &&
 	    (drive->media == ide_disk || drive->media == ide_floppy)) {
diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
index 35ab3af..e0573cb 100644
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -731,6 +731,30 @@ #else
 #endif
 }
 
+static void ide_drive_speed_changed(ide_drive_t *drive, u8 speed)
+{
+	switch(speed) {
+		case XFER_UDMA_7:   drive->id->dma_ultra |= 0x8080; break;
+		case XFER_UDMA_6:   drive->id->dma_ultra |= 0x4040; break;
+		case XFER_UDMA_5:   drive->id->dma_ultra |= 0x2020; break;
+		case XFER_UDMA_4:   drive->id->dma_ultra |= 0x1010; break;
+		case XFER_UDMA_3:   drive->id->dma_ultra |= 0x0808; break;
+		case XFER_UDMA_2:   drive->id->dma_ultra |= 0x0404; break;
+		case XFER_UDMA_1:   drive->id->dma_ultra |= 0x0202; break;
+		case XFER_UDMA_0:   drive->id->dma_ultra |= 0x0101; break;
+		case XFER_MW_DMA_2: drive->id->dma_mword |= 0x0404; break;
+		case XFER_MW_DMA_1: drive->id->dma_mword |= 0x0202; break;
+		case XFER_MW_DMA_0: drive->id->dma_mword |= 0x0101; break;
+		case XFER_SW_DMA_2: drive->id->dma_1word |= 0x0404; break;
+		case XFER_SW_DMA_1: drive->id->dma_1word |= 0x0202; break;
+		case XFER_SW_DMA_0: drive->id->dma_1word |= 0x0101; break;
+		default: break;
+	}
+	if (!drive->init_speed)
+		drive->init_speed = speed;
+	drive->current_speed = speed;
+}
+
 /*
  * Similar to ide_wait_stat(), except it never calls ide_error internally.
  * This is a kludge to handle the new ide_config_drive_speed() function,
@@ -742,32 +766,12 @@ #endif
  *
  * const char *msg == consider adding for verbose errors.
  */
-int ide_config_drive_speed (ide_drive_t *drive, u8 speed)
+int ide_config_drive_speed_irq(ide_drive_t *drive, u8 speed)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
 	int	i, error	= 1;
 	u8 stat;
 
-	/*
-	 * Just use ide_wait_cmd() if the drive has been initialized and we
-	 * aren't in an interrupt handler, to avoid changing the xfer speed
-	 * while requests are in flight.
-	 *
-	 * If we are in an interrupt, it should be safe to issue
-	 * SETFEATURES manually, since there shouldn't be any requests in
-	 * flight.
-	 */
-	if (drive->queue != NULL && !in_interrupt()) {
-		error = ide_wait_cmd(drive, WIN_SETFEATURES, speed,
-		    SETFEATURES_XFER, 0, NULL);
-		if (error) {
-			stat = hwif->INB(IDE_STATUS_REG);
-			ide_dump_status(drive, "set_drive_speed_status", stat);
-			return (error);
-		}
-		goto done;
-	}
-
 #ifdef CONFIG_BLK_DEV_IDEDMA
 	if (hwif->ide_dma_check)	 /* check if host supports DMA */
 		hwif->dma_host_off(drive);
@@ -839,28 +843,39 @@ #ifdef CONFIG_BLK_DEV_IDEDMA
 		hwif->dma_off_quietly(drive);
 #endif
 
-done:
-	switch(speed) {
-		case XFER_UDMA_7:   drive->id->dma_ultra |= 0x8080; break;
-		case XFER_UDMA_6:   drive->id->dma_ultra |= 0x4040; break;
-		case XFER_UDMA_5:   drive->id->dma_ultra |= 0x2020; break;
-		case XFER_UDMA_4:   drive->id->dma_ultra |= 0x1010; break;
-		case XFER_UDMA_3:   drive->id->dma_ultra |= 0x0808; break;
-		case XFER_UDMA_2:   drive->id->dma_ultra |= 0x0404; break;
-		case XFER_UDMA_1:   drive->id->dma_ultra |= 0x0202; break;
-		case XFER_UDMA_0:   drive->id->dma_ultra |= 0x0101; break;
-		case XFER_MW_DMA_2: drive->id->dma_mword |= 0x0404; break;
-		case XFER_MW_DMA_1: drive->id->dma_mword |= 0x0202; break;
-		case XFER_MW_DMA_0: drive->id->dma_mword |= 0x0101; break;
-		case XFER_SW_DMA_2: drive->id->dma_1word |= 0x0404; break;
-		case XFER_SW_DMA_1: drive->id->dma_1word |= 0x0202; break;
-		case XFER_SW_DMA_0: drive->id->dma_1word |= 0x0101; break;
-		default: break;
-	}
-	if (!drive->init_speed)
-		drive->init_speed = speed;
-	drive->current_speed = speed;
-	return error;
+		ide_drive_speed_changed(drive, speed);
+
+		return (error);
+}
+
+int ide_config_drive_speed (ide_drive_t *drive, u8 speed)
+{
+	ide_hwif_t *hwif	= HWIF(drive);
+	int error;
+	u8 stat;
+
+	/*
+	 * Just use ide_wait_cmd() if the drive has been initialized and we
+	 * aren't in an interrupt handler, to avoid changing the xfer speed
+	 * while requests are in flight.
+	 *
+	 * If we are in an interrupt, it should be safe to issue
+	 * SETFEATURES manually, since there shouldn't be any requests in
+	 * flight.
+	 */
+	if (drive->queue != NULL && !in_interrupt()) {
+		error = ide_wait_cmd(drive, WIN_SETFEATURES, speed,
+		    SETFEATURES_XFER, 0, NULL);
+		if (error) {
+			stat = hwif->INB(IDE_STATUS_REG);
+			ide_dump_status(drive, "set_drive_speed_status", stat);
+			return (error);
+		}
+		ide_drive_speed_changed(drive, speed);
+	} else
+		error = ide_config_drive_speed_irq(drive, speed);
+
+	return (error);
 }
 
 EXPORT_SYMBOL(ide_config_drive_speed);
@@ -1093,6 +1108,10 @@ static void pre_reset(ide_drive_t *drive
 	if (HWIF(drive)->pre_reset != NULL)
 		HWIF(drive)->pre_reset(drive);
 
+	/* Make sure we issue a SETFEATURES before the next request. */
+	if (drive->current_speed != 0xff)
+		drive->desired_speed = drive->current_speed;
+	drive->current_speed = 0xff;
 }
 
 /*
diff --git a/include/linux/ide.h b/include/linux/ide.h
index 3861753..c7f6027 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -616,6 +616,7 @@ typedef struct ide_drive_s {
         u8	init_speed;	/* transfer rate set at boot */
         u8	pio_speed;      /* unused by core, used by some drivers for fallback from DMA */
         u8	current_speed;	/* current transfer rate set */
+        u8	desired_speed;	/* desired transfer rate set */
         u8	dn;		/* now wide spread use */
         u8	wcache;		/* status of write cache */
 	u8	acoustic;	/* acoustic management */
@@ -1184,6 +1185,7 @@ extern int system_bus_clock(void);
 extern int ide_driveid_update(ide_drive_t *);
 extern int ide_ata66_check(ide_drive_t *, ide_task_t *);
 extern int ide_config_drive_speed(ide_drive_t *, u8);
+extern int ide_config_drive_speed_irq(ide_drive_t *, u8);
 extern u8 eighty_ninty_three (ide_drive_t *);
 extern int set_transfer(ide_drive_t *, ide_task_t *);
 extern int taskfile_lib_get_identify(ide_drive_t *drive, u8 *);

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/