[PATCH 06/11] qla4xxx: added srb referance counter

[PATCH 06/11] qla4xxx: added srb referance counter

[Ravi Anand]

From: Vikas Chaudhary <vikas.chaudhary@qlogic.com>

Serialization between the error handler and recovery code.

Signed-off-by: Karen Higgins <karen.higgins@qlogic.com>
Signed-off-by: Vikas Chaudhary <vikas.chaudhary@qlogic.com>
Signed-off-by: Ravi Anand <ravi.anand@qlogic.com>
---
 drivers/scsi/qla4xxx/ql4_glbl.h |    2 +
 drivers/scsi/qla4xxx/ql4_isr.c  |    6 ++--
 drivers/scsi/qla4xxx/ql4_os.c   |   58 +++++++++++++++++++++++++++++++++++----
 3 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/qla4xxx/ql4_glbl.h b/drivers/scsi/qla4xxx/ql4_glbl.h
index 96ebfb0..6400714 100644
--- a/drivers/scsi/qla4xxx/ql4_glbl.h
+++ b/drivers/scsi/qla4xxx/ql4_glbl.h
@@ -73,6 +73,8 @@ void qla4xxx_dump_buffer(void *b, uint32_t size);
 int qla4xxx_send_marker_iocb(struct scsi_qla_host *ha,
 	struct ddb_entry *ddb_entry, int lun, uint16_t mrkr_mod);
 
+void sp_put(struct scsi_qla_host *ha, struct srb *sp);
+
 extern int ql4xextended_error_logging;
 extern int ql4xdiscoverywait;
 extern int ql4xdontresethba;
diff --git a/drivers/scsi/qla4xxx/ql4_isr.c b/drivers/scsi/qla4xxx/ql4_isr.c
index 36e6e85..21c4d02 100644
--- a/drivers/scsi/qla4xxx/ql4_isr.c
+++ b/drivers/scsi/qla4xxx/ql4_isr.c
@@ -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);
+		sp_put(ha, srb);
 		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);
+		sp_put(ha, srb);
 }
 
 /**
@@ -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);
+			sp_put(ha, srb);
 			break;
 
 		case ET_CONTINUE:
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index 97d35fd..9057860 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -372,6 +372,16 @@ void qla4xxx_mark_device_missing(struct scsi_qla_host *ha,
 	iscsi_conn_error_event(ddb_entry->conn, ISCSI_ERR_CONN_FAILED);
 }
 
+/***
+ * qla4xxx_get_new_srb - Allocate memory for a local srb.
+ * @ha: Pointer to host adapter structure.
+ * @ddb_entry: Pointer to device database entry
+ * @cmd: Pointer to Linux's SCSI command structure
+ * @done: Pointer to Linux's SCSI mid-layer done function
+ *
+ * NOTE: Sets te ref_count for non-NULL srb to one,
+ *       and initializes some fields.
+ **/
 static struct srb* qla4xxx_get_new_srb(struct scsi_qla_host *ha,
 				       struct ddb_entry *ddb_entry,
 				       struct scsi_cmnd *cmd,
@@ -417,6 +427,33 @@ void qla4xxx_srb_compl(struct scsi_qla_host *ha, struct srb *srb)
 }
 
 /**
+ * sp_put - Decrement reference count and call callback.
+ * @ha: Pointer to host adapter structure.
+ * @sp: Pointer to srb structure
+ **/
+void sp_put(struct scsi_qla_host *ha, struct srb *sp)
+{
+	if (atomic_read(&sp->ref_count) == 0) {
+		DEBUG2(printk("%s: SP->ref_count ZERO\n", __func__));
+		DEBUG2(BUG());
+		return;
+	}
+	if (!atomic_dec_and_test(&sp->ref_count)) {
+		return;
+	}
+	qla4xxx_srb_compl(ha, sp);
+}
+
+/**
+ * sp_get - Increment reference count of the specified sp.
+ * @sp: Pointer to srb structure
+ **/
+void sp_get(struct srb *sp)
+{
+	atomic_inc(&sp->ref_count);
+}
+
+/**
  * qla4xxx_queuecommand - scsi layer issues scsi command to driver.
  * @cmd: Pointer to Linux's SCSI command structure
  * @done_fn: Function that the driver calls to notify the SCSI mid-layer
@@ -886,7 +923,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);
+			sp_put(ha, srb);
 		}
 	}
 	spin_unlock_irqrestore(&ha->hardware_lock, flags);
@@ -1462,16 +1499,20 @@ struct srb * qla4xxx_del_from_active_array(struct scsi_qla_host *ha, uint32_t in
  * qla4xxx_eh_wait_on_command - waits for command to be returned by firmware
  * @ha: actual ha whose done queue will contain the comd returned by firmware.
  * @cmd: Scsi Command to wait on.
+ * @got_ref: Additional reference retrieved by caller.
  *
  * This routine waits for the command to be returned by the Firmware
  * for some max time.
  **/
 static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha,
