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

[Bartlomiej Zolnierkiewicz <B.Zolnierkiewicz@elka.pw.edu.pl>]

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