Skip to contentSkip to conversation

qla4xxx: added srb reference counter

PATCH In review Ravi Anand 2 participants 2 messages Apr 6, 2010 – Apr 7, 2010

Subsystems (2)
SCSI QLogic QLA4XXX iSCSI driver
lore
Share & tools
lore

Ravi Anand

author
#0 lore raw

PATCH 9/12 qla4xxx: added srb reference counter

From: Vikas Chaudhary <[email protected]>

Serialization srb between error handler and command completion path.

Signed-off-by Vikas Chaudhary <[email protected]>
Signed-off-by Ravi Anand <[email protected]>
---
 drivers/scsi/qla4xxx/ql4_def.h  |    2 +-
 drivers/scsi/qla4xxx/ql4_glbl.h |    2 +-
 drivers/scsi/qla4xxx/ql4_isr.c  |    6 ++--
 drivers/scsi/qla4xxx/ql4_os.c   |   50 +++++++++++++++++++++++---------------
 4 files changed, 35 insertions(+), 25 deletions(-)
drivers/scsi/qla4xxx/ql4_def.h +1 -1
index af339be..8ff06f0 100644
@@ -171,7 +171,7 @@ struct srb {
 
 	struct scsi_cmnd *cmd;	/* (4) SCSI command block */
 	dma_addr_t dma_handle;	/* (4) for unmap of single transfers */
-	atomic_t ref_count;	/* reference count for this srb */
+	struct kref srb_ref;	/* reference count for this srb */
 	uint32_t fw_ddb_index;
 	uint8_t err_id;		/* error id */
 #define SRB_ERR_PORT	   1	/* Request failed because "port down" */
drivers/scsi/qla4xxx/ql4_glbl.h +1 -1
index 3c8f753..c4636f6 100644
@@ -66,7 +66,7 @@ void qla4xxx_interrupt_service_routine(struct scsi_qla_host * ha,
 int qla4xxx_init_rings(struct scsi_qla_host * ha);
 struct srb * qla4xxx_del_from_active_array(struct scsi_qla_host *ha,
 					uint32_t index);
-void qla4xxx_srb_compl(struct scsi_qla_host *ha, struct srb *srb);
+void qla4xxx_srb_compl(struct kref *ref);
 int qla4xxx_reinitialize_ddb_list(struct scsi_qla_host * ha);
 int qla4xxx_process_ddb_changed(struct scsi_qla_host *ha, uint32_t fw_ddb_index,
 		uint32_t state, uint32_t conn_error);
drivers/scsi/qla4xxx/ql4_isr.c +3 -3
index ce5838e..596c303 100644
@@ -97,7 +97,7 @@ qla4xxx_status_cont_entry(struct scsi_qla_host *ha,
 
 	/* Place command on done queue. */
 	if (srb->req_sense_len == 0) {
-		qla4xxx_srb_compl(ha, srb);
+		kref_put(&srb->srb_ref, qla4xxx_srb_compl);
 		ha->status_srb = NULL;
 	}
 }

@@ -329,7 +329,7 @@ status_entry_exit:
 	/* complete the request, if not waiting for status_continuation pkt */
 	srb->cc_stat = sts_entry->completionStatus;
 	if (ha->status_srb == NULL)
-		qla4xxx_srb_compl(ha, srb);
+		kref_put(&srb->srb_ref, qla4xxx_srb_compl);
 }
 
 /**

@@ -393,7 +393,7 @@ static void qla4xxx_process_response_queue(struct scsi_qla_host * ha)
 			/* ETRY normally by sending it back with
 			 * DID_BUS_BUSY */
 			srb->cmd->result = DID_BUS_BUSY << 16;
-			qla4xxx_srb_compl(ha, srb);
+			kref_put(&srb->srb_ref, qla4xxx_srb_compl);
 			break;
 
 		case ET_CONTINUE:
drivers/scsi/qla4xxx/ql4_os.c +30 -20

108 lines not on this page · raw message

Signature · 3 lines
1.6.0.2

----- End forwarded message -----
#1 lore raw
Quoted 16 lines · Ravi Anand
drivers/scsi/qla4xxx/ql4_os.c +1 -1
@@ -888,7 +890,7 @@ static void qla4xxx_flush_active_srbs(struct scsi_qla_host *ha)
  		srb = qla4xxx_del_from_active_array(ha, i);
  		if (srb != NULL) {
  			srb->cmd->result = DID_RESET<<  16;
-			qla4xxx_srb_compl(ha, srb);
+			kref_put(&srb->srb_ref, qla4xxx_srb_compl);
  		}

*
* This routine waits for the command to be returned by the Firmware

drivers/scsi/qla4xxx/ql4_os.c +0 -1
@@ -1507,17 +1509,14 @@ static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha,
  				      struct scsi_cmnd *cmd)
  {
  	int done = 0;
-	struct srb *rp;

drivers/scsi/qla4xxx/ql4_os.c static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha, · line 1519

Quoted 10 lines
drivers/scsi/qla4xxx/ql4_os.c +1 -2
  	uint32_t max_wait_time = EH_WAIT_CMD_TOV;
 
  	do {
  		/* Checking to see if its returned to OS */
-		rp = (struct srb *) cmd->SCp.ptr;
-		if (rp == NULL) {
+		if (cmd->host_scribble == NULL) {
  			done++;
  			break;
  		}

Is this racey? It seems like qla4xxx_flush_active_srbs will call qla4xxx_del_from_active_array and that will clear host_scribble, and here we will see it is NULL and proceed to complete the eh abort process. When we get back to qla4xxx_eh_abort we could call kref_put(&srb->srb_ref, qla4xxx_srb_compl) and then return SUCCESS to the scsi eh. The scsi eh will then begin to reuse the scsi cmd to send a TUR. But I think the problem could be that if happens before qla4xxx_flush_active_srbs has called its kref_put, then when qla4xxx_flush_active_srbs now calls it it is going to call qla4xxx_srb_compl and the driver will be completing a completely different command (it would now be the TUR).

I thought I had said this before, but it looks like I did not get a reply. I think you want to drop the kref coming into qla4xxx_eh_wait_on_command because nothing is touching the srb now. And then you want to add some locking in around the host_scribble checking maybe.