-				      struct scsi_cmnd *cmd)
+				      struct scsi_cmnd *cmd, int got_ref)
 {
+#define ABORT_POLLING_PERIOD	1000
+#define ABORT_WAIT_ITER		1
+
 	int done = 0;
 	struct srb *rp;
-	uint32_t max_wait_time = EH_WAIT_CMD_TOV;
+	unsigned long wait_iter = ABORT_WAIT_ITER;
 
 	do {
 		/* Checking to see if its returned to OS */
@@ -1481,8 +1522,13 @@ static int qla4xxx_eh_wait_on_command(struct scsi_qla_host *ha,
 			break;
 		}
 
-		msleep(2000);
-	} while (max_wait_time--);
+		if (got_ref && (atomic_read(&rp->ref_count) == 1)) {
+			done++;
+			break;
+		}
+
+		msleep(ABORT_POLLING_PERIOD);
+	} while (!(--wait_iter));
 
 	return done;
 }
@@ -1534,7 +1580,7 @@ static int qla4xxx_eh_wait_for_commands(struct scsi_qla_host *ha,
 		cmd = scsi_host_find_tag(ha->host, cnt);
 		if (cmd && stgt == scsi_target(cmd->device) &&
 		    (!sdev || sdev == cmd->device)) {
-			if (!qla4xxx_eh_wait_on_command(ha, cmd)) {
+			if (!qla4xxx_eh_wait_on_command(ha, cmd, 0)) {
 				status++;
 				break;
 			}
-- 
1.6.0.2

Re: [PATCH 06/11] qla4xxx: added srb referance counter

[Mike Christie]

On 01/30/2010 12:28 AM, Ravi Anand wrote:
>
> -		msleep(2000);
> -	} while (max_wait_time--);
> +		if (got_ref&&  (atomic_read(&rp->ref_count) == 1)) {
> +			done++;
> +			break;
> +		}
> +
> +		msleep(ABORT_POLLING_PERIOD);


Did you want to use krefs for the refcounting?

And why is this so funky (got_ref arg and refcount peak) compared to the 
qla2xxx one?

Re: [PATCH 06/11] qla4xxx: added srb referance counter

[Ravi Anand]

On Mon, 01 Feb 2010, Mike Christie wrote:
> 
> On 01/30/2010 12:28 AM, Ravi Anand wrote:
> >
> > -		msleep(2000);
> > -	} while (max_wait_time--);
> > +		if (got_ref&&  (atomic_read(&rp->ref_count) == 1)) {
> > +			done++;
> > +			break;
> > +		}
> > +
> > +		msleep(ABORT_POLLING_PERIOD);
> 
> 
> Did you want to use krefs for the refcounting?

We will add it to our to do list and submit a patch later on.
For right now we will like to stick to it as kref will require
additional testing.

> And why is this so funky (got_ref arg and refcount peak) compared to the 
> qla2xxx one?

I don't think qla2xxx is doing any reference counting in eh_abort() path. 
Basically its trying to differentiate for case where it takes an additional
reference when the cmd is with the F/W. In that case if its the last guy, 
then it can go ahead and complete the command.

Hope this helps.

THanks
Ravi

Re: [PATCH 06/11] qla4xxx: added srb referance counter

[Mike Christie]

On 02/11/2010 05:08 AM, Ravi Anand wrote:
> On Mon, 01 Feb 2010, Mike Christie wrote:
>>
>> On 01/30/2010 12:28 AM, Ravi Anand wrote:
>>>
>>> -		msleep(2000);
>>> -	} while (max_wait_time--);
>>> +		if (got_ref&&   (atomic_read(&rp->ref_count) == 1)) {
>>> +			done++;
>>> +			break;
>>> +		}
>>> +
>>> +		msleep(ABORT_POLLING_PERIOD);
>>
>>
>> Did you want to use krefs for the refcounting?
>
> We will add it to our to do list and submit a patch later on.
> For right now we will like to stick to it as kref will require
> additional testing.
>
>> And why is this so funky (got_ref arg and refcount peak) compared to the
>> qla2xxx one?
>
> I don't think qla2xxx is doing any reference counting in eh_abort() path.
> Basically its trying to differentiate for case where it takes an additional
> reference when the cmd is with the F/W. In that case if its the last guy,
> then it can go ahead and complete the command.
>

