login
Search
[ Resending with correct address for lkml, sorry. ] Hi all, this is the second version of the patch series I posted a month ago. There are the following changes: - ata_id_has_unload() checks for the major version of the ATA spec the drive claims to comply with. - A disk head unload request issued to a device will effectively cause the same to be executed for all devices on the same port and stop all I/O to that port. Tejun told me that modern CD/DVD writers should have no difficulties to recover from buffer under-runs caused by such a behaviour (I haven't had a chance to put it to the test). - As for the part dealing with libata, I have been following Tejun's advice to rely on EH for the purposes of serialisation and in order to prevent spurious resets. Hopefully, this has turned out satisfactorily. As a nice side effect I don't have to touch any scsi stuff at all due to this approach. - Various minor changes intended to optimise the code or simply to make more compliant with kernel coding conventions. Unless there are any immediate objections from anyone, could the subsystem maintainers please voice their opinion whether these patches are likely to make it into 2.6.28? For obvious reasons, I'd like to make sure that the changes to libata and ide are introduced at the same time even though they don't depend on each other technically. Does that make the patch set a candidate for the mm tree, or should the patches go through the libata and ide tree respectively? Here are the short descriptions of the four patches (based on next-20080829): 1. This is a small patch to ata.h in order to provide a simple check for support of the unload feature as indicated in a device's ID. 2. Here disk head unloading is implemented in the libata subsystem. 3. The same for ide. 4. A little bit of documentation. Regards, Elias --
Add a function to check an ATA device's id for head unload support as
specified in ATA-7.
Signed-off-by: Elias Oltmanns <eo@nebensachen.de>
---
include/linux/ata.h | 17 +++++++++++++++++
1 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 80364b6..d9a94bd 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -707,6 +707,23 @@ static inline int ata_id_has_dword_io(const u16 *id)
return 0;
}
+static inline int ata_id_has_unload(const u16 *id)
+{
+ /*
+ * ATA-7 specifies two places to indicate unload feature
+ * support. Since I don't really understand the difference,
+ * I'll just check both and only return zero if none of them
+ * indicates otherwise.
+ */
+ if (ata_id_major_version(id) >= 7
+ && (((id[ATA_ID_CFSSE] & 0xC000) == 0x4000
+ && id[ATA_ID_CFSSE] & (1 << 13))
+ || ((id[ATA_ID_CSF_DEFAULT] & 0xC000) == 0x4000
+ && (id[ATA_ID_CSF_DEFAULT] & (1 << 13)))))
+ return 1;
+ return 0;
+}
+
static inline int ata_id_current_chs_valid(const u16 *id)
{
/* For ATA-1 devices, if the INITIALIZE DEVICE PARAMETERS command
--
Hello. If you read the comments to the words 82:84 and 85:87, they say that the former indicate the supported features, and the latter indicate the enabed features AND in case a feature can't be disabled, the latter words will have the corresponding bit set. So it should be sufficient to I think that it's preferrable to leave the operator on the same line with the first operand... WBR, Sergei --
Yes, I tend to agree with you and, in fact, I have been leaning in this direction myself. However, there is something that really bothers me. Both entries describing bit 13 of word 87 and 84 are worded alike. In particular, it says *supported* in both places, whereas in the case of the other features it would say enabled in one and supported in the other place. Well, I'm willing to drop the check for word 87 since I don't like it myself. Due to my lack of personal experience with inexplicable implemenations of ATA standards in hardware though, I have to take your Not having too strong an opinion about it, I just thought that an operator at the beginning of the line was another indication (apart from indentation) that this still belongs to the condition. Still, I can change it for the next series round. Regards, Elias --
I think it says "supported" where the feature can't be disabled and "enabled" where it can. Otherwise, this would make a little sense indeed. Hm, I even found a quote in ATA/PI-7 rev. 4b backing this claim (should've pasted it into previous mail): 6.17.43 Words (87:85): Features/command sets enabled Words (87:85) shall indicate features/command sets enabled. If a defined bit is cleared to zero, the indicated features/command set is not enabled. If a supported features/command set is supported and cannot be disabled, it is MBR, Sergei --
On user request (through sysfs), the IDLE IMMEDIATE command with UNLOAD
FEATURE as specified in ATA-7 is issued to the device and processing of
the request queue is stopped thereafter until the speified timeout
expires or user space asks to resume normal operation. This is supposed
to prevent the heads of a hard drive from accidentally crashing onto the
platter when a heavy shock is anticipated (like a falling laptop
expected to hit the floor). In fact, the whole port stops processing
commands until the timeout has expired in order to avoid any resets due
to failed commands on another device.
Signed-off-by: Elias Oltmanns <eo@nebensachen.de>
---
drivers/ata/ahci.c | 2
drivers/ata/ata_piix.c | 7 ++
drivers/ata/libata-core.c | 8 ++
drivers/ata/libata-eh.c | 51 +++++++++++
drivers/ata/libata-scsi.c | 205 +++++++++++++++++++++++++++++++++++++++++++++
drivers/ata/libata.h | 9 ++
include/linux/libata.h | 5 +
7 files changed, 287 insertions(+), 0 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c729e69..78281af 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -316,6 +316,8 @@ static struct device_attribute *ahci_shost_attrs[] = {
static struct device_attribute *ahci_sdev_attrs[] = {
&dev_attr_sw_activity,
+ &dev_attr_unload_feature,
+ &dev_attr_unload_heads,
NULL
};
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index b1d08a8..9b42f8d 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -298,8 +298,15 @@ static struct pci_driver piix_pci_driver = {
#endif
};
+static struct device_attribute *piix_sdev_attrs[] = {
+ &dev_attr_unload_feature,
+ &dev_attr_unload_heads,
+ NULL
+};
+
static struct scsi_host_template piix_sht = {
ATA_BMDMA_SHT(DRV_NAME),
+ .sdev_attrs = piix_sdev_attrs,
};
static struct ata_port_operations piix_pata_ops = {
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 79e3a8e..f1e036f ...Hello,
Ehhh... This really should be in libata core layer. Please create the
Which would make this unnecessary and make disk unloading available to
And does the device need this explicit wake up? It will wake up when
And these all can go. If you're worried about recurring events you
can just update timestamp from the sysfs write function and do...
deadline = last_timestamp + delay;
while ((now = jiffies) < deadline) {
set_current_state(TASK_UNINTERRUPTIBLE);
schedule_timeout(deadline - now);
set_current_state(TASK_RUNNING);
Hmmm.... Maybe you can just disable it by echoing -1 to the unload file?
Thanks.
--
tejun
--
On user request (through sysfs), the IDLE IMMEDIATE command with UNLOAD
FEATURE as specified in ATA-7 is issued to the device and processing of
the request queue is stopped thereafter until the speified timeout
expires or user space asks to resume normal operation. This is supposed
to prevent the heads of a hard drive from accidentally crashing onto the
platter when a heavy shock is anticipated (like a falling laptop
expected to hit the floor). In fact, the whole port stops processing
commands until the timeout has expired in order to avoid resets due to
failed commands on another device.
Signed-off-by: Elias Oltmanns <eo@nebensachen.de>
---
drivers/ide/ide-io.c | 30 +++++
drivers/ide/ide-probe.c | 3
drivers/ide/ide-taskfile.c | 10 +-
drivers/ide/ide.c | 287 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/ide.h | 17 ++-
5 files changed, 341 insertions(+), 6 deletions(-)
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index d0579f1..657c0d8 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -675,7 +675,33 @@ EXPORT_SYMBOL_GPL(ide_devset_execute);
static ide_startstop_t ide_special_rq(ide_drive_t *drive, struct request *rq)
{
+ ide_hwif_t *hwif = drive->hwif;
+ ide_task_t task;
+ struct ide_taskfile *tf = &task.tf;
+
+ memset(&task, 0, sizeof(task));
switch (rq->cmd[0]) {
+ case REQ_PARK_HEADS: {
+ struct completion *waiting = rq->end_io_data;
+
+ drive->sleep = drive->hwif->park_timer.expires;
+ drive->dev_flags |= IDE_DFLAG_SLEEPING;
+ complete(waiting);
+ if (drive->dev_flags & IDE_DFLAG_NO_UNLOAD) {
+ ide_end_request(drive, 1, 0);
+ return ide_stopped;
+ }
+ tf->command = ATA_CMD_IDLEIMMEDIATE;
+ tf->feature = 0x44;
+ tf->lbal = 0x4c;
+ tf->lbam = 0x4e;
+ tf->lbah = 0x55;
+ task.tf_flags |= IDE_TFLAG_CUSTOM_HANDLER;
+ break;
+ }
+ case REQ_UNPARK_HEADS:
+ tf->command = ATA_CMD_CHK_POWER;
+ break;
case REQ_DEVSET_EXEC:
{
int err, ...[ continuing the discussion from 'patch #2' thread ] While I'm still not fully convinced this is the best way to go in the long-term I'm well aware that if we won't get in 2.6.28 it will mean at least 3 more months until it hits users so lets concentrate on existing user/kernel-space solution first... There are some issues to address before it can go in but once they are fixed I'm fine with the patch and I'll merge it as soon as patches #1-2 are in. ide_port_tune_devices() is not a best suited place for it, please move it to ide_port_init_devices(). How's about just looping on hwif->drives[] instead? [ this would also allow removal of loops in issue_park_cmd() This is unnecessary (IDE_DFLAG_PRESENT won't be cleared as long as there are references on &drive->gendev and we should have such No need for getting additional reference on hwif, it won't go away ide_lock taking here is superfluous (as it doesn't protect against changing IDE settings, hwgroup->busy does) Also could you please move the new code to a separate file (i.e. ide-park.c) instead of stuffing it all in ide.c? Otherwise it looks OK (modulo PM notifiers concerns raised by Tejun but the code is identical to libata's version so it is sufficient to duplicate the potential fixes here). --
Put some information (and pointers to more) into the kernel's doc tree, describing briefly the interface to the kernel's disk head unloading facility. Information about how to set up a complete shock protection system under GNU/Linux can be found on the web and is referenced accordingly. Signed-off-by: Elias Oltmanns <eo@nebensachen.de> --- Documentation/laptops/disk-shock-protection.txt | 131 +++++++++++++++++++++++ 1 files changed, 131 insertions(+), 0 deletions(-) create mode 100644 Documentation/laptops/disk-shock-protection.txt diff --git a/Documentation/laptops/disk-shock-protection.txt b/Documentation/laptops/disk-shock-protection.txt new file mode 100644 index 0000000..bd483a3 --- /dev/null +++ b/Documentation/laptops/disk-shock-protection.txt @@ -0,0 +1,131 @@ +Hard disk shock protection +========================== + +Author: Elias Oltmanns <eo@nebensachen.de> +Last modified: 2008-08-28 + + +0. Contents +----------- + +1. Intro +2. The interface +3. References +4. CREDITS + + +1. Intro +-------- + +ATA/ATAPI-7 specifies the IDLE IMMEDIATE command with unload feature. +Issuing this command should cause the drive to switch to idle mode and +unload disk heads. This feature is being used in modern laptops in +conjunction with accelerometers and appropriate software to implement +a shock protection facility. The idea is to stop all I/O operations on +the internal hard drive and park its heads on the ramp when critical +situations are anticipated. The desire to have such a feature +available on GNU/Linux systems has been the original motivation to +implement a generic disk head parking interface in the Linux kernel. +Please note, however, that other components have to be set up on your +system in order to get disk shock protection working (see section +3. Referneces below for pointers to more information about that). + + +2. The interface +---------------- + +The interface works as follows: Writing an integer value ...
--- ~Randy Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA http://linuxplumbersconf.org/ --
Thanks for reviewing, Randy. In addition to your annotations, I've made various adjustments reflecting changes to the interface as they have evolved in the discussion over the last two weeks. Please feel free to point out some more mistakes and shortcomings ;-). Regards, Elias From: Elias Oltmanns <eo@nebensachen.de> Subject: [PATCH] Add documentation for hard disk shock protection interface Put some information (and pointers to more) into the kernel's doc tree, describing briefly the interface to the kernel's disk head unloading facility. Information about how to set up a complete shock protection system under GNU/Linux can be found on the web and is referenced accordingly. Signed-off-by: Elias Oltmanns <eo@nebensachen.de> --- Documentation/laptops/disk-shock-protection.txt | 144 +++++++++++++++++++++++ 1 files changed, 144 insertions(+), 0 deletions(-) create mode 100644 Documentation/laptops/disk-shock-protection.txt diff --git a/Documentation/laptops/disk-shock-protection.txt b/Documentation/laptops/disk-shock-protection.txt new file mode 100644 index 0000000..1f93462 --- /dev/null +++ b/Documentation/laptops/disk-shock-protection.txt @@ -0,0 +1,144 @@ +Hard disk shock protection +========================== + +Author: Elias Oltmanns <eo@nebensachen.de> +Last modified: 2008-09-16 + + +0. Contents +----------- + +1. Intro +2. The interface +3. References +4. CREDITS + + +1. Intro +-------- + +ATA/ATAPI-7 specifies the IDLE IMMEDIATE command with unload feature. +Issuing this command should cause the drive to switch to idle mode and +unload disk heads. This feature is being used in modern laptops in +conjunction with accelerometers and appropriate software to implement +a shock protection facility. The idea is to stop all I/O operations on +the internal hard drive and park its heads on the ramp when critical +situations are anticipated. The desire to have such a feature +available on GNU/Linux systems has been the original motivation ...
