Handle deadlock in LPC error path

Fixed a mutex issue in the error path of the LPC driver.

Change-Id: I59afed0654ee58d34cfc3d34d6c1d6e31bc4cb22
RTC: 115682
Reviewed-on: http://gfw160.aus.stglabs.ibm.com:8080/gerrit/13566
Reviewed-by: Michael Baiocchi <baiocchi@us.ibm.com>
Tested-by: Jenkins Server
Reviewed-by: Corey V. Swenson <cswenson@us.ibm.com>
Reviewed-by: A. Patrick Williams III <iawillia@us.ibm.com>

6 files changed, 368 insertions, 87 deletions

diff --git a/src/makefile b/src/makefile
index 33e74f778..3dacfc1ba 100644
--- a/src/makefile
+++ b/src/makefile
@@ -199,6 +199,7 @@ TESTCASE_MODULES += testsecureboot
 TESTCASE_MODULES += testfsiscom
 TESTCASE_MODULES += testrtloader
 TESTCASE_MODULES += testsbe
+TESTCASE_MODULES += testlpc
 
 RUNTIME_OBJECTS += rt_start.o
 RUNTIME_OBJECTS += rt_main.o
diff --git a/src/usr/lpc/lpcdd.C b/src/usr/lpc/lpcdd.C
index 6c57f6010..9124f0803 100644
--- a/src/usr/lpc/lpcdd.C
+++ b/src/usr/lpc/lpcdd.C
@@ -249,25 +249,6 @@ DEVICE_REGISTER_ROUTE( DeviceFW::WRITE,
                        TARGETING::TYPE_PROC,
                        lpcWrite );
 
-/**
- * STATIC
- * @brief Initialize driver and hardware to ready state
- */
-static void init( errlHndl_t& io_rtaskRetErrl )
-{
-    TRACFCOMP(g_trac_lpc, "LPC::init> " );
-    errlHndl_t l_errl = NULL;
-
-    // Initialize the hardware
-    l_errl = Singleton<LpcDD>::instance().hwReset(LpcDD::RESET_INIT);
-    if( l_errl )
-    {
-        TRACFCOMP( g_trac_lpc, "Errors initializing LPC logic... Beware! PLID=%.8X", l_errl->plid() );
-    }
-
-    io_rtaskRetErrl = l_errl;
-}
-
 
 /**
  * @brief Create/delete software objects to support non-master access
@@ -385,11 +366,6 @@ errlHndl_t create_altmaster_objects( bool i_create,
 ///////////////////////////////////////////////////////////////////////////////
 ///////////////////////////////////////////////////////////////////////////////
 
-/**
- * @brief   set up _start() task entry procedure
- */
-TASK_ENTRY_MACRO( LPC::init );
-
 mutex_t LpcDD::cv_mutex = MUTEX_INITIALIZER;
 
 LpcDD::LpcDD( TARGETING::Target* i_proc )
@@ -400,6 +376,8 @@ LpcDD::LpcDD( TARGETING::Target* i_proc )
 ,iv_errorRecoveryFailed(false)
 ,iv_resetActive(false)
 {
+    TRACFCOMP(g_trac_lpc, "LpcDD::LpcDD> " );
+
     mutex_init( &iv_mutex );
 
     if( i_proc == TARGETING::MASTER_PROCESSOR_CHIP_TARGET_SENTINEL )
@@ -408,27 +386,23 @@ LpcDD::LpcDD( TARGETING::Target* i_proc )
     }
     else
     {
-        // Check if processor is MASTER
-        TARGETING::ATTR_PROC_MASTER_TYPE_type type_enum =
-          iv_proc->getAttr<TARGETING::ATTR_PROC_MASTER_TYPE>();
-        if ( type_enum == TARGETING::PROC_MASTER_TYPE_ACTING_MASTER )
-        {
-            // Master target needs global mutex to avoid collisions with
-            // default singleton object used for ddRead/ddWrite with
-            // TARGETING::MASTER_PROCESSOR_CHIP_TARGET_SENTINEL
-            ivp_mutex = &cv_mutex;
-            // IMPORTANT :: cv_mutex can never be held while accessing any
-            //  code outside of the base image or this could lead to deadlocks
-            //  accessing PNOR
-        }
-        else
-        {
-            // Just use the local mutex
-            ivp_mutex = &iv_mutex;
-        }
+        // Master target could collide and cause deadlocks with singleton
+        // used for ddRead/ddWrite with MASTER_PROCESSOR_CHIP_TARGET_SENTINEL
+        TARGETING::Target* master = NULL;
+        TARGETING::targetService().masterProcChipTargetHandle( master );
+        assert( i_proc != master );
+
+        // Just use the local mutex
+        ivp_mutex = &iv_mutex;
     }
 
