Re: [PATCH 3/4] ide: Implement disk shock protection support

From: Elias Oltmanns
Date: Wed Sep 03 2008 - 16:04:35 EST

Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx> wrote:
> On Friday 29 August 2008, Elias Oltmanns wrote:
>> 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@xxxxxxxxxxxxxx>
>
> [ 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.

Thank you very much Bart, I really do appreciate that. Some more
questions though:
>
> [...]
>
>> @@ -842,6 +842,9 @@ static void ide_port_tune_devices(ide_hwif_t *hwif)
>>
>> if (hwif->dma_ops)
>> ide_set_dma(drive);
>> +
>> + if (!ata_id_has_unload(drive->id))
>> + drive->dev_flags |= IDE_DFLAG_NO_UNLOAD;
>
> ide_port_tune_devices() is not a best suited place for it,
> please move it to ide_port_init_devices().

... We need to have IDENTIFY data present in drive->id at that point
which is not the case before ide_probe_port() has been executed. Should
I perhaps move it to ide_port_setup_devices() instead?

[...]
>> + spin_lock_irq(&ide_lock);
>> + if (unlikely(!hwif->present || timer_pending(&hwif->park_timer)))
>> + goto done;
>> +
>> + drive = hwif->hwgroup->drive;
>> + while (drive->hwif != hwif)
>> + drive = drive->next;
>
> How's about just looping on hwif->drives[] instead?
>
> [ this would also allow removal of loops in issue_park_cmd()
> and simplify locking there ]

Yes, I've reorganised it all a bit in order to account for all the
issues addressed in the discussion. In particular, I loop over
hwif->drives now as you suggested.

[...]
>> +static ssize_t park_store(struct device *dev, struct device_attribute *attr,
>> + const char *buf, size_t len)
>> +{
>> +#define MAX_PARK_TIMEOUT 30
>> + ide_drive_t *drive = to_ide_device(dev);
>> + ide_hwif_t *hwif = drive->hwif;
>> + DECLARE_COMPLETION_ONSTACK(wait);
>> + unsigned long timeout;
>> + int rc, count = 0;
>> +
>> + rc = strict_strtoul(buf, 10, &timeout);
>> + if (rc || timeout > MAX_PARK_TIMEOUT)
>> + return -EINVAL;
>> +
>> + mutex_lock(&ide_setting_mtx);
>
> No need to hold ide_settings_mtx here.

Even though the next version of the patch is different in various ways,
we have a similar problem. As far as I can see, we need to hold the
ide_setting_mtx here because the spin_lock will be taken and released
several times subsequently and therefore cannot protect hwif->park_timer
(or hwif->park_timeout in the new patch) against concurrent writes to
this sysfs attribute.

>
>> + spin_lock_irq(&ide_lock);
>> + if (unlikely(!(drive->dev_flags & IDE_DFLAG_PRESENT))) {
>> + rc = -ENODEV;
>> + goto unlock;
>> + }

[...]

> 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?

This is probably a sensible idea especially since there may be more once
we go ahead with the in-kernel solution. This means, however, that some
more random stuff is going into include/linux/ide.h. If it wasn't so
huge and if I had an idea what was to be taken into account so as not to
break user space applications, I'd offer to try my hand at moving things
to a private header file drivers/ide/ide.h. But as it is, I'm rather
scared.

>
> 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).

On popular request, they're gone now. With the new patches I can't
reproduce the system freezes anymore.

The patch below applies to next-20080903. I'll resend the whole series
once this (and the libata one) has been reviewed and potential glitches
have been ironed out.

Regards,

Elias

---

drivers/ide/Makefile | 2 -
drivers/ide/ide-io.c | 27 +++++++++
drivers/ide/ide-park.c | 133 ++++++++++++++++++++++++++++++++++++++++++++
drivers/ide/ide-probe.c | 3 +
drivers/ide/ide-taskfile.c | 10 +++
drivers/ide/ide.c | 1
include/linux/ide.h | 16 +++++
7 files changed, 190 insertions(+), 2 deletions(-)
create mode 100644 drivers/ide/ide-park.c

diff --git a/drivers/ide/Makefile b/drivers/ide/Makefile
index e6e7811..16795fe 100644
--- a/drivers/ide/Makefile
+++ b/drivers/ide/Makefile
@@ -5,7 +5,7 @@
EXTRA_CFLAGS += -Idrivers/ide

ide-core-y += ide.o ide-ioctls.o ide-io.o ide-iops.o ide-lib.o ide-probe.o \
- ide-taskfile.o ide-pio-blacklist.o
+ ide-taskfile.o ide-park.o ide-pio-blacklist.o

# core IDE code
ide-core-$(CONFIG_IDE_TIMINGS) += ide-timings.o
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index e205f46..c9f6325 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -672,7 +672,30 @@ 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:
+ drive->sleep = drive->hwif->park_timeout;
+ drive->dev_flags |= IDE_DFLAG_SLEEPING;
+ complete((struct completion *)rq->end_io_data);
+ 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, (*setfunc)(ide_drive_t *, int) = rq->special;
@@ -692,6 +715,10 @@ static ide_startstop_t ide_special_rq(ide_drive_t *drive, struct request *rq)
ide_end_request(drive, 0, 0);
return ide_stopped;
}
+ task.tf_flags |= IDE_TFLAG_TF | IDE_TFLAG_DEVICE;
+ task.rq = rq;
+ hwif->data_phase = task.data_phase = TASKFILE_NO_DATA;
+ return do_rw_taskfile(drive, &task);
}

