[ofw][patch][IBBUS] crash on re-initialization of remove lock

[Smith, Stan]

Leo,
  Code review of the patch looks reasonable with the exception of the very last part

core/complib/kernel/cl_pnp_po.c

__cancel_stop()

<...>

 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
   {

#endif
 }

The issue is between your patch and the #if 0 the block  'if( p_ext->last_pnp_state == Started ) {}  contains no useful code.

Why not remove the entire  if block?

Will let you know how the testing goes.

stan.



________________________________
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/20090826/baeb365b/attachment.html>