[ofw][patch][IBBUS] crash on re-initialization of remove lock
Smith, Stan
stan.smith at intel.com
Wed Sep 2 10:08:53 PDT 2009
Done in svn.2417 (Trunk) & 2417 (WOF2-1)
________________________________
From: Leonid Keller [mailto:leonid at mellanox.co.il]
Sent: Wednesday, September 02, 2009 8:57 AM
To: Smith, Stan; Fab Tillier; Hefty, Sean
Cc: ofw_list
Subject: RE: [ofw][patch][IBBUS] crash on re-initialization of remove lock
Thank you. Go ahead.
________________________________
From: Stan C. Smith [mailto:stan.smith at intel.com]
Sent: Wednesday, September 02, 2009 6:50 PM
To: Leonid Keller; Fab Tillier; Hefty, Sean
Cc: ofw_list
Subject: RE: [ofw][patch][IBBUS] crash on re-initialization of remove lock
Leo,
I've done some testing of your suggested patch with WHQL enable/disable tests + standard WinOF load/unload and seen no problems.
Enclosed are the patches which I suggested to you in previous email - specifically remove the empty if block.
On your approval I will commit to branch and trunk.
stan.
--- C:/Documents and Settings/scsmith/Local Settings/Temp/bus_pnp.c-revBASE.svn000.tmp.c Wed Sep 02 08:44:56 2009
+++ 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
@@ -216,7 +216,6 @@
RtlZeroMemory(p_ext, sizeof *p_ext);
cl_init_pnp_po_ext( g_ControlDeviceObject, NULL,
NULL, bus_globals.dbg_lvl, NULL, NULL );
- IoInitializeRemoveLock( &p_ext->cl_ext.stop_lock, 'dtci', 0, 1000 );
/* enable user-mode access to IB stack */
BUS_PRINT( BUS_DBG_PNP, ("Remove-n-reCreate dos_name symlink\n") );
--- C:/Documents and Settings/scsmith/Local Settings/Temp/cl_pnp_po.c-revBASE.svn000.tmp.c Wed Sep 02 08:43:28 2009
+++ 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
@@ -136,7 +136,9 @@
/* Store the pointer to our own device. */
p_ext->p_self_do = p_dev_obj;
+
IoInitializeRemoveLock( &p_ext->remove_lock, 'bilc', 15, 1000 );
+ IoInitializeRemoveLock( &p_ext->stop_lock, 'dtci', 0, 1000 );
/* Initialize the PnP states. */
p_ext->pnp_state = NotStarted;
@@ -428,15 +430,6 @@
if( NT_SUCCESS( status ) )
cl_set_pnp_state( p_ext, Started );
- /*
- * If we get the start request when we're already started, don't
- * re-initialize the stop lock.
- */
- if( p_ext->last_pnp_state != Started ) {
- CL_TRACE( CL_DBG_PNP, p_ext->dbg_lvl, ("IoInitializeRemoveLock: stop_lock %p[\n", &p_ext->stop_lock));
- IoInitializeRemoveLock( &p_ext->stop_lock, 'dtci', 0, 1000 );
- }
-
CL_EXIT( CL_DBG_PNP, p_ext->dbg_lvl );
return status;
}
@@ -553,33 +546,6 @@
CL_TRACE_EXIT( CL_DBG_PNP, p_ext->dbg_lvl,
("IRP_MN_CANCEL_STOP_DEVICE received in invalid state.\n") );
return status;
- }
-
- if( p_ext->last_pnp_state == Started )
- {
- /*
- * Re-initialize the stop lock before rolling back the PnP
- * state so that there's no contention while it's uninitialized.
- */
- CL_TRACE( CL_DBG_PNP, p_ext->dbg_lvl, ("IoInitializeRemoveLock: stop_lock %p[\n", &p_ext->stop_lock));
- IoInitializeRemoveLock( &p_ext->stop_lock, 'dtci', 0, 1000 );
-#if 0
- // leo: it seems like a bug, because it can never get released
- {
- /*
- * Acquire the stop lock to allow releasing and waiting when stopping.
- */
- NTSTATUS status1;
- CL_TRACE( CL_DBG_PNP, p_ext->dbg_lvl, ("IoAcquireRemoveLock: stop_lock %p[\n", &p_ext->stop_lock));
- status1 = IoAcquireRemoveLock( &p_ext->stop_lock, NULL );
- if( !NT_SUCCESS( status1 ) )
- {
- CL_TRACE( CL_DBG_ERROR, p_ext->dbg_lvl,
- ("IoAcquireRemoveLock returned %08x. Continue anyway ...\n", status) );
- }
- CL_TRACE( CL_DBG_PNP, p_ext->dbg_lvl, ("IoAcquireRemoveLock: stop_lock ]\n"));
- }
-#endif
}
/* Return to the previous PnP state. */
________________________________
From: Leonid Keller [mailto:leonid at mellanox.co.il]
Sent: Monday, August 17, 2009 5:29 AM
To: Fab Tillier; Smith, Stan; Hefty, Sean
Cc: ofw_list
Subject: FW: [ofw][patch][IBBUS] crash on re-initialization of remove lock
I think this patch should go also into the branch.
I'll be on vacation 08/19-26, so i'll appreciate a lot if you could look it through and opine.
Also if someone could check it, running WHQL tests ...
There are two Remove Locks in IBBUS code. They are called 'remove_lock' and 'stop_lock'.
The 'stop_lock' get initialized twice, on device' start and stop.
The 'remove_lock' is initialized only once on the start up.
In my patch I did the same with 'stop_lock'.
But honestly, I don't quite understand why the author of the code didn't do this in the first place ...
________________________________
From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Leonid Keller
Sent: Sunday, August 16, 2009 7:46 PM
To: ofw at lists.openfabrics.org
Subject: [ofw][patch][IBBUS] crash on re-initialization of remove lock
I was reported a crash upon running "System Common Scenario" WHQL test with our stack.
The crash: C4 (0xd7), which means Driver Verifier revealed a re-initializing of Remove Lock.
fffff880`01fa73a8 fffff800`0191b3dc : nt!KeBugCheckEx
fffff880`01fa73b0 fffff800`0192d2e6 : nt!NtShutdownSystem+0x7f0c
fffff880`01fa73f0 fffff880`02a06974 : nt!NtShutdownSystem+0x19e16
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]
fffff880`01fa74b0 fffff800`0193a52a : nt!NtShutdownSystem+0x24746
fffff880`01fa7510 fffff800`01937c16 : nt!NtShutdownSystem+0x2705a
It happens on win2k8 R2.
One can see that IBAL PnP code, written in 2002 (?) contains in fact re-initialization of stop Remove Lock.
Maybe sometimes it was possible and was not checked by Driver Verifier or we just haven't come accross it...
Here is a patch that fixes the problem by eliminating re-initialization of the stop lock.
Index: core/bus/kernel/bus_pnp.c
===================================================================
--- core/bus/kernel/bus_pnp.c (revision 4659)
+++ core/bus/kernel/bus_pnp.c (working copy)
@@ -216,7 +216,6 @@
RtlZeroMemory(p_ext, sizeof *p_ext);
cl_init_pnp_po_ext( g_ControlDeviceObject, NULL,
NULL, bus_globals.dbg_lvl, NULL, NULL );
- IoInitializeRemoveLock( &p_ext->cl_ext.stop_lock, 'dtci', 0, 1000 );
/* enable user-mode access to IB stack */
BUS_PRINT( BUS_DBG_PNP, ("Remove-n-reCreate dos_name symlink\n") );
Index: core/complib/kernel/cl_pnp_po.c
===================================================================
--- core/complib/kernel/cl_pnp_po.c (revision 4659)
+++ core/complib/kernel/cl_pnp_po.c (working copy)
@@ -137,7 +137,8 @@
/* Store the pointer to our own device. */
p_ext->p_self_do = p_dev_obj;
IoInitializeRemoveLock( &p_ext->remove_lock, 'bilc', 15, 1000 );
-
+ IoInitializeRemoveLock( &p_ext->stop_lock, 'dtci', 0, 1000 );
+
/* Initialize the PnP states. */
p_ext->pnp_state = NotStarted;
p_ext->last_pnp_state = NotStarted;
@@ -428,15 +429,6 @@
if( NT_SUCCESS( status ) )
cl_set_pnp_state( p_ext, Started );
- /*
- * If we get the start request when we're already started, don't
- * re-initialize the stop lock.
- */
- if( p_ext->last_pnp_state != Started ) {
- CL_TRACE( CL_DBG_PNP, p_ext->dbg_lvl, ("IoInitializeRemoveLock: stop_lock %p[\n", &p_ext->stop_lock));
- IoInitializeRemoveLock( &p_ext->stop_lock, 'dtci', 0, 1000 );
- }
-
CL_EXIT( CL_DBG_PNP, p_ext->dbg_lvl );
return status;
}
@@ -557,12 +549,6 @@
if( p_ext->last_pnp_state == Started )
{
- /*
- * Re-initialize the stop lock before rolling back the PnP
- * state so that there's no contention while it's uninitialized.
- */
- CL_TRACE( CL_DBG_PNP, p_ext->dbg_lvl, ("IoInitializeRemoveLock: stop_lock %p[\n", &p_ext->stop_lock));
- IoInitializeRemoveLock( &p_ext->stop_lock, 'dtci', 0, 1000 );
#if 0
// leo: it seems like a bug, because it can never get released
{
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20090902/09c0dc19/attachment.html>
More information about the ofw
mailing list