2.6.4-rc-bk3: hdparm -X locks up IDE

2.6.4-rc-bk3: hdparm -X locks up IDE

[Denis Vlasenko]

I discovered that hdparm -X <mode> /dev/hda can lock up IDE
interface if there is some activity.

I walked up from via driver's handler for it:

via_set_speed(): directly programs IDE controller timings.

via_set_drive -> via_set_speed

via82cxxx_tune_drive -> via_set_drive
via82cxxx_ide_dma_check -> via_set_drive

init_hwif_via82cxxx:
        hwif->tuneproc = &via82cxxx_tune_drive;
        hwif->speedproc = &via_set_drive;
	...
	hwif->ide_dma_check = &via82cxxx_ide_dma_check;

I think via_set_drive is eventually gets called
by hdparm -X, not other two functions.

ide_set_xfer_rate -> HWIF(drive)->speedproc

pre_reset -> check_dma_crc -> ide_set_xfer_rate

set_xfer_rate -> ide_set_xfer_rate

void ide_add_generic_settings(drive):
...
ide_add_setting(drive, "current_speed", SETTING_RW,
	-1, -1, TYPE_BYTE, 0, 70, 1, 1, &drive->current_speed,
	set_xfer_rate);
        ^^^^^^^^^^^^^

Seems like there is no locking protecting mode change.
--
vda

Re: 2.6.4-rc-bk3: hdparm -X locks up IDE

[Jens Axboe]

On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> > I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> > interface if there is some activity.
> 
> Known bug and is on TODO but fixing it ain't easy.
> Thanks for a report anyway.

Wouldn't it be possible to do the stuff that needs serializing from the
end_request() part and get automatic synchronization with normal
requests?

-- 
Jens Axboe

Re: 2.6.4-rc-bk3: hdparm -X locks up IDE

[Bartlomiej Zolnierkiewicz]

On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> interface if there is some activity.

Known bug and is on TODO but fixing it ain't easy.
Thanks for a report anyway.

Regards,
Bartlomiej

Re: 2.6.4-rc-bk3: hdparm -X locks up IDE

[Jens Axboe]

On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> On Thursday 11 of March 2004 15:48, Jens Axboe wrote:
> > On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> > > On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> > > > I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> > > > interface if there is some activity.
> > >
> > > Known bug and is on TODO but fixing it ain't easy.
> > > Thanks for a report anyway.
> >
> > Wouldn't it be possible to do the stuff that needs serializing from the
> > end_request() part and get automatic synchronization with normal
> > requests?
> 
> That's the way to do it (REQ_SPECIAL) but unfortunately on some chipsets
> we need to synchronize both channels (whereas we don't need to serialize
> normal operations).

Ugh yes, that gets nasty... Good luck with that :)

-- 
Jens Axboe

Re: 2.6.4-rc-bk3: hdparm -X locks up IDE

[Bartlomiej Zolnierkiewicz]

On Thursday 11 of March 2004 15:48, Jens Axboe wrote:
> On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> > On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> > > I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> > > interface if there is some activity.
> >
> > Known bug and is on TODO but fixing it ain't easy.
> > Thanks for a report anyway.
>
> Wouldn't it be possible to do the stuff that needs serializing from the
> end_request() part and get automatic synchronization with normal
> requests?

That's the way to do it (REQ_SPECIAL) but unfortunately on some chipsets
we need to synchronize both channels (whereas we don't need to serialize
normal operations).

Regards,
Bartlomiej

Re: 2.6.4-rc-bk3: hdparm -X locks up IDE

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> On Thursday 11 of March 2004 15:48, Jens Axboe wrote:
> 
>>On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
>>
>>>On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
>>>
>>>>I discovered that hdparm -X <mode> /dev/hda can lock up IDE
>>>>interface if there is some activity.
>>>
>>>Known bug and is on TODO but fixing it ain't easy.
>>>Thanks for a report anyway.
>>
>>Wouldn't it be possible to do the stuff that needs serializing from the
>>end_request() part and get automatic synchronization with normal
>>requests?
> 
> 
> That's the way to do it (REQ_SPECIAL) but unfortunately on some chipsets
> we need to synchronize both channels (whereas we don't need to serialize
> normal operations).

