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@xxxxxxxxxx>
> >>
> >> ---
> >> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
