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

[Suleiman Souhlal]

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  
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/