[PATCH 3/3] Use correct IDE error recovery

[PATCH 3/3] Use correct IDE error recovery

[Suleiman Souhlal]

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 *);

Re: [PATCH 3/3] Use correct IDE error recovery

[Bartlomiej Zolnierkiewicz]

Hi,

(sorry for the long delay)

On Wednesday 21 February 2007, Suleiman Souhlal wrote:
> 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.

This change looks fine, indeed we are better of using SRST + SET_FEATURES
than IDLE_IMMEDIATE.

> 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;

Is the removal of ERROR_RECAL handling intentional?
There is nothing about it in the patch description...

> +	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.

Could this part of the comment be merged into the patch description?
We don't want to clutter the code with the history of the changes.

> + *	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

hmm, it doesn't have to be UDMA mode,
->current_speed can also be PIO/SWDMA/MWDMA

> + *	for error recovery and what Western Digital says Windows does.  It
> + *	also does not violate the ATA spec as far as I can tell.
>   */

The patch fixes code in ide_ata_error() and updates the comment
for ide_error() but ide_atapi_error() is not left untouched
(it still uses IDLE IMMEDIATE).

I suppose that ide_atapi_error() (for ATAPI devices) needs similar fix?

>  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);

Please update the patch to not depend on ide_config_drive_speed() fixes
[PATCH 2/3] which need more work (shouldn't be a problem as the code here
uses _irq variant anyway).

Please respin the patch so I could merge it.

Thanks,
Bart

Re: [PATCH 3/3] Use correct IDE error recovery

[Alan Cox]

> On Wednesday 21 February 2007, Suleiman Souhlal wrote:
> > 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.
> 
> This change looks fine, indeed we are better of using SRST + SET_FEATURES
> than IDLE_IMMEDIATE.
> 
> > Signed-off-by:	Suleiman Souhlal <suleiman@google.com>

Acked-by: Alan Cox <alan@redhat.com>

And this is well worth doing - IDLEIMMEDIATE blows the mind of some later
drive firmware that doesn't expect to be treated in an IDE v1 manner.


Alan

Re: [PATCH 3/3] Use correct IDE error recovery

[Suleiman Souhlal]

On Mar 7, 2007, at 1:16 PM, Bartlomiej Zolnierkiewicz wrote:

>
> Hi,
>
> (sorry for the long delay)
>
> On Wednesday 21 February 2007, Suleiman Souhlal wrote:
>> 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.
>
> This change looks fine, indeed we are better of using SRST +  
> SET_FEATURES
> than IDLE_IMMEDIATE.
>
>> 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;
>
> Is the removal of ERROR_RECAL handling intentional?
> There is nothing about it in the patch description...

Yes, it was intentional, but I forgot to add "while there remove some  
useless code" to the description. Maybe it's better if I send this as  
a separate patch.

>
>> +	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.
>
> Could this part of the comment be merged into the patch description?
> We don't want to clutter the code with the history of the changes.
>
>> + *	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
>
> hmm, it doesn't have to be UDMA mode,
> ->current_speed can also be PIO/SWDMA/MWDMA
>
>> + *	for error recovery and what Western Digital says Windows  
>> does.  It
>> + *	also does not violate the ATA spec as far as I can tell.
>>   */
>
> The patch fixes code in ide_ata_error() and updates the comment
> for ide_error() but ide_atapi_error() is not left untouched
> (it still uses IDLE IMMEDIATE).
>
> I suppose that ide_atapi_error() (for ATAPI devices) needs similar  
> fix?

I'm not sure.. I don't have any ATAPI hardware to test this on  
either, so I preferred not to touch it.

>>  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);
>
> Please update the patch to not depend on ide_config_drive_speed()  
> fixes
> [PATCH 2/3] which need more work (shouldn't be a problem as the  
> code here
> uses _irq variant anyway).
>
> Please respin the patch so I could merge it.

Ok.

-- Suleiman

Re: [PATCH 3/3] Use correct IDE error recovery

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday 08 March 2007, Suleiman Souhlal wrote:
> 
> On Mar 7, 2007, at 1:16 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> >
> > Hi,
> >
> > (sorry for the long delay)
> >
> > On Wednesday 21 February 2007, Suleiman Souhlal wrote:
> >> 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.
> >
> > This change looks fine, indeed we are better of using SRST +  
> > SET_FEATURES than IDLE_IMMEDIATE.
> >
> >> 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;
> >
> > Is the removal of ERROR_RECAL handling intentional?
> > There is nothing about it in the patch description...
> 
> Yes, it was intentional, but I forgot to add "while there remove some  

