Re: PATCH: ide: ide-disk freeze support for hdaps

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

Hi,

On 8/25/05, Yani Ioannou <yani.ioannou@gmail.com> wrote:
> Hi all,
> 
> Attached below is a patch heavily based on Jon Escombe's patch, but
> implemented as a sysfs attribute as Jens described, with a timeout
> (configurable by module/kernel parameter) to ensure the queue isn't
> stopped forever.
> 
> The driver creates a sysfs attribute "/sys/block/hdX/device/freeze",
> which when set (echo 1 >
> /sys/block/hda/device/freeze/sys/block/hda/device/freeze) freezes the
> queue and parks the head on the drive, protecting the drive
> (theoretically) from severe damage, until the drive is thawed (echo 0
> > /sys/block/hda/device/freeze) or a timeout is reached (default 10s).
> 
> The patch is meant for usage with the hdaps (hard drive active
> protection system) for Thinkpads, however would also be useful for
> similair systems implemented on powerbooks, and other laptops out
> there.
> 
> I've tested it out on my T42p and it seems to work well, but as Jon
> warns this is experimental...
> 
> Thanks,
> Yani
> 
> Signed-off-by: Yani Ioannou <yani.ioannou@gmail.com>

Few comments about the patch below:

diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -148,6 +148,24 @@ config BLK_DEV_IDEDISK
 
 	  If unsure, say Y.
 
+if BLK_DEV_IDEDISK
+
+config IDEDISK_FREEZE

Is there any advantage of having it as a config option?

+	bool "Enable IDE DISK Freeze support"
+	depends on EXPERIMENTAL 
+	help
+	  This will include support for freezing an IDE drive (parking the 
+	  head, freezing the queue), allowing hard drive protection drivers
+	  to park the head quickly and keep it parked. If you don't use such
+	  a driver, you can say N here.
+
+	  If you say yes here a sysfs attribute "/sys/block/hda/device/freeze"
+	  will be created which can be used to freeze/thaw the drive.
+	  
+	  If in doubt, say N.
+
+endif
+
  config IDEDISK_MULTI_MODE
  	bool "Use multi-mode by default"
 	help
diff --git a/drivers/ide/ide-disk.c b/drivers/ide/ide-disk.c
--- a/drivers/ide/ide-disk.c
+++ b/drivers/ide/ide-disk.c
@@ -48,6 +48,9 @@
 //#define DEBUG
 
  #include <linux/config.h>
+#ifdef CONFIG_IDEDISK_FREEZE
+#include <linux/device.h>
+#endif /* CONFIG_IDEDISK_FREEZE */

This shouldn't be needed, 
<linux/device.h> is already included by <linux/ide.h>.

 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/string.h>
@@ -97,6 +100,158 @@ static struct ide_disk_obj *ide_disk_get
  	return idkp;
 }
 
+#ifdef CONFIG_IDEDISK_FREEZE
+/* IDE Freeze support - park the head and stop the queue, 
+ * 			for hard drive protection systems.
+ * Jon Escombe
+ * Yani Ioannou <yani.ioannou@gmail.com> 
+ */
+/* (spins down drive - more obvious for testing) */
+#define STANDBY_IMMEDIATE_ARGS {\
+	0xe0, 			\
+	0x44, 			\
+	0x00, 			\
+	0x4c, 			\
+	0x4e, 			\
+	0x55, 			\
+	0x00, 			\
+}
+
+#define UNLOAD_IMMEDIATE_ARGS {\
+	0xe1, 			\
+	0x44, 			\
+	0x00, 			\
+	0x4c, 			\
+	0x4e, 			\
+	0x55, 			\
+	0x00, 			\
+}
+
+#define IDE_FREEZE_TIMEOUT_SEC 10
+static int ide_freeze_timeout = IDE_FREEZE_TIMEOUT_SEC;
+module_param(ide_freeze_timeout, int, 0);
+MODULE_PARM_DESC(ide_freeze_timeout, 
+		"IDE Freeze timeout in seconds. (0<ide_freeze_timeout<65536, default=" 
+		__MODULE_STRING(IDE_FREEZE_TIMEOUT_SEC) ")");

