isp(4) - kernel panic on initialization of driver

[Alexander Sack]

On Fri, Aug 29, 2008 at 3:17 PM, Ross <westr at connection.ca> wrote:
> AS> Give me a sec to look a this some more....bottom line is isp
> AS> should not be servicing async interrupts until its absolutely
> AS> ready!
>
> Okay, I've done a bit more digging on my side here, and have come up
> against a small wall -
>
> The trace shows:
>
>> isp_freeze_loopdown(c81d0e00,2,c06f800f,0,c0cf1f6c,...) at isp_freeze_loopdown+0x42
>> isp_async(c81d0e00,6,0,14e1,2001d5c3,...) at isp_async+0xa72
>> isp_intr(c81d0e00,8012,1,8014,c0af6718,...) at isp_intr+0xbc7
>> isp_mbox_wait_complete(c81d0e00,c0af67e8,50000000,0,8,...) at isp_mbox_wait_complete+0x120
>
> But what's missing in the trace is the call to isp_async() from
> isp_intr(). What I have found is the call to isp_parse_async() (see line
> 4560 in isp.c) which in turn calls isp_async().
>
> Setting DEBUG to 0x14F shows the extra debug lines in there confirming
> the pass through [console output includes the "Async Mbox 0x8014"
> line before the mbox checkpoint].
>
> I'm guessing that would be the spot to add a check against isp_state,
> but am not sure whether to just return from the function, or to do the
> goto jump to 'out' for the cleanup.
>
> Let me know if I'm on track.

I think your doing some great work but I don't think this is the
*right* direction to take.  The bottom line is the ISP should have
interrupts disabled until it completes a full reset and loads the
firmware, period.  You shouldn't have to ignore ASYNC events during a
reset - that doesn't make sense to me....yet....!

Can we try something else:

@@ -1192,6 +1192,8 @@
 		isp->isp_touched = 1;
 	}

+	ISP_DISABLE_INTS(isp);
+
	/*
 	 * Make sure we're in reset state.
 	 */

--- isp.c.0	2008-08-29 13:35:01.000000000 -0400
+++ isp.c	2008-08-29 14:15:40.000000000 -0400
@@ -226,8 +226,6 @@
 		isp->isp_touched = 1;
 	}

-	ISP_DISABLE_INTS(isp);
-
 	/*
 	 * Pick an initial maxcmds value which will be used
 	 * to allocate xflist pointer space. It may be changed
@@ -684,7 +682,8 @@
 	/*
 	 * Do MD specific post initialization
 	 */
-	ISP_RESET1(isp);
+	if (!IS_24XX(isp))
+		ISP_RESET1(isp);

 	/*
 	 * Wait for everything to finish firing up.

--- isp_freebsd.c.0	2008-08-29 14:05:05.000000000 -0400
+++ isp_freebsd.c	2008-08-29 14:05:32.000000000 -0400
@@ -231,6 +231,7 @@

 	if (isp->isp_role != ISP_ROLE_NONE) {
 		isp->isp_state = ISP_RUNSTATE;
+		ISP_ENABLE_INTS(isp);
 	}
 	if (isplist == NULL) {
 		isplist = isp;

I wanted to put back that line that we removed so we test one thing at a time.

I so wish I could reproduce your exact panic but I can't!!!  I've
tried about a dozen different ways but I just can't.  I'm trying to
ignore ALL ASYNC's until after we complete the isp_reset|init|attach
cycle and let the intrhook enable interrupts and start the enumeration
stuff (at that point simq's have been enabled, bus registered etc.).

The mailbox commands should be ok since we use polling to complete
them anyway (I have to verify that).  I just tried this on my box to
verify that my RAID FC array gets enumerated and the driver doesn't
panic (its the best I can do right now).

Btw, this wouldn't be the final patch but if its effective we are on
the right track!  :D

-aps