Messages in this thread | | | Date | Sat, 24 Mar 2007 17:07:13 +0300 | From | Sergei Shtylyov <> | Subject | Re: [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/
| |