Skip to content

Commit

Permalink
[nrf5] Repair the newlib locking (project-chip#2100)
Browse files Browse the repository at this point in the history
* [nrf5] Repair the newlib locking

Calling stdio functions in certain circumstances currently crashes the
device. Enabling configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES makes this
more likely to happen.

The problem is that the types used in FreeRTOSNewlibLockSupport.c don't
match those used by newlib; we're mixing up struct __lock * with its
content.

Fix the implementation and remove all the typecasts. Note that we need a
2nd allocation for the dynamic case as FreeRTOS and newlib APIs don't
match up well.

Possibly we should remove this code, but for now just repair it.

fixes project-chip#2084

* Add include paths to automake
  • Loading branch information
mspang authored Aug 13, 2020
1 parent 4d8e12e commit c1ff7f9
Show file tree
Hide file tree
Showing 11 changed files with 215 additions and 55 deletions.
3 changes: 2 additions & 1 deletion examples/lighting-app/nrf5/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ executable("lighting_app") {
"${nrf5_platform_dir}/app/include/Server.h",
"${nrf5_platform_dir}/app/include/chipinit.h",
"${nrf5_platform_dir}/app/support/CXXExceptionStubs.cpp",
"${nrf5_platform_dir}/app/support/FreeRTOSNewlibLockSupport.c",
"${nrf5_platform_dir}/app/support/nRF5Sbrk.c",
"${nrf5_platform_dir}/util/LEDWidget.cpp",
"${nrf5_platform_dir}/util/include/LEDWidget.h",
Expand Down Expand Up @@ -94,6 +93,8 @@ executable("lighting_app") {
"${chip_root}/third_party/openthread/platforms/nrf528xx:libopenthread-nrf52840-softdevice-sdk",
"${openthread_root}:libopenthread-cli-ftd",
"${openthread_root}:libopenthread-ftd",
"${nrf5_platform_dir}/app/support:freertos_newlib_lock_support",
"${nrf5_platform_dir}/app/support:freertos_newlib_lock_support_test",
]

output_dir = root_out_dir
Expand Down
2 changes: 2 additions & 0 deletions examples/lighting-app/nrf5/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ SRCS = \
$(NRF5_PLATFORM_DIR)/app/support/CXXExceptionStubs.cpp \
$(NRF5_PLATFORM_DIR)/app/support/nRF5Sbrk.c \
$(NRF5_PLATFORM_DIR)/app/support/FreeRTOSNewlibLockSupport.c \
$(NRF5_PLATFORM_DIR)/app/support/FreeRTOSNewlibLockSupport_test.c \
$(NRF5_PLATFORM_DIR)/app/Server.cpp \
$(NRF5_PLATFORM_DIR)/app/chipinit.cpp \
$(NRF5_PLATFORM_DIR)/util/LEDWidget.cpp \
Expand Down Expand Up @@ -128,6 +129,7 @@ INC_DIRS = \
$(PROJECT_ROOT)/main/schema/include \
$(PROJECT_ROOT)/third_party/printf \
$(CHIP_OUTPUT_DIR)/third_party/openthread/include \
$(CHIP_ROOT)/examples/platform \
$(CHIP_ROOT)/src/include/ \
$(CHIP_ROOT)/src/lib \
$(CHIP_ROOT)/src/ \
Expand Down
6 changes: 6 additions & 0 deletions examples/lighting-app/nrf5/main/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ extern "C" {
#endif // NRF_LOG_ENABLED

#include "chipinit.h"
#include "nrf528xx/app/support/FreeRTOSNewlibLockSupport_test.h"
#include <AppTask.h>
#include <platform/CHIPDeviceLayer.h>

Expand Down Expand Up @@ -158,6 +159,11 @@ int main(void)
NRF_LOG_INFO("==================================================");
NRF_LOG_FLUSH();

#ifndef NDEBUG
// TODO: Move this into a standalone test.
freertos_newlib_lock_test();
#endif

#if defined(SOFTDEVICE_PRESENT) && SOFTDEVICE_PRESENT

NRF_LOG_INFO("Enabling SoftDevice");
Expand Down
3 changes: 2 additions & 1 deletion examples/lock-app/nrf5/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ executable("lock_app") {
"${nrf5_platform_dir}/app/include/Server.h",
"${nrf5_platform_dir}/app/include/chipinit.h",
"${nrf5_platform_dir}/app/support/CXXExceptionStubs.cpp",
"${nrf5_platform_dir}/app/support/FreeRTOSNewlibLockSupport.c",
"${nrf5_platform_dir}/app/support/nRF5Sbrk.c",
"${nrf5_platform_dir}/util/LEDWidget.cpp",
"${nrf5_platform_dir}/util/include/LEDWidget.h",
Expand Down Expand Up @@ -93,6 +92,8 @@ executable("lock_app") {
"${chip_root}/src/lib",
"${chip_root}/third_party/openthread/platforms/nrf528xx:libnordicsemi_nrf52840_radio_driver_softdevice",
"${chip_root}/third_party/openthread/platforms/nrf528xx:libopenthread-nrf52840-softdevice-sdk",
"${nrf5_platform_dir}/app/support:freertos_newlib_lock_support",
"${nrf5_platform_dir}/app/support:freertos_newlib_lock_support_test",
"${openthread_root}:libopenthread-cli-ftd",
"${openthread_root}:libopenthread-ftd",
]
Expand Down
2 changes: 2 additions & 0 deletions examples/lock-app/nrf5/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ SRCS = \
$(NRF5_PLATFORM_DIR)/app/support/CXXExceptionStubs.cpp \
$(NRF5_PLATFORM_DIR)/app/support/nRF5Sbrk.c \
$(NRF5_PLATFORM_DIR)/app/support/FreeRTOSNewlibLockSupport.c \
$(NRF5_PLATFORM_DIR)/app/support/FreeRTOSNewlibLockSupport_test.c \
$(NRF5_PLATFORM_DIR)/app/Server.cpp \
$(NRF5_PLATFORM_DIR)/app/chipinit.cpp \
$(NRF5_PLATFORM_DIR)/util/LEDWidget.cpp \
Expand Down Expand Up @@ -128,6 +129,7 @@ INC_DIRS = \
$(PROJECT_ROOT)/main/schema/include \
$(PROJECT_ROOT)/third_party/printf \
$(CHIP_OUTPUT_DIR)/third_party/openthread/include \
$(CHIP_ROOT)/examples/platform \
$(CHIP_ROOT)/src/include/ \
$(CHIP_ROOT)/src/lib \
$(CHIP_ROOT)/src/ \
Expand Down
6 changes: 6 additions & 0 deletions examples/lock-app/nrf5/main/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ extern "C" {
#endif // NRF_LOG_ENABLED

#include "chipinit.h"
#include "nrf528xx/app/support/FreeRTOSNewlibLockSupport_test.h"
#include <AppTask.h>
#include <platform/CHIPDeviceLayer.h>

Expand Down Expand Up @@ -158,6 +159,11 @@ int main(void)
NRF_LOG_INFO("==================================================");
NRF_LOG_FLUSH();

#ifndef NDEBUG
// TODO: Move this into a standalone test.
freertos_newlib_lock_test();
#endif

#if defined(SOFTDEVICE_PRESENT) && SOFTDEVICE_PRESENT

NRF_LOG_INFO("Enabling SoftDevice");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,10 @@
#define configASSERT(x) ASSERT(x)
#endif

#ifndef NDEBUG
#define configUSE_LIST_DATA_INTEGRITY_CHECK_BYTES 1
#endif

/* FreeRTOS MPU specific definitions. */
#define configINCLUDE_APPLICATION_DEFINED_PRIVILEGED_FUNCTIONS 1

Expand Down
37 changes: 37 additions & 0 deletions examples/platform/nrf528xx/app/support/BUILD.gn
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Copyright (c) 2020 Project CHIP Authors
#
# 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.

import("//build_overrides/chip.gni")
import("//build_overrides/nrf5_sdk.gni")

config("support_config") {
include_dirs = [ "../../.." ]
}

source_set("freertos_newlib_lock_support") {
sources = [ "FreeRTOSNewlibLockSupport.c" ]

public_deps = [ "${nrf5_sdk_build_root}:nrf5_sdk" ]

public_configs = [ ":support_config" ]
}

source_set("freertos_newlib_lock_support_test") {
sources = [
"FreeRTOSNewlibLockSupport_test.c",
"FreeRTOSNewlibLockSupport_test.h",
]

public_deps = [ ":freertos_newlib_lock_support" ]
}
96 changes: 43 additions & 53 deletions examples/platform/nrf528xx/app/support/FreeRTOSNewlibLockSupport.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,21 +26,27 @@
#include "FreeRTOS.h"
#include "semphr.h"
#include "task.h"
#include <stdlib.h>
#include <sys/lock.h>

struct __lock
{
SemaphoreHandle_t semaphore;
};

/*
* Global mutex objects used by newlib.
*/

SemaphoreHandle_t __lock___sinit_recursive_mutex;
SemaphoreHandle_t __lock___sfp_recursive_mutex;
SemaphoreHandle_t __lock___atexit_recursive_mutex;
SemaphoreHandle_t __lock___at_quick_exit_mutex;
SemaphoreHandle_t __lock___malloc_recursive_mutex;
SemaphoreHandle_t __lock___env_recursive_mutex;
SemaphoreHandle_t __lock___tz_mutex;
SemaphoreHandle_t __lock___dd_hash_mutex;
SemaphoreHandle_t __lock___arc4random_mutex;
struct __lock __lock___sinit_recursive_mutex;
struct __lock __lock___sfp_recursive_mutex;
struct __lock __lock___atexit_recursive_mutex;
struct __lock __lock___at_quick_exit_mutex;
struct __lock __lock___malloc_recursive_mutex;
struct __lock __lock___env_recursive_mutex;
struct __lock __lock___tz_mutex;
struct __lock __lock___dd_hash_mutex;
struct __lock __lock___arc4random_mutex;

/**
* Global "constructor" function for initializing newlib mutexes at system start.
Expand All @@ -49,94 +55,78 @@ SemaphoreHandle_t __lock___arc4random_mutex;
*/
__attribute__((constructor)) static void InitNewlibMutexes(void)
{
#if USE_STATIC_NEWLIB_MUTEXES

static StaticSemaphore_t sSemBuf_sinit_recursive_mutex;
static StaticSemaphore_t sSemBuf_sfp_recursive_mutex;
static StaticSemaphore_t sSemBuf_atexit_recursive_mutex;
static StaticSemaphore_t sSemBuf_at_quick_exit_mutex;
static StaticSemaphore_t sSemBuf_env_recursive_mutex;
static StaticSemaphore_t sSemBuf_tz_mutex;
static StaticSemaphore_t sSemBuf_dd_hash_mutex;
static StaticSemaphore_t sSemBuf_arc4random_mutex;

__lock___sinit_recursive_mutex = xSemaphoreCreateRecursiveMutexStatic(&sSemBuf_sinit_recursive_mutex);
__lock___sfp_recursive_mutex = xSemaphoreCreateRecursiveMutexStatic(&sSemBuf_sfp_recursive_mutex);
__lock___atexit_recursive_mutex = xSemaphoreCreateRecursiveMutexStatic(&sSemBuf_atexit_recursive_mutex);
__lock___at_quick_exit_mutex = xSemaphoreCreateMutexStatic(&sSemBuf_at_quick_exit_mutex);
__lock___env_recursive_mutex = xSemaphoreCreateRecursiveMutexStatic(&sSemBuf_env_recursive_mutex);
__lock___tz_mutex = xSemaphoreCreateMutexStatic(&sSemBuf_tz_mutex);
__lock___dd_hash_mutex = xSemaphoreCreateMutexStatic(&sSemBuf_dd_hash_mutex);
__lock___arc4random_mutex = xSemaphoreCreateMutexStatic(&sSemBuf_arc4random_mutex);

#else /* USE_STATIC_NEWLIB_MUTEXES */

__lock___sinit_recursive_mutex = xSemaphoreCreateRecursiveMutex();
__lock___sfp_recursive_mutex = xSemaphoreCreateRecursiveMutex();
__lock___atexit_recursive_mutex = xSemaphoreCreateRecursiveMutex();
__lock___at_quick_exit_mutex = xSemaphoreCreateMutex();
__lock___env_recursive_mutex = xSemaphoreCreateRecursiveMutex();
__lock___tz_mutex = xSemaphoreCreateMutex();
__lock___dd_hash_mutex = xSemaphoreCreateMutex();
__lock___arc4random_mutex = xSemaphoreCreateMutex();

#endif /* USE_STATIC_NEWLIB_MUTEXES */
__lock___sinit_recursive_mutex.semaphore = xSemaphoreCreateRecursiveMutex();
__lock___sfp_recursive_mutex.semaphore = xSemaphoreCreateRecursiveMutex();
__lock___atexit_recursive_mutex.semaphore = xSemaphoreCreateRecursiveMutex();
__lock___at_quick_exit_mutex.semaphore = xSemaphoreCreateMutex();
__lock___env_recursive_mutex.semaphore = xSemaphoreCreateRecursiveMutex();
__lock___tz_mutex.semaphore = xSemaphoreCreateMutex();
__lock___dd_hash_mutex.semaphore = xSemaphoreCreateMutex();
__lock___arc4random_mutex.semaphore = xSemaphoreCreateMutex();
}

/*
* Overrides for newlib's retargetable locking functions.
*/

void __retarget_lock_init(_LOCK_T * lock)
void __retarget_lock_init(_LOCK_T * lock_ptr)
{
*lock = (_LOCK_T) xSemaphoreCreateMutex();
_LOCK_T lock = malloc(sizeof(*lock));
ASSERT(lock);
*lock_ptr = lock;
lock->semaphore = xSemaphoreCreateMutex();
}

void __retarget_lock_init_recursive(_LOCK_T * lock)
void __retarget_lock_init_recursive(_LOCK_T * lock_ptr)
{
*lock = (_LOCK_T) xSemaphoreCreateRecursiveMutex();
_LOCK_T lock = malloc(sizeof(*lock));
ASSERT(lock);
*lock_ptr = lock;
lock->semaphore = xSemaphoreCreateRecursiveMutex();
}

void __retarget_lock_close(_LOCK_T lock)
{
vSemaphoreDelete((SemaphoreHandle_t) lock);
vSemaphoreDelete(lock->semaphore);
free(lock);
}

void __retarget_lock_close_recursive(_LOCK_T lock)
{
vSemaphoreDelete((SemaphoreHandle_t) lock);
vSemaphoreDelete(lock->semaphore);
free(lock);
}

void __retarget_lock_acquire(_LOCK_T lock)
{
TickType_t waitTicks = (xTaskGetSchedulerState() == taskSCHEDULER_NOT_STARTED) ? 0 : portMAX_DELAY;
xSemaphoreTake((SemaphoreHandle_t) lock, waitTicks);
xSemaphoreTake(lock->semaphore, waitTicks);
}

void __retarget_lock_acquire_recursive(_LOCK_T lock)
{
TickType_t waitTicks = (xTaskGetSchedulerState() == taskSCHEDULER_NOT_STARTED) ? 0 : portMAX_DELAY;
xSemaphoreTakeRecursive((SemaphoreHandle_t) lock, waitTicks);
xSemaphoreTakeRecursive(lock->semaphore, waitTicks);
}

int __retarget_lock_try_acquire(_LOCK_T lock)
{
return xSemaphoreTake((SemaphoreHandle_t) lock, 0) == pdTRUE ? 1 : 0;
return xSemaphoreTake(lock->semaphore, 0) == pdTRUE ? 1 : 0;
}

int __retarget_lock_try_acquire_recursive(_LOCK_T lock)
{
return xSemaphoreTakeRecursive((SemaphoreHandle_t) lock, 0) == pdTRUE ? 1 : 0;
return xSemaphoreTakeRecursive(lock->semaphore, 0) == pdTRUE ? 1 : 0;
}

void __retarget_lock_release(_LOCK_T lock)
{
xSemaphoreGive((SemaphoreHandle_t) lock);
xSemaphoreGive(lock->semaphore);
}

void __retarget_lock_release_recursive(_LOCK_T lock)
{
xSemaphoreGiveRecursive((SemaphoreHandle_t) lock);
xSemaphoreGiveRecursive(lock->semaphore);
}

/*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/**
*
* Copyright (c) 2020 Project CHIP Authors
*
* 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.
*/

#include "FreeRTOSNewlibLockSupport_test.h"

#include <FreeRTOS.h>
#include <assert.h>
#include <semphr.h>
#include <stdbool.h>
#include <sys/lock.h>

#ifndef __SINGLE_THREAD__
__LOCK_INIT(static, test_lock);
__LOCK_INIT_RECURSIVE(static, test_lock_recursive);

struct __lock
{
SemaphoreHandle_t semaphore;
};

struct __lock __lock_test_lock;
struct __lock __lock_test_lock_recursive;

__attribute__((constructor)) static void init_static_lock_test_mutexes(void)
{
__lock_test_lock.semaphore = xSemaphoreCreateMutex();
__lock_test_lock_recursive.semaphore = xSemaphoreCreateRecursiveMutex();
}

void freertos_newlib_lock_test()
{
__lock_acquire(test_lock);
bool acquired = __lock_try_acquire(test_lock);
ASSERT(!acquired);
__lock_release(test_lock);
acquired = __lock_try_acquire(test_lock);
ASSERT(acquired);
__lock_release(test_lock);

__lock_acquire_recursive(test_lock_recursive);
__lock_acquire_recursive(test_lock_recursive);
acquired = __lock_try_acquire_recursive(test_lock_recursive);
ASSERT(acquired);
__lock_release_recursive(test_lock_recursive);
__lock_release_recursive(test_lock_recursive);
__lock_release_recursive(test_lock_recursive);

_LOCK_T dynamic_lock;
__lock_init(dynamic_lock);
__lock_acquire(dynamic_lock);
acquired = __lock_try_acquire(dynamic_lock);
ASSERT(!acquired);
__lock_release(dynamic_lock);
acquired = __lock_try_acquire(dynamic_lock);
ASSERT(acquired);
__lock_release(dynamic_lock);

_LOCK_T dynamic_lock_recursive;
__lock_init_recursive(dynamic_lock_recursive);
__lock_acquire_recursive(dynamic_lock_recursive);
acquired = __lock_try_acquire_recursive(dynamic_lock_recursive);
ASSERT(acquired);
__lock_release_recursive(dynamic_lock);
acquired = __lock_try_acquire_recursive(dynamic_lock_recursive);
ASSERT(acquired);
__lock_release_recursive(dynamic_lock_recursive);
}
#else
void freertos_newlib_lock_test() {}
#endif
Loading

0 comments on commit c1ff7f9

Please sign in to comment.