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] [day] [month] [year] [list]

Date:	Sat, 24 Mar 2007 17:07:13 +0300
From:	Sergei Shtylyov <sshtylyov@...mvista.com>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	Suleiman Souhlal <ssouhlal@...ebsd.org>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org, Alan Cox <alan@...hat.com>
Subject: Re: [PATCH 3/3] Use correct IDE error recovery

Hello.

Bartlomiej Zolnierkiewicz wrote:

> Since I think that it's worth to have it in 2.6.21-final and respin didn't
> happen I did the required changes myself (it also turned out that I missed
> few things during initial review), then applied the patch...

> Please let my know whether you are fine with my changes, thanks.

> [PATCH] ide: use correct IDE error recovery

> From: Suleiman Souhlal <suleiman@...gle.com>

> IDE error recovery is using IDLE IMMEDIATE if the drive is busy or has DRQ set.
> This violates the ATA spec (can only send IDLE IMMEDIATE when drive is not
> busy) and really hoses up some drives (modern drives will not be able to

    I wonder if that really ever worked...

> recover using this error handling).  The correct thing to do is issue a SRST
> followed by a SET FEATURES command.  This is what Western Digital recommends
> 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 even does things *not* required by the spec. :-)

> Index: b/drivers/ide/ide-io.c
> ===================================================================
> --- a/drivers/ide/ide-io.c
> +++ b/drivers/ide/ide-io.c
> @@ -519,21 +519,24 @@ 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;
> +	if ((rq->errors & ERROR_RESET) == ERROR_RESET) {
>  		++rq->errors;
> +		return ide_do_reset(drive);
>  	}
> +
> +	if ((rq->errors & ERROR_RECAL) == ERROR_RECAL)

    Hm, is there any need for ==?

> +		drive->special.b.recalibrate = 1;
> +
> +	++rq->errors;
> +
>  	return ide_stopped;
>  }
>  
> @@ -1025,6 +1028,13 @@ static ide_startstop_t start_request (id
>  	if (!drive->special.all) {
>  		ide_driver_t *drv;
>  
> +		/*
> +		 * We reset the drive so we need to issue a SETFEATURES.
> +		 * Do it _after_ do_special() restored device parameters.
> +		 */
> +		if (drive->current_speed == 0xff)
> +			ide_config_drive_speed(drive, drive->desired_speed);
> +

    Hmm, IIRC, drive's UltraDMA mode shall *not* be cleared by reset, 
therefore there's no need to restore it.

>  		if (rq->cmd_type == REQ_TYPE_ATA_CMD ||
>  		    rq->cmd_type == REQ_TYPE_ATA_TASK ||
>  		    rq->cmd_type == REQ_TYPE_ATA_TASKFILE)
> Index: b/drivers/ide/ide-iops.c
> ===================================================================
> --- a/drivers/ide/ide-iops.c
> +++ b/drivers/ide/ide-iops.c
> @@ -1094,6 +1094,9 @@ static void pre_reset(ide_drive_t *drive
>  	if (HWIF(drive)->pre_reset != NULL)
>  		HWIF(drive)->pre_reset(drive);
>  
> +	if (drive->current_speed != 0xff)
> +		drive->desired_speed = drive->current_speed;
> +	drive->current_speed = 0xff;
>  }
>  /*
> Index: b/include/linux/ide.h
> ===================================================================
> --- a/include/linux/ide.h
> +++ b/include/linux/ide.h
> @@ -615,6 +615,7 @@ typedef struct ide_drive_s {
>          u8	init_speed;	/* transfer rate set at boot */
>          u8	pio_speed;      /* unused by core, used by some drivers for fallback from DMA */
>          u8	current_speed;	/* current transfer rate set */
> +	u8	desired_speed;	/* desired transfer rate set */

    Oh, more of that *_speed crap. :-)

>          u8	dn;		/* now wide spread use */
>          u8	wcache;		/* status of write cache */
>  	u8	acoustic;	/* acoustic management */

MBR, Sergei
-
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/