On 2004.03.05 03:40, Jens Axboe wrote:
> Hi Willem
>
> Sorry for the (too long) delay. I just looked closer at your patch
> today, and it is an improvement. The code I looked at is the one that is
> merged in 2.6.4-rc1-mm2, jfyi.
>
> A few comments:
>
> - Don't even BUG() in error handling. BUG() should only be used for
> system consistency problems, not a silly tape driver going nuts. Use
> WARN_ON() and maybe add a printk before that if you need to, and then
> gracefully leave.
>
> - You need to set task state before calling schedule_timeout(),
> otherwise your nice loop turns into a nasty busy loop. So something
> ala:
>
> do {
> set_current_state(TASK_UNINTERRUPTIBLE);
> spin_unlock_irq(cmd->device->host->host_lock);
> schedule_timeout(HZ / 20);
> spin_lock_irq(cmd->device->host->host_lock);
> } while (HWGROUP(drive)->handler);
>
> (moved the lock as well, for ->handler check)
>
> - The drive queue nuking code isn't completely correct with the
> anticipatory io scheduler. I'll get this fixed up, basically we need
> to mark the queue as dead first to force it to return what it has
> always (it could return NULL for anticipation issues, while there
> still are requests in the queue).
>
> - The expiry() addition is nice.
>
> - idescsi_dump_status() should probably be in ide-*.c somewhere, called
> ide_atapi_dump_status() and reused by ide-cd and ide-scsi.
>
> If you can fix these up, then I think the patch should go into -linus
> right away. Just leave the queue nuking stuff, I'll apply that on top of
> your fix.
I trust the patch below meets your recommendations (it works for me).
This patch (against 2.6.4-rc1-mm2) does:
- move ide_cdrom_dump_status() from ide-cd.c to ide-lib.c as
ide_dump_atapi_status() and both ide-cd and ide-scsi call it.
- replaces BUG() by WARN_ON()/printk in the error handling code.
- sets TASK_UNINTERRUPTIBLE before schedule_timeout() and moves the host
unlock/lock around the while loop inside the loop in idescsi_eh_reset().
Regards, Willem Riede.
diff -ur m0/drivers/ide/ide-cd.c m2/drivers/ide/ide-cd.c
--- m0/drivers/ide/ide-cd.c 2004-03-05 19:53:28.000000000 -0500
+++ m2/drivers/ide/ide-cd.c 2004-03-07 09:54:08.000000000 -0500
@@ -554,53 +554,6 @@
(void) ide_do_drive_cmd(drive, rq, ide_preempt);
}
-
-/*
- * Error reporting, in human readable form (luxurious, but a memory hog).
- */
-byte ide_cdrom_dump_status (ide_drive_t *drive, const char *msg, byte stat)
-{
- unsigned long flags;
-
- atapi_status_t status;
- atapi_error_t error;
-
- status.all = stat;
- local_irq_set(flags);
- printk("%s: %s: status=0x%02x", drive->name, msg, stat);
-#if FANCY_STATUS_DUMPS
- printk(" { ");
- if (status.b.bsy)
- printk("Busy ");
- else {
- if (status.b.drdy) printk("DriveReady ");
- if (status.b.df) printk("DeviceFault ");
- if (status.b.dsc) printk("SeekComplete ");
- if (status.b.drq) printk("DataRequest ");
- if (status.b.corr) printk("CorrectedError ");
- if (status.b.idx) printk("Index ");
- if (status.b.check) printk("Error ");
- }
- printk("}");
-#endif /* FANCY_STATUS_DUMPS */
- printk("\n");
- if ((status.all & (status.b.bsy|status.b.check)) == status.b.check) {
- error.all = HWIF(drive)->INB(IDE_ERROR_REG);
- printk("%s: %s: error=0x%02x", drive->name, msg, error.all);
-#if FANCY_STATUS_DUMPS
- if (error.b.ili) printk("IllegalLengthIndication ");
- if (error.b.eom) printk("EndOfMedia ");
- if (error.b.abrt) printk("Aborted Command ");
- if (error.b.mcr) printk("MediaChangeRequested ");
- if (error.b.sense_key) printk("LastFailedSense 0x%02x ",
- error.b.sense_key);
-#endif /* FANCY_STATUS_DUMPS */
- printk("\n");
- }
- local_irq_restore(flags);
- return error.all;
-}
-
/*
* ide_error() takes action based on the error returned by the drive.
*/
@@ -609,7 +562,7 @@
struct request *rq;
byte err;
- err = ide_cdrom_dump_status(drive, msg, stat);
+ err = ide_dump_atapi_status(drive, msg, stat);
if (drive == NULL || (rq = HWGROUP(drive)->rq) == NULL)
return ide_stopped;
/* retry only "normal" I/O: */
@@ -3411,7 +3364,7 @@
.supports_dsc_overlap = 1,
.cleanup = ide_cdrom_cleanup,
.do_request = ide_do_rw_cdrom,
- .sense = ide_cdrom_dump_status,
+ .sense = ide_dump_atapi_status,
.error = ide_cdrom_error,
.abort = ide_cdrom_abort,
.capacity = ide_cdrom_capacity,
diff -ur m0/drivers/ide/ide-lib.c m2/drivers/ide/ide-lib.c
--- m0/drivers/ide/ide-lib.c 2004-02-17 22:59:44.000000000 -0500
+++ m2/drivers/ide/ide-lib.c 2004-03-07 09:52:34.000000000 -0500
@@ -447,3 +447,55 @@
EXPORT_SYMBOL_GPL(ide_set_xfer_rate);
+/**
+ * ide_dump_atapi_status - print human readable atapi status
+ * @drive: drive that status applies to
+ * @msg: text message to print
+ * @stat: status byte to decode
+ *
+ * Error reporting, in human readable form (luxurious, but a memory hog).
+ */
+byte ide_dump_atapi_status (ide_drive_t *drive, const char *msg, byte stat)
+{
+ unsigned long flags;
+
+ atapi_status_t status;
+ atapi_error_t error;
+
+ status.all = stat;
+ local_irq_set(flags);
+ printk("%s: %s: status=0x%02x", drive->name, msg, stat);
+#if FANCY_STATUS_DUMPS
+ printk(" { ");
+ if (status.b.bsy)
+ printk("Busy ");
+ else {
+ if (status.b.drdy) printk("DriveReady ");
+ if (status.b.df) printk("DeviceFault ");
+ if (status.b.dsc) printk("SeekComplete ");
+ if (status.b.drq) printk("DataRequest ");
+ if (status.b.corr) printk("CorrectedError ");
+ if (status.b.idx) printk("Index ");
+ if (status.b.check) printk("Error ");
+ }
+ printk("}");
+#endif /* FANCY_STATUS_DUMPS */
+ printk("\n");
+ if ((status.all & (status.b.bsy|status.b.check)) == status.b.check) {
+ error.all = HWIF(drive)->INB(IDE_ERROR_REG);
+ printk("%s: %s: error=0x%02x", drive->name, msg, error.all);
+#if FANCY_STATUS_DUMPS
+ if (error.b.ili) printk("IllegalLengthIndication ");
+ if (error.b.eom) printk("EndOfMedia ");
+ if (error.b.abrt) printk("Aborted Command ");
+ if (error.b.mcr) printk("MediaChangeRequested ");
+ if (error.b.sense_key) printk("LastFailedSense 0x%02x ",
+ error.b.sense_key);
+#endif /* FANCY_STATUS_DUMPS */
+ printk("\n");
+ }
+ local_irq_restore(flags);
+ return error.all;
+}
+
+EXPORT_SYMBOL(ide_dump_atapi_status);
diff -ur m0/drivers/scsi/ide-scsi.c m2/drivers/scsi/ide-scsi.c
--- m0/drivers/scsi/ide-scsi.c 2004-03-05 19:54:33.000000000 -0500
+++ m2/drivers/scsi/ide-scsi.c 2004-03-07 09:57:07.000000000 -0500
@@ -308,60 +308,12 @@
return ide_do_drive_cmd(drive, rq, ide_preempt);
}
-/* Code derived from ide_cdrom_error()/abort() -- see ide-cd.c */
-
-/*
- * Error reporting, in human readable form (luxurious, but a memory hog).
- */
-byte idescsi_dump_status (ide_drive_t *drive, const char *msg, byte stat)
-{
- unsigned long flags;
-
- atapi_status_t status;
- atapi_error_t error;
-
- status.all = stat;
- local_irq_set(flags);
- printk(KERN_WARNING "ide-scsi: %s: %s: status=0x%02x", drive->name, msg, stat);
-#if IDESCSI_DEBUG_LOG
- printk(" { ");
- if (status.b.bsy)
- printk("Busy ");
- else {
- if (status.b.drdy) printk("DriveReady ");
- if (status.b.df) printk("DeviceFault ");
- if (status.b.dsc) printk("SeekComplete ");
- if (status.b.drq) printk("DataRequest ");
- if (status.b.corr) printk("CorrectedError ");
- if (status.b.idx) printk("Index ");
- if (status.b.check) printk("Error ");
- }
- printk("}");
-#endif /* IDESCSI_DEBUG_LOG */
- printk("\n");
- if ((status.all & (status.b.bsy|status.b.check)) == status.b.check) {
- error.all = HWIF(drive)->INB(IDE_ERROR_REG);
- printk(KERN_WARNING "ide-scsi: %s: %s: error=0x%02x", drive->name, msg, error.all);
-#if IDESCSI_DEBUG_LOG
- if (error.b.ili) printk("IllegalLengthIndication ");
- if (error.b.eom) printk("EndOfMedia ");
- if (error.b.abrt) printk("Aborted Command ");
- if (error.b.mcr) printk("MediaChangeRequested ");
- if (error.b.sense_key) printk("LastFailedSense 0x%02x ",
- error.b.sense_key);
-#endif /* IDESCSI_DEBUG_LOG */
- printk("\n");
- }
- local_irq_restore(flags);
- return error.all;
-}
-
ide_startstop_t idescsi_atapi_error (ide_drive_t *drive, const char *msg, byte stat)
{
struct request *rq;
byte err;
- err = idescsi_dump_status(drive, msg, stat);
+ err = ide_dump_atapi_status(drive, msg, stat);
if (drive == NULL || (rq = HWGROUP(drive)->rq) == NULL)
return ide_stopped;
@@ -980,7 +932,8 @@
printk (KERN_WARNING "ide-scsi: abort called for %lu\n", cmd->serial_number);
if (!drive) {
- BUG();
+ printk (KERN_WARNING "ide-scsi: Drive not set in idescsi_eh_abort\n");
+ WARN_ON(1);
goto no_drive;
}
@@ -1039,14 +992,15 @@
printk (KERN_WARNING "ide-scsi: reset called for %lu\n", cmd->serial_number);
if (!drive) {
- BUG();
+ printk (KERN_WARNING "ide-scsi: Drive not set in idescsi_eh_reset\n");
+ WARN_ON(1);
return FAILED;
}
spin_lock_irq(&ide_lock);
if (!scsi->pc || (req = scsi->pc->rq) != HWGROUP(drive)->rq || !HWGROUP(drive)->handler) {
- BUG();
+ printk (KERN_WARNING "ide-scsi: No active request in idescsi_eh_reset\n");
spin_unlock(&ide_lock);
return FAILED;
}
@@ -1074,14 +1028,14 @@
ide_do_reset(drive);
- spin_unlock_irq(cmd->device->host->host_lock);
-
/* ide_do_reset starts a polling handler which restarts itself every 50ms until the reset finishes */
- do
- /* There should be no locks taken at this point */
+ do {
+ set_current_state(TASK_UNINTERRUPTIBLE);
+ spin_unlock_irq(cmd->device->host->host_lock);
schedule_timeout(HZ/20);
- while ( HWGROUP(drive)->handler );
+ spin_lock_irq(cmd->device->host->host_lock);
+ } while ( HWGROUP(drive)->handler );
ready = drive_is_ready(drive);
HWGROUP(drive)->busy--;
@@ -1090,8 +1044,6 @@
ret = FAILED;
}
- spin_lock_irq(cmd->device->host->host_lock);
-
return ret;
}
diff -ur m0/include/linux/ide.h m2/include/linux/ide.h
--- m0/include/linux/ide.h 2004-03-05 19:54:34.000000000 -0500
+++ m2/include/linux/ide.h 2004-03-07 10:17:30.000000000 -0500
@@ -1614,6 +1614,7 @@
extern char *ide_xfer_verbose(u8 xfer_rate);
extern void ide_toggle_bounce(ide_drive_t *drive, int on);
extern int ide_set_xfer_rate(ide_drive_t *drive, u8 rate);
+extern byte ide_dump_atapi_status(ide_drive_t *drive, const char *msg, byte stat);
typedef struct ide_pio_timings_s {
int setup_time; /* Address setup (ns) minimum */