Fix race conditions in initservice shutdown path.

Change-Id: I0da3c2050d5d64d20975031e093dd10978684e2b
Reviewed-on: http://gfw160.austin.ibm.com:8080/gerrit/6663
Tested-by: Jenkins Server
Reviewed-by: Daniel M. Crowell <dcrowell@us.ibm.com>
Reviewed-by: Brian H. Horton <brianh@linux.ibm.com>
Reviewed-by: Andrea Y. Ma <ayma@us.ibm.com>
Reviewed-by: A. Patrick Williams III <iawillia@us.ibm.com>

9 files changed, 162 insertions, 144 deletions

diff --git a/src/include/usr/initservice/initserviceif.H b/src/include/usr/initservice/initserviceif.H
index 9825e041d..05231fd59 100644
--- a/src/include/usr/initservice/initserviceif.H
+++ b/src/include/usr/initservice/initserviceif.H
@@ -84,6 +84,7 @@ bool unregisterShutdownEvent(msg_q_t i_msgQ);
  * @brief Perform necessary steps, such as FLUSHing, to registered blocks.
  *
  * @param[in] i_status - Shutdown status to be passed along on shutdown
+ * @param[in] i_inBackground - Shutdown should be handled by a background task.
  * @param[in] i_payload_base - The base address (target HRMOR) of the
  *                             payload.
  * @param[in] i_payload_entry - The offset from base address of the
@@ -92,26 +93,16 @@ bool unregisterShutdownEvent(msg_q_t i_msgQ);
  *
  * @return Nothing
  *