Please make the interface accept number of seconds (as suggested by Jens)
and remove this module parameter. This way interface will be more flexible
and cleaner.  I really don't see any advantage in doing "echo 1 > ..." instead
of "echo x > ..." (Pavel, please explain).

+static void freeze_expire(unsigned long data);
+static struct timer_list freeze_timer = 
+	TIMER_INITIALIZER(freeze_expire, 0, 0);

There needs to be a timer per device not a global one
(it works for a current special case of T42 but sooner
 or later we will hit this problem).

+static ssize_t ide_freeze_show(struct device *dev, 
+		struct device_attribute *attr, char *buf){
+	ide_drive_t *drive = to_ide_device(dev);
+	int queue_stopped = 
+		test_bit(QUEUE_FLAG_STOPPED, &drive->queue->queue_flags);
+	
+	return snprintf(buf, 10, "%u\n", queue_stopped);
+}
+
+void ide_end_freeze_rq(struct request *rq)
+{
+	struct completion *waiting = rq->waiting;
+	u8 *argbuf = rq->buffer;
+
+	/* Spinlock is already acquired */
+	if (argbuf[3] == 0xc4) {
+		blk_stop_queue(rq->q);
+		printk(KERN_DEBUG "ide_end_freeze_rq(): Head parked\n");
+	}
+	else
+		printk(KERN_DEBUG "ide_end_freeze_rq(): Head not parked\n");
+	
+	complete(waiting);
+}
+
+static int ide_freeze_drive(ide_drive_t *drive)
+{
+	DECLARE_COMPLETION(wait);
+	struct request rq;
+	int error = 0;
+	
+	/* STANDBY IMMEDIATE COMMAND (testing) */
+	/* u8 argbuf[] = STANDBY_IMMEDIATE_ARGS; */
+
+	/* UNLOAD IMMEDIATE COMMAND */
+	u8 argbuf[] = UNLOAD_IMMEDIATE_ARGS;
+
+	/* Check we're not already frozen */
+	if(blk_queue_stopped(drive->queue)){
+		printk(KERN_DEBUG "ide_freeze_drive: Queue already stopped\n");
+		return error;
+	}
+	
+	memset(&rq, 0, sizeof(rq));
+	
+	ide_init_drive_cmd(&rq);
+
+	rq.flags = REQ_DRIVE_TASK;
+	rq.buffer = argbuf;
+	rq.waiting = &wait;
+	rq.end_io = ide_end_freeze_rq;
+
+	error = ide_do_drive_cmd(drive, &rq, ide_next);
+	wait_for_completion(&wait);
+	rq.waiting = NULL;
+	
+	if(error)
+		printk(KERN_ERR "ide_freeze_drive: Error sending command to drive %s: %d\n",
+				drive->name, error);
+	else{
+		printk(KERN_DEBUG "ide_freeze_drive: Queue stopped\n");
+		
+		freeze_timer.data = (unsigned long) drive;
+		mod_timer(&freeze_timer, jiffies+(ide_freeze_timeout*HZ));
+		printk(KERN_DEBUG "ide_freeze_drive: Timer (re)initialized to %ds
timeout\n",
+				ide_freeze_timeout);
+	}
+	return error;
+}
+
+static void ide_thaw_drive(ide_drive_t *drive)
+{
+	unsigned long flags;
+	if (!blk_queue_stopped(drive->queue))
+	{
+		printk(KERN_DEBUG "ide_thaw_drive(): Queue not stopped\n");
+		return;
+	}
+	
+	spin_lock_irqsave(&ide_lock, flags);
+	blk_start_queue(drive->queue);
+	spin_unlock_irqrestore(&ide_lock, flags);
+	printk(KERN_DEBUG "ide_thaw_drive(): Queue started\n");
+	del_timer(&freeze_timer);
+	printk(KERN_DEBUG "ide_thaw_drive: Timer stopped\n");
+}

