[RFT][PATCH] ide-disk.c: more write cache fixes

[RFT][PATCH] ide-disk.c: more write cache fixes

[Bartlomiej Zolnierkiewicz]

[ Andrew, please merge this patch in -mm in place of the one you have now. ]

Comments, suggestions, complains?

[PATCH] ide-disk.c: more write cache fixes

- many Maxtor disks incorrectly claim CACHE FLUSH EXT command support,
  fix it by checking both CACHE FLUSH EXT command and LBA48 support
  (thanks to Eric D. Mudama for help in fixing this)

- write_cache() was called with 'drive->id->cfs_enable_2 & 0x3000' as 'int arg'
  argument which was always truncated to zero due to 'u8 drive->wcache = arg'
  assignment so write cache was indeed enabled but drive->wcache was zero
  (thanks to Rene Herman for help in debugging this)

- flush cache in idedisk_start_power_step() only if ATA-6 CACHE FLUSH (EXT)
  bits are present in disk's identify data (prevents sending unknown commands)

- set drive->wcache in idedisk_setup() not idedisk_attach() (no need to check
  id->command_set_2 - we check id->cfs_enable_2 instead in write_cache() call)

- use ide_cacheflush_p() in idedisk_setup()

- minor cleanups

 linux-2.6.6-bk1-bzolnier/drivers/ide/ide-disk.c |   70 +++++++++---------------
 1 files changed, 27 insertions(+), 43 deletions(-)

diff -puN drivers/ide/ide-disk.c~idedisk_wcache drivers/ide/ide-disk.c
--- linux-2.6.6-bk1/drivers/ide/ide-disk.c~idedisk_wcache	2004-05-13 16:42:43.963896336 +0200
+++ linux-2.6.6-bk1-bzolnier/drivers/ide/ide-disk.c	2004-05-13 20:30:34.619640768 +0200
@@ -740,8 +740,6 @@ static ide_startstop_t ide_do_rw_disk (i
 		return __ide_do_rw_disk(drive, rq, block);
 }
 