Why is it useless?  What am I missing?

> useless code" to the description. Maybe it's better if I send this as  
> a separate patch.

Yes, please do so.

[ ... ]

> > The patch fixes code in ide_ata_error() and updates the comment
> > for ide_error() but ide_atapi_error() is not left untouched
> > (it still uses IDLE IMMEDIATE).
> >
> > I suppose that ide_atapi_error() (for ATAPI devices) needs similar  
> > fix?
> 
> I'm not sure.. I don't have any ATAPI hardware to test this on  
> either, so I preferred not to touch it.

OK, this could be fixed later in the incremental patch.

> >>  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);
> >
> > Please update the patch to not depend on ide_config_drive_speed() fixes
> > [PATCH 2/3] which need more work (shouldn't be a problem as the code here
> > uses _irq variant anyway).
> >
> > Please respin the patch so I could merge it.
> 
> Ok.

Thanks,
Bart

Re: [PATCH 3/3] Use correct IDE error recovery

[Suleiman Souhlal]

On Mar 8, 2007, at 12:34 PM, Bartlomiej Zolnierkiewicz wrote:

>
> Hi,
>
> On Thursday 08 March 2007, Suleiman Souhlal wrote:
>>
>> On Mar 7, 2007, at 1:16 PM, Bartlomiej Zolnierkiewicz wrote:
>>
>>>
>>> Hi,
>>>
>>> (sorry for the long delay)
>>>
>>> On Wednesday 21 February 2007, Suleiman Souhlal wrote:
>>>> 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.
>>>
>>> This change looks fine, indeed we are better of using SRST +
>>> SET_FEATURES than IDLE_IMMEDIATE.
>>>
>>>> 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;
>>>
>>> Is the removal of ERROR_RECAL handling intentional?
>>> There is nothing about it in the patch description...
>>
>> Yes, it was intentional, but I forgot to add "while there remove some
>
> Why is it useless?  What am I missing?

I thought the recalibration code didn't do anything, but upon  
rereading the code I'm not so sure anymore..

-- Suleiman

Re: [PATCH 3/3] Use correct IDE error recovery

[Bartlomiej Zolnierkiewicz]

Hi,

On Thursday 08 March 2007, Suleiman Souhlal wrote:
> 
> On Mar 7, 2007, at 1:16 PM, Bartlomiej Zolnierkiewicz wrote:

> > Please respin the patch so I could merge it.
> 
> Ok.

Since I think that it's worth to have it in 2.6.21-final and respin didn't
happen I did the required changes myself (it also turned out that I missed
few things during initial review), then applied the patch...

Please let my know whether you are fine with my changes, thanks.

[PATCH] ide: use correct IDE error recovery

From: Suleiman Souhlal <suleiman@google.com>

IDE error recovery is using IDLE IMMEDIATE if the drive is busy or has DRQ set.
This violates the ATA spec (can only send IDLE IMMEDIATE when drive is not
busy) and really hoses up some drives (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 command.  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.

Bart:
* port the patch over the current tree
* undo the recalibration code removal
* send SET FEATURES command after checking for good drive status
* don't check whether the current request is of REQ_TYPE_ATA_{CMD,TASK}
  type because we need to send SET FEATURES before handling any requests
* some pre-ATA4 drives require INITIALIZE DEVICE PARAMETERS command before
  other commands (except IDENTIFY) so send SET FEATURES only if there are
  no pending drive->special requests
* update comments and patch description
* any bugs introduced by this patch are mine and not Suleiman's :-)

Signed-off-by: Suleiman Souhlal <suleiman@google.com>
Acked-by: Alan Cox <alan@redhat.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---

 drivers/ide/ide-io.c   |   32 +++++++++++++++++++++-----------
 drivers/ide/ide-iops.c |    3 +++
 include/linux/ide.h    |    1 +
 3 files changed, 25 insertions(+), 11 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -519,21 +519,24 @@ 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);
 	}
