Skip to content

Commit

Permalink
Add missing mutex in LPC error path
Browse files Browse the repository at this point in the history
There is a pre-check to look for errors before every LPC read or
write operation.  This check was running outside of the mutex,
which meant that if an operation caused an error and then another
thread attempted a separate LPC op, the second thread would end
up reporting the error from the first operation,  This would
typically just result in a double log, but there are cases where
an error is expected (the SIO problem for the hiomap protocol)
and is deleted by the caller.  Because of the second thread
seeing the error, the error condition ends up being a visible
log.

The change here is to move the pre-check inside the mutex so that
only 1 thread can ever be checking the LPC status at a time.

Change-Id: I9ce0ce48252b7d2b271aa5dd6e0a819dfb728a25
CQ: SW450825
Reviewed-on: http://rchgit01.rchland.ibm.com/gerrit1/68609
Tested-by: Jenkins Server <pfd-jenkins+hostboot@us.ibm.com>
Tested-by: Jenkins OP Build CI <op-jenkins+hostboot@us.ibm.com>
Tested-by: FSP CI Jenkins <fsp-CI-jenkins+hostboot@us.ibm.com>
Reviewed-by: Corey V. Swenson <cswenson@us.ibm.com>
Reviewed-by: Christian R. Geddes <crgeddes@us.ibm.com>
Reviewed-by: William G. Hoffa <wghoffa@us.ibm.com>
Tested-by: Jenkins OP HW <op-hw-jenkins+hostboot@us.ibm.com>
Reviewed-by: Nicholas E. Bofferding <bofferdn@us.ibm.com>
  • Loading branch information
dcrowell77 authored and Nicholas E. Bofferding committed Nov 14, 2018
1 parent 1aae1ba commit 9d418f5
Showing 1 changed file with 60 additions and 27 deletions.
87 changes: 60 additions & 27 deletions src/usr/lpc/lpcdd.C
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,6 @@ errlHndl_t lpcRead(DeviceFW::OperationType i_opType,
// then we have to use our special side copy of the driver
if( i_target == TARGETING::MASTER_PROCESSOR_CHIP_TARGET_SENTINEL )
{
//First check/clear the LPC bus of errors and commit any errors found
l_err = Singleton<LpcDD>::instance().checkForLpcErrors();
if (l_err)
{
errlCommit(l_err, LPC_COMP_ID);
}

l_err = Singleton<LpcDD>::instance().readLPC( l_type,
l_addr,
io_buffer,
Expand All @@ -126,14 +119,6 @@ errlHndl_t lpcRead(DeviceFW::OperationType i_opType,
if( g_altLpcDD
&& (i_target == g_altLpcDD->getProc()) )
{
//First check/clear the LPC bus of errors and commit
// any errors found
l_err = g_altLpcDD->checkForLpcErrors();
if (l_err)
{
errlCommit(l_err, LPC_COMP_ID);
}

l_err = g_altLpcDD->readLPC( l_type,
l_addr,
io_buffer,
Expand Down Expand Up @@ -230,14 +215,6 @@ errlHndl_t lpcWrite(DeviceFW::OperationType i_opType,
if( g_altLpcDD
&& (i_target == g_altLpcDD->getProc()) )
{
//First check/clear the LPC bus of errors and commit
// any errors found
l_err = g_altLpcDD->checkForLpcErrors();
if (l_err)
{
errlCommit(l_err, LPC_COMP_ID);
}

l_err = g_altLpcDD->writeLPC( l_type,
l_addr,
io_buffer,
Expand Down Expand Up @@ -1232,10 +1209,38 @@ errlHndl_t LpcDD::readLPC(LPC::TransType i_type,
{
// 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 );

//First check/clear the LPC bus of errors and commit any errors found
errlHndl_t l_err_precheck = checkForLpcErrors();
if (l_err_precheck)
{
l_err_precheck->setSev(ERRORLOG::ERRL_SEV_INFORMATIONAL);
}

// Now do the operation
errlHndl_t l_err_op = _readLPC( i_type, i_addr, o_buffer, io_buflen );

// If this op failed and there was something wrong before we started,
// attach the logs together to aid debug
if( l_err_op && l_err_precheck )
{
l_err_precheck->plid(l_err_op->plid());
//Note-ideally we would up the severity of l_err_precheck here
// as well so that it would be visible everywhere, but we can't
// because that breaks the scenario where the caller might want
// to delete the log they get back (see SIO). We don't want
// any visible logs in that case.
}

// Always just commit the log for any errors that were present
if( l_err_precheck )
{
errlCommit(l_err_precheck, LPC_COMP_ID);
}

mutex_unlock(ivp_mutex);

return l_err;
return l_err_op;
}

/**
Expand All @@ -1248,8 +1253,36 @@ errlHndl_t LpcDD::writeLPC(LPC::TransType i_type,
{
// 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 );

//First check/clear the LPC bus of errors and commit any errors found
errlHndl_t l_err_precheck = checkForLpcErrors();
if (l_err_precheck)
{
l_err_precheck->setSev(ERRORLOG::ERRL_SEV_INFORMATIONAL);
}

// Now do the operation
errlHndl_t l_err_op = _writeLPC( i_type, i_addr, i_buffer, io_buflen );

// If this op failed and there was something wrong before we started,
// attach the logs together to aid debug
if( l_err_op && l_err_precheck )
{
l_err_precheck->plid(l_err_op->plid());
//Note-ideally we would up the severity of l_err_precheck here
// as well so that it would be visible everywhere, but we can't
// because that breaks the scenario where the caller might want
// to delete the log they get back (see SIO). We don't want
// any visible logs in that case.
}

// Always just commit the log for any errors that were present
if( l_err_precheck )
{
errlCommit(l_err_precheck, LPC_COMP_ID);
}

mutex_unlock(ivp_mutex);

return l_err;
return l_err_op;
}

0 comments on commit 9d418f5

Please sign in to comment.