static void ide_check_pm_state(ide_drive_t *drive, struct request *rq)
diff --git a/drivers/ide/ide-park.c b/drivers/ide/ide-park.c
new file mode 100644
index 0000000..fd04cb7
--- /dev/null
+++ b/drivers/ide/ide-park.c
@@ -0,0 +1,133 @@
+#include <linux/kernel.h>
+#include <linux/ide.h>
+#include <linux/jiffies.h>
+#include <linux/blkdev.h>
+#include <linux/completion.h>
+
+static void issue_park_cmd(ide_drive_t *drive, unsigned long timeout)
+{
+ ide_hwif_t *hwif = drive->hwif;
+ int i, restart;
+
+ if (!timeout && time_before(hwif->park_timeout, jiffies))
+ return;
+ timeout += jiffies;
+ restart = time_before(timeout, hwif->park_timeout);
+ hwif->park_timeout = timeout;
+
+ for (i = 0; i < MAX_DRIVES; i++) {
+ ide_drive_t *drive = &hwif->drives[i];
+ struct request_queue *q;
+ struct request *rq;
+ DECLARE_COMPLETION_ONSTACK(wait);
+
+ spin_lock_irq(&ide_lock);
+ if (!(drive->dev_flags & IDE_DFLAG_PRESENT) ||
+ ide_device_get(drive)) {
+ spin_unlock_irq(&ide_lock);
+ continue;
+ }
+
+ if (drive->dev_flags & IDE_DFLAG_SLEEPING) {
+ drive->sleep = timeout;
+ spin_unlock_irq(&ide_lock);
+ goto next_step;
+ }
+ spin_unlock_irq(&ide_lock);
+
+ q = drive->queue;
+ rq = blk_get_request(q, READ, __GFP_WAIT);
+ rq->cmd[0] = REQ_PARK_HEADS;
+ rq->cmd_len = 1;
+ rq->cmd_type = REQ_TYPE_SPECIAL;
+ rq->end_io_data = &wait;
+ blk_execute_rq_nowait(q, NULL, rq, 1, NULL);
+
+ /*
+ * This really is only to make sure that the request
+ * has been started yet, not necessarily completed
+ * though.
+ */
+ wait_for_completion(&wait);
+ if (q->rq.count[READ] + q->rq.count[WRITE] <= 1 &&
+ !(drive->dev_flags & IDE_DFLAG_NO_UNLOAD)) {
+ rq = blk_get_request(q, READ, GFP_NOWAIT);
+ if (unlikely(!rq))
+ goto next_step;
+
+ rq->cmd[0] = REQ_UNPARK_HEADS;
+ rq->cmd_len = 1;
+ rq->cmd_type = REQ_TYPE_SPECIAL;
+ elv_add_request(q, rq, ELEVATOR_INSERT_FRONT, 0);
+ }
+
+next_step:
+ ide_device_put(drive);
+ }
+
+ if (restart) {
+ ide_hwgroup_t *hwgroup = hwif->hwgroup;
+
+ spin_lock_irq(&ide_lock);
+ if (hwgroup->sleeping && del_timer(&hwgroup->timer)) {
+ hwgroup->sleeping = 0;
+ hwgroup->busy = 0;
+ __blk_run_queue(drive->queue);
+ }
+ spin_unlock_irq(&ide_lock);
+ }
+}
+
+ide_devset_w_flag(no_unload, IDE_DFLAG_NO_UNLOAD);
+
+ssize_t ide_park_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ ide_drive_t *drive = to_ide_device(dev);
+ ide_hwif_t *hwif = drive->hwif;
+ unsigned int seconds;
+
+ mutex_lock(&ide_setting_mtx);
+ if (drive->dev_flags & IDE_DFLAG_SLEEPING &&
+ time_after(hwif->park_timeout, jiffies))
+ /*
+ * Adding 1 in order to guarantee nonzero value until timer
+ * has actually expired.
+ */
+ seconds = jiffies_to_msecs(hwif->park_timeout - jiffies)
+ / 1000 + 1;
+ else
+ seconds = 0;
+ mutex_unlock(&ide_setting_mtx);
+
+ return snprintf(buf, 20, "%u\n", seconds);
+}
+
+ssize_t ide_park_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+#define MAX_PARK_TIMEOUT 30
+ ide_drive_t *drive = to_ide_device(dev);
+ long int input;
+ int rc;
+
+ rc = strict_strtol(buf, 10, &input);
+ if (rc || input < -2 || input > MAX_PARK_TIMEOUT)
+ return -EINVAL;
+
+ mutex_lock(&ide_setting_mtx);
+ if (input >= 0) {
+ if (drive->dev_flags & IDE_DFLAG_NO_UNLOAD) {
+ mutex_unlock(&ide_setting_mtx);
+ return -EOPNOTSUPP;
+ }
+
+ issue_park_cmd(drive, msecs_to_jiffies(input * 1000));
+ } else
+ /* input can either be -1 or -2 at this point */
+ rc = ide_devset_execute(drive, &ide_devset_no_unload,
+ input + 1);
+ mutex_unlock(&ide_setting_mtx);
+
+ return rc ? rc : len;
+}
diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c
index f5cb55b..0ba2420 100644
--- a/drivers/ide/ide-probe.c
+++ b/drivers/ide/ide-probe.c
@@ -842,6 +842,9 @@ static void ide_port_tune_devices(ide_hwif_t *hwif)