-static int do_idedisk_flushcache(ide_drive_t *drive);
-
 static u8 idedisk_dump_status (ide_drive_t *drive, const char *msg, u8 stat)
 {
 	ide_hwif_t *hwif = HWIF(drive);
@@ -1359,11 +1357,18 @@ static int set_nowerr(ide_drive_t *drive
 	return 0;
 }
 
+/* check if CACHE FLUSH (EXT) command is supported (bits defined in ATA-6) */
+#define ide_id_has_flush_cache(id)	((id)->cfs_enable_2 & 0x3000)
+
+/* some Maxtor disks have bit 13 defined incorrectly so check bit 10 too */
+#define ide_id_has_flush_cache_ext(id)	\
+	(((id)->cfs_enable_2 & 0x2400) == 0x2400)
+
 static int write_cache (ide_drive_t *drive, int arg)
 {
 	ide_task_t args;
 
-	if (!(drive->id->cfs_enable_2 & 0x3000))
+	if (!ide_id_has_flush_cache(drive->id))
 		return 1;
 
 	memset(&args, 0, sizeof(ide_task_t));
@@ -1383,7 +1388,7 @@ static int do_idedisk_flushcache (ide_dr
 	ide_task_t args;
 
 	memset(&args, 0, sizeof(ide_task_t));
-	if (drive->id->cfs_enable_2 & 0x2400)
+	if (ide_id_has_flush_cache_ext(drive->id))
 		args.tfRegister[IDE_COMMAND_OFFSET]	= WIN_FLUSH_CACHE_EXT;
 	else
 		args.tfRegister[IDE_COMMAND_OFFSET]	= WIN_FLUSH_CACHE;
@@ -1513,11 +1518,11 @@ static ide_startstop_t idedisk_start_pow
 	switch (rq->pm->pm_step) {
 	case idedisk_pm_flush_cache:	/* Suspend step 1 (flush cache) */
 		/* Not supported? Switch to next step now. */
-		if (!drive->wcache) {
+		if (!drive->wcache || !ide_id_has_flush_cache(drive->id)) {
 			idedisk_complete_power_step(drive, rq, 0, 0);
 			return ide_stopped;
 		}
-		if (drive->id->cfs_enable_2 & 0x2400)
+		if (ide_id_has_flush_cache_ext(drive->id))
 			args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE_EXT;
 		else
 			args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE;
@@ -1678,8 +1683,12 @@ static void idedisk_setup (ide_drive_t *
 #endif	/* CONFIG_IDEDISK_MULTI_MODE */
 	}
 	drive->no_io_32bit = id->dword_io ? 1 : 0;
-	if (drive->id->cfs_enable_2 & 0x3000)
-		write_cache(drive, (id->cfs_enable_2 & 0x3000));
+
+	/* write cache enabled? */
+	if ((id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)))
+		drive->wcache = 1;
+
+	write_cache(drive, 1);
 
 #ifdef CONFIG_BLK_DEV_IDE_TCQ_DEFAULT
 	if (drive->using_dma)
@@ -1687,9 +1696,17 @@ static void idedisk_setup (ide_drive_t *
 #endif
 }
 
+static void ide_cacheflush_p(ide_drive_t *drive)
+{
+	if (!drive->wcache || !ide_id_has_flush_cache(drive->id))
+		return;
+
+	if (do_idedisk_flushcache(drive))
+		printk(KERN_INFO "%s: wcache flush failed!\n", drive->name);
+}
+
 static int idedisk_cleanup (ide_drive_t *drive)
 {
-	static int ide_cacheflush_p(ide_drive_t *drive);
 	struct gendisk *g = drive->disk;
 	ide_cacheflush_p(drive);
 	if (ide_unregister_subdriver(drive))
@@ -1740,7 +1757,6 @@ static ide_driver_t idedisk_driver = {
 
 static int idedisk_open(struct inode *inode, struct file *filp)
 {
-	u8 cf;
 	ide_drive_t *drive = inode->i_bdev->bd_disk->private_data;
 	drive->usage++;
 	if (drive->removable && drive->usage == 1) {
@@ -1758,35 +1774,6 @@ static int idedisk_open(struct inode *in
 		if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL))
 			drive->doorlocking = 0;
 	}
-	drive->wcache = 0;
-	/* Cache enabled? */
-	if (drive->id->csfo & 1)
-		drive->wcache = 1;
-	/* Cache command set available? */
-	if (drive->id->cfs_enable_1 & (1 << 5))
-		drive->wcache = 1;
-	/* ATA6 cache extended commands */
-	cf = drive->id->command_set_2 >> 24;
-	if ((cf & 0xC0) == 0x40 && (cf & 0x30) != 0)
-		drive->wcache = 1;
-	return 0;
-}
-
-static int ide_cacheflush_p(ide_drive_t *drive)
-{
-	if (!(drive->id->cfs_enable_2 & 0x3000))
-		return 0;
-
-	if(drive->wcache)
-	{
-		if (do_idedisk_flushcache(drive))
-		{
-			printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
-				drive->name);
-			return -EIO;
-		}
-		return 1;
-	}
 	return 0;
 }
 
@@ -1867,10 +1854,7 @@ static int idedisk_attach(ide_drive_t *d
 	if ((!drive->head || drive->head > 16) && !drive->select.b.lba) {
 		printk(KERN_ERR "%s: INVALID GEOMETRY: %d PHYSICAL HEADS?\n",
 			drive->name, drive->head);
-		if ((drive->id->cfs_enable_2 & 0x3000) && drive->wcache)
-			if (do_idedisk_flushcache(drive))
-				printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
-					drive->name);
+		ide_cacheflush_p(drive);
 		ide_unregister_subdriver(drive);
 		DRIVER(drive)->busy--;
 		goto failed;

_

Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Rene Herman]

Bartlomiej Zolnierkiewicz wrote:

> Comments, suggestions, complains?

Yes, this works to stop it from complaining (on 6Y120P0). It comes up 
with write cache enabled, and hdparm -W0/-W1 work to disable/enable 
write cache as evidenced by the tiobench results. Not as evidenced by 
/proc/ide/hda/settings (drive->wcache) which is always 1 and which will 
probably confuse more users than just me -- I believe I saw hdparm just 
pushes a drive command through an ioctl?

Question though:

> @@ -1678,8 +1683,12 @@ static void idedisk_setup (ide_drive_t *
>  #endif	/* CONFIG_IDEDISK_MULTI_MODE */
>  	}
>  	drive->no_io_32bit = id->dword_io ? 1 : 0;
> -	if (drive->id->cfs_enable_2 & 0x3000)
> -		write_cache(drive, (id->cfs_enable_2 & 0x3000));
> +
> +	/* write cache enabled? */
> +	if ((id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)))
> +		drive->wcache = 1;
> +
> +	write_cache(drive, 1);

write_cache() also sets drive->wcache (to the argument, 1 in this case) 
and you call that unconditionally, so the "if (foo) drive->wcache = 1" 
seems superfluous. If the idea indeed is to unconditionally enable write 
cache, it seems just

write_cache(drive, 1);

would be equivalent. Or if that wasn't the intention, maybe:

if (foo)
	write_cache(drive, 1);

or if it should in fact be disabled if (!foo):

write_cache(drive, (id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)));

or ...

Ignore me if I completely missed the point, just looks odd.

Rene.

Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Bartlomiej Zolnierkiewicz]

On Friday 14 of May 2004 01:28, Rene Herman wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > Comments, suggestions, complains?
>
> Yes, this works to stop it from complaining (on 6Y120P0). It comes up
> with write cache enabled, and hdparm -W0/-W1 work to disable/enable
> write cache as evidenced by the tiobench results. Not as evidenced by
> /proc/ide/hda/settings (drive->wcache) which is always 1 and which will
> probably confuse more users than just me -- I believe I saw hdparm just
> pushes a drive command through an ioctl?

Yep, you are right and we shouldn't worry about confusion too much,
we have similar situation till now (wcache was zero even if enabled).

> Question though:
> > @@ -1678,8 +1683,12 @@ static void idedisk_setup (ide_drive_t *
> >  #endif	/* CONFIG_IDEDISK_MULTI_MODE */
> >  	}
> >  	drive->no_io_32bit = id->dword_io ? 1 : 0;
> > -	if (drive->id->cfs_enable_2 & 0x3000)
> > -		write_cache(drive, (id->cfs_enable_2 & 0x3000));
> > +
> > +	/* write cache enabled? */
> > +	if ((id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)))
> > +		drive->wcache = 1;
> > +
> > +	write_cache(drive, 1);
>
> write_cache() also sets drive->wcache (to the argument, 1 in this case)
> and you call that unconditionally, so the "if (foo) drive->wcache = 1"
> seems superfluous. If the idea indeed is to unconditionally enable write
> cache, it seems just

