Messages in this thread | | | From | Suleiman Souhlal <> | Subject | Re: [PATCH 3/3] Use correct IDE error recovery | Date | Thu, 8 Mar 2007 12:16:10 -0800 |
| |
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
- 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/
| |