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]

Date:	Thu, 8 Mar 2007 12:53:07 -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 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@...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
>
> 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/