queue handling should be done through block layer helpers
(as described in Jens' email) - we will need them for libata too.

+static void freeze_expire(unsigned long data)
+{
+	ide_drive_t *drive = (ide_drive_t *) data;
+	printk(KERN_DEBUG "freeze_expire(): Freeze timer timeout, thawing...\n");
+	ide_thaw_drive(drive);
+}
+	
+static ssize_t ide_freeze_store(struct device *dev, 
+		struct device_attribute *attr, const char *buf, size_t count){
+	ide_drive_t *drive = to_ide_device(dev);
+	int freeze = 0;
+	int error = 0;
+	
+	freeze = simple_strtoul(buf, NULL, 10);
+	
+	if(freeze)
+		error = ide_freeze_drive(drive);
+	else
+		ide_thaw_drive(drive);
+	
+	return error ? error : count;
+}
+
+static DEVICE_ATTR(freeze, S_IRUSR | S_IWUSR, ide_freeze_show, 
+		ide_freeze_store);

sorry but this needs to be CLASS_DEVICE_ATTR, see below

+#endif /* CONFIG_IDEDISK_FREEZE */
+
  static void ide_disk_release(struct kref *);
 
  static void ide_disk_put(struct ide_disk_obj *idkp)
@@ -1034,6 +1189,10 @@ static int ide_disk_remove(struct device
  	struct ide_disk_obj *idkp = drive->driver_data;
  	struct gendisk *g = idkp->disk;
 
+	#ifdef CONFIG_IDEDISK_FREEZE
+	device_remove_file(dev, &dev_attr_freeze);
+	#endif /* CONFIG_IDEDISK_FREEZE */
+	

At this time attribute can still be in use (because refcounting is done
on drive->gendev), you need to add "disk" class to ide-disk driver
(drivers/scsi/st.c looks like a good example how to do it).

 	ide_cacheflush_p(drive);
 
  	ide_unregister_subdriver(drive, idkp->driver);
@@ -1248,6 +1407,10 @@ static int ide_disk_probe(struct device 
 	} else
 		drive->attach = 1;
 
+	#ifdef CONFIG_IDEDISK_FREEZE
+	device_create_file(dev, &dev_attr_freeze);
+	#endif /* CONFIG_IDEDISK_FREEZE */
+	
 	g->minors = 1 << PARTN_BITS;
 	strcpy(g->devfs_name, drive->devfs_name);
  	g->driverfs_dev = &drive->gendev;
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1181,6 +1181,16 @@ static void ide_do_request (ide_hwgroup_
 		}
 
 		/*
+		 * Don't accept a request when the queue is stopped
+		 * (unless we are resuming from suspend)
+		 */
+		if (test_bit(QUEUE_FLAG_STOPPED, &drive->queue->queue_flags) &&
!blk_pm_resume_request(rq)) {
+			printk(KERN_ERR "%s: queue is stopped!\n", drive->name);
+			hwgroup->busy = 0;
+			break;
+		}

IMO this should also be handled by block layer
which has all needed information, Jens?

While at it: I think that sysfs support should be moved to block layer (queue
attributes) and storage driver should only need to provide queue_freeze_fn
and queue_thaw_fn functions (similarly to cache flush support).  This should
be done now not later because this stuff is exposed to the user-space.

+		/*
 		 * Sanity: don't accept a request that isn't a PM request
 		 * if we are currently power managed. This is very important as
 		 * blk_stop_queue() doesn't prevent the elv_next_request()
@@ -1661,6 +1671,9 @@ int ide_do_drive_cmd (ide_drive_t *drive
 		where = ELEVATOR_INSERT_FRONT;
  		rq->flags |= REQ_PREEMPT;
 	}
+	if (action == ide_next)
+		where = ELEVATOR_INSERT_FRONT;
+
  	__elv_add_request(drive->queue, rq, where, 0);
  	ide_do_request(hwgroup, IDE_NO_IRQ);
 	spin_unlock_irqrestore(&ide_lock, flags);

Why is this needed?


Overall, very promising work!

Thanks,
Bartlomiej