Please look at write_cache(), it will try to {en,dis}able write cache
and then set drive->wcache *only* if disk has ATA-6 cache flush bits.

I think that we can change write_cache() to always allow enabling of
write cache so hdparm can be fixed to use HDIO_{GET,SET}_WCACHE ioctls
but we should be careful about always enabling write cache in a driver
(there may be some unknown yet side-effects).

Whatever we decide we can do this later after this patch is merged.

> write_cache(drive, 1);
>
> would be equivalent. Or if that wasn't the intention, maybe:
>
> if (foo)
> 	write_cache(drive, 1);
>
> or if it should in fact be disabled if (!foo):
>
> write_cache(drive, (id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)));
>
> or ...
>
> Ignore me if I completely missed the point, just looks odd.

:-)

Thanks,
Bartlomiej

Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Rene Herman]

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

Bartlomiej Zolnierkiewicz wrote:

> Please look at write_cache(), it will try to {en,dis}able write cache
> and then set drive->wcache *only* if disk has ATA-6 cache flush bits.

Yes, you're quite right.

Have again attached a 'rollup' patch against vanilla 2.6.6, including 
this, Andrew's SYSTEM_SHUTDOWN split and the quick "don't switch of 
spindle if rebooting" hack. Again, just in case anyone finds it useful.

(by the way, the SYSTEM_SHUTDOWN split is already in -mm2)

Rene.

[-- Attachment #2: linux-2.6.6_rollup2.diff --]
[-- Type: text/plain, Size: 7344 bytes --]

diff -urN linux-2.6.6.orig/drivers/ide/ide-disk.c linux-2.6.6/drivers/ide/ide-disk.c
--- linux-2.6.6.orig/drivers/ide/ide-disk.c	2004-05-10 09:31:44.000000000 +0200
+++ linux-2.6.6/drivers/ide/ide-disk.c	2004-05-14 00:33:44.000000000 +0200
@@ -740,8 +740,6 @@
 		return __ide_do_rw_disk(drive, rq, block);
 }
 
-static int do_idedisk_flushcache(ide_drive_t *drive);
-
 static u8 idedisk_dump_status (ide_drive_t *drive, const char *msg, u8 stat)
 {
 	ide_hwif_t *hwif = HWIF(drive);
@@ -1359,11 +1357,18 @@
 	return 0;
 }
 
+/* check if CACHE FLUSH (EXT) command is supported (bits defined in ATA-6) */
+#define ide_id_has_flush_cache(id)	((id)->cfs_enable_2 & 0x3000)
+
+/* some Maxtor disks have bit 13 defined incorrectly so check bit 10 too */
+#define ide_id_has_flush_cache_ext(id)	\
+	(((id)->cfs_enable_2 & 0x2400) == 0x2400)
+
 static int write_cache (ide_drive_t *drive, int arg)
 {
 	ide_task_t args;
 
-	if (!(drive->id->cfs_enable_2 & 0x3000))
+	if (!ide_id_has_flush_cache(drive->id))
 		return 1;
 
 	memset(&args, 0, sizeof(ide_task_t));
@@ -1383,7 +1388,7 @@
 	ide_task_t args;
 
 	memset(&args, 0, sizeof(ide_task_t));
-	if (drive->id->cfs_enable_2 & 0x2400)
+	if (ide_id_has_flush_cache_ext(drive->id))
 		args.tfRegister[IDE_COMMAND_OFFSET]	= WIN_FLUSH_CACHE_EXT;
 	else
 		args.tfRegister[IDE_COMMAND_OFFSET]	= WIN_FLUSH_CACHE;
@@ -1513,11 +1518,11 @@
 	switch (rq->pm->pm_step) {
 	case idedisk_pm_flush_cache:	/* Suspend step 1 (flush cache) */
 		/* Not supported? Switch to next step now. */
-		if (!drive->wcache) {
+		if (!drive->wcache || !ide_id_has_flush_cache(drive->id)) {
 			idedisk_complete_power_step(drive, rq, 0, 0);
 			return ide_stopped;
 		}
-		if (drive->id->cfs_enable_2 & 0x2400)
+		if (ide_id_has_flush_cache_ext(drive->id))
 			args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE_EXT;
 		else
 			args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE;
@@ -1678,8 +1683,12 @@
 #endif	/* CONFIG_IDEDISK_MULTI_MODE */
 	}
 	drive->no_io_32bit = id->dword_io ? 1 : 0;
-	if (drive->id->cfs_enable_2 & 0x3000)
-		write_cache(drive, (id->cfs_enable_2 & 0x3000));
+
+	/* write cache enabled? */
+	if ((id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)))
+		drive->wcache = 1;
+
+	write_cache(drive, 1);
 
 #ifdef CONFIG_BLK_DEV_IDE_TCQ_DEFAULT
 	if (drive->using_dma)
@@ -1687,9 +1696,17 @@
 #endif
 }
 
