by Elias Oltmanns

[permalink] [raw]

Subject: [PATCH 4/4 v2] IDE: Report errors during drive reset back to user space

Make sure that each error condition during the execution of an
HDIO_DRIVE_RESET ioctl is actually reported to the calling process.
Also, unify the exit path of reset_pollfunc() when returning ide_stopped
since the need of ->port_ops->reset_poll() to be treated specially has
vanished (way back, it seems).

Signed-off-by: Elias Oltmanns <[email protected]>
---

Documentation/ioctl/hdio.txt | 2 ++
drivers/ide/ide-iops.c | 18 +++++++++++-------
drivers/ide/ide.c | 10 ++++++----
drivers/ide/pci/siimage.c | 3 +--
4 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/Documentation/ioctl/hdio.txt b/Documentation/ioctl/hdio.txt
index 44d283d..91a6ecb 100644
--- a/Documentation/ioctl/hdio.txt
+++ b/Documentation/ioctl/hdio.txt
@@ -508,6 +508,8 @@ HDIO_DRIVE_RESET execute a device reset

error returns:
EACCES Access denied: requires CAP_SYS_ADMIN
+ ENXIO No such device: phy dead or ctl_addr == 0
+ EIO I/O error: reset timed out or hardware error

notes:

diff --git a/drivers/ide/ide-iops.c b/drivers/ide/ide-iops.c
index 80e782b..6a8b955 100644
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -905,12 +905,12 @@ void ide_execute_pkt_cmd(ide_drive_t *drive)
}
EXPORT_SYMBOL_GPL(ide_execute_pkt_cmd);

-static inline void ide_complete_drive_reset(ide_drive_t *drive)
+static inline void ide_complete_drive_reset(ide_drive_t *drive, int err)
{
struct request *rq = drive->hwif->hwgroup->rq;

if (rq && blk_special_request(rq) && rq->cmd[0] == REQ_DRIVE_RESET)
- ide_end_request(drive, 1, 0);
+ ide_end_request(drive, err ? err : 1, 0);
}

/* needed below */
@@ -948,7 +948,7 @@ static ide_startstop_t atapi_reset_pollfunc (ide_drive_t *drive)
}
/* done polling */
hwgroup->polling = 0;
- ide_complete_drive_reset(drive);
+ ide_complete_drive_reset(drive, 0);
return ide_stopped;
}

@@ -964,9 +964,11 @@ static ide_startstop_t reset_pollfunc (ide_drive_t *drive)
ide_hwif_t *hwif = HWIF(drive);
const struct ide_port_ops *port_ops = hwif->port_ops;
u8 tmp;
+ int err = 0;

if (port_ops && port_ops->reset_poll) {
- if (port_ops->reset_poll(drive)) {
+ err = port_ops->reset_poll(drive);
+ if (err) {
printk(KERN_ERR "%s: host reset_poll failure for %s.\n",
hwif->name, drive->name);
goto out;
@@ -983,6 +985,7 @@ static ide_startstop_t reset_pollfunc (ide_drive_t *drive)
}
printk("%s: reset timed-out, status=0x%02x\n", hwif->name, tmp);
drive->failures++;
+ err = -EIO;
} else {
printk("%s: reset: ", hwif->name);
tmp = ide_read_error(drive);
@@ -1009,11 +1012,12 @@ static ide_startstop_t reset_pollfunc (ide_drive_t *drive)
if (tmp & 0x80)
printk("; slave: failed");
printk("\n");
+ err = -EIO;
}
}
- hwgroup->polling = 0; /* done polling */
out:
- ide_complete_drive_reset(drive);
+ hwgroup->polling = 0; /* done polling */
+ ide_complete_drive_reset(drive, err);
return ide_stopped;
}

@@ -1120,7 +1124,7 @@ static ide_startstop_t do_reset1 (ide_drive_t *drive, int do_not_try_atapi)

if (io_ports->ctl_addr == 0) {
spin_unlock_irqrestore(&ide_lock, flags);
- ide_complete_drive_reset(drive);
+ ide_complete_drive_reset(drive, -ENXIO);
return ide_stopped;
}

diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index dbedb02..dfdc48a 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -529,17 +529,20 @@ static int generic_ide_resume(struct device *dev)
return err;
}

-static void generic_drive_reset(ide_drive_t *drive)
+static int generic_drive_reset(ide_drive_t *drive)
{
struct request *rq;
+ int ret = 0;

rq = blk_get_request(drive->queue, READ, __GFP_WAIT);
rq->cmd_type = REQ_TYPE_SPECIAL;
rq->cmd_len = 1;
rq->cmd[0] = REQ_DRIVE_RESET;
rq->cmd_flags |= REQ_SOFTBARRIER;
- blk_execute_rq(drive->queue, NULL, rq, 1);
+ if (blk_execute_rq(drive->queue, NULL, rq, 1))
+ ret = rq->errors;
blk_put_request(rq);
+ return ret;
}

int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device *bdev,
@@ -616,8 +619,7 @@ int generic_ide_ioctl(ide_drive_t *drive, struct file *file, struct block_device
if (!capable(CAP_SYS_ADMIN))
return -EACCES;

- generic_drive_reset(drive);
- return 0;
+ return generic_drive_reset(drive);

case HDIO_GET_BUSSTATE:
if (!capable(CAP_SYS_ADMIN))
diff --git a/drivers/ide/pci/siimage.c b/drivers/ide/pci/siimage.c
index b75e9bb..6e9d765 100644
--- a/drivers/ide/pci/siimage.c
+++ b/drivers/ide/pci/siimage.c
@@ -421,8 +421,7 @@ static int sil_sata_reset_poll(ide_drive_t *drive)
if ((sata_stat & 0x03) != 0x03) {
printk(KERN_WARNING "%s: reset phy dead, status=0x%08x\n",
hwif->name, sata_stat);
- HWGROUP(drive)->polling = 0;
- return ide_started;
+ return -ENXIO;
}
}

2008-06-25 20:34:00

by Bartlomiej Zolnierkiewicz

[permalink] [raw]

Subject: Re: [PATCH] IDE: Fix HDIO_DRIVE_RESET handling

On Wednesday 25 June 2008, Elias Oltmanns wrote:
> "Bartlomiej Zolnierkiewicz" <[email protected]> wrote:
> > On Tue, Jun 24, 2008 at 3:35 PM, Alan Cox <[email protected]> wrote:
> >>> > I don't see why you think it's "hard". We have timeout handlers for many
> >
> >>> > commands and those reset/abort just fine.
> >>>
> >>> They are different beasts from user-space initiated abort operation
> >>
> >> No they are not. They are the *same* thing in every respect.
> >>
> >> You have the drive in an unknown state, you want it back. If your drive
> >> lost a command due to noise or a firmware flaw you have no idea about the
> >> state it is actually in (supposed to be is irrelevant)
> >
> > I generally agree with you w.r.t. to drive side of the operations but
> > the drive is only part of the equation (the host and the request states
> > are the others) so 'supposed to be is' is quite relevant.
> >
> > Also abort request can happen i.e. while the command is being prepared
> > & issued (it is done without ide_lock being taken and the timeout is not
> > even armed yet) so there are additional issues to take care of.
>
> Yes there are. Still, I think it should be feasible which is why I
> personally would prefer to drop the second patch in the series for the
> time being. But then I can keep it around for reference locally and the
> original infrastructure can be found in the history after all.
>
> Even though the patch series currently doesn't fully restore the
> intended functionality, I'd like to merge it now. Command aborting

Thanks, I applied everything and queued it for 2.6.27.

[ including patch #2, we should re-add aborting when fixed/necessary ]

> didn't work reliably (if at all) before and now, at least, a simple
> ioctl won't harm a healthy system anymore. Since I think that we can add
> command aborting back later, I'd like to keep the HDIO_DRIVE_RESET ioctl
> even if it should currently be superfluous given SG_IO.

SG_IO ATA pass-through is unsupported currently in drivers/ide/
(though nowadays it should be quite easy to add it if somebody is
interested) so it is indeed the best to leave the ioctl for now.

Bart