[PATCH 2/3] Don't change transfer speed while requests are in flight

[PATCH 2/3] Don't change transfer speed while requests are in flight

[Suleiman Souhlal]

Use ide_wait_cmd() in ide_config_drive_speed() if the drive has been
initialized and we're not in an interrupt, to avoid changing the
xfer speed while requests are in flight.

An easy way to trigger the problem is to dd the disk while doing
while :; do hdparm -d 1 /dev/hdc> /dev/null; done

While there, remove some commented-out code.

Signed-off-by:	Suleiman Souhlal <suleiman@google.com>

---
 drivers/ide/ide-iops.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
index c67b3b1..35ab3af 100644
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -748,32 +748,36 @@ int ide_config_drive_speed (ide_drive_t 
 	int	i, error	= 1;
 	u8 stat;
 
-//	while (HWGROUP(drive)->busy)
-//		msleep(50);
+	/*
+	 * Just use ide_wait_cmd() if the drive has been initialized and we
+	 * aren't in an interrupt handler, to avoid changing the xfer speed
+	 * while requests are in flight.
+	 *
+	 * If we are in an interrupt, it should be safe to issue
+	 * SETFEATURES manually, since there shouldn't be any requests in
+	 * flight.
+	 */
+	if (drive->queue != NULL && !in_interrupt()) {
+		error = ide_wait_cmd(drive, WIN_SETFEATURES, speed,
+		    SETFEATURES_XFER, 0, NULL);
+		if (error) {
+			stat = hwif->INB(IDE_STATUS_REG);
+			ide_dump_status(drive, "set_drive_speed_status", stat);
+			return (error);
+		}
+		goto done;
+	}
 
 #ifdef CONFIG_BLK_DEV_IDEDMA
 	if (hwif->ide_dma_check)	 /* check if host supports DMA */
 		hwif->dma_host_off(drive);
 #endif
 
-	/*
-	 * Don't use ide_wait_cmd here - it will
-	 * attempt to set_geometry and recalibrate,
-	 * but for some reason these don't work at
-	 * this point (lost interrupt).
-	 */
         /*
          * Select the drive, and issue the SETFEATURES command
          */
 	disable_irq_nosync(hwif->irq);
 	
-	/*
-	 *	FIXME: we race against the running IRQ here if
-	 *	this is called from non IRQ context. If we use
-	 *	disable_irq() we hang on the error path. Work
-	 *	is needed.
-	 */
-	 
 	udelay(1);
 	SELECT_DRIVE(drive);
 	SELECT_MASK(drive, 0);
@@ -835,6 +839,7 @@ #ifdef CONFIG_BLK_DEV_IDEDMA
 		hwif->dma_off_quietly(drive);
 #endif
 
+done:
 	switch(speed) {
 		case XFER_UDMA_7:   drive->id->dma_ultra |= 0x8080; break;
 		case XFER_UDMA_6:   drive->id->dma_ultra |= 0x4040; break;

Re: [PATCH 2/3] Don't change transfer speed while requests are in flight

[Bartlomiej Zolnierkiewicz]

On Wednesday 21 February 2007 02:21, Suleiman Souhlal wrote:
> Use ide_wait_cmd() in ide_config_drive_speed() if the drive has been
> initialized and we're not in an interrupt, to avoid changing the
> xfer speed while requests are in flight.

Many devices have problems with SETFEATURES_XFER if the WIN_SETFEATURES
command is driven by IRQ so we must resort to the polling mode.
This also implies that ide_config_drive_speed() generally does its
job right and the problem lies in the higher layers.

set_using_dma() should queue special driver specific request (same goes
for some other options from /proc/ide/hdx/settings / ioctls).

I have a patch re-writting /proc/ide/hdx/settings so that there are
->get/->set methods for each setting.  This should help in designing
special driver request but I need some more time to sync the patch
with the recent IDE changes.  I will get back to you on this one.

Thanks,
Bart

> An easy way to trigger the problem is to dd the disk while doing
> while :; do hdparm -d 1 /dev/hdc> /dev/null; done
> 
> While there, remove some commented-out code.
> 
> Signed-off-by:	Suleiman Souhlal <suleiman@google.com>
> 
> ---
>  drivers/ide/ide-iops.c |   35 ++++++++++++++++++++---------------
>  1 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
> index c67b3b1..35ab3af 100644
> --- a/drivers/ide/ide-iops.c
> +++ b/drivers/ide/ide-iops.c
> @@ -748,32 +748,36 @@ int ide_config_drive_speed (ide_drive_t 
>  	int	i, error	= 1;
>  	u8 stat;
>  
> -//	while (HWGROUP(drive)->busy)
> -//		msleep(50);
> +	/*
> +	 * Just use ide_wait_cmd() if the drive has been initialized and we
> +	 * aren't in an interrupt handler, to avoid changing the xfer speed
> +	 * while requests are in flight.
> +	 *
> +	 * If we are in an interrupt, it should be safe to issue
> +	 * SETFEATURES manually, since there shouldn't be any requests in
> +	 * flight.
> +	 */
> +	if (drive->queue != NULL && !in_interrupt()) {
> +		error = ide_wait_cmd(drive, WIN_SETFEATURES, speed,
> +		    SETFEATURES_XFER, 0, NULL);
> +		if (error) {
> +			stat = hwif->INB(IDE_STATUS_REG);
> +			ide_dump_status(drive, "set_drive_speed_status", stat);
> +			return (error);
> +		}
> +		goto done;
> +	}
>  
>  #ifdef CONFIG_BLK_DEV_IDEDMA
>  	if (hwif->ide_dma_check)	 /* check if host supports DMA */
>  		hwif->dma_host_off(drive);
>  #endif
>  
> -	/*
> -	 * Don't use ide_wait_cmd here - it will
> -	 * attempt to set_geometry and recalibrate,
> -	 * but for some reason these don't work at
> -	 * this point (lost interrupt).
> -	 */
>          /*
>           * Select the drive, and issue the SETFEATURES command
>           */
>  	disable_irq_nosync(hwif->irq);
>  	
> -	/*
> -	 *	FIXME: we race against the running IRQ here if
> -	 *	this is called from non IRQ context. If we use
> -	 *	disable_irq() we hang on the error path. Work
> -	 *	is needed.
> -	 */
> -	 
>  	udelay(1);
>  	SELECT_DRIVE(drive);
>  	SELECT_MASK(drive, 0);
> @@ -835,6 +839,7 @@ #ifdef CONFIG_BLK_DEV_IDEDMA
>  		hwif->dma_off_quietly(drive);
>  #endif
>  
> +done:
>  	switch(speed) {
>  		case XFER_UDMA_7:   drive->id->dma_ultra |= 0x8080; break;
>  		case XFER_UDMA_6:   drive->id->dma_ultra |= 0x4040; break;
> 
> 

Re: [PATCH 2/3] Don't change transfer speed while requests are in flight

[Alan]

> +	 * If we are in an interrupt, it should be safe to issue
> +	 * SETFEATURES manually, since there shouldn't be any requests in
> +	 * flight.

There may be error recovery going on from a timeout on another processor.
I don't see how your code protects against that (and the old code is
broken too)