+static void ide_cacheflush_p(ide_drive_t *drive)
+{
+	if (!drive->wcache || !ide_id_has_flush_cache(drive->id))
+		return;
+
+	if (do_idedisk_flushcache(drive))
+		printk(KERN_INFO "%s: wcache flush failed!\n", drive->name);
+}
+
 static int idedisk_cleanup (ide_drive_t *drive)
 {
-	static int ide_cacheflush_p(ide_drive_t *drive);
 	struct gendisk *g = drive->disk;
 	ide_cacheflush_p(drive);
 	if (ide_unregister_subdriver(drive))
@@ -1704,10 +1721,11 @@
 
 static void ide_device_shutdown(struct device *dev)
 {
-	ide_drive_t *drive = container_of(dev, ide_drive_t, gendev);
-
-	printk("Shutdown: %s\n", drive->name);
-	dev->bus->suspend(dev, PM_SUSPEND_STANDBY);
+	if (system_state != SYSTEM_RESTART) {
+		ide_drive_t *drive = container_of(dev, ide_drive_t, gendev);
+		printk("Shutdown: %s\n", drive->name);
+		dev->bus->suspend(dev, PM_SUSPEND_STANDBY);
+	}
 }
 
 /*
@@ -1740,7 +1758,6 @@
 
 static int idedisk_open(struct inode *inode, struct file *filp)
 {
-	u8 cf;
 	ide_drive_t *drive = inode->i_bdev->bd_disk->private_data;
 	drive->usage++;
 	if (drive->removable && drive->usage == 1) {
@@ -1758,35 +1775,6 @@
 		if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL))
 			drive->doorlocking = 0;
 	}
-	drive->wcache = 0;
-	/* Cache enabled? */
-	if (drive->id->csfo & 1)
-		drive->wcache = 1;
-	/* Cache command set available? */
-	if (drive->id->cfs_enable_1 & (1 << 5))
-		drive->wcache = 1;
-	/* ATA6 cache extended commands */
-	cf = drive->id->command_set_2 >> 24;
-	if ((cf & 0xC0) == 0x40 && (cf & 0x30) != 0)
-		drive->wcache = 1;
-	return 0;
-}
-
-static int ide_cacheflush_p(ide_drive_t *drive)
-{
-	if (!(drive->id->cfs_enable_2 & 0x3000))
-		return 0;
-
-	if(drive->wcache)
-	{
-		if (do_idedisk_flushcache(drive))
-		{
-			printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
-				drive->name);
-			return -EIO;
-		}
-		return 1;
-	}
 	return 0;
 }
 
@@ -1867,10 +1855,7 @@
 	if ((!drive->head || drive->head > 16) && !drive->select.b.lba) {
 		printk(KERN_ERR "%s: INVALID GEOMETRY: %d PHYSICAL HEADS?\n",
 			drive->name, drive->head);
-		if ((drive->id->cfs_enable_2 & 0x3000) && drive->wcache)
-			if (do_idedisk_flushcache(drive))
-				printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
-					drive->name);
+		ide_cacheflush_p(drive);
 		ide_unregister_subdriver(drive);
 		DRIVER(drive)->busy--;
 		goto failed;
diff -urN linux-2.6.6.orig/include/linux/kernel.h linux-2.6.6/include/linux/kernel.h
--- linux-2.6.6.orig/include/linux/kernel.h	2004-05-10 09:31:47.000000000 +0200
+++ linux-2.6.6/include/linux/kernel.h	2004-05-14 00:33:35.000000000 +0200
@@ -109,14 +109,17 @@
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;		/* If set, an oops, panic(), BUG() or die() is in progress */
 extern int panic_on_oops;
-extern int system_state;		/* See values below */
 extern int tainted;
 extern const char *print_tainted(void);
 
 /* Values used for system_state */
-#define SYSTEM_BOOTING 0
-#define SYSTEM_RUNNING 1
-#define SYSTEM_SHUTDOWN 2
+extern enum system_states {
+	SYSTEM_BOOTING,
+	SYSTEM_RUNNING,
+	SYSTEM_HALT,
+	SYSTEM_POWER_OFF,
+	SYSTEM_RESTART,
+} system_state;
 
 #define TAINT_PROPRIETARY_MODULE	(1<<0)
 #define TAINT_FORCED_MODULE		(1<<1)
