Re: PATCH: ide: ide-disk freeze support for hdaps
Hi! 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). Either way is pretty easy enough to implement. Note though that I'd expect the userspace app should thaw the device when danger is out of the way (the timeout is mainly there to ensure that the queue isn't frozen forever, and should probably be higher). Personally I don't have too much of an opinion either way though... what's the consensus? :). Yes please, I don't understand why you would want a 0/1 interface instead, when the timer-seconds method gives you the exact same ability plus a way to control when to unfreeze... Well, with my power-managment hat on: we probably want freeze functionality to be generic; it makes sense for other devices, too. My battery is so low I can not use wifi any more = userspace freezes wifi. Now, having this kind of timeout in all the cases looks pretty ugly to my eyes. Pavel -- 64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PATCH: ide: ide-disk freeze support for hdaps
Hi Pavel, On 8/27/05, Pavel Machek [EMAIL PROTECTED] wrote: Well, with my power-managment hat on: we probably want freeze functionality to be generic; it makes sense for other devices, too. My battery is so low I can not use wifi any more = userspace freezes wifi. Now, having this kind of timeout in all the cases looks pretty ugly to my eyes. Thing is the freeze attribute hasn't got much to do with power management, this is just to freeze the queue, and park the drive head ASAP (preferably with the unload immediate command supported by new drives) in order to protect the drive in an impact. Unload immediate doesn't even stop spinning the drive, so little power is saved. Maybe a suspend attribute would be a good idea for something along the lines of what you have in mind? A enable/disable attribute would definitely make sense for that application. I suppose renaming the attribute to ramming_speed or brace_for_impact, might make the purpose more clear ;). Thanks, Yani - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: PATCH: ide: ide-disk freeze support for hdaps
On Fri, Aug 26 2005, Yani Ioannou wrote: 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). Either way is pretty easy enough to implement. Note though that I'd expect the userspace app should thaw the device when danger is out of the way (the timeout is mainly there to ensure that the queue isn't frozen forever, and should probably be higher). Personally I don't have too much of an opinion either way though... what's the consensus? :). Yes please, I don't understand why you would want a 0/1 interface instead, when the timer-seconds method gives you the exact same ability plus a way to control when to unfreeze... +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). I was considering that, but I am confused as to whether each drive has it's own queue or not? (I really am a newbie to this stuff...). If so then yes there should be a per-device timer. Each drive has its own queue. queue handling should be done through block layer helpers (as described in Jens' email) - we will need them for libata too. Good point, I'll try to move as much as I can up to the block layer, it helps when it comes to implementing freeze for libata as you point out too. That includes things like the timer as well, reuse the queue plugging timer as I described in my initial posting on how to implement this. 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). I missed that completely, I'll look at changing it. 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. I was actually considering using a queue attribute originally, but in my indecision I decided to go with Jen's suggestion. A queue attribute does make sense in that the attribute primarily is there to freeze the queue, but it would also be performing the head park. Would a queue attribute be confusing because of that? I fully agree with Bart here. The only stuff that should be ide special is the actual command setup and completion check, since that is a hardware property. libata will get a few little helpers for that as well. The rest should be block layer implementation. * 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? I think Jon discussed that in a previous thread, but basically although ide_next is documented in the comment for ide_do_drive_cmd, there isn't (as far as Jon or I could see) anything actually handling it. This patch is carried over from Jon's work and adds the code to handle ide_next by inserting the request at the front of the queue. As per my previous mail, I will ack that bit. Overall, very promising work! Thanks :-), most of it is Jon's work, and Jen's suggestions though. My name is Jens, not Jen :-) -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH: ide: ide-disk freeze support for hdaps
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 [EMAIL PROTECTED]
---
drivers/ide/Kconfig| 18 +
drivers/ide/ide-disk.c | 163
drivers/ide/ide-io.c | 13
3 files changed, 194 insertions(+), 0 deletions(-)
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
+ 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 */
#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 [EMAIL PROTECTED]
+ */
+/* (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. (0ide_freeze_timeout65536,
default=
+ __MODULE_STRING(IDE_FREEZE_TIMEOUT_SEC) ));
+static void freeze_expire(unsigned long data);
+static struct timer_list freeze_timer =
+ TIMER_INITIALIZER(freeze_expire, 0, 0);
+
+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
Re: PATCH: ide: ide-disk freeze support for hdaps
Hi,
On 8/25/05, Yani Ioannou [EMAIL PROTECTED] 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 [EMAIL PROTECTED]
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 [EMAIL PROTECTED]
+ */
+/* (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. (0ide_freeze_timeout65536,
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
Re: PATCH: ide: ide-disk freeze support for hdaps
Hi Bartlomiej, Thank you for your feedback :), as this is my first dabble in ide/block drivers I certainly need it! On 8/25/05, Bartlomiej Zolnierkiewicz [EMAIL PROTECTED] wrote: +config IDEDISK_FREEZE Is there any advantage of having it as a config option? The main reasons I added the config option: - the freeze feature is really only useful to an (increasing) niche of mobile computers with an accelerometer. - it might actually be detrimental to most other systems, you would never want to freeze the queue on most machines - especially a production system, and for that reason alone it seemed sensible to me to be able to selectively remove it completely. - to re-inforce the experimental nature of the patch, and disable it by default (although this could be achieved just with EXPERIMENTAL I suppose). 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). Either way is pretty easy enough to implement. Note though that I'd expect the userspace app should thaw the device when danger is out of the way (the timeout is mainly there to ensure that the queue isn't frozen forever, and should probably be higher). Personally I don't have too much of an opinion either way though... what's the consensus? :). I can understand Pavel's opinion in that a enable/disable attribute in sysfs seems the norm, and is more intuitive. Also what should 'cat /sys/block/hda/device/freeze' return for a 'echo x /sys/block/hda/device/freeze' sysfs attribute? The seconds remaining? 1/0 for frozen/thawed? +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). I was considering that, but I am confused as to whether each drive has it's own queue or not? (I really am a newbie to this stuff...). If so then yes there should be a per-device timer. queue handling should be done through block layer helpers (as described in Jens' email) - we will need them for libata too. Good point, I'll try to move as much as I can up to the block layer, it helps when it comes to implementing freeze for libata as you point out too. 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). I missed that completely, I'll look at changing it. 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. I was actually considering using a queue attribute originally, but in my indecision I decided to go with Jen's suggestion. A queue attribute does make sense in that the attribute primarily is there to freeze the queue, but it would also be performing the head park. Would a queue attribute be confusing because of that? + /* * 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? I think Jon discussed that in a previous thread, but basically although ide_next is documented in the comment for ide_do_drive_cmd, there isn't (as far as Jon or I could see) anything actually handling it. This patch is carried over from Jon's work and adds the code to handle ide_next by inserting the request at the front of the queue. Overall, very promising work! Thanks :-), most of it is Jon's work, and Jen's suggestions though. Yani P.S. Sorry about the lack of [] around PATCH...lack of sleep. Its more of a RFC anyway. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
