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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <B2D4536E-D449-4D10-B745-30D25CAC40A4@freebsd.org>
Date:	Thu, 8 Mar 2007 12:16:10 -0800
From:	Suleiman Souhlal <ssouhlal@...ebsd.org>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] Use correct IDE error recovery


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