From 48625474743a0c511a9058e758603fb1ac9da4c5 Mon Sep 17 00:00:00 2001
From: Hugues Kamba-Mpiana <41612201+hugueskamba@users.noreply.github.com>
Date: Mon, 8 Jan 2024 04:58:39 +0000
Subject: [PATCH] bsp: Synchronize serial driver access (#29)

The serial driver is used by multiple tasks and not all tasks use
the Log APIs to send data to the serial driver.
This was causing serial data buffer corruption.

Synchronise the access of the serial driver using mutual exclusion
to make it thread-safe.

This also removes code duplication and fixes return values for
the write system call:
* Arm GNU Toolchain: On failure, `-1`.
                     On success, total chars written.
* Arm Compiler: On failure, a positive number representing the number of
                chars NOT written or a negative number indicating an error.
                On success, `0`.

Signed-off-by: Devaraj Ranganna <devaraj.ranganna@arm.com>
Signed-off-by: Hugues Kamba-Mpiana <hugues.kambampiana@arm.com>
Co-authored-by: Devaraj Ranganna <devaraj.ranganna@arm.com>
---
 bsp/CMakeLists.txt                  |   1 +
 bsp/common/bsp_serial.c             | 107 +++++++++++++++++++++++-----
 release_changes/202401041514.change |   1 +
 3 files changed, 92 insertions(+), 17 deletions(-)
 create mode 100644 release_changes/202401041514.change

diff --git a/bsp/CMakeLists.txt b/bsp/CMakeLists.txt
index fa68a411..cb88756f 100644
--- a/bsp/CMakeLists.txt
+++ b/bsp/CMakeLists.txt
@@ -65,4 +65,5 @@ target_include_directories(fri-bsp
 target_link_libraries(fri-bsp
     PUBLIC
         arm-corstone-platform-bsp
+        freertos_kernel
 )
diff --git a/bsp/common/bsp_serial.c b/bsp/common/bsp_serial.c
index 990a5a9b..0021f0e5 100644
--- a/bsp/common/bsp_serial.c
+++ b/bsp/common/bsp_serial.c
@@ -9,12 +9,47 @@
 #include "Driver_USART.h"
 #include "bsp_serial.h"
 
+#include "FreeRTOS.h"
+#include "semphr.h"
+
+#define STDIN_FILENO     0
+#define STDOUT_FILENO    1
+#define STDERR_FILENO    2
+
+typedef enum
+{
+    WRITE_ERROR_SEND_FAIL = -3,
+    WRITE_ERROR_SYNC_FAILED = -2,
+    WRITE_ERROR_INVALID_ARGS = -1,
+    WRITE_ERROR_NONE = 0
+} WriteError_t;
+
+typedef struct
+{
+    WriteError_t error;
+    unsigned int charsWritten;
+} WriteResult_t;
+
 extern ARM_DRIVER_USART Driver_USART0;
 
+static SemaphoreHandle_t xLoggingMutex = NULL;
+
+static bool prvValidFdHandle( int fd );
+static void prvWriteChars( int fd,
+                           const unsigned char * str,
+                           unsigned int len,
+                           WriteResult_t * result );
+
 void bsp_serial_init( void )
 {
     Driver_USART0.Initialize( NULL );
     Driver_USART0.Control( ARM_USART_MODE_ASYNCHRONOUS, DEFAULT_UART_BAUDRATE );
+
+    if( xLoggingMutex == NULL )
+    {
+        xLoggingMutex = xSemaphoreCreateMutex();
+        configASSERT( xLoggingMutex );
+    }
 }
 
 void bsp_serial_print( char * str )
@@ -28,10 +63,6 @@ void bsp_serial_print( char * str )
 
     #include <rt_sys.h>
 
-    #define STDIN_FILENO     0
-    #define STDOUT_FILENO    1
-    #define STDERR_FILENO    2
-
     FILEHANDLE _sys_open( const char * name,
                           int openmode )
     {
@@ -73,20 +104,24 @@ void bsp_serial_print( char * str )
                     unsigned int len,
                     int mode )
     {
-        /* From <rt_sys.h>: `mode' exists for historical reasons and must be ignored. */
+        /* From <rt_sys.h>: `mode` exists for historical reasons and must be ignored. */
         ( void ) mode;
 
-        if( ( fd != STDOUT_FILENO ) && ( fd != STDERR_FILENO ) )
+        WriteResult_t result = { .error = WRITE_ERROR_NONE, .charsWritten = 0 };
+        prvWriteChars( ( int ) fd, str, len, &result );
+
+        if( ( result.error == WRITE_ERROR_NONE ) && ( result.charsWritten == len ) )
         {
-            return -1;
+            return 0;
         }
-
-        if( Driver_USART0.Send( str, len ) != ARM_DRIVER_OK )
+        else if( result.error == WRITE_ERROR_SEND_FAIL )
         {
-            return -1;
+            return len - result.charsWritten;
+        }
+        else
+        {
+            return ( int ) result.error;
         }
-
-        return 0;
     }
 
     int _sys_read( FILEHANDLE fd,
@@ -133,12 +168,50 @@ void bsp_serial_print( char * str )
                 char * str,
                 int len )
     {
-        if( Driver_USART0.Send( str, len ) == ARM_DRIVER_OK )
-        {
-            return len;
-        }
+        WriteResult_t result = { .error = WRITE_ERROR_NONE, .charsWritten = 0 };
 
-        return 0;
+        prvWriteChars( fd, str, len, &result );
+
+        return ( ( result.error == WRITE_ERROR_NONE ) && ( result.charsWritten == len ) ) ? result.charsWritten : -1;
     }
 
 #endif /* if defined( __ARMCOMPILER_VERSION ) */
+
+static bool prvValidFdHandle( int fd )
+{
+    return ( bool ) ( ( fd == STDOUT_FILENO ) || ( fd == STDERR_FILENO ) );
+}
+
+static void prvWriteChars( int fd,
+                           const unsigned char * str,
+                           unsigned int len,
+                           WriteResult_t * result )
+{
+    result->charsWritten = 0;
+
+    if( prvValidFdHandle( fd ) == false )
+    {
+        result->error = WRITE_ERROR_INVALID_ARGS;
+        return;
+    }
+
+    if( xSemaphoreTake( xLoggingMutex, portMAX_DELAY ) != pdTRUE )
+    {
+        result->error = WRITE_ERROR_SYNC_FAILED;
+        return;
+    }
+
+    bool allCharsWritten = ( bool ) ( Driver_USART0.Send( str, len ) == ARM_DRIVER_OK );
+
+    ( void ) xSemaphoreGive( xLoggingMutex );
+
+    if( allCharsWritten == true )
+    {
+        result->charsWritten = len;
+        result->error = WRITE_ERROR_NONE;
+    }
+    else
+    {
+        result->error = WRITE_ERROR_SEND_FAIL;
+    }
+}
diff --git a/release_changes/202401041514.change b/release_changes/202401041514.change
new file mode 100644
index 00000000..38bddc68
--- /dev/null
+++ b/release_changes/202401041514.change
@@ -0,0 +1 @@
+serial: Fix multithread synchronisation