lkml.org 
[lkml]   [2007]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/3] Use correct IDE error recovery
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
-
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/

\
 
 \ /
  Last update: 2007-03-24 15:09    [W:0.077 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site