if (hwif->dma_ops)
ide_set_dma(drive);
+
+ if (!ata_id_has_unload(drive->id))
+ drive->dev_flags |= IDE_DFLAG_NO_UNLOAD;
}
}

diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index a4c2d91..f032c96 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -152,7 +152,15 @@ static ide_startstop_t task_no_data_intr(ide_drive_t *drive)

if (!custom)
ide_end_drive_cmd(drive, stat, ide_read_error(drive));
- else if (tf->command == ATA_CMD_SET_MULTI)
+ else if (tf->command == ATA_CMD_IDLEIMMEDIATE) {
+ drive->hwif->tp_ops->tf_read(drive, task);
+ if (tf->lbal != 0xc4) {
+ printk(KERN_ERR "%s: head unload failed!\n",
+ drive->name);
+ ide_tf_dump(drive->name, tf);
+ }
+ ide_end_drive_cmd(drive, stat, ide_read_error(drive));
+ } else if (tf->command == ATA_CMD_SET_MULTI)
drive->mult_count = drive->mult_req;

return ide_stopped;
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index a498245..73caaa8 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -588,6 +588,7 @@ static struct device_attribute ide_dev_attrs[] = {
__ATTR_RO(model),
__ATTR_RO(firmware),
__ATTR(serial, 0400, serial_show, NULL),
+ __ATTR(unload_heads, 0644, ide_park_show, ide_park_store),
__ATTR_NULL
};

diff --git a/include/linux/ide.h b/include/linux/ide.h
index 3eece03..99d8ee1 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -156,6 +156,8 @@ enum {
*/
#define REQ_DRIVE_RESET 0x20
#define REQ_DEVSET_EXEC 0x21
+#define REQ_PARK_HEADS 0x22
+#define REQ_UNPARK_HEADS 0x23

/*
* Check for an interrupt and acknowledge the interrupt status
@@ -571,6 +573,8 @@ enum {
/* retrying in PIO */
IDE_DFLAG_DMA_PIO_RETRY = (1 << 25),
IDE_DFLAG_LBA = (1 << 26),
+ /* don't unload heads */
+ IDE_DFLAG_NO_UNLOAD = (1 << 27),
};

struct ide_drive_s {
@@ -818,6 +822,8 @@ typedef struct hwif_s {
unsigned sharing_irq: 1; /* 1 = sharing irq with another hwif */
unsigned sg_mapped : 1; /* sg_table and sg_nents are ready */

+ unsigned long park_timeout; /* protected by ide_setting_mtx */
+
struct device gendev;
struct device *portdev;

@@ -950,6 +956,10 @@ __IDE_DEVSET(_name, 0, get_##_func, set_##_func)
#define ide_ext_devset_rw_sync(_name, _func) \
__IDE_DEVSET(_name, DS_SYNC, get_##_func, set_##_func)

+#define ide_devset_w_flag(_name, _field) \
+ide_devset_set_flag(_name, _field); \
+IDE_DEVSET(_name, DS_SYNC, NULL, set_##_name)
+
#define ide_decl_devset(_name) \
extern const struct ide_devset ide_devset_##_name

@@ -1198,6 +1208,12 @@ int ide_check_atapi_device(ide_drive_t *, const char *);

void ide_init_pc(struct ide_atapi_pc *);

+/* Disk head parking */
+ssize_t ide_park_show(struct device *dev, struct device_attribute *attr,
+ char *buf);
+ssize_t ide_park_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t len);
+
/*
* Special requests for ide-tape block device strategy routine.
*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/