blk_stop_queue() on all queues attached to the hardware?

You need to synchronize anyway for the rare hardware that reports itself 
as "simplex" -- one DMA engine for both channels.

	Jeff

[PATCH] hdparm_X.patch (was: Re: 2.6.4-rc-bk3: hdparm -X locks up IDE)

[Denis Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]

On Thursday 11 March 2004 19:59, Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Thursday 11 of March 2004 15:48, Jens Axboe wrote:
> >>On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> >>>On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> >>>>I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> >>>>interface if there is some activity.
> >>>
> >>>Known bug and is on TODO but fixing it ain't easy.
> >>>Thanks for a report anyway.
> >>
> >>Wouldn't it be possible to do the stuff that needs serializing from the
> >>end_request() part and get automatic synchronization with normal
> >>requests?
> >
> > That's the way to do it (REQ_SPECIAL) but unfortunately on some chipsets
> > we need to synchronize both channels (whereas we don't need to serialize
> > normal operations).
>
> blk_stop_queue() on all queues attached to the hardware?
>
> You need to synchronize anyway for the rare hardware that reports itself
> as "simplex" -- one DMA engine for both channels.

This patch survived cyclic switching of all DMA modes from mwdma0 to udma4
and continuous write load of type
while(1) { dd if=/dev/zero of=file bs=1M count=<RAM size>; sync; }
for five minutes.

It does not handle crippled/simplex chipset, it is a TODO.

