<!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=219483716-26082009><FONT color=#0000ff
size=2 face=Arial>Leo,</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT color=#0000ff
size=2 face=Arial> Code review of the patch looks reasonable with the
exception of the very last part</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT color=#0000ff
size=2 face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT size=2
face=Arial>core/complib/kernel/cl_pnp_po.c</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT size=2
face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT size=2
face=Arial>__cancel_stop()</FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT size=2
face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT size=2
face=Arial><...></FONT></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT size=2
face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT size=2
face=Arial> 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> {</FONT><BR>
<DIV><SPAN class=232323116-16082009><FONT color=#0000ff size=2
face=Arial></FONT></SPAN> </DIV>
<DIV><SPAN class=232323116-16082009><SPAN class=219483716-26082009><FONT
color=#0000ff size=2 face=Arial>#endif</FONT></SPAN></SPAN></DIV>
<DIV><SPAN class=232323116-16082009><SPAN class=219483716-26082009><FONT
color=#0000ff size=2 face=Arial> }</FONT></SPAN></SPAN></DIV>
<DIV><SPAN class=232323116-16082009><SPAN class=219483716-26082009><FONT
color=#0000ff size=2 face=Arial></FONT></SPAN></SPAN> </DIV>
<DIV><SPAN class=232323116-16082009><SPAN class=219483716-26082009><FONT
color=#0000ff size=2 face=Arial>The issue is between your patch and the #if
0 the block <FONT color=#000000>'if( p_ext->last_pnp_state == Started )
{} <FONT color=#0000ff>contains no useful
code</FONT>.</FONT></FONT></SPAN></SPAN></DIV>
<DIV><SPAN class=232323116-16082009><SPAN class=219483716-26082009><FONT size=2
face=Arial><FONT color=#0000ff></FONT></FONT></SPAN></SPAN> </DIV>
<DIV><SPAN class=232323116-16082009><SPAN class=219483716-26082009><FONT size=2
face=Arial><FONT color=#0000ff>Why not remove the entire if
block?</FONT></FONT></SPAN></SPAN></DIV>
<DIV><SPAN class=232323116-16082009><SPAN class=219483716-26082009><FONT
color=#0000ff size=2 face=Arial></FONT></SPAN></SPAN> </DIV>
<DIV><SPAN class=232323116-16082009><SPAN class=219483716-26082009><FONT
color=#0000ff size=2 face=Arial>Will let you know how the testing
goes.</FONT></SPAN></SPAN></DIV><SPAN class=232323116-16082009><SPAN
class=219483716-26082009><FONT size=2 face=Arial><FONT
color=#0000ff></FONT></FONT></SPAN></SPAN></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><SPAN
class=232323116-16082009><SPAN class=219483716-26082009><FONT face=Arial><FONT
color=#0000ff><FONT
size=2></FONT></FONT></FONT></SPAN></SPAN></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><SPAN
class=232323116-16082009><SPAN class=219483716-26082009>
<DIV><SPAN class=219483716-26082009></SPAN><FONT color=#0000ff size=2
face=Arial>s<SPAN
class=219483716-26082009>tan.</SPAN><BR></FONT></DIV></SPAN></SPAN></SPAN></DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT size=2
face=Arial></FONT></SPAN> </DIV>
<DIV dir=ltr align=left><SPAN class=219483716-26082009><FONT 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>