got_ref is always 0 isn't it (at least in the patch it is)? It seems 
like you can just get rid of all that and just copy qla2xxx's code.

Re: [PATCH 06/11] qla4xxx: added srb referance counter

[Mike Christie]

On 02/11/2010 04:57 PM, Mike Christie wrote:
> On 02/11/2010 05:08 AM, Ravi Anand wrote:
>> On Mon, 01 Feb 2010, Mike Christie wrote:
>>>
>>> On 01/30/2010 12:28 AM, Ravi Anand wrote:
>>>>
>>>> - msleep(2000);
>>>> - } while (max_wait_time--);
>>>> + if (got_ref&& (atomic_read(&rp->ref_count) == 1)) {
>>>> + done++;
>>>> + break;
>>>> + }
>>>> +
>>>> + msleep(ABORT_POLLING_PERIOD);
>>>
>>>
>>> Did you want to use krefs for the refcounting?
>>
>> We will add it to our to do list and submit a patch later on.
>> For right now we will like to stick to it as kref will require
>> additional testing.
>>
>>> And why is this so funky (got_ref arg and refcount peak) compared to the
>>> qla2xxx one?
>>
>> I don't think qla2xxx is doing any reference counting in eh_abort() path.
>> Basically its trying to differentiate for case where it takes an
>> additional
>> reference when the cmd is with the F/W. In that case if its the last guy,
>> then it can go ahead and complete the command.
>>
>
> got_ref is always 0 isn't it (at least in the patch it is)? It seems
> like you can just get rid of all that and just copy qla2xxx's code.

Hey, since it is always 0, then

if (got_ref && (atomic_read(&rp->ref_count) == 1)) {

will never be true, right? I think you can just drop that chunk or let 
me know if it is used in another patch I missed or something.

Re: [PATCH 06/11] qla4xxx: added srb referance counter

[Mike Christie]

On 02/11/2010 05:19 PM, ravi.anand wrote:
>
> On Feb 11, 2010, at 3:02 PM, Mike Christie wrote:
>
>> On 02/11/2010 04:57 PM, Mike Christie wrote:
>>> On 02/11/2010 05:08 AM, Ravi Anand wrote:
>>>> On Mon, 01 Feb 2010, Mike Christie wrote:
>>>>>
>>>>> On 01/30/2010 12:28 AM, Ravi Anand wrote:
>>>>>>
>>>>>> - msleep(2000);
>>>>>> - } while (max_wait_time--);
>>>>>> + if (got_ref&& (atomic_read(&rp->ref_count) == 1)) {
>>>>>> + done++;
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> + msleep(ABORT_POLLING_PERIOD);
>>>>>
>>>>>
>>>>> Did you want to use krefs for the refcounting?
>>>>
>>>> We will add it to our to do list and submit a patch later on.
>>>> For right now we will like to stick to it as kref will require
>>>> additional testing.
>>>>
>>>>> And why is this so funky (got_ref arg and refcount peak) compared
>>>>> to the
>>>>> qla2xxx one?
>>>>
>>>> I don't think qla2xxx is doing any reference counting in eh_abort()
>>>> path.
>>>> Basically its trying to differentiate for case where it takes an
>>>> additional
>>>> reference when the cmd is with the F/W. In that case if its the last
>>>> guy,
>>>> then it can go ahead and complete the command.
>>>>
>>>
>>> got_ref is always 0 isn't it (at least in the patch it is)? It seems
>>> like you can just get rid of all that and just copy qla2xxx's code.
>>
>
> In the abort path (PATCH 10/11) it gets incremented. So its not

Ah I see.

> always zero. And the reason we need a reference is because it can
> get completed behind our back leading to a dangling srb. Since
> now two guys are working on it : eh_abort() path as well as
> the normal completion path.
>

Except for the code you added above, I am not seeing how 
qla4xxx_eh_wait_on_command interacts with the srb or why it needs a 
refcount on it. I do see why  qla4xxx_abort_task needs it. If 
qla4xxx_eh_wait_on_command does need the ref then it seems like the 
device reset path is going to have a issue.

Can't you do sp_put() after qla4xxx_abort_task(). It seems like after 
the qla4xxx_abort_task() call, then no one needs to touch the srb. 
Without your patch, qla4xxx_eh_wait_on_command does not access the srb. 
It only checks if the cmd has been completed by checking if SCp.ptr has 
been cleared.

And why does qla4xxx_eh_abort() do this loop

+       /* Check active list for command */
+       spin_lock_irqsave(&ha->hardware_lock, flags);
+       for (i = 1; i < MAX_SRBS; i++) {
+               srb_cmd = scsi_host_find_tag(ha->host, i);
+               if (srb_cmd == NULL)
+                       continue;



It seems like you would want to do

qla4xxx_wait_for_hba_online()
spin_lock_irqsave(&ha->hardware_lock, flags);

srb = (struct srb *) cmd->SCp.ptr;
if (!srb)
	/* raced while waiting for hba online */
	return SUCCESS;

/* Get a reference to the sp and drop the lock.*/
sp_get(srb);
spin_unlock_irqrestore(&ha->hardware_lock, flags);
qla4xxx_abort_task()
sp_put(ha, srb)
spin_lock_irqsave(&ha->hardware_lock, flags);

qla4xxx_eh_wait_on_command(ha, cmd);

Re: [PATCH 06/11] qla4xxx: added srb referance counter

[Mike Christie]

On 02/11/2010 05:54 PM, Mike Christie wrote:
> spin_lock_irqsave(&ha->hardware_lock, flags);
>

The lock should not be held while we sleep of course :)

> qla4xxx_eh_wait_on_command(ha, cmd);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/11] qla4xxx: added srb referance counter