- * @note Never returns.
+ * @note If inBackground = true, the shutdown call will spawn a child task
+ *       which will do the shutdown processing.  If inBackground = false, the
+ *       shutdown processing will be handled in the context of the caller and
+ *       will never return.
  */
 void doShutdown ( uint64_t i_status,
+                  bool i_inBackground = false,
                   uint64_t i_payload_base = 0,
                   uint64_t i_payload_entry = 0,
-                  uint64_t i_payload_data = 0) NO_RETURN;
-
-
-/**
- * @brief Creates a separate thread and calls doShutdown
- *
- * @param[in] i_status - Shutdown status to be passed along on shutdown
- *
- * @return Nothing
- *
- * @Note: added for errlmanager to call so errlmanager continues
- * to run in case there are errors in the shutdown path
- *
- */
-void Shutdown( uint64_t i_status );
+                  uint64_t i_payload_data = 0);
 
 /**
  * @brief Returns if Service Processor Base Services are available
diff --git a/src/usr/errl/errlmanager.C b/src/usr/errl/errlmanager.C
index ed3a97452..269a5305f 100644
--- a/src/usr/errl/errlmanager.C
+++ b/src/usr/errl/errlmanager.C
@@ -278,7 +278,7 @@ void ErrlManager::errlogMsgHndlr ( void )
                                    "Terminating error was commited"
                                    " errlmanager is reqesting a shutdown.");
 
-                        INITSERVICE::Shutdown(l_err->plid());
+                        INITSERVICE::doShutdown(l_err->plid(), true);
 
                         TRACDCOMP( g_trac_errl,
                                 INFO_MRK"shutdown in progress" );
@@ -585,8 +585,13 @@ void ErrlManager::errlogShutdown(void)
     // Un-register error log message queue from the mailbox service
     MBOX::msgq_unregister( MBOX::HB_ERROR_MSGQ );
 
-    // destroy the queue
-    msg_q_destroy(iv_msgQ);
+    // Do not destroy the queue... there are paths where the daemon thread
+    // still has references to the queue or the unregisterShutdownEvent did
+    // not take effect because we were already in the middle of a system
+    // shutdown.
+    // Leaving this message queue around really isn't a leak because we are
+    // shutting down.
+    // msg_q_destroy(iv_msgQ);
 
     return;
 }
diff --git a/src/usr/hwas/hwasPlatCallout.C b/src/usr/hwas/hwasPlatCallout.C
index 95f2b4a47..7c7ea4732 100644
--- a/src/usr/hwas/hwasPlatCallout.C
+++ b/src/usr/hwas/hwasPlatCallout.C
@@ -130,7 +130,7 @@ errlHndl_t platHandleHWCallout(
             {
                 HWAS_ERR("master proc deconfigured - Shutdown due to plid 0x%X",
                         io_errl->plid());
-                INITSERVICE::doShutdown(io_errl->plid());
+                INITSERVICE::doShutdown(io_errl->plid(), true);
             }
         }
     } // PLD
diff --git a/src/usr/hwpf/hwp/start_payload/start_payload.C b/src/usr/hwpf/hwp/start_payload/start_payload.C
index 0d9031da8..9394dfaf1 100644
--- a/src/usr/hwpf/hwp/start_payload/start_payload.C
+++ b/src/usr/hwpf/hwp/start_payload/start_payload.C
@@ -551,6 +551,7 @@ errlHndl_t callShutdown ( void )
                    "callShutdown finished, shutdown = 0x%x.",
                    status );
         INITSERVICE::doShutdown( status,
+                                 false,
                                  payloadBase,
                                  payloadEntry,
                                  payloadData);
diff --git a/src/usr/initservice/baseinitsvc/initservice.C b/src/usr/initservice/baseinitsvc/initservice.C
index bf72d7179..fb8fc3484 100644
--- a/src/usr/initservice/baseinitsvc/initservice.C
+++ b/src/usr/initservice/baseinitsvc/initservice.C
@@ -42,6 +42,7 @@
 #include    <sys/sync.h>
 #include    <sys/mm.h>
 #include    <vmmconst.h>
+#include    <sys/time.h>
 
 #include    <errl/errludstring.H>
 
@@ -567,7 +568,7 @@ void InitService::init( void *io_ptr )
             l_shutdownStatus );
 
     //  Tell kernel to perform shutdown sequence
-    InitService::getTheInstance().doShutdown( l_shutdownStatus );
+    INITSERVICE::doShutdown( l_shutdownStatus );
 
     printk( "InitService exit.\n" );
     // return to _start() to exit the task.
@@ -580,8 +581,11 @@ InitService& InitService::getTheInstance( )
 }
 
 
-InitService::InitService( )
-{ }
+InitService::InitService( ) :
+    iv_shutdownInProgress(false)
+{
+    mutex_init(&iv_registryMutex);
+}
 
 
 InitService::~InitService( )
@@ -595,86 +599,97 @@ void registerBlock(void* i_vaddr, uint64_t i_size, BlockPriority i_priority)
 void InitService::registerBlock(void* i_vaddr, uint64_t i_size,
                                 BlockPriority i_priority)
 {
-    //Order priority from largest to smallest upon inserting
-    std::vector<regBlock_t*>::iterator regBlock_iter = iv_regBlock.begin();
-    for (; regBlock_iter!=iv_regBlock.end(); ++regBlock_iter)
+    mutex_lock(&iv_registryMutex);
+
+    if (!iv_shutdownInProgress)
     {
-        if ((uint64_t)i_priority >= (*regBlock_iter)->priority)
+
+        //Order priority from largest to smallest upon inserting
+        std::vector<regBlock_t*>::iterator regBlock_iter = iv_regBlock.begin();
+        for (; regBlock_iter!=iv_regBlock.end(); ++regBlock_iter)
         {
-            iv_regBlock.insert(regBlock_iter,
-                               new regBlock_t(i_vaddr,i_size,
-                                              (uint64_t)i_priority));
-            regBlock_iter=iv_regBlock.begin();
-            break;
+            if ((uint64_t)i_priority >= (*regBlock_iter)->priority)
+            {
+                iv_regBlock.insert(regBlock_iter,
+                        new regBlock_t(i_vaddr,i_size,
+                            (uint64_t)i_priority));
+                regBlock_iter=iv_regBlock.begin();
+                break;
+            }
+        }
+        if (regBlock_iter == iv_regBlock.end())
+        {
+            iv_regBlock.push_back(new regBlock_t(i_vaddr,i_size,
+                        (uint64_t)i_priority));
         }
     }
-    if (regBlock_iter == iv_regBlock.end())
-    {
-        iv_regBlock.push_back(new regBlock_t(i_vaddr,i_size,
-                                             (uint64_t)i_priority));
-    }
-}
-
-
-void Shutdown(uint64_t i_status )
-{
-    void * plid = new uint64_t;
-
-    *((uint64_t *)plid) = i_status;
-
-    // spawn a detached thread to handle the shutdown
-    // request - need to do this because the initservice
-    // is going to try and send a sync message to the errl
-    // manager to shutdown
-    tid_t l_tid = task_create(
-            &InitService::Shutdown, plid );
-
-    TRACFCOMP( g_trac_initsvc,
-            INFO_MRK"shutdown tid=%d", l_tid );
 
+    mutex_unlock(&iv_registryMutex);
 }
 
-void * InitService::Shutdown( void * i_args )
-{
-
-    TRACFCOMP( g_trac_initsvc, ENTER_MRK"Shutdown()" );
-
-    // detach the process from the calling process.
-    task_detach();
-
-    uint64_t plid = *(reinterpret_cast<uint64_t*>(i_args));
 
-    TRACDCOMP( g_trac_initsvc, "plid 0x%x", plid );
-    // request a shutdown, passing in the terminating
-    // error plid as the status.
-    INITSERVICE::doShutdown( plid );
+void doShutdown(uint64_t i_status,
+                bool i_inBackground,
+                uint64_t i_payload_base,
+                uint64_t i_payload_entry,
+                uint64_t i_payload_data)
+{
+    class ShutdownExecute
+    {
+        public:
+            ShutdownExecute(uint64_t i_status,
+                            uint64_t i_payload_base,
+                            uint64_t i_payload_entry,
+                            uint64_t i_payload_data)
+                : status(i_status),
+                  payload_base(i_payload_base),
+                  payload_entry(i_payload_entry),
+                  payload_data(i_payload_data)
+            { }
+
+            void execute()
+            {
+                Singleton<InitService>::instance().doShutdown(status,
+                                                              payload_base,
+                                                              payload_entry,
+                                                              payload_data);
+            }
+            void startThread()
+            {
+                task_create(ShutdownExecute::run, this);
+            }
 
-    // delete the storage for the plid;
-    delete ((uint64_t *)i_args);
+        private:
+            uint64_t status;
+            uint64_t payload_base;
+            uint64_t payload_entry;
+            uint64_t payload_data;
 
-    i_args = NULL;
+            static void* run(void* _self)
+            {
+                task_detach();
 
-    TRACFCOMP( g_trac_initsvc, EXIT_MRK"Shutdown()" );
-
-    return i_args;
-}
+                ShutdownExecute* self =
+                    reinterpret_cast<ShutdownExecute*>(_self);
 
+                self->execute();
 
+                return NULL;
+            }
+    };
 
-void doShutdown ( uint64_t i_status,
-                  uint64_t i_payload_base,
-                  uint64_t i_payload_entry,
-                  uint64_t i_payload_data)
-{
-    Singleton<InitService>::instance().doShutdown( i_status,
-                                                   i_payload_base,
-                                                   i_payload_entry,
-                                                   i_payload_data);
+    ShutdownExecute* s = new ShutdownExecute(i_status, i_payload_base,
+                                             i_payload_entry, i_payload_data);
 
-    while(1)
+    if (i_inBackground)
     {
-        task_yield();
-    };
+        s->startThread();
+    }
+    else
+    {
+        s->execute();
+        while(1) nanosleep(1,0);
+    }
 }
 
 void InitService::doShutdown(uint64_t i_status,
@@ -685,6 +700,17 @@ void InitService::doShutdown(uint64_t i_status,
     int l_rc = 0;
     errlHndl_t l_err = NULL;
 
+    // Ensure no one is manpulating the registry lists and that only one
+    // thread actually executes the shutdown path.
+    mutex_lock(&iv_registryMutex);
+    if (iv_shutdownInProgress)
+    {
+        mutex_unlock(&iv_registryMutex);
+        return;
+    }
+    iv_shutdownInProgress = true;
+    mutex_unlock(&iv_registryMutex);
+
     // Call registered services and notify of shutdown
     msg_t * l_msg = msg_allocate();
     l_msg->data[0] = i_status;
@@ -699,7 +725,7 @@ void InitService::doShutdown(uint64_t i_status,
         msg_sendrecv(i->msgQ,l_msg);
     }
 
-     msg_free(l_msg);
+    msg_free(l_msg);
 
     std::vector<regBlock_t*>::iterator l_rb_iter = iv_regBlock.begin();
     //FLUSH each registered block in order
@@ -739,29 +765,38 @@ bool InitService::registerShutdownEvent(msg_q_t i_msgQ,
                                         EventPriority_t i_priority)
 {
     bool result = true;
-    EventRegistry_t::iterator in_pos = iv_regMsgQ.end();
 
-    for(EventRegistry_t::iterator r = iv_regMsgQ.begin();
-        r != iv_regMsgQ.end();
-        ++r)
+    mutex_lock(&iv_registryMutex);
+
+    if (!iv_shutdownInProgress)
     {
-        if(r->msgQ == i_msgQ)
+
+        EventRegistry_t::iterator in_pos = iv_regMsgQ.end();
+
+        for(EventRegistry_t::iterator r = iv_regMsgQ.begin();
+                r != iv_regMsgQ.end();
+                ++r)
         {
-            result = false;
-            break;
+            if(r->msgQ == i_msgQ)
+            {
+                result = false;
+                break;
+            }
+
+            if(r->msgPriority <= (uint32_t)i_priority)
+            {
+                in_pos = r;
+            }
         }
 
-        if(r->msgPriority <= (uint32_t)i_priority)
+        if(result)
         {
-            in_pos = r;
+            in_pos = iv_regMsgQ.insert(in_pos,
+                            regMsgQ_t(i_msgQ, i_msgType, i_priority));
         }
     }
 
-    if(result)
-    {
-        in_pos = iv_regMsgQ.insert(in_pos,
-                                   regMsgQ_t(i_msgQ, i_msgType, i_priority));
-    }
+    mutex_unlock(&iv_registryMutex);
 
     return result;
 }
@@ -769,17 +804,27 @@ bool InitService::registerShutdownEvent(msg_q_t i_msgQ,
 bool InitService::unregisterShutdownEvent(msg_q_t i_msgQ)
 {
     bool result = false;
-    for(EventRegistry_t::iterator r = iv_regMsgQ.begin();
-        r != iv_regMsgQ.end();
-        ++r)
+
+    mutex_lock(&iv_registryMutex);
+
+    if (!iv_shutdownInProgress)
     {
-        if(r->msgQ == i_msgQ)
+
+        for(EventRegistry_t::iterator r = iv_regMsgQ.begin();
+                r != iv_regMsgQ.end();
+                ++r)
         {
-            result = true;
-            iv_regMsgQ.erase(r);
-            break;
+            if(r->msgQ == i_msgQ)
+            {
+                result = true;
+                iv_regMsgQ.erase(r);
+                break;
+            }
         }
     }
+
+    mutex_unlock(&iv_registryMutex);
+
     return result;
 }
 
diff --git a/src/usr/initservice/baseinitsvc/initservice.H b/src/usr/initservice/baseinitsvc/initservice.H
index 4b48d528f..26e620b95 100644
--- a/src/usr/initservice/baseinitsvc/initservice.H
+++ b/src/usr/initservice/baseinitsvc/initservice.H
@@ -191,7 +191,7 @@ public:
 
     /**
      * @brief Un register a service for a Shutdown event
-     * 
+     *
      * @param[in] i_msgQ, The message queue to be removed.
      *
      * @return true  - i_msgQ was removed from the event notification list. |
@@ -219,19 +219,6 @@ public:
                       uint64_t i_payload_entry = 0,
                       uint64_t i_payload_data = 0);
 
-    /**
-     * @brief Creates detatched thread and calls doShutdown
-     *
-     * @param[in] i_status - Shutdown status to be passed along on shutdown
-     *
-     * @return Nothing
-     * @note Added to enable errl manager to continue to run in termination
-     * path
-     */
-
-static void * Shutdown( void * io_args );
-
-
 protected:
 
     /**
@@ -309,6 +296,9 @@ private:
     // List of Services to notify on shutdown
     EventRegistry_t iv_regMsgQ;
 
+    mutex_t iv_registryMutex;
+    bool iv_shutdownInProgress;
+
 };  //  class InitService
 
 } // namespace INITSERVICE
diff --git a/src/usr/initservice/extinitsvc/extinitsvc.C b/src/usr/initservice/extinitsvc/extinitsvc.C
index c6a48fa28..323624681 100644
--- a/src/usr/initservice/extinitsvc/extinitsvc.C
+++ b/src/usr/initservice/extinitsvc/extinitsvc.C
@@ -5,7 +5,7 @@
 /*                                                                        */
 /* IBM CONFIDENTIAL                                                       */
 /*                                                                        */