Things commented by C++ style comments (//) aren't meant to stay.

I think original code was a bit buggy:
set_transfer() returned 0 for modes < XFER_SW_DMA_0, ie,
for all PIO modes. Later,
       if (set_transfer(drive, &tfargs)) {
               xfer_rate = args[1];
               if (ide_ata66_check(drive, &tfargs))
                       goto abort;
        }
        err = ide_wait_cmd(drive, args[0], args[1], args[2], args[3], argbuf);
        if (!err && xfer_rate) {
               ide_set_xfer_rate(drive, xfer_rate);
               ide_driveid_update(drive);
       }
This will never execute ide_set_xfer_rate() for PIO modes
--
vda

[-- Attachment #2: hdparm_X.patch --]
[-- Type: text/x-diff, Size: 4538 bytes --]

diff -urN linux-2.6.4.orig/drivers/ide/ide-io.c linux-2.6.4/drivers/ide/ide-io.c
--- linux-2.6.4.orig/drivers/ide/ide-io.c	Thu Mar 11 21:25:17 2004
+++ linux-2.6.4/drivers/ide/ide-io.c	Fri Mar 12 02:03:49 2004
@@ -1387,10 +1387,32 @@
 
 	err = 0;
 	if (must_wait) {
+		int xfer_rate = -1;
+		/* Are we going to do hdparm -X n ? */
+		if(rq->buffer
+		&& rq->buffer[0] == (char)WIN_SETFEATURES
+		// original code checked for !0 below. Is that right?
+		&& (int)rq->buffer[1] != 0
+		&& rq->buffer[2] == (char)SETFEATURES_XFER
+		// original code checked for 0 below. It is always true, can we nuke it?
+		&& rq->buffer[3] == 0
+		// original code did it but I think it's wrong:
+		//&& (drive->id->dma_ultra || drive->id->dma_mword
+		//			|| drive->id->dma_1word)
+		) {
+			xfer_rate = rq->buffer[1]; /* -X n */
+		}
+			
 		wait_for_completion(&wait);
 		if (rq->errors)
 			err = -EIO;
 
+		if(!err && xfer_rate != -1) {
+			/* asking chipset to change DMA/PIO timings */
+			ide_set_xfer_rate(drive, xfer_rate);
+			ide_driveid_update(drive);
+			mdelay(2000); // give it a name like WAIT_XFER_RATE
+		}
 		blk_put_request(rq);
 	}
 
diff -urN linux-2.6.4.orig/drivers/ide/ide-iops.c linux-2.6.4/drivers/ide/ide-iops.c
--- linux-2.6.4.orig/drivers/ide/ide-iops.c	Wed Feb 18 05:57:24 2004
+++ linux-2.6.4/drivers/ide/ide-iops.c	Fri Mar 12 02:04:19 2004
@@ -660,52 +660,34 @@
 
 EXPORT_SYMBOL(eighty_ninty_three);
 
-int ide_ata66_check (ide_drive_t *drive, ide_task_t *args)
+/*
+ * Is drive/channel capable of handling this?
+ * Currently checks only for ioctl(HDIO_DRIVE_CMD, SETFEATURES_XFER)
+ * (hdparm -X n)
+ */
+int unsupported_by_drive (ide_drive_t *drive, ide_task_t *args)
 {
-	if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
-	    (args->tfRegister[IDE_SECTOR_OFFSET] > XFER_UDMA_2) &&
-	    (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER)) {
+	if (args->tfRegister[IDE_COMMAND_OFFSET] != WIN_SETFEATURES) return 0;
+	if (args->tfRegister[IDE_FEATURE_OFFSET] != SETFEATURES_XFER) return 0;
+	if (args->tfRegister[IDE_SECTOR_OFFSET] <= XFER_UDMA_2) return 0;
+
 #ifndef CONFIG_IDEDMA_IVB
-		if ((drive->id->hw_config & 0x6000) == 0) {
+	if ( (drive->id->hw_config & 0x6000) == 0) {
 #else /* !CONFIG_IDEDMA_IVB */
-		if (((drive->id->hw_config & 0x2000) == 0) ||
-		    ((drive->id->hw_config & 0x4000) == 0)) {
+	if ( ((drive->id->hw_config & 0x2000) == 0) ||
+	     ((drive->id->hw_config & 0x4000) == 0) ) {
 #endif /* CONFIG_IDEDMA_IVB */
-			printk("%s: Speed warnings UDMA 3/4/5 is not "
-				"functional.\n", drive->name);
-			return 1;
-		}
-		if (!HWIF(drive)->udma_four) {
-			printk("%s: Speed warnings UDMA 3/4/5 is not "
-				"functional.\n",
-				HWIF(drive)->name);
-			return 1;
-		}
+		printk("%s is not capable of UDMA 3/4/5\n", drive->name);
+		return 1;
 	}
-	return 0;
-}
-
-EXPORT_SYMBOL(ide_ata66_check);
-
-/*
- * Backside of HDIO_DRIVE_CMD call of SETFEATURES_XFER.
- * 1 : Safe to update drive->id DMA registers.
- * 0 : OOPs not allowed.
- */
-int set_transfer (ide_drive_t *drive, ide_task_t *args)
-{
-	if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
-	    (args->tfRegister[IDE_SECTOR_OFFSET] >= XFER_SW_DMA_0) &&
-	    (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER) &&
-	    (drive->id->dma_ultra ||
-	     drive->id->dma_mword ||
-	     drive->id->dma_1word))
+	if (!HWIF(drive)->udma_four) {
+		printk("%s is not capable of UDMA 3/4/5\n", HWIF(drive)->name);
 		return 1;
-
+	}
 	return 0;
 }
 
-EXPORT_SYMBOL(set_transfer);
+EXPORT_SYMBOL(unsupported_by_drive);
 
 u8 ide_auto_reduce_xfer (ide_drive_t *drive)
 {
diff -urN linux-2.6.4.orig/drivers/ide/ide-taskfile.c linux-2.6.4/drivers/ide/ide-taskfile.c
--- linux-2.6.4.orig/drivers/ide/ide-taskfile.c	Thu Mar 11 21:25:18 2004
+++ linux-2.6.4/drivers/ide/ide-taskfile.c	Fri Mar 12 02:03:55 2004
@@ -1592,7 +1592,6 @@
 {
 	int err = 0;
 	u8 args[4], *argbuf = args;
-	u8 xfer_rate = 0;
 	int argsize = 4;
 	ide_task_t tfargs;
 
@@ -1621,19 +1620,13 @@
 			return -ENOMEM;
 		memcpy(argbuf, args, 4);
 	}
-	if (set_transfer(drive, &tfargs)) {
-		xfer_rate = args[1];
-		if (ide_ata66_check(drive, &tfargs))
-			goto abort;
+
+	if (unsupported_by_drive(drive, &tfargs)) {
+		goto abort;
 	}
 
 	err = ide_wait_cmd(drive, args[0], args[1], args[2], args[3], argbuf);
 
-	if (!err && xfer_rate) {
-		/* active-retuning-calls future */
-		ide_set_xfer_rate(drive, xfer_rate);
-		ide_driveid_update(drive);
-	}
 abort:
 	if (copy_to_user((void *)arg, argbuf, argsize))
 		err = -EFAULT;

Re: [PATCH] hdparm_X.patch (was: Re: 2.6.4-rc-bk3: hdparm -X locks up IDE)

[Bartlomiej Zolnierkiewicz]

On Friday 12 of March 2004 01:21, Denis Vlasenko wrote:
> On Thursday 11 March 2004 19:59, Jeff Garzik wrote:
> > Bartlomiej Zolnierkiewicz wrote:
> > > On Thursday 11 of March 2004 15:48, Jens Axboe wrote:
> > >>On Thu, Mar 11 2004, Bartlomiej Zolnierkiewicz wrote:
> > >>>On Thursday 11 of March 2004 15:14, Denis Vlasenko wrote:
> > >>>>I discovered that hdparm -X <mode> /dev/hda can lock up IDE
> > >>>>interface if there is some activity.
> > >>>
> > >>>Known bug and is on TODO but fixing it ain't easy.
> > >>>Thanks for a report anyway.
> > >>
> > >>Wouldn't it be possible to do the stuff that needs serializing from the
> > >>end_request() part and get automatic synchronization with normal
> > >>requests?
> > >
> > > That's the way to do it (REQ_SPECIAL) but unfortunately on some
> > > chipsets we need to synchronize both channels (whereas we don't need to
> > > serialize normal operations).
> >
> > blk_stop_queue() on all queues attached to the hardware?
> >
> > You need to synchronize anyway for the rare hardware that reports itself
> > as "simplex" -- one DMA engine for both channels.
>
> This patch survived cyclic switching of all DMA modes from mwdma0 to udma4
> and continuous write load of type
> while(1) { dd if=/dev/zero of=file bs=1M count=<RAM size>; sync; }
> for five minutes.
>
> It does not handle crippled/simplex chipset, it is a TODO.
>
> Things commented by C++ style comments (//) aren't meant to stay.
>
> I think original code was a bit buggy:
> set_transfer() returned 0 for modes < XFER_SW_DMA_0, ie,
> for all PIO modes. Later,
>        if (set_transfer(drive, &tfargs)) {
>                xfer_rate = args[1];
>                if (ide_ata66_check(drive, &tfargs))
>                        goto abort;
>         }
>         err = ide_wait_cmd(drive, args[0], args[1], args[2], args[3],
> argbuf); if (!err && xfer_rate) {
>                ide_set_xfer_rate(drive, xfer_rate);
>                ide_driveid_update(drive);
>        }
> This will never execute ide_set_xfer_rate() for PIO modes

It will also never execute ide_set_xfer_rate() for DMA on PIO only drive.
I must check if "no PIO" thing is a bug or a design decision.

Thanks,
Bartlomiej

Re: [PATCH] hdparm_X.patch (was: Re: 2.6.4-rc-bk3: hdparm -X locks up IDE)

[Denis Vlasenko]

> > This patch survived cyclic switching of all DMA modes from mwdma0 to
> > udma4 and continuous write load of type
> > while(1) { dd if=/dev/zero of=file bs=1M count=<RAM size>; sync; }
> > for five minutes.
> >
> > It does not handle crippled/simplex chipset, it is a TODO.
> >
> > Things commented by C++ style comments (//) aren't meant to stay.
> >
> > I think original code was a bit buggy:
> > set_transfer() returned 0 for modes < XFER_SW_DMA_0, ie,
> > for all PIO modes. Later,
> >        if (set_transfer(drive, &tfargs)) {
> >                xfer_rate = args[1];
> >                if (ide_ata66_check(drive, &tfargs))
> >                        goto abort;
> >         }
> >         err = ide_wait_cmd(drive, args[0], args[1], args[2], args[3],
> > argbuf); if (!err && xfer_rate) {
> >                ide_set_xfer_rate(drive, xfer_rate);
> >                ide_driveid_update(drive);
> >        }
> > This will never execute ide_set_xfer_rate() for PIO modes
>
> It will also never execute ide_set_xfer_rate() for DMA on PIO only drive.
> I must check if "no PIO" thing is a bug or a design decision.

I think IDE driver will never ask for DMA on PIO only drive

Bartlomiej, do you have comments on my patch? So far, I myself spotted
some minor problems:

I forgot to remove now-extraneous if() from ide.c:

static int set_xfer_rate (ide_drive_t *drive, int arg)
{
        int err = ide_wait_cmd(drive,
                        WIN_SETFEATURES, (u8) arg,
                        SETFEATURES_XFER, 0, NULL);

        if (!err && arg) {
                ide_set_xfer_rate(drive, (u8) arg);
                ide_driveid_update(drive);
        }
        return err;
}

And second, in ide_do_drive_cmd(), mdelay(2000)
after WIN_SETFEATURES is a bit rude.
-- 
vda

Re: [PATCH] hdparm_X.patch (was: Re: 2.6.4-rc-bk3: hdparm -X locks up IDE)

[Denis Vlasenko]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

> I forgot to remove now-extraneous if() from ide.c:
>
> static int set_xfer_rate (ide_drive_t *drive, int arg)
> {
>         int err = ide_wait_cmd(drive,
>                         WIN_SETFEATURES, (u8) arg,
>                         SETFEATURES_XFER, 0, NULL);
>
>         if (!err && arg) {
>                 ide_set_xfer_rate(drive, (u8) arg);
>                 ide_driveid_update(drive);
>         }
>         return err;
> }
>
> And second, in ide_do_drive_cmd(), mdelay(2000)
> after WIN_SETFEATURES is a bit rude.

...and wrongly placed. I just realized that ide_driveid_update(drive)
actually talks with the drive!

New patch is attached.
-- 
vda

[-- Attachment #2: hdparm_X-2.patch --]
[-- Type: text/x-diff, Size: 4191 bytes --]

diff -urN linux-2.6.4.orig/drivers/ide/ide-io.c linux-2.6.4/drivers/ide/ide-io.c
--- linux-2.6.4.orig/drivers/ide/ide-io.c	Fri Mar 12 11:15:00 2004
+++ linux-2.6.4/drivers/ide/ide-io.c	Fri Mar 12 11:33:30 2004
@@ -1387,10 +1387,25 @@
 
 	err = 0;
 	if (must_wait) {
+		int xfer_rate = -1;
+		/* Are we going to do hdparm -X n ? */
+		if(rq->buffer
+		&& rq->buffer[0] == (char)WIN_SETFEATURES
+		&& rq->buffer[2] == (char)SETFEATURES_XFER
+		) {
+			xfer_rate = rq->buffer[1]; /* -X n */
+		}
+			
 		wait_for_completion(&wait);
 		if (rq->errors)
 			err = -EIO;
 
+		if(!err && xfer_rate != -1) {
+			ide_delay_50ms(); /* be gentle */
+			/* ask chipset to change DMA/PIO timings */
+			ide_set_xfer_rate(drive, xfer_rate);
+			ide_driveid_update(drive);
+		}
 		blk_put_request(rq);
 	}
 
diff -urN linux-2.6.4.orig/drivers/ide/ide-iops.c linux-2.6.4/drivers/ide/ide-iops.c
--- linux-2.6.4.orig/drivers/ide/ide-iops.c	Fri Mar 12 11:07:07 2004
+++ linux-2.6.4/drivers/ide/ide-iops.c	Fri Mar 12 11:26:55 2004
@@ -660,52 +660,34 @@
 
 EXPORT_SYMBOL(eighty_ninty_three);
 
-int ide_ata66_check (ide_drive_t *drive, ide_task_t *args)
+/*
+ * Is drive/channel capable of handling this?
+ * Currently checks only for ioctl(HDIO_DRIVE_CMD, SETFEATURES_XFER)
+ * (hdparm -X n)
+ */
+int unsupported_by_drive (ide_drive_t *drive, ide_task_t *args)
 {
-	if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
-	    (args->tfRegister[IDE_SECTOR_OFFSET] > XFER_UDMA_2) &&
-	    (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER)) {
+	if (args->tfRegister[IDE_COMMAND_OFFSET] != WIN_SETFEATURES) return 0;
+	if (args->tfRegister[IDE_FEATURE_OFFSET] != SETFEATURES_XFER) return 0;
+	if (args->tfRegister[IDE_SECTOR_OFFSET] <= XFER_UDMA_2) return 0;
+
 #ifndef CONFIG_IDEDMA_IVB
-		if ((drive->id->hw_config & 0x6000) == 0) {
+	if ( (drive->id->hw_config & 0x6000) == 0) {
 #else /* !CONFIG_IDEDMA_IVB */
-		if (((drive->id->hw_config & 0x2000) == 0) ||
-		    ((drive->id->hw_config & 0x4000) == 0)) {
+	if ( ((drive->id->hw_config & 0x2000) == 0) ||
+	     ((drive->id->hw_config & 0x4000) == 0) ) {
 #endif /* CONFIG_IDEDMA_IVB */
-			printk("%s: Speed warnings UDMA 3/4/5 is not "
-				"functional.\n", drive->name);
-			return 1;
-		}
-		if (!HWIF(drive)->udma_four) {
-			printk("%s: Speed warnings UDMA 3/4/5 is not "
-				"functional.\n",
-				HWIF(drive)->name);
-			return 1;
-		}
+		printk("%s is not capable of UDMA 3/4/5\n", drive->name);
+		return 1;
 	}
-	return 0;
-}
-
-EXPORT_SYMBOL(ide_ata66_check);
-
-/*
- * Backside of HDIO_DRIVE_CMD call of SETFEATURES_XFER.
- * 1 : Safe to update drive->id DMA registers.
- * 0 : OOPs not allowed.
- */
-int set_transfer (ide_drive_t *drive, ide_task_t *args)
-{
-	if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
-	    (args->tfRegister[IDE_SECTOR_OFFSET] >= XFER_SW_DMA_0) &&
-	    (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER) &&
-	    (drive->id->dma_ultra ||
-	     drive->id->dma_mword ||
-	     drive->id->dma_1word))
+	if (!HWIF(drive)->udma_four) {
+		printk("%s is not capable of UDMA 3/4/5\n", HWIF(drive)->name);
 		return 1;
-
+	}
 	return 0;
 }
 
-EXPORT_SYMBOL(set_transfer);
+EXPORT_SYMBOL(unsupported_by_drive);
 
 u8 ide_auto_reduce_xfer (ide_drive_t *drive)
 {
diff -urN linux-2.6.4.orig/drivers/ide/ide-taskfile.c linux-2.6.4/drivers/ide/ide-taskfile.c
--- linux-2.6.4.orig/drivers/ide/ide-taskfile.c	Fri Mar 12 11:15:00 2004
+++ linux-2.6.4/drivers/ide/ide-taskfile.c	Fri Mar 12 11:26:55 2004
@@ -1592,7 +1592,6 @@
 {
 	int err = 0;
 	u8 args[4], *argbuf = args;
-	u8 xfer_rate = 0;
 	int argsize = 4;
 	ide_task_t tfargs;
 
@@ -1621,19 +1620,13 @@
 			return -ENOMEM;
 		memcpy(argbuf, args, 4);
 	}
-	if (set_transfer(drive, &tfargs)) {
-		xfer_rate = args[1];
-		if (ide_ata66_check(drive, &tfargs))
-			goto abort;
+
+	if (unsupported_by_drive(drive, &tfargs)) {
+		goto abort;
 	}
 
 	err = ide_wait_cmd(drive, args[0], args[1], args[2], args[3], argbuf);
 
-	if (!err && xfer_rate) {
-		/* active-retuning-calls future */
-		ide_set_xfer_rate(drive, xfer_rate);
-		ide_driveid_update(drive);
-	}
 abort:
 	if (copy_to_user((void *)arg, argbuf, argsize))
 		err = -EFAULT;

Re: [PATCH] hdparm_X.patch (was: Re: 2.6.4-rc-bk3: hdparm -X locks up IDE)

[Bartlomiej Zolnierkiewicz]

On Friday 12 of March 2004 10:39, Denis Vlasenko wrote:
> > I forgot to remove now-extraneous if() from ide.c:
> >
> > static int set_xfer_rate (ide_drive_t *drive, int arg)
> > {
> >         int err = ide_wait_cmd(drive,
> >                         WIN_SETFEATURES, (u8) arg,
> >                         SETFEATURES_XFER, 0, NULL);
> >
> >         if (!err && arg) {
> >                 ide_set_xfer_rate(drive, (u8) arg);
> >                 ide_driveid_update(drive);
> >         }
> >         return err;
> > }
> >
> > And second, in ide_do_drive_cmd(), mdelay(2000)
> > after WIN_SETFEATURES is a bit rude.
>
> ...and wrongly placed. I just realized that ide_driveid_update(drive)
> actually talks with the drive!
>
> New patch is attached.

diff -urN linux-2.6.4.orig/drivers/ide/ide-io.c linux-2.6.4/drivers/ide/ide-io.c
--- linux-2.6.4.orig/drivers/ide/ide-io.c	Fri Mar 12 11:15:00 2004
+++ linux-2.6.4/drivers/ide/ide-io.c	Fri Mar 12 11:33:30 2004
@@ -1387,10 +1387,25 @@

 	err = 0;
 	if (must_wait) {
+		int xfer_rate = -1;
+		/* Are we going to do hdparm -X n ? */

HDIO_DRIVE_CMD is an ordinary ioctl, not some hdparm specific thing.

+		if(rq->buffer
+		&& rq->buffer[0] == (char)WIN_SETFEATURES
+		&& rq->buffer[2] == (char)SETFEATURES_XFER
+		) {
+			xfer_rate = rq->buffer[1]; /* -X n */
+		}
+
 		wait_for_completion(&wait);
 		if (rq->errors)
 			err = -EIO;

+		if(!err && xfer_rate != -1) {
+			ide_delay_50ms(); /* be gentle */

Why?

+			/* ask chipset to change DMA/PIO timings */
+			ide_set_xfer_rate(drive, xfer_rate);
+			ide_driveid_update(drive);
+		}
 		blk_put_request(rq);
 	}

diff -urN linux-2.6.4.orig/drivers/ide/ide-iops.c linux-2.6.4/drivers/ide/ide-iops.c
--- linux-2.6.4.orig/drivers/ide/ide-iops.c	Fri Mar 12 11:07:07 2004
+++ linux-2.6.4/drivers/ide/ide-iops.c	Fri Mar 12 11:26:55 2004
@@ -660,52 +660,34 @@

 EXPORT_SYMBOL(eighty_ninty_three);

-int ide_ata66_check (ide_drive_t *drive, ide_task_t *args)
+/*
+ * Is drive/channel capable of handling this?
+ * Currently checks only for ioctl(HDIO_DRIVE_CMD, SETFEATURES_XFER)
+ * (hdparm -X n)
+ */
+int unsupported_by_drive (ide_drive_t *drive, ide_task_t *args)
 {
-	if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
-	    (args->tfRegister[IDE_SECTOR_OFFSET] > XFER_UDMA_2) &&
-	    (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER)) {
+	if (args->tfRegister[IDE_COMMAND_OFFSET] != WIN_SETFEATURES) return 0;
+	if (args->tfRegister[IDE_FEATURE_OFFSET] != SETFEATURES_XFER) return 0;
+	if (args->tfRegister[IDE_SECTOR_OFFSET] <= XFER_UDMA_2) return 0;
+
 #ifndef CONFIG_IDEDMA_IVB
-		if ((drive->id->hw_config & 0x6000) == 0) {
+	if ( (drive->id->hw_config & 0x6000) == 0) {
 #else /* !CONFIG_IDEDMA_IVB */
-		if (((drive->id->hw_config & 0x2000) == 0) ||
-		    ((drive->id->hw_config & 0x4000) == 0)) {
+	if ( ((drive->id->hw_config & 0x2000) == 0) ||
+	     ((drive->id->hw_config & 0x4000) == 0) ) {
 #endif /* CONFIG_IDEDMA_IVB */
-			printk("%s: Speed warnings UDMA 3/4/5 is not "
-				"functional.\n", drive->name);
-			return 1;
-		}
-		if (!HWIF(drive)->udma_four) {
-			printk("%s: Speed warnings UDMA 3/4/5 is not "
-				"functional.\n",
-				HWIF(drive)->name);
-			return 1;
-		}
+		printk("%s is not capable of UDMA 3/4/5\n", drive->name);
+		return 1;
 	}
-	return 0;
-}
-
-EXPORT_SYMBOL(ide_ata66_check);

-/*
- * Backside of HDIO_DRIVE_CMD call of SETFEATURES_XFER.
- * 1 : Safe to update drive->id DMA registers.
- * 0 : OOPs not allowed.
- */
-int set_transfer (ide_drive_t *drive, ide_task_t *args)
-{
-	if ((args->tfRegister[IDE_COMMAND_OFFSET] == WIN_SETFEATURES) &&
-	    (args->tfRegister[IDE_SECTOR_OFFSET] >= XFER_SW_DMA_0) &&
-	    (args->tfRegister[IDE_FEATURE_OFFSET] == SETFEATURES_XFER) &&
-	    (drive->id->dma_ultra ||
-	     drive->id->dma_mword ||
-	     drive->id->dma_1word))
+	if (!HWIF(drive)->udma_four) {
+		printk("%s is not capable of UDMA 3/4/5\n", HWIF(drive)->name);
 		return 1;
-
+	}
 	return 0;
 }

-EXPORT_SYMBOL(set_transfer);
+EXPORT_SYMBOL(unsupported_by_drive);

This ide_ata66_check() -> unsupported_by_driver()
change is totally unnecessary.

Please also leave set_transfer() in place,
"no PIO" issue can be addressed later.

Regards,
Bartlomiej