[Mike Christie]

On 02/11/2010 05:54 PM, Mike Christie wrote:
> It seems like you would want to do
>

Maybe one more fix.


> qla4xxx_wait_for_hba_online()
> spin_lock_irqsave(&ha->hardware_lock, flags);
>
> srb = (struct srb *) cmd->SCp.ptr;
> if (!srb)
> /* raced while waiting for hba online */
> return SUCCESS;
>
> /* Get a reference to the sp and drop the lock.*/
> sp_get(srb);
> spin_unlock_irqrestore(&ha->hardware_lock, flags);
> qla4xxx_abort_task()

 From what I can tell, we used to call qla4xxx_srb_compl (and now do the 
final sp_put in the normal completion path) while holding the 
hardware_lock. If this is not the case then the loop in the eh abort 
patch is racey and my pseudo code above is too).

If I am right, then I think here we want to do

spin_lock_irqsave(&ha->hardware_lock, flags);
sp_put(ha, srb);

So it is consistent with the other code, and then I think we want to add 
a comment in sp_put() that we should be holing the hardware_lock when 
calling it.


> sp_put(ha, srb)
> spin_lock_irqsave(&ha->hardware_lock, flags);
>
> qla4xxx_eh_wait_on_command(ha, cmd);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/11] qla4xxx: added srb referance counter

[Mike Christie]

On 02/11/2010 05:08 AM, Ravi Anand wrote:
> On Mon, 01 Feb 2010, Mike Christie wrote:
>>
>> On 01/30/2010 12:28 AM, Ravi Anand wrote:
>>>
>>> -		msleep(2000);
>>> -	} while (max_wait_time--);
>>> +		if (got_ref&&   (atomic_read(&rp->ref_count) == 1)) {
>>> +			done++;
>>> +			break;
>>> +		}
>>> +
>>> +		msleep(ABORT_POLLING_PERIOD);
>>
>>
>> Did you want to use krefs for the refcounting?
>
> We will add it to our to do list and submit a patch later on.
> For right now we will like to stick to it as kref will require
> additional testing.
>

Please use krefs from the start. Other drivers like beiscsi have had to 
switch from its own refcount code to krefs, and have had to retest their 
patchsets. You have to change your code in some other patches and maybe 
this one too, so you have to retest everything either way.

Re: [PATCH 06/11] qla4xxx: added srb referance counter

[James Bottomley]

On Fri, 2010-02-12 at 11:29 -0600, Mike Christie wrote:
> On 02/11/2010 05:08 AM, Ravi Anand wrote:
> > On Mon, 01 Feb 2010, Mike Christie wrote:
> >>
> >> On 01/30/2010 12:28 AM, Ravi Anand wrote:
> >>>
> >>> -		msleep(2000);
> >>> -	} while (max_wait_time--);
> >>> +		if (got_ref&&   (atomic_read(&rp->ref_count) == 1)) {
> >>> +			done++;
> >>> +			break;
> >>> +		}
> >>> +
> >>> +		msleep(ABORT_POLLING_PERIOD);
> >>
> >>
> >> Did you want to use krefs for the refcounting?
> >
> > We will add it to our to do list and submit a patch later on.
> > For right now we will like to stick to it as kref will require
> > additional testing.
> >
> 
> Please use krefs from the start. Other drivers like beiscsi have had to 
> switch from its own refcount code to krefs, and have had to retest their 
> patchsets. You have to change your code in some other patches and maybe 
> this one too, so you have to retest everything either way.

That goes for me too, please ... it's a standard kernel paradigm and all
the queries about what the refs were doing earlier would have been a lot
simpler since the embedded krefs would largely have answered the
questions.

James