diff -urN linux-2.6.6.orig/init/main.c linux-2.6.6/init/main.c
--- linux-2.6.6.orig/init/main.c	2004-05-10 09:31:47.000000000 +0200
+++ linux-2.6.6/init/main.c	2004-05-14 00:33:35.000000000 +0200
@@ -95,7 +95,8 @@
 extern void tc_init(void);
 #endif
 
-int system_state;	/* SYSTEM_BOOTING/RUNNING/SHUTDOWN */
+enum system_states system_state;
+EXPORT_SYMBOL(system_state);
 
 /*
  * Boot command-line arguments
diff -urN linux-2.6.6.orig/kernel/sys.c linux-2.6.6/kernel/sys.c
--- linux-2.6.6.orig/kernel/sys.c	2004-05-10 09:31:47.000000000 +0200
+++ linux-2.6.6/kernel/sys.c	2004-05-14 00:33:35.000000000 +0200
@@ -447,7 +447,7 @@
 	switch (cmd) {
 	case LINUX_REBOOT_CMD_RESTART:
 		notifier_call_chain(&reboot_notifier_list, SYS_RESTART, NULL);
-		system_state = SYSTEM_SHUTDOWN;
+		system_state = SYSTEM_RESTART;
 		device_shutdown();
 		printk(KERN_EMERG "Restarting system.\n");
 		machine_restart(NULL);
@@ -463,7 +463,7 @@
 
 	case LINUX_REBOOT_CMD_HALT:
 		notifier_call_chain(&reboot_notifier_list, SYS_HALT, NULL);
-		system_state = SYSTEM_SHUTDOWN;
+		system_state = SYSTEM_HALT;
 		device_shutdown();
 		printk(KERN_EMERG "System halted.\n");
 		machine_halt();
@@ -473,7 +473,7 @@
 
 	case LINUX_REBOOT_CMD_POWER_OFF:
 		notifier_call_chain(&reboot_notifier_list, SYS_POWER_OFF, NULL);
-		system_state = SYSTEM_SHUTDOWN;
+		system_state = SYSTEM_POWER_OFF;
 		device_shutdown();
 		printk(KERN_EMERG "Power down.\n");
 		machine_power_off();
@@ -489,7 +489,7 @@
 		buffer[sizeof(buffer) - 1] = '\0';
 
 		notifier_call_chain(&reboot_notifier_list, SYS_RESTART, buffer);
-		system_state = SYSTEM_SHUTDOWN;
+		system_state = SYSTEM_RESTART;
 		device_shutdown();
 		printk(KERN_EMERG "Restarting system with command '%s'.\n", buffer);
 		machine_restart(buffer);

Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Alan Cox]

On Fri, May 14, 2004 at 01:58:58PM +0200, Rene Herman wrote:
> Have again attached a 'rollup' patch against vanilla 2.6.6, including 
> this, Andrew's SYSTEM_SHUTDOWN split and the quick "don't switch of 
> spindle if rebooting" hack. Again, just in case anyone finds it useful.

This reintroduces corruption on my thinkpad 600.

Alan

Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Bartlomiej Zolnierkiewicz]

On Sunday 16 of May 2004 21:58, Alan Cox wrote:
> On Fri, May 14, 2004 at 01:58:58PM +0200, Rene Herman wrote:
> > Have again attached a 'rollup' patch against vanilla 2.6.6, including
> > this, Andrew's SYSTEM_SHUTDOWN split and the quick "don't switch of
> > spindle if rebooting" hack. Again, just in case anyone finds it useful.
>
> This reintroduces corruption on my thinkpad 600.

[ this corruption was fixed by kernel 2.6.6 ]

Please see if reverting changes to ide_device_shutdown() helps.

Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Rene Herman]

Bartlomiej Zolnierkiewicz wrote:

> On Sunday 16 of May 2004 21:58, Alan Cox wrote:
> 
>>On Fri, May 14, 2004 at 01:58:58PM +0200, Rene Herman wrote:
>>
>>>Have again attached a 'rollup' patch against vanilla 2.6.6, including
>>>this, Andrew's SYSTEM_SHUTDOWN split and the quick "don't switch of
>>>spindle if rebooting" hack. Again, just in case anyone finds it useful.
>>
>>This reintroduces corruption on my thinkpad 600.
> 
> [ this corruption was fixed by kernel 2.6.6 ]
> 
> Please see if reverting changes to ide_device_shutdown() helps.

Bart, could something like:

if (system_state == SYSTEM_RESTART) {
	ide_cache_flush_p(drive)
	return;
}

(as opposed to just the return in that patch and -mm3) possibly help? I 
sort of expect that ide_cache_flush_p() is already called further on up?

Alan, if you know, that drive fails ide_id_has_flush_cache()?

Note, very aware I don't know what the fuck I'm doing here (and equally 
aware I don't _want_ to be here :-) Having the drive spin down on each 
reboot is totally unacceptable though. Not only does spinning up again 
take significant time and noise, it's also actively bad for the drive.

If there's no sane way to fix this, an explicit blacklist for drives 
that really need to be shutdown? Eew.

Rene.

Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Bartlomiej Zolnierkiewicz]

On Sunday 16 of May 2004 23:15, Rene Herman wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Sunday 16 of May 2004 21:58, Alan Cox wrote:
> >>On Fri, May 14, 2004 at 01:58:58PM +0200, Rene Herman wrote:
> >>>Have again attached a 'rollup' patch against vanilla 2.6.6, including
> >>>this, Andrew's SYSTEM_SHUTDOWN split and the quick "don't switch of
> >>>spindle if rebooting" hack. Again, just in case anyone finds it useful.
> >>
> >>This reintroduces corruption on my thinkpad 600.
> >
> > [ this corruption was fixed by kernel 2.6.6 ]
> >
> > Please see if reverting changes to ide_device_shutdown() helps.
>
> Bart, could something like:
>
> if (system_state == SYSTEM_RESTART) {
> 	ide_cache_flush_p(drive)
> 	return;
> }
>
> (as opposed to just the return in that patch and -mm3) possibly help? I

Yep, this may fix it.

> sort of expect that ide_cache_flush_p() is already called further on up?
>
> Alan, if you know, that drive fails ide_id_has_flush_cache()?
>
> Note, very aware I don't know what the fuck I'm doing here (and equally
> aware I don't _want_ to be here :-) Having the drive spin down on each

hehe

> reboot is totally unacceptable though. Not only does spinning up again
> take significant time and noise, it's also actively bad for the drive.
>
> If there's no sane way to fix this, an explicit blacklist for drives
> that really need to be shutdown? Eew.
>
> Rene.

Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Rene Herman]

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

Bartlomiej Zolnierkiewicz wrote:

>>Bart, could something like:
>>
>>if (system_state == SYSTEM_RESTART) {
>>	ide_cache_flush_p(drive)
>>	return;
>>}
>>
>>(as opposed to just the return in that patch and -mm3) possibly help? I
> 
> Yep, this may fix it.

Oh, wonderful. Alan, first attachment is a new version of the rollup 
with this incorporated (ie, again versus 2.6.6 vanilla).

Andrew, second attachment is for you. Guess it's largely a "can't hurt", 
but please wait for Alan to ack. If that drive is really screwy, this 
still won't do.

Rene.

[-- Attachment #2: linux-2.6.6_rollup3.diff --]
[-- Type: text/plain, Size: 7170 bytes --]

diff -urN linux-2.6.6.orig/drivers/ide/ide-disk.c linux-2.6.6/drivers/ide/ide-disk.c
--- linux-2.6.6.orig/drivers/ide/ide-disk.c	2004-05-10 09:31:44.000000000 +0200
+++ linux-2.6.6/drivers/ide/ide-disk.c	2004-05-16 23:38:55.000000000 +0200
@@ -740,8 +740,6 @@
 		return __ide_do_rw_disk(drive, rq, block);
 }
 
-static int do_idedisk_flushcache(ide_drive_t *drive);
-
 static u8 idedisk_dump_status (ide_drive_t *drive, const char *msg, u8 stat)
 {
 	ide_hwif_t *hwif = HWIF(drive);
@@ -1359,11 +1357,18 @@
 	return 0;
 }
 
+/* check if CACHE FLUSH (EXT) command is supported (bits defined in ATA-6) */
+#define ide_id_has_flush_cache(id)	((id)->cfs_enable_2 & 0x3000)
+
+/* some Maxtor disks have bit 13 defined incorrectly so check bit 10 too */
+#define ide_id_has_flush_cache_ext(id)	\
+	(((id)->cfs_enable_2 & 0x2400) == 0x2400)
+
 static int write_cache (ide_drive_t *drive, int arg)
 {
 	ide_task_t args;
 
-	if (!(drive->id->cfs_enable_2 & 0x3000))
+	if (!ide_id_has_flush_cache(drive->id))
 		return 1;
 
 	memset(&args, 0, sizeof(ide_task_t));
@@ -1383,7 +1388,7 @@
 	ide_task_t args;
 
 	memset(&args, 0, sizeof(ide_task_t));
-	if (drive->id->cfs_enable_2 & 0x2400)
+	if (ide_id_has_flush_cache_ext(drive->id))
 		args.tfRegister[IDE_COMMAND_OFFSET]	= WIN_FLUSH_CACHE_EXT;
 	else
 		args.tfRegister[IDE_COMMAND_OFFSET]	= WIN_FLUSH_CACHE;
@@ -1513,11 +1518,11 @@
 	switch (rq->pm->pm_step) {
 	case idedisk_pm_flush_cache:	/* Suspend step 1 (flush cache) */
 		/* Not supported? Switch to next step now. */
-		if (!drive->wcache) {
+		if (!drive->wcache || !ide_id_has_flush_cache(drive->id)) {
 			idedisk_complete_power_step(drive, rq, 0, 0);
 			return ide_stopped;
 		}
-		if (drive->id->cfs_enable_2 & 0x2400)
+		if (ide_id_has_flush_cache_ext(drive->id))
 			args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE_EXT;
 		else
 			args->tfRegister[IDE_COMMAND_OFFSET] = WIN_FLUSH_CACHE;
@@ -1678,8 +1683,12 @@
 #endif	/* CONFIG_IDEDISK_MULTI_MODE */
 	}
 	drive->no_io_32bit = id->dword_io ? 1 : 0;
-	if (drive->id->cfs_enable_2 & 0x3000)
-		write_cache(drive, (id->cfs_enable_2 & 0x3000));
+
+	/* write cache enabled? */
+	if ((id->csfo & 1) || (id->cfs_enable_1 & (1 << 5)))
+		drive->wcache = 1;
+
+	write_cache(drive, 1);
 
 #ifdef CONFIG_BLK_DEV_IDE_TCQ_DEFAULT
 	if (drive->using_dma)
@@ -1687,9 +1696,17 @@
 #endif
 }
 
+static void ide_cacheflush_p(ide_drive_t *drive)
+{
+	if (!drive->wcache || !ide_id_has_flush_cache(drive->id))
+		return;
+
+	if (do_idedisk_flushcache(drive))
+		printk(KERN_INFO "%s: wcache flush failed!\n", drive->name);
+}
+
 static int idedisk_cleanup (ide_drive_t *drive)
 {
-	static int ide_cacheflush_p(ide_drive_t *drive);
 	struct gendisk *g = drive->disk;
 	ide_cacheflush_p(drive);
 	if (ide_unregister_subdriver(drive))
@@ -1706,6 +1723,11 @@
 {
 	ide_drive_t *drive = container_of(dev, ide_drive_t, gendev);
 
+	if (system_state == SYSTEM_RESTART) {
+		ide_cacheflush_p(drive);
+		return;
+	}
+
 	printk("Shutdown: %s\n", drive->name);
 	dev->bus->suspend(dev, PM_SUSPEND_STANDBY);
 }
@@ -1740,7 +1762,6 @@
 
 static int idedisk_open(struct inode *inode, struct file *filp)
 {
-	u8 cf;
 	ide_drive_t *drive = inode->i_bdev->bd_disk->private_data;
 	drive->usage++;
 	if (drive->removable && drive->usage == 1) {
@@ -1758,35 +1779,6 @@
 		if (drive->doorlocking && ide_raw_taskfile(drive, &args, NULL))
 			drive->doorlocking = 0;
 	}
-	drive->wcache = 0;
-	/* Cache enabled? */
-	if (drive->id->csfo & 1)
-		drive->wcache = 1;
-	/* Cache command set available? */
-	if (drive->id->cfs_enable_1 & (1 << 5))
-		drive->wcache = 1;
-	/* ATA6 cache extended commands */
-	cf = drive->id->command_set_2 >> 24;
-	if ((cf & 0xC0) == 0x40 && (cf & 0x30) != 0)
-		drive->wcache = 1;
-	return 0;
-}
-
-static int ide_cacheflush_p(ide_drive_t *drive)
-{
-	if (!(drive->id->cfs_enable_2 & 0x3000))
-		return 0;
-
-	if(drive->wcache)
-	{
-		if (do_idedisk_flushcache(drive))
-		{
-			printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
-				drive->name);
-			return -EIO;
-		}
-		return 1;
-	}
 	return 0;
 }
 
@@ -1867,10 +1859,7 @@
 	if ((!drive->head || drive->head > 16) && !drive->select.b.lba) {
 		printk(KERN_ERR "%s: INVALID GEOMETRY: %d PHYSICAL HEADS?\n",
 			drive->name, drive->head);
-		if ((drive->id->cfs_enable_2 & 0x3000) && drive->wcache)
-			if (do_idedisk_flushcache(drive))
-				printk (KERN_INFO "%s: Write Cache FAILED Flushing!\n",
-					drive->name);
+		ide_cacheflush_p(drive);
 		ide_unregister_subdriver(drive);
 		DRIVER(drive)->busy--;
 		goto failed;
diff -urN linux-2.6.6.orig/include/linux/kernel.h linux-2.6.6/include/linux/kernel.h
--- linux-2.6.6.orig/include/linux/kernel.h	2004-05-10 09:31:47.000000000 +0200
+++ linux-2.6.6/include/linux/kernel.h	2004-05-16 23:35:26.000000000 +0200
@@ -109,14 +109,17 @@
 extern void bust_spinlocks(int yes);
 extern int oops_in_progress;		/* If set, an oops, panic(), BUG() or die() is in progress */
 extern int panic_on_oops;
-extern int system_state;		/* See values below */
 extern int tainted;
 extern const char *print_tainted(void);
 
 /* Values used for system_state */
-#define SYSTEM_BOOTING 0
-#define SYSTEM_RUNNING 1
-#define SYSTEM_SHUTDOWN 2
+extern enum system_states {
+	SYSTEM_BOOTING,
+	SYSTEM_RUNNING,
+	SYSTEM_HALT,
+	SYSTEM_POWER_OFF,
+	SYSTEM_RESTART,
+} system_state;
 
 #define TAINT_PROPRIETARY_MODULE	(1<<0)
 #define TAINT_FORCED_MODULE		(1<<1)
diff -urN linux-2.6.6.orig/init/main.c linux-2.6.6/init/main.c
--- linux-2.6.6.orig/init/main.c	2004-05-10 09:31:47.000000000 +0200
+++ linux-2.6.6/init/main.c	2004-05-16 23:35:26.000000000 +0200
@@ -95,7 +95,8 @@
 extern void tc_init(void);
 #endif
 
-int system_state;	/* SYSTEM_BOOTING/RUNNING/SHUTDOWN */
+enum system_states system_state;
+EXPORT_SYMBOL(system_state);
 
 /*
  * Boot command-line arguments
diff -urN linux-2.6.6.orig/kernel/sys.c linux-2.6.6/kernel/sys.c
--- linux-2.6.6.orig/kernel/sys.c	2004-05-10 09:31:47.000000000 +0200
+++ linux-2.6.6/kernel/sys.c	2004-05-16 23:35:26.000000000 +0200
@@ -447,7 +447,7 @@
 	switch (cmd) {
 	case LINUX_REBOOT_CMD_RESTART:
 		notifier_call_chain(&reboot_notifier_list, SYS_RESTART, NULL);
-		system_state = SYSTEM_SHUTDOWN;
+		system_state = SYSTEM_RESTART;
 		device_shutdown();
 		printk(KERN_EMERG "Restarting system.\n");
 		machine_restart(NULL);
@@ -463,7 +463,7 @@
 
 	case LINUX_REBOOT_CMD_HALT:
 		notifier_call_chain(&reboot_notifier_list, SYS_HALT, NULL);
-		system_state = SYSTEM_SHUTDOWN;
+		system_state = SYSTEM_HALT;
 		device_shutdown();
 		printk(KERN_EMERG "System halted.\n");
 		machine_halt();
@@ -473,7 +473,7 @@
 
 	case LINUX_REBOOT_CMD_POWER_OFF:
 		notifier_call_chain(&reboot_notifier_list, SYS_POWER_OFF, NULL);
-		system_state = SYSTEM_SHUTDOWN;
+		system_state = SYSTEM_POWER_OFF;
 		device_shutdown();
 		printk(KERN_EMERG "Power down.\n");
 		machine_power_off();
@@ -489,7 +489,7 @@
 		buffer[sizeof(buffer) - 1] = '\0';
 
 		notifier_call_chain(&reboot_notifier_list, SYS_RESTART, buffer);
-		system_state = SYSTEM_SHUTDOWN;
+		system_state = SYSTEM_RESTART;
 		device_shutdown();
 		printk(KERN_EMERG "Restarting system with command '%s'.\n", buffer);
 		machine_restart(buffer);

[-- Attachment #3: linux-2.6.6-mm3_reboot.diff --]
[-- Type: text/plain, Size: 464 bytes --]

--- linux-2.6.6-mm3/drivers/ide/ide-disk.c.orig	2004-05-16 23:43:15.000000000 +0200
+++ linux-2.6.6-mm3/drivers/ide/ide-disk.c	2004-05-16 23:44:51.000000000 +0200
@@ -1723,8 +1723,10 @@
 {
 	ide_drive_t *drive = container_of(dev, ide_drive_t, gendev);
 
-	if (system_state == SYSTEM_RESTART)
+	if (system_state == SYSTEM_RESTART) {
+		ide_cacheflush_p(drive);
 		return;
+	}
 
 	printk("Shutdown: %s\n", drive->name);
 	dev->bus->suspend(dev, PM_SUSPEND_STANDBY);

Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Alan Cox]

On Sun, May 16, 2004 at 10:20:23PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > Have again attached a 'rollup' patch against vanilla 2.6.6, including
> > > this, Andrew's SYSTEM_SHUTDOWN split and the quick "don't switch of
> > > spindle if rebooting" hack. Again, just in case anyone finds it useful.
> >
> > This reintroduces corruption on my thinkpad 600.
> 
> [ this corruption was fixed by kernel 2.6.6 ]
> 
> Please see if reverting changes to ide_device_shutdown() helps.

Something odd going on. I need to look further into this. I'm not sure
now its specifically this patch

Re: [RFT][PATCH] ide-disk.c: more write cache fixes

[Alan Cox]

On Sun, May 16, 2004 at 11:15:18PM +0200, Rene Herman wrote:
> Alan, if you know, that drive fails ide_id_has_flush_cache()?

It does. I'm getting confusing results so I need to work out what
is up.

> Note, very aware I don't know what the fuck I'm doing here (and equally 
> aware I don't _want_ to be here :-) Having the drive spin down on each 
> reboot is totally unacceptable though. Not only does spinning up again 
> take significant time and noise, it's also actively bad for the drive.

I'd guess that the laptop may power off the drive during the reboot
cycle. If so then your suggestion makes sense