eh_active checks in qla2xxx error handling

DISCUSSION Christoph Hellwig 3 participants 4 messages Sep 6, 2005 – Oct 28, 2005

Subsystems (2)

SCSI QLogic QLA2XXX FC-SCSI driver

lore

Share & tools

lore

Christoph Hellwig

author

2005-09-06 12:20 UTC #0 lore raw

qla2xxx is the only driver looking at eh_active, and doing the wait for oustanding commands only from EH thread resets but not from the ioctls looks rather wrong to me. Do you remember the rational for it?

Christoph Hellwig

author

2005-09-19 20:05 UTC #1 lore raw

Quoted 3 lines · Christoph Hellwig

qla2xxx is the only driver looking at eh_active, and doing the wait for oustanding commands only from EH thread resets but not from the ioctls looks rather wrong to me. Do you remember the rational for it?

ping?

Andrew Vasquez

2005-09-20 20:25 UTC #2 lore raw

In reply to Christoph Hellwig

Quoted 3 lines · Christoph Hellwig

qla2xxx is the only driver looking at eh_active, and doing the wait for oustanding commands only from EH thread resets but not from the ioctls looks rather wrong to me. Do you remember the rational for it?

As Duane mentioned, this looks to be some residual from the 2.4 era which unfortunately I've yet to find a reasonably valid justification for...Alas, another loss of history...

---
Here's a patch which drops the eh_active checks in the qla2xxx eh_handler callbacks.

Signed-off-by Andrew Vasquez <[email protected]>

drivers/scsi/qla2xxx/qla_os.c +15 ~~-26~~

@@ -797,29 +797,19 @@ qla2xxx_eh_device_reset(struct scsi_cmnd
 		goto eh_dev_reset_done;
 	}
 
-	/*
-	 * If we are coming down the EH path, wait for all commands to
-	 * complete for the device.
-	 */
-	if (cmd->device->host->eh_active) {
-		if (qla2x00_eh_wait_for_pending_target_commands(ha, id))
-			ret = FAILED;
-
-		if (ret == FAILED) {
-			DEBUG3(printk("%s(%ld): failed while waiting for "
-			    "commands\n", __func__, ha->host_no));
-			qla_printk(KERN_INFO, ha,
-			    "%s: failed while waiting for commands\n",
-			    __func__);
-
-			goto eh_dev_reset_done;
-		}
-	}
-
-	qla_printk(KERN_INFO, ha,
-	    "scsi(%ld:%d:%d): DEVICE RESET SUCCEEDED.\n", ha->host_no, id, lun);
+	/* Flush outstanding commands. */
+	if (qla2x00_eh_wait_for_pending_target_commands(ha, id))
+		ret = FAILED;
+	if (ret == FAILED) {
+		DEBUG3(printk("%s(%ld): failed while waiting for commands\n",
+		    __func__, ha->host_no));
+		qla_printk(KERN_INFO, ha,
+		    "%s: failed while waiting for commands\n", __func__);
+	} else
+		qla_printk(KERN_INFO, ha,
+		    "scsi(%ld:%d:%d): DEVICE RESET SUCCEEDED.\n", ha->host_no,
+		    id, lun);
 
-eh_dev_reset_done:
 	return ret;
 }

@@ -921,10 +911,9 @@ qla2xxx_eh_bus_reset(struct scsi_cmnd *c
 	if (ret == FAILED)
 		goto eh_bus_reset_done;
 
-	/* Waiting for our command in done_queue to be returned to OS.*/
-	if (cmd->device->host->eh_active)
-		if (!qla2x00_eh_wait_for_pending_commands(ha))
-			ret = FAILED;
+	/* Flush outstanding commands. */
+	if (!qla2x00_eh_wait_for_pending_commands(ha))
+		ret = FAILED;
 
 eh_bus_reset_done:
 	qla_printk(KERN_INFO, ha, "%s: reset %s\n", __func__,

Christoph Hellwig

2005-10-28 22:48 UTC #3 lore raw

Quoted 10 lines · Andrew Vasquez

Quoted 3 lines · Christoph Hellwig
qla2xxx is the only driver looking at eh_active, and doing the wait for oustanding commands only from EH thread resets but not from the ioctls looks rather wrong to me. Do you remember the rational for it?
As Duane mentioned, this looks to be some residual from the 2.4 era which unfortunately I've yet to find a reasonably valid justification for...Alas, another loss of history...

Quoted 5 lines

---
Here's a patch which drops the eh_active checks in the qla2xxx eh_handler callbacks.
Signed-off-by Andrew Vasquez <[email protected]>

James, can you put this patch in? I have a patch pending that depends on it.

drivers/scsi/qla2xxx/qla_os.c +15 ~~-26~~

@@ -797,29 +797,19 @@ qla2xxx_eh_device_reset(struct scsi_cmnd
 		goto eh_dev_reset_done;
 	}
 
-	/*
-	 * If we are coming down the EH path, wait for all commands to
-	 * complete for the device.
-	 */
-	if (cmd->device->host->eh_active) {
-		if (qla2x00_eh_wait_for_pending_target_commands(ha, id))
-			ret = FAILED;
-
-		if (ret == FAILED) {
-			DEBUG3(printk("%s(%ld): failed while waiting for "
-			    "commands\n", __func__, ha->host_no));
-			qla_printk(KERN_INFO, ha,
-			    "%s: failed while waiting for commands\n",
-			    __func__);
-
-			goto eh_dev_reset_done;
-		}
-	}
-
-	qla_printk(KERN_INFO, ha,
-	    "scsi(%ld:%d:%d): DEVICE RESET SUCCEEDED.\n", ha->host_no, id, lun);
+	/* Flush outstanding commands. */
+	if (qla2x00_eh_wait_for_pending_target_commands(ha, id))
+		ret = FAILED;
+	if (ret == FAILED) {
+		DEBUG3(printk("%s(%ld): failed while waiting for commands\n",
+		    __func__, ha->host_no));
+		qla_printk(KERN_INFO, ha,
+		    "%s: failed while waiting for commands\n", __func__);
+	} else
+		qla_printk(KERN_INFO, ha,
+		    "scsi(%ld:%d:%d): DEVICE RESET SUCCEEDED.\n", ha->host_no,
+		    id, lun);
 
-eh_dev_reset_done:
 	return ret;
 }

@@ -921,10 +911,9 @@ qla2xxx_eh_bus_reset(struct scsi_cmnd *c
 	if (ret == FAILED)
 		goto eh_bus_reset_done;
 
-	/* Waiting for our command in done_queue to be returned to OS.*/
-	if (cmd->device->host->eh_active)
-		if (!qla2x00_eh_wait_for_pending_commands(ha))
-			ret = FAILED;
+	/* Flush outstanding commands. */
+	if (!qla2x00_eh_wait_for_pending_commands(ha))
+		ret = FAILED;
 
 eh_bus_reset_done:
 	qla_printk(KERN_INFO, ha, "%s: reset %s\n", __func__,

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html