-
+    // Initialize the hardware
+    errlHndl_t l_errl = hwReset(LpcDD::RESET_INIT);
+    if( l_errl )
+    {
+        TRACFCOMP( g_trac_lpc, "Errors initializing LPC logic... Beware! PLID=%.8X", l_errl->plid() );
+        errlCommit(l_errl, LPC_COMP_ID);
+    }
 }
 
 LpcDD::~LpcDD()
@@ -490,10 +464,10 @@ errlHndl_t LpcDD::hwReset( ResetLevels i_resetLevel )
 
                         // First read OPB_MASTER_LS_CONTROL_REG
                         uint32_t lpc_data = 0x0;
-                        l_err = readLPC( LPC::TRANS_REG,
-                                         OPB_MASTER_LS_CONTROL_REG,
-                                         &lpc_data,
-                                         opsize );
+                        l_err = _readLPC( LPC::TRANS_REG,
+                                          OPB_MASTER_LS_CONTROL_REG,
+                                          &lpc_data,
+                                          opsize );
 
                         if (l_err) { break; }
 
@@ -501,10 +475,10 @@ errlHndl_t LpcDD::hwReset( ResetLevels i_resetLevel )
                         // - set bit 29 to 0b1
                         lpc_data |= 0x00000004;
 
-                        l_err = writeLPC( LPC::TRANS_REG,
-                                          OPB_MASTER_LS_CONTROL_REG,
-                                          &lpc_data,
-                                          opsize );
+                        l_err = _writeLPC( LPC::TRANS_REG,
+                                           OPB_MASTER_LS_CONTROL_REG,
+                                           &lpc_data,
+                                           opsize );
                         if (l_err) { break; }
 
 
@@ -512,10 +486,10 @@ errlHndl_t LpcDD::hwReset( ResetLevels i_resetLevel )
                         // No wait-time needed
                         lpc_data &= 0xFFFFFFFB;
 
-                        l_err = writeLPC( LPC::TRANS_REG,
-                                          OPB_MASTER_LS_CONTROL_REG,
-                                          &lpc_data,
-                                          opsize );
+                        l_err = _writeLPC( LPC::TRANS_REG,
+                                           OPB_MASTER_LS_CONTROL_REG,
+                                           &lpc_data,
+                                           opsize );
 
                         if (l_err) { break; }
 
@@ -538,10 +512,10 @@ errlHndl_t LpcDD::hwReset( ResetLevels i_resetLevel )
 
                         size_t opsize = sizeof(uint32_t);
                         uint32_t lpc_data = 0x0;
-                        l_err = writeLPC( LPC::TRANS_REG,
-                                          LPCHC_RESET_REG,
-                                          &lpc_data,
-                                          opsize );
+                        l_err = _writeLPC( LPC::TRANS_REG,
+                                           LPCHC_RESET_REG,
+                                           &lpc_data,
+                                           opsize );
                         if (l_err) { break; }
 
                         // sleep 1ms for LPCHC to execute internal
@@ -600,7 +574,10 @@ errlHndl_t LpcDD::hwReset( ResetLevels i_resetLevel )
             else
             {
                 // Successful, so increment recovery count
-                iv_errorHandledCount++;
+                if( i_resetLevel != RESET_CLEAR )
+                {
+                    iv_errorHandledCount++;
+                }
 
                 TRACFCOMP( g_trac_lpc,INFO_MRK"LpcDD::hwReset> Successful LPC reset at level 0x%X (recovery count=%d)", i_resetLevel, iv_errorHandledCount);
             }
