RE: [PATCH] - mptfusion - adding back the spin locks in eh handle rs

RE: [PATCH] - mptfusion - adding back the spin locks in eh handle rs

[Moore, Eric Dean]

On Thursday, June 23, 2005 1:04 PM, Luben Tuikov wrote:
> On 06/23/05 14:20, Moore, Eric Dean wrote:
> > The eh handlers are called with host_lock acquired, and 
> interrupts disabled.
> > However fusion waits for completion of task managment 
> request.  With these spin locks 
> > removed, the driver is going to hang within the sleep 
> calls.   Someone recently posted 
> > a patch to remove them(don´t know who),  and can be found 
> in James Bottomely scsi-misc branch.  
> 
> That was Jeff, and I thought those patches made it in already.
> They are needed and are a good thing.
> 
> 	Luben
> 


Ok, explain that to me why having our driver hang is a good thing.
The task managment requests to our firmware stack are done in interrupt
mode,
thus we have sleep and have interrupts enabled in the eh threads waiting on
the commands to complete.
Christoph H. requested this change about 6 months ago, asking that we remove
our watchdog timers in the eh threads, in favor of this design.

Eric Moore
LSI Logic Corporation

-
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] - mptfusion - adding back the spin locks in eh handle rs

[Luben Tuikov]

On 06/23/05 16:12, Moore, Eric Dean wrote:
> Ok, explain that to me why having our driver hang is a good thing.
> The task managment requests to our firmware stack are done in interrupt
> mode,
> thus we have sleep and have interrupts enabled in the eh threads waiting on
> the commands to complete.
> Christoph H. requested this change about 6 months ago, asking that we remove
> our watchdog timers in the eh threads, in favor of this design.

Hi Eric,

It is up to the LLDD to know how to deliver a TMF to the device.
Whether this is done purely in process context (iSCSI) or
in mixed context is up to the LLDD.

Also, most often than not, you'd want to wait for the TMF to
return (iSCSI), to know its status (although you can assume it without fault).

Nevertheless, calling the eh routines in process context is the way
to go. (in order to allow for "sleeping", on delivery you can disable ints
as appropriate for the hw)

	Luben

RE: [PATCH] - mptfusion - adding back the spin locks in eh handle rs

[Moore, Eric Dean]

Luben

Ok - So you agree that completing Task management request
in the same context as the eh threads is the way to go, right?

Because that is what this LLD is doing.  Have you had
a chance to look at our code?

That previous patch submitted by Jeff(I guess) was removing code which
enabled interrupts.  My patch was restoring it back to previous
working code which doesn't hang the driver.

The reason that the previous patch hangs the driver is we are issuing
the task managment request using interrupt mode, not polling.
Meaning it requires interrupts to be enabled. 
As you probably know, you can't go to sleep when interrupts are disabled.
All eh entry points are called with interrupts disabled. Take a look
at scsi_try_bus_device_reset (scsi_error.c).

Eric

Luben Tuikov wrote:
> 
> 
> On 06/23/05 16:12, Moore, Eric Dean wrote:
> > Ok, explain that to me why having our driver hang is a good thing.
> > The task managment requests to our firmware stack are done 
> in interrupt
> > mode,
> > thus we have sleep and have interrupts enabled in the eh 
> threads waiting on
> > the commands to complete.
> > Christoph H. requested this change about 6 months ago, 
> asking that we remove
> > our watchdog timers in the eh threads, in favor of this design.
> 
> Hi Eric,
> 
> It is up to the LLDD to know how to deliver a TMF to the device.
> Whether this is done purely in process context (iSCSI) or
> in mixed context is up to the LLDD.
> 
> Also, most often than not, you'd want to wait for the TMF to
> return (iSCSI), to know its status (although you can assume 
> it without fault).
> 
> Nevertheless, calling the eh routines in process context is the way
> to go. (in order to allow for "sleeping", on delivery you can 
> disable ints
> as appropriate for the hw)
> 
> 	Luben
> 

Re: [PATCH] - mptfusion - adding back the spin locks in eh handle rs

[Jeff Garzik]

