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@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?

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

-- Suleiman


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/