@@ -691,25 +668,21 @@ errlHndl_t LpcDD::checkAddr(LPC::TransType i_type,
 }
 
 /**
- * @brief Read an address from LPC space
+ * @brief Read an address from LPC space, assumes lock is already held
  */
-errlHndl_t LpcDD::readLPC(LPC::TransType i_type,
-                          uint32_t i_addr,
-                          void* o_buffer,
-                          size_t& io_buflen)
+errlHndl_t LpcDD::_readLPC(LPC::TransType i_type,
+                           uint32_t i_addr,
+                           void* o_buffer,
+                           size_t& io_buflen)
 {
     errlHndl_t l_err = NULL;
     uint32_t l_addr = 0;
-    bool need_unlock = false;
 
     do {
         // Generate the full absolute LPC address
         l_err = checkAddr( i_type, i_addr, &l_addr );
         if( l_err ) { break; }
 
-        mutex_lock(ivp_mutex);
-        need_unlock = true;
-
         // Execute command.
         ControlReg_t eccb_cmd;
         eccb_cmd.data_len = io_buflen;
@@ -737,9 +710,6 @@ errlHndl_t LpcDD::readLPC(LPC::TransType i_type,
         }
         else
         {
-            mutex_unlock(ivp_mutex);
-            need_unlock = false;
-
             TRACFCOMP( g_trac_lpc, "readLPC> Unsupported buffer size : %d", io_buflen );
             /*@
              * @errortype
@@ -748,7 +718,7 @@ errlHndl_t LpcDD::readLPC(LPC::TransType i_type,
              * @userdata1[0:31]   LPC Address
              * @userdata1[32:63]  LPC Transaction Type
              * @userdata2    Requested buffer size
-             * @devdesc      LpcDD::readLPC> Invalid buffer size requested
+             * @devdesc      LpcDD::_readLPC> Invalid buffer size requested
              *               (>4 bytes)
              * @custdesc     Firmware error accessing internal bus during IPL
              */
@@ -764,23 +734,21 @@ errlHndl_t LpcDD::readLPC(LPC::TransType i_type,
 
     } while(0);
 
-    if( need_unlock ) { mutex_unlock(ivp_mutex); };
-
     LPC_TRACFCOMP( g_trac_lpc, "readLPC> %08X[%d] = %08X", l_addr, io_buflen, *reinterpret_cast<uint32_t*>( o_buffer )  >> (8 * (4 - io_buflen)) );
 
     return l_err;
 }
 
 /**
- * @brief Write an address from LPC space
+ * @brief Write an address from LPC space, assumes lock is already held
  */
-errlHndl_t LpcDD::writeLPC(LPC::TransType i_type, uint32_t i_addr,
-                           const void* i_buffer,
-                           size_t& io_buflen)
+errlHndl_t LpcDD::_writeLPC(LPC::TransType i_type,
+                            uint32_t i_addr,
+                            const void* i_buffer,
+                            size_t& io_buflen)
 {
     errlHndl_t l_err = NULL;
     uint32_t l_addr = 0;
-    bool need_unlock = false;
 
     do {
         // Generate the full absolute LPC address
@@ -812,9 +780,6 @@ errlHndl_t LpcDD::writeLPC(LPC::TransType i_type, uint32_t i_addr,
         LPC_TRACFCOMP(g_trac_lpc, "writeLPC> %08X[%d] = %08X", l_addr, io_buflen,
                       eccb_data >> (32 + 8 * (4 - io_buflen)));
 
-        mutex_lock(ivp_mutex);
-        need_unlock = true;
-
         // Write data out
         size_t scom_size = sizeof(uint64_t);
         l_err = deviceOp( DeviceFW::WRITE,
@@ -844,8 +809,6 @@ errlHndl_t LpcDD::writeLPC(LPC::TransType i_type, uint32_t i_addr,
 
     } while(0);
 
-    if( need_unlock ) { mutex_unlock(ivp_mutex); }
-
     return l_err;
 }
 
@@ -1180,3 +1143,32 @@ errlHndl_t LpcDD::checkForOpbErrors( ResetLevels &o_resetLevel )
 }
 
 
+/**
+ * @brief Read an address from LPC space
+ */
+errlHndl_t LpcDD::readLPC(LPC::TransType i_type,
+                          uint32_t i_addr,
+                          void* o_buffer,
+                          size_t& io_buflen)
+{
+    // Grab the lock and call the internal function
+    mutex_lock(ivp_mutex);
+    errlHndl_t l_err = _readLPC( i_type, i_addr, o_buffer, io_buflen );
+    mutex_unlock(ivp_mutex);
+    return l_err;
+}
+
+/**
+ * @brief Write an address to LPC space
+ */
+errlHndl_t LpcDD::writeLPC(LPC::TransType i_type,
+                           uint32_t i_addr,
+                           const void* i_buffer,
+                           size_t& io_buflen)
+{
+    // Grab the lock and call the internal function
+    mutex_lock(ivp_mutex);
+    errlHndl_t l_err = _writeLPC( i_type, i_addr, i_buffer, io_buflen );
+    mutex_unlock(ivp_mutex);
+    return l_err;
+}
diff --git a/src/usr/lpc/lpcdd.H b/src/usr/lpc/lpcdd.H
index 598c5fab8..074c5da98 100644
--- a/src/usr/lpc/lpcdd.H
+++ b/src/usr/lpc/lpcdd.H
@@ -289,6 +289,38 @@ class LpcDD
      */
     void addFFDC(errlHndl_t& io_errl);
 
+    /**
+     * @brief Performs a LPC Read Operation, assumes lock is already held
+     *
+     * @param i_trans    LPC transaction type
+     * @param i_address  LPC address
+     * @param o_buffer   Buffer to read data into
+     * @param io_buflen  Input: Number of bytes to read,
+     *       Output: Number of bytes actually read
+     *
+     * @return Error from operation
+     */
+    errlHndl_t _readLPC(LPC::TransType i_type,
+                        uint32_t i_address,
+                        void* o_buffer,
+                        size_t& io_buflen);
+
+    /**
+     * @brief Performs a LPC Write Operation, assumes lock is already held
+     *
+     * @param i_trans    LPC transaction type
+     * @param i_address  LPC address
+     * @param i_buffer  Buffer to write data from
+     * @param io_buflen  Input: Number of bytes to write,
+     *       Output: Number of bytes actually written
+     *
+     * @return Error from operation
+     */
+    errlHndl_t _writeLPC(LPC::TransType i_type,
+                         uint32_t i_address,
+                         const void* i_buffer,
+                         size_t& io_buflen);
+
   private:
     /**
      * @brief Mutex to prevent concurrent LPC accesses to the master
diff --git a/src/usr/lpc/makefile b/src/usr/lpc/makefile
index 4fbad8436..63c1eb774 100644
--- a/src/usr/lpc/makefile
+++ b/src/usr/lpc/makefile
@@ -28,7 +28,6 @@ MODULE = lpc
 
 OBJS = lpcdd.o
 
-#@fixme - RTC:107788 Add tests after 37744 is merged
-#SUBDIRS = test.d
+SUBDIRS = test.d
 
 include ${ROOTPATH}/config.mk
diff --git a/src/usr/lpc/test/lpcddtest.H b/src/usr/lpc/test/lpcddtest.H
new file mode 100644
index 000000000..0883f83e7
--- /dev/null
+++ b/src/usr/lpc/test/lpcddtest.H
@@ -0,0 +1,227 @@
+/* IBM_PROLOG_BEGIN_TAG                                                   */
+/* This is an automatically generated prolog.                             */
+/*                                                                        */
+/* $Source: src/usr/lpc/test/lpcddtest.H $                                */
+/*                                                                        */
+/* OpenPOWER HostBoot Project                                             */
+/*                                                                        */
+/* Contributors Listed Below - COPYRIGHT 2014                             */
+/* [+] International Business Machines Corp.                              */
+/*                                                                        */
+/*                                                                        */
+/* Licensed under the Apache License, Version 2.0 (the "License");        */
+/* you may not use this file except in compliance with the License.       */
+/* You may obtain a copy of the License at                                */
+/*                                                                        */
+/*     http://www.apache.org/licenses/LICENSE-2.0                         */
+/*                                                                        */
+/* Unless required by applicable law or agreed to in writing, software    */
+/* distributed under the License is distributed on an "AS IS" BASIS,      */
+/* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or        */
+/* implied. See the License for the specific language governing           */
+/* permissions and limitations under the License.                         */
+/*                                                                        */
+/* IBM_PROLOG_END_TAG                                                     */
+#ifndef __LPCDDTEST_H
+#define __LPCDDTEST_H
+
+/**
+ *  @file lpcddtest.H
+ *
+ *  @brief Test case for LPC Device Driver
+*/
+
+#include <cxxtest/TestSuite.H>
+#include <errl/errlmanager.H>
+#include <errl/errlentry.H>
+#include <devicefw/userif.H>
+#include <lpc/lpcif.H>
+#include <kernel/console.H>
+
+extern trace_desc_t* g_trac_lpc;
+
+class LpcDdTest : public CxxTest::TestSuite
+{
+  private:
+    // Test task for bad size
+    static void* task_badsize(void * ignored)
+    {
+        errlHndl_t l_err = NULL;
+        TARGETING::Target* sentinel =
+          TARGETING::MASTER_PROCESSOR_CHIP_TARGET_SENTINEL;
+        size_t opsize = 3;
+        uint8_t data[12];
+        l_err = deviceRead( sentinel,
+                            data,
+                            opsize,
+                            DEVICE_LPC_ADDRESS(LPC::TRANS_ABS,0x12345678) );
+        TS_FAIL("LpcDD::task_badsize> Task should have died");
+        return l_err;
+    };
+
+  public:
+
+    /**
+     * @brief LPC Bad Args
+     *        Test handling of bad arguments
+     */
+    void test_badargs(void)
+    {
+        TRACFCOMP( g_trac_lpc, "LpcDdTest::test_badargs>" );
+        errlHndl_t l_err = NULL;
+
+        TARGETING::Target* master = NULL;
+        TARGETING::targetService().masterProcChipTargetHandle( master );
+        size_t opsize = 0;
+        uint8_t data[12];
+
+        // Invalid target
+        opsize = 4;
+        l_err = deviceRead( master,
+                            data,
+                            opsize,
+                            DEVICE_LPC_ADDRESS(LPC::TRANS_ABS,0x12345678) );
+        if( l_err )
+        {
+            delete l_err;
+        }
+        else
+        {
+            TS_FAIL("LpcDD::test_badargs> No error from bad target");
+        }
+
+        // Invalid size
+        // Expects an assert so need to create a task
+        int l_childsts = 0;
+        void* l_childerrl = NULL;
+        printk("\nExpect to see exception for assert! ");
+        tid_t tid1 = task_create( task_badsize, NULL );
+        tid_t tid2 = task_wait_tid( tid1,
+                                    &l_childsts,
+                                    &l_childerrl);
+        if( (tid2 != tid1)
+            || (l_childsts == TASK_STATUS_EXITED_CLEAN) )
+        {
+            TS_FAIL("LpcDD::test_badargs> No error from bad target");
+        }
+        else if( l_childerrl )
+        {
+            delete ((errlHndl_t)l_childerrl);
+        }
+    }
+
+
+
+    /**
+     * @brief LPC Alt Master
+     *        Test creation/deletion of altmaster objects
+     */
+    void test_altmaster(void)
+    {
+        TRACFCOMP( g_trac_lpc, "LpcDdTest::test_altmaster>" );
+        errlHndl_t l_err = NULL;
+        TARGETING::Target* master = NULL;
+        TARGETING::targetService().masterProcChipTargetHandle( master );
+
+        // Should get error if we use the master proc
+        l_err = LPC::create_altmaster_objects( true, master );
+        if( !l_err )
+        {
+            TS_FAIL("LpcDD::test_altmaster> No error from master target");
+        }
+        else
+        {
+            delete l_err;
+        }
+
+        // Should get error if we use the sentinel master
+        l_err = LPC::create_altmaster_objects( true,
+                 TARGETING::MASTER_PROCESSOR_CHIP_TARGET_SENTINEL );
+        if( !l_err )
+        {
+            TS_FAIL("LpcDD::test_altmaster> No error from sentinel target");
+        }
+        else
+        {
+            delete l_err;
+        }
+
+        // Should not get error doing a NOOP destroy
+        l_err = LPC::create_altmaster_objects( false, NULL );
+        if( l_err )
+        {
+            TS_FAIL("LpcDD::test_altmaster> Error from destroy");
+            errlCommit( l_err, LPC_COMP_ID );
+        }
+
+        // find other processor target (physical:sys-0/node-0/proc-1)
+        TARGETING::Target* proc1 = NULL;
+        TARGETING::EntityPath epath(TARGETING::EntityPath::PATH_PHYSICAL);
+        epath.addLast(TARGETING::TYPE_SYS,0);
+        epath.addLast(TARGETING::TYPE_NODE,0);
+        epath.addLast(TARGETING::TYPE_PROC,1);
+        proc1 = TARGETING::targetService().toTarget(epath);
+        if( proc1 )
+        {
+            // Should not get error using proc1
+            l_err = LPC::create_altmaster_objects( true, proc1 );
+            if( l_err )
+            {
+                TS_FAIL("LpcDD::test_altmaster> Error from proc1 create");
+                errlCommit( l_err, LPC_COMP_ID );
+            }
+
+            // Should get error if we create twice in a row
+            l_err = LPC::create_altmaster_objects( true, proc1 );
+            if( !l_err )
+            {
+                TS_FAIL("LpcDD::test_altmaster> No error from double create");
+            }
+            else
+            {
+                delete l_err;
+            }
+
+            // Should not get error when we delete
+            l_err = LPC::create_altmaster_objects( false, proc1 );
+            if( l_err )
+            {
+                TS_FAIL("LpcDD::test_altmaster> Error from proc1 delete");
+                errlCommit( l_err, LPC_COMP_ID );
+            }
+        }
+    }
+
+
+    //DISABLED because Sim doesn't do what we expect
+    /**
+     * @brief LPC HW Failure
+     *        Test handling of hw error
+     */
+    void _test_hwfail(void)
+    {
+        TRACFCOMP( g_trac_lpc, "LpcDdTest::test_hwfail>" );
+        errlHndl_t l_err = NULL;
+
+        TARGETING::Target* sentinel =
+          TARGETING::MASTER_PROCESSOR_CHIP_TARGET_SENTINEL;
+        size_t opsize = 4;
+        uint8_t data[12];
+
+        // pass a bad address
+        l_err = deviceRead( sentinel,
+                            data,
+                            opsize,
+                            DEVICE_LPC_ADDRESS(LPC::TRANS_ABS,0x12345678) );
+        if( l_err )
+        {
+            delete l_err;
+        }
+        else
+        {
+            TS_FAIL("LpcDD::test_hwfail> No error from bad address");
+        }
+    }
+};
+
+#endif
diff --git a/src/usr/lpc/test/makefile b/src/usr/lpc/test/makefile
new file mode 100644
index 000000000..4d034cc84
--- /dev/null
+++ b/src/usr/lpc/test/makefile
@@ -0,0 +1,30 @@
+# IBM_PROLOG_BEGIN_TAG
+# This is an automatically generated prolog.
+#
+# $Source: src/usr/lpc/test/makefile $
+#
+# OpenPOWER HostBoot Project
+#
+# Contributors Listed Below - COPYRIGHT 2014
+# [+] International Business Machines Corp.
+#
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied. See the License for the specific language governing
+# permissions and limitations under the License.
+#
+# IBM_PROLOG_END_TAG
+ROOTPATH = ../../../..
+
+MODULE = testlpc
+TESTS = *.H
+
+include ${ROOTPATH}/config.mk