-/* COPYRIGHT International Business Machines Corp. 2011,2012              */
+/* COPYRIGHT International Business Machines Corp. 2011,2013              */
 /*                                                                        */
 /* p1                                                                     */
 /*                                                                        */
@@ -116,12 +116,11 @@ void ExtInitSvc::init( errlHndl_t   &io_rtaskRetErrl )
 
         //  Tell the kernel to shut down.  This will not actually
         //  happen until the last thread has ended.
-        InitService::getTheInstance().doShutdown(
-                                      SHUTDOWN_STATUS_EXTINITSVC_FAILED);
+        INITSERVICE::doShutdown(SHUTDOWN_STATUS_EXTINITSVC_FAILED);
 
         //  end the task.
         io_rtaskRetErrl=NULL;
-        return; 
+        return;
 #endif
 
         //  end the task and pass the errorlog to initservice to be committed.
@@ -131,7 +130,7 @@ void ExtInitSvc::init( errlHndl_t   &io_rtaskRetErrl )
                 l_errl );
 
         io_rtaskRetErrl=l_errl;
-        return; 
+        return;
     }
 
     //  finish things up, return to initservice with goodness.
diff --git a/src/usr/pnor/pnordd.C b/src/usr/pnor/pnordd.C
index 69ca4baf9..d05e66a28 100644
--- a/src/usr/pnor/pnordd.C
+++ b/src/usr/pnor/pnordd.C
@@ -600,7 +600,7 @@ errlHndl_t PnorDD::writeRegSfc(SfcRange i_range,
                                uint32_t i_data)
 {
     errlHndl_t l_err = NULL;
-    uint32_t lpc_addr;
+    uint32_t lpc_addr = 0;
 
     switch(i_range)
     {
@@ -646,7 +646,7 @@ errlHndl_t PnorDD::readRegSfc(SfcRange i_range,
                               uint32_t& o_data)
 {
     errlHndl_t l_err = NULL;
-    uint32_t lpc_addr;
+    uint32_t lpc_addr = 0;
 
     switch(i_range)
     {
diff --git a/src/usr/pnor/pnorrp.C b/src/usr/pnor/pnorrp.C
index c13f4a306..dd782058f 100644
--- a/src/usr/pnor/pnorrp.C
+++ b/src/usr/pnor/pnorrp.C
@@ -142,19 +142,6 @@ void* wait_for_message( void* unused )
     return NULL;
 }
 
-/**
- * @brief  Static function wrapper to call doShutdown
- *   to avoid deadlock in main task
- */
-void* pnor_shutdown( void* unused )
-{
-    TRACFCOMP(g_trac_pnor, "pnor_shutdown> " );
-    printk( "PNOR errors causing shutdown\n" );
-    INITSERVICE::doShutdown( PNOR::RC_ECC_UE );
-    return NULL;
-}
-
-
 /********************
  Private/Protected Methods
  ********************/
@@ -727,7 +714,7 @@ errlHndl_t PnorRP::readFromDevice( uint64_t i_offset,
                 //  that happen during shutdown.
                 iv_shutdownUE = true;
                 o_fatalError = true;
-                task_create( pnor_shutdown, NULL );
+                INITSERVICE::doShutdown( PNOR::RC_ECC_UE, true );
             }
             // found an error so we need to fix something
             else if( ecc_stat != PNOR::ECC::CLEAN )