Moore, Eric Dean wrote:
> Luben
> 
> Ok - So you agree that completing Task management request
> in the same context as the eh threads is the way to go, right?
> 
> Because that is what this LLD is doing.  Have you had
> a chance to look at our code?
> 
> That previous patch submitted by Jeff(I guess) was removing code which
> enabled interrupts.  My patch was restoring it back to previous
> working code which doesn't hang the driver.

What was broken?

MPT abort handler would release the spinlock at the beginning, and 
reacquire it at the end, essentially reversing the spinlocks in the old 
spinlocked EH code.

	Jeff

Re: [PATCH] - mptfusion - adding back the spin locks in eh handle rs

[Luben Tuikov]

On 06/23/05 18:06, Moore, Eric Dean wrote:
> Luben
> 
> Ok - So you agree that completing Task management request
> in the same context as the eh threads is the way to go, right?

I think so -- process context.
 
> Because that is what this LLD is doing.  Have you had
> a chance to look at our code?

No, I haven't looked -- I'm swamped.
 
> That previous patch submitted by Jeff(I guess) was removing code which
> enabled interrupts.  My patch was restoring it back to previous
> working code which doesn't hang the driver.

In LLDD yes (since they were never disabled when eh_abort() was called).
In SCSI Core -- the opposite: removing code which disabled ints and then
calling eh_abort() of the LLDD.
 
> The reason that the previous patch hangs the driver is we are issuing
> the task managment request using interrupt mode, not polling.
> Meaning it requires interrupts to be enabled. 

They are!  The abort handler is called with ints enabled.
See the core of Jeff's patch here:

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -528,10 +528,8 @@ static int scsi_send_eh_cmnd(struct scsi
 		 * abort a timed out command or not.  not sure how
 		 * we should treat them differently anyways.
 		 */
-		spin_lock_irqsave(shost->host_lock, flags);
 		if (shost->hostt->eh_abort_handler)
 			shost->hostt->eh_abort_handler(scmd);
-		spin_unlock_irqrestore(shost->host_lock, flags);
 			
 		scmd->request->rq_status = RQ_SCSI_DONE;
 		scmd->owner = SCSI_OWNER_ERROR_HANDLER;
@@ -737,11 +735,8 @@ static int scsi_eh_get_sense(struct list
  **/
 static int scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)
 {
-	unsigned long flags;
-	int rtn = FAILED;
-
 	if (!scmd->device->host->hostt->eh_abort_handler)
-		return rtn;
+		return FAILED;
 
 	/*
 	 * scsi_done was called just after the command timed out and before
@@ -752,11 +747,7 @@ static int scsi_try_to_abort_cmd(struct 
 
 	scmd->owner = SCSI_OWNER_LOWLEVEL;
 
-	spin_lock_irqsave(scmd->device->host->host_lock, flags);
-	rtn = scmd->device->host->hostt->eh_abort_handler(scmd);
-	spin_unlock_irqrestore(scmd->device->host->host_lock, flags);
-
-	return rtn;
+	return scmd->device->host->hostt->eh_abort_handler(scmd);
 }
 
 /**

So the LLDD is free to sleep in eh_abort().  If it needs to do
something with ints disabled, it should disable them itself.

	Luben

RE: [PATCH] - mptfusion - adding back the spin locks in eh handle rs

[Moore, Eric Dean]

Jeff - I didn't realize that the host-lock spin locks were removed inside
scsi mid layer scsi_error.c code.  I just applied James
scsi-misc-2.6.diff against the 2.6.12 kerne.  Just built this kernel
and tested the fusion driver, and I'm happy.  No problem, ignore yesterdays
patch.

I will have to repost the other two patch's later today; e.g. 
- change_queue_depth API
- FC transport support.

Eric


On Thursday, June 23, 2005 5:18 PM, Jeff Garzik wrote:
> Moore, Eric Dean wrote:
> > Luben
> > 
> > Ok - So you agree that completing Task management request
> > in the same context as the eh threads is the way to go, right?
> > 
> > Because that is what this LLD is doing.  Have you had
> > a chance to look at our code?
> > 
> > That previous patch submitted by Jeff(I guess) was removing 
> code which
> > enabled interrupts.  My patch was restoring it back to previous
> > working code which doesn't hang the driver.
> 
> What was broken?
> 
> MPT abort handler would release the spinlock at the beginning, and 
> reacquire it at the end, essentially reversing the spinlocks 
> in the old 
> spinlocked EH code.
>