+
+	if ((rq->errors & ERROR_RECAL) == ERROR_RECAL)
+		drive->special.b.recalibrate = 1;
+
+	++rq->errors;
+
 	return ide_stopped;
 }
 
@@ -1025,6 +1028,13 @@ static ide_startstop_t start_request (id
 	if (!drive->special.all) {
 		ide_driver_t *drv;
 
+		/*
+		 * We reset the drive so we need to issue a SETFEATURES.
+		 * Do it _after_ do_special() restored device parameters.
+		 */
+		if (drive->current_speed == 0xff)
+			ide_config_drive_speed(drive, drive->desired_speed);
+
 		if (rq->cmd_type == REQ_TYPE_ATA_CMD ||
 		    rq->cmd_type == REQ_TYPE_ATA_TASK ||
 		    rq->cmd_type == REQ_TYPE_ATA_TASKFILE)
Index: b/drivers/ide/ide-iops.c
===================================================================
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -1094,6 +1094,9 @@ static void pre_reset(ide_drive_t *drive
 	if (HWIF(drive)->pre_reset != NULL)
 		HWIF(drive)->pre_reset(drive);
 
+	if (drive->current_speed != 0xff)
+		drive->desired_speed = drive->current_speed;
+	drive->current_speed = 0xff;
 }
 
 /*
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -615,6 +615,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 */

Re: [PATCH 3/3] Use correct IDE error recovery

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

> Since I think that it's worth to have it in 2.6.21-final and respin didn't
> happen I did the required changes myself (it also turned out that I missed
> few things during initial review), then applied the patch...

> Please let my know whether you are fine with my changes, thanks.

> [PATCH] ide: use correct IDE error recovery

> From: Suleiman Souhlal <suleiman@google.com>

> IDE error recovery is using IDLE IMMEDIATE if the drive is busy or has DRQ set.
> This violates the ATA spec (can only send IDLE IMMEDIATE when drive is not
> busy) and really hoses up some drives (modern drives will not be able to

    I wonder if that really ever worked...

> recover using this error handling).  The correct thing to do is issue a SRST
> followed by a SET FEATURES command.  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.

    The patch even does things *not* required by the spec. :-)

> Index: b/drivers/ide/ide-io.c
> ===================================================================
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -519,21 +519,24 @@ 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);
>  	}
> +
> +	if ((rq->errors & ERROR_RECAL) == ERROR_RECAL)

    Hm, is there any need for ==?

> +		drive->special.b.recalibrate = 1;
> +
> +	++rq->errors;
> +
>  	return ide_stopped;
>  }
>  
> @@ -1025,6 +1028,13 @@ static ide_startstop_t start_request (id
>  	if (!drive->special.all) {
>  		ide_driver_t *drv;
>  
> +		/*
> +		 * We reset the drive so we need to issue a SETFEATURES.
> +		 * Do it _after_ do_special() restored device parameters.
> +		 */
> +		if (drive->current_speed == 0xff)
> +			ide_config_drive_speed(drive, drive->desired_speed);
> +

    Hmm, IIRC, drive's UltraDMA mode shall *not* be cleared by reset, 
therefore there's no need to restore it.

>  		if (rq->cmd_type == REQ_TYPE_ATA_CMD ||
>  		    rq->cmd_type == REQ_TYPE_ATA_TASK ||
>  		    rq->cmd_type == REQ_TYPE_ATA_TASKFILE)
> Index: b/drivers/ide/ide-iops.c
> ===================================================================
> --- a/drivers/ide/ide-iops.c
> +++ b/drivers/ide/ide-iops.c
> @@ -1094,6 +1094,9 @@ static void pre_reset(ide_drive_t *drive
>  	if (HWIF(drive)->pre_reset != NULL)
>  		HWIF(drive)->pre_reset(drive);
>  
> +	if (drive->current_speed != 0xff)
> +		drive->desired_speed = drive->current_speed;
> +	drive->current_speed = 0xff;
>  }
>  /*
> Index: b/include/linux/ide.h
> ===================================================================
> --- a/include/linux/ide.h
> +++ b/include/linux/ide.h
> @@ -615,6 +615,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 */

    Oh, more of that *_speed crap. :-)

>          u8	dn;		/* now wide spread use */
>          u8	wcache;		/* status of write cache */
>  	u8	acoustic;	/* acoustic management */

MBR, Sergei