Re: [PATCH 3/3] Use correct IDE error recovery

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

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

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