<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN">
<HTML><HEAD>
<META content="text/html; charset=us-ascii" http-equiv=Content-Type>
<META name=GENERATOR content="MSHTML 8.00.6001.18812"></HEAD>
<BODY dir=ltr>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial>Leo,</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial> I've done some testing of your suggested patch with
WHQL enable/disable tests + standard WinOF load/unload and seen no
problems.</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial>Enclosed are the patches which I suggested to you in previous
email - specifically remove the empty if block.</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial>On your approval I will commit to branch and
trunk.</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial>stan.</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial>--- C:/Documents and Settings/scsmith/Local
Settings/Temp/bus_pnp.c-revBASE.svn000.tmp.c Wed Sep 02 08:44:56
2009<BR>+++ C:/Documents and Settings/scsmith/My
Documents/openIB-windows/SVN/gen1/branches/WOF2-1/core/bus/kernel/bus_pnp.c Fri
Aug 28 16:35:21 2009<BR>@@ -216,7 +216,6
@@<BR> RtlZeroMemory(p_ext, sizeof
*p_ext);<BR> cl_init_pnp_po_ext( g_ControlDeviceObject,
NULL, <BR> NULL,
bus_globals.dbg_lvl, NULL, NULL );<BR>- IoInitializeRemoveLock(
&p_ext->cl_ext.stop_lock, 'dtci', 0, 1000
);<BR> <BR> /* enable user-mode access to IB stack
*/<BR> BUS_PRINT( BUS_DBG_PNP, ("Remove-n-reCreate
dos_name symlink\n") );<BR></FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial>--- C:/Documents and Settings/scsmith/Local
Settings/Temp/cl_pnp_po.c-revBASE.svn000.tmp.c Wed Sep 02 08:43:28
2009<BR>+++ C:/Documents and Settings/scsmith/My
Documents/openIB-windows/SVN/gen1/branches/WOF2-1/core/complib/kernel/cl_pnp_po.c Fri
Aug 28 16:36:18 2009<BR>@@ -136,7 +136,9 @@<BR> <BR> /* Store
the pointer to our own device. */<BR> p_ext->p_self_do =
p_dev_obj;<BR>+<BR> IoInitializeRemoveLock(
&p_ext->remove_lock, 'bilc', 15, 1000
);<BR>+ IoInitializeRemoveLock( &p_ext->stop_lock, 'dtci', 0, 1000
);<BR> <BR> /* Initialize the PnP states.
*/<BR> p_ext->pnp_state = NotStarted;<BR>@@ -428,15 +430,6
@@<BR> if( NT_SUCCESS( status )
)<BR> cl_set_pnp_state( p_ext, Started
);<BR> <BR>- /*<BR>- * If we get the start request when we're
already started, don't <BR>- * re-initialize the stop lock.<BR>-
*/<BR>- if( p_ext->last_pnp_state != Started )
{<BR>- CL_TRACE( CL_DBG_PNP, p_ext->dbg_lvl,
("IoInitializeRemoveLock: stop_lock %p[\n",
&p_ext->stop_lock));<BR>- IoInitializeRemoveLock(
&p_ext->stop_lock, 'dtci', 0, 1000
);<BR>- }<BR>-<BR> CL_EXIT( CL_DBG_PNP, p_ext->dbg_lvl
);<BR> return status;<BR> }<BR>@@ -553,33 +546,6
@@<BR> CL_TRACE_EXIT( CL_DBG_PNP,
p_ext->dbg_lvl,<BR> ("IRP_MN_CANCEL_STOP_DEVICE
received in invalid state.\n") );<BR> return
status;<BR>- }<BR>-<BR>- if( p_ext->last_pnp_state == Started
)<BR>- {<BR>- /*<BR>- * Re-initialize the stop lock
before rolling back the PnP<BR>- * state so that there's no
contention while it's uninitialized.<BR>-
*/<BR>- CL_TRACE( CL_DBG_PNP, p_ext->dbg_lvl,
("IoInitializeRemoveLock: stop_lock %p[\n",
&p_ext->stop_lock));<BR>- IoInitializeRemoveLock(
&p_ext->stop_lock, 'dtci', 0, 1000 );<BR>-#if
0 <BR>- // leo: it seems like a bug, because it can never
get released<BR>- {<BR>- /* <BR>- * Acquire
the stop lock to allow releasing and waiting when stopping.<BR>-
*/<BR>- NTSTATUS status1;<BR>- CL_TRACE(
CL_DBG_PNP, p_ext->dbg_lvl, ("IoAcquireRemoveLock: stop_lock %p[\n",
&p_ext->stop_lock));<BR>- status1 = IoAcquireRemoveLock(
&p_ext->stop_lock, NULL );<BR>- if( !NT_SUCCESS( status1
) )<BR>- {<BR>- CL_TRACE( CL_DBG_ERROR,
p_ext->dbg_lvl, <BR>- ("IoAcquireRemoveLock
returned %08x. Continue anyway ...\n", status)
);<BR>- }<BR>- CL_TRACE( CL_DBG_PNP,
p_ext->dbg_lvl, ("IoAcquireRemoveLock: stop_lock
]\n"));<BR>- }<BR>-#endif <BR> }<BR> <BR> /*
Return to the previous PnP state. */<BR></DIV></FONT></SPAN>
<DIV dir=ltr align=left><SPAN class=082104615-02092009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV><BR>
<DIV dir=ltr lang=en-us class=OutlookMessageHeader align=left>
<HR tabIndex=-1>
<FONT size=2 face=Tahoma><B>From:</B> Leonid Keller
[mailto:leonid@mellanox.co.il] <BR><B>Sent:</B> Monday, August 17, 2009 5:29
AM<BR><B>To:</B> Fab Tillier; Smith, Stan; Hefty, Sean<BR><B>Cc:</B>
ofw_list<BR><B>Subject:</B> FW: [ofw][patch][IBBUS] crash on re-initialization
of remove lock<BR></FONT><BR></DIV>
<DIV></DIV>
<DIV><FONT color=#0000ff size=2 face=Arial><SPAN class=757405711-17082009>I
think this patch should go also into the branch.</SPAN></FONT></DIV>
<DIV><FONT color=#0000ff size=2 face=Arial><SPAN class=757405711-17082009>I'll
be on vacation 08/19-26, so i'll appreciate a lot if you could look it through
and opine.</SPAN></FONT></DIV>
<DIV><FONT color=#0000ff size=2 face=Arial><SPAN class=757405711-17082009>Also
if someone could check it, running WHQL tests ...</SPAN></FONT></DIV>
<DIV><FONT color=#0000ff size=2 face=Arial><SPAN
class=757405711-17082009></SPAN></FONT> </DIV>
<DIV><FONT color=#0000ff size=2 face=Arial><SPAN class=757405711-17082009>There
are two Remove Locks in IBBUS code. They are called 'remove_lock' and
'stop_lock'.</SPAN></FONT></DIV>
<DIV><FONT color=#0000ff size=2 face=Arial><SPAN class=757405711-17082009>The
'stop_lock' get initialized twice, on device' start and
stop.</SPAN></FONT></DIV>
<DIV><FONT color=#0000ff size=2 face=Arial><SPAN class=757405711-17082009>The
'remove_lock' is initialized only once on the start up.</SPAN></FONT></DIV>
<DIV><FONT color=#0000ff size=2 face=Arial><SPAN class=757405711-17082009>In my
patch I did the same with 'stop_lock'.</SPAN></FONT></DIV>
<DIV><FONT color=#0000ff size=2 face=Arial><SPAN class=757405711-17082009>But
honestly, I don't quite understand why the author of the code didn't
do this in the first place ...</SPAN></FONT></DIV><BR>
<DIV dir=ltr lang=en-us class=OutlookMessageHeader align=left>
<HR tabIndex=-1>
<FONT size=2 face=Tahoma><B>From:</B> ofw-bounces@lists.openfabrics.org
[mailto:ofw-bounces@lists.openfabrics.org] <B>On Behalf Of </B>Leonid
Keller<BR><B>Sent:</B> Sunday, August 16, 2009 7:46 PM<BR><B>To:</B>
ofw@lists.openfabrics.org<BR><B>Subject:</B> [ofw][patch][IBBUS] crash on
re-initialization of remove lock<BR></FONT><BR></DIV>
<DIV></DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2 face=Arial>I was reported a
crash upon running <FONT size=3 face="Times New Roman">“System Common
Scenario”</FONT> WHQL test with our stack.</FONT></SPAN></DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2 face=Arial>The crash: C4
(0xd7), which means Driver Verifier revealed a re-initializing of Remove
Lock.</FONT></SPAN></DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2
face=Arial></FONT></SPAN> </DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2 face=Arial>fffff880`01fa73a8
fffff800`0191b3dc : nt!KeBugCheckEx<BR>fffff880`01fa73b0 fffff800`0192d2e6 :
nt!NtShutdownSystem+0x7f0c<BR>fffff880`01fa73f0 fffff880`02a06974 :
nt!NtShutdownSystem+0x19e16<BR><STRONG>fffff880`01fa7450 fffff800`01937c16 :
ibbus!cl_pnp+0x1d8
[f:\ribel\mlnx_winof_2.0.5\4335\branches\mlnx_winof_2-0\core\complib\kernel\cl_pnp_po.c
@ 211]<BR></STRONG>fffff880`01fa74b0 fffff800`0193a52a :
nt!NtShutdownSystem+0x24746<BR>fffff880`01fa7510 fffff800`01937c16 :
nt!NtShutdownSystem+0x2705a<BR></DIV>
<DIV></FONT></SPAN><SPAN class=232323116-16082009><FONT size=2
face=Arial></FONT></SPAN> </DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2 face=Arial>It happens on win2k8
R2.</FONT></SPAN></DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2 face=Arial>One can see that
IBAL PnP code, written in 2002 (?) contains in fact re-initialization of
stop Remove Lock.</FONT></SPAN></DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2 face=Arial>Maybe sometimes it
was possible and was not checked by Driver Verifier or we just haven't come
accross it...</FONT></SPAN></DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2
face=Arial></FONT></SPAN> </DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2 face=Arial>Here is a patch that
fixes the problem by eliminating re-initialization of the stop
lock.</FONT></SPAN></DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2
face=Arial></FONT></SPAN> </DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2
face=Arial></FONT></SPAN> </DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2 face=Arial>Index:
core/bus/kernel/bus_pnp.c<BR>===================================================================<BR>---
core/bus/kernel/bus_pnp.c (revision 4659)<BR>+++
core/bus/kernel/bus_pnp.c (working copy)<BR>@@ -216,7 +216,6
@@<BR> RtlZeroMemory(p_ext, sizeof
*p_ext);<BR> cl_init_pnp_po_ext( g_ControlDeviceObject,
NULL, <BR> NULL,
bus_globals.dbg_lvl, NULL, NULL );<BR>- IoInitializeRemoveLock(
&p_ext->cl_ext.stop_lock, 'dtci', 0, 1000
);<BR> <BR> /* enable user-mode access to IB stack
*/<BR> BUS_PRINT( BUS_DBG_PNP, ("Remove-n-reCreate
dos_name symlink\n") );<BR>Index:
core/complib/kernel/cl_pnp_po.c<BR>===================================================================<BR>---
core/complib/kernel/cl_pnp_po.c (revision 4659)<BR>+++
core/complib/kernel/cl_pnp_po.c (working copy)<BR>@@ -137,7 +137,8
@@<BR> /* Store the pointer to our own device.
*/<BR> p_ext->p_self_do =
p_dev_obj;<BR> IoInitializeRemoveLock( &p_ext->remove_lock,
'bilc', 15, 1000 );<BR>-<BR>+ IoInitializeRemoveLock(
&p_ext->stop_lock, 'dtci', 0, 1000 );<BR>+ <BR> /*
Initialize the PnP states. */<BR> p_ext->pnp_state =
NotStarted;<BR> p_ext->last_pnp_state = NotStarted;<BR>@@ -428,15
+429,6 @@<BR> if( NT_SUCCESS( status )
)<BR> cl_set_pnp_state( p_ext, Started
);<BR> <BR>- /*<BR>- * If we get the start request when we're
already started, don't <BR>- * re-initialize the stop lock.<BR>-
*/<BR>- if( p_ext->last_pnp_state != Started )
{<BR>- CL_TRACE( CL_DBG_PNP, p_ext->dbg_lvl,
("IoInitializeRemoveLock: stop_lock %p[\n",
&p_ext->stop_lock));<BR>- IoInitializeRemoveLock(
&p_ext->stop_lock, 'dtci', 0, 1000
);<BR>- }<BR>-<BR> CL_EXIT( CL_DBG_PNP, p_ext->dbg_lvl
);<BR> return status;<BR> }<BR>@@ -557,12 +549,6
@@<BR> <BR> if( p_ext->last_pnp_state == Started
)<BR> {<BR>- /*<BR>- * Re-initialize the stop
lock before rolling back the PnP<BR>- * state so that there's no
contention while it's uninitialized.<BR>-
*/<BR>- CL_TRACE( CL_DBG_PNP, p_ext->dbg_lvl,
("IoInitializeRemoveLock: stop_lock %p[\n",
&p_ext->stop_lock));<BR>- IoInitializeRemoveLock(
&p_ext->stop_lock, 'dtci', 0, 1000 );<BR> #if
0 <BR> // leo: it seems like a bug, because it can
never get released<BR> {<BR></FONT></SPAN></DIV>
<DIV><SPAN class=232323116-16082009></SPAN> </DIV>
<DIV><SPAN class=232323116-16082009><FONT size=2
face=Arial></FONT></SPAN> </DIV></BODY></HTML>