From 1a220e30d8c0d3123790f3bc02341029194ea73e Mon Sep 17 00:00:00 2001
From: Abtin Keshavarzian <abtink@nestlabs.com>
Date: Wed, 14 Dec 2016 11:39:43 -0800
Subject: [PATCH] NcpSpi: Fix a race situation and simply the tx state model
 (#1075)

This commit fixes a race issue in tx path of `NcpSpi`: If the SPI
transaction is completed while in middle of `PrepareTxFrame()`task
handler before clearing the `mHandlingSendDone` flag, we can
have a situation where `PrepareTxFrame` task is not posted again and
then the next queued tx frames are not handled immediately.

It also simplifies the tx state model by adding a single `mTxState`
variable to track the current state as an `enum`.
---
 src/ncp/ncp_spi.cpp | 37 +++++++++++++++++++++----------------
 src/ncp/ncp_spi.hpp | 10 ++++++++--
 2 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/src/ncp/ncp_spi.cpp b/src/ncp/ncp_spi.cpp
index e3f26abaa..37f7b9dba 100644
--- a/src/ncp/ncp_spi.cpp
+++ b/src/ncp/ncp_spi.cpp
@@ -95,8 +95,7 @@ NcpSpi::NcpSpi(otInstance *aInstance):
     memset(mEmptySendFrame, 0, kSpiHeaderLength);
     memset(mSendFrame, 0, kSpiHeaderLength);
 
-    mSending = false;
-    mHandlingSendDone = false;
+    mTxState = kTxStateIdle;
     mHandlingRxFrame = false;
 
     mTxFrameBuffer.SetCallbacks(NULL, TxFrameBufferHasData, this);
@@ -181,14 +180,13 @@ NcpSpi::SpiTransactionComplete(
             mHandleRxFrameTask.Post();
         }
 
-        if ( mSending
-          && !mHandlingSendDone
+        if ( (mTxState == kTxStateSending)
           && (tx_data_len > 0)
           && (tx_data_len <= (aTransactionLength - kSpiHeaderLength))
           && (tx_data_len <= tx_accept_len)
         ) {
             // Our transmission was successful.
-            mHandlingSendDone = true;
+            mTxState = kTxStateHandlingSendDone;
             mPrepareTxFrameTask.Post();
         }
     }
@@ -201,7 +199,7 @@ NcpSpi::SpiTransactionComplete(
         spi_header_set_flag_byte(mEmptySendFrame, SPI_PATTERN_VALUE);
     }
 
-    if (mSending && !mHandlingSendDone)
+    if (mTxState == kTxStateSending)
     {
         aMISOBuf = mSendFrame;
         aMISOBufLen = mSendFrameLen;
@@ -230,7 +228,7 @@ NcpSpi::SpiTransactionComplete(
         aMISOBufLen,
         aMOSIBuf,
         aMOSIBufLen,
-        mSending && !mHandlingSendDone
+        (mTxState == kTxStateSending)
     );
 }
 
@@ -290,7 +288,7 @@ ThreadError NcpSpi::PrepareNextSpiSendFrame(void)
 
     mSendFrameLen = frameLength + kSpiHeaderLength;
 
-    mSending = true;
+    mTxState = kTxStateSending;
 
     errorCode = otPlatSpiSlavePrepareTransaction(
         mSendFrame,
@@ -310,7 +308,7 @@ ThreadError NcpSpi::PrepareNextSpiSendFrame(void)
 
     if (errorCode != kThreadError_None)
     {
-        mSending = false;
+        mTxState = kTxStateIdle;
     }
 
     // Remove the frame from tx buffer and inform the base
@@ -329,15 +327,22 @@ void NcpSpi::PrepareTxFrame(void *aContext)
 
 void NcpSpi::PrepareTxFrame(void)
 {
-    if (mHandlingSendDone)
-    {
-        mSending = false;
-        PrepareNextSpiSendFrame();
-        mHandlingSendDone = false;
-    }
-    else if (!mSending)
+    switch (mTxState)
     {
+    case kTxStateHandlingSendDone:
+        mTxState = kTxStateIdle;
+
+        // Fall-through to next case to prepare the next frame (if any).
+
+    case kTxStateIdle:
         PrepareNextSpiSendFrame();
+        break;
+
+    case kTxStateSending:
+        // The next frame in queue (if any) will be prepared when the
+        // current frame is successfully sent and this task is posted
+        // again from the `SpiTransactionComplete()` callback.
+        break;
     }
 }
 
diff --git a/src/ncp/ncp_spi.hpp b/src/ncp/ncp_spi.hpp
index 75d20e9f4..12303001c 100644
--- a/src/ncp/ncp_spi.hpp
+++ b/src/ncp/ncp_spi.hpp
@@ -73,6 +73,13 @@ private:
         kSpiHeaderLength = 5,                                     // Size of spi header.
     };
 
+    enum TxState
+    {
+        kTxStateIdle,                      // No frame to send
+        kTxStateSending,                   // A frame is ready to be sent
+        kTxStateHandlingSendDone           // The frame was sent successfully, waiting to prepare the next one (if any)
+    };
+
     uint16_t OutboundFrameSize(void);
 
     static void SpiTransactionComplete(
@@ -102,9 +109,8 @@ private:
 
     ThreadError PrepareNextSpiSendFrame(void);
 
-    bool mSending;
+    TxState mTxState;
     bool mHandlingRxFrame;
-    bool mHandlingSendDone;
 
     Tasklet mHandleRxFrameTask;
     Tasklet mPrepareTxFrameTask;