lkml.org 
[lkml]   [2008]   [Sep]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH 3/4] ide: Implement disk shock protection support
    Date
    Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> 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@nebensachen.de>
    >
    > [ 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.
    *

    \
     
     \ /
      Last update: 2008-09-03 22:07    [W:2.558 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site