Skip to content

Commit

Permalink
Fix issues with wrong context switching and global lock calls (#1567)
Browse files Browse the repository at this point in the history
***NO_CI***
  • Loading branch information
martin-kuhn authored Mar 3, 2020
1 parent facc643 commit 31f9aa8
Show file tree
Hide file tree
Showing 9 changed files with 86 additions and 38 deletions.
13 changes: 8 additions & 5 deletions src/CLR/Core/NativeEventDispatcher/NativeEventDispatcher.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,25 +141,28 @@ void CLR_RT_HeapBlock_NativeEventDispatcher::RemoveFromHALQueue()
{
// Since we are going to analyze and update the queue we need to disable interrupts.
// Interrupt service routines add records to this queue.
GLOBAL_LOCK();
CLR_UINT32 elemCount = g_CLR_HW_Hardware.m_interruptData.m_HalQueue.NumberOfElements();
CLR_UINT32 elemCount = 0;
GLOBAL_LOCK();
elemCount = g_CLR_HW_Hardware.m_interruptData.m_HalQueue.NumberOfElements();
GLOBAL_UNLOCK();

// For all elements in the queue
for ( CLR_UINT32 curElem = 0; curElem < elemCount; curElem++ )
{
// Retrieve the element ( actually remove it from the queue )
CLR_HW_Hardware::HalInterruptRecord* testRec = NULL;
GLOBAL_LOCK();
CLR_HW_Hardware::HalInterruptRecord* testRec = g_CLR_HW_Hardware.m_interruptData.m_HalQueue.Pop();
testRec = g_CLR_HW_Hardware.m_interruptData.m_HalQueue.Pop();
GLOBAL_UNLOCK();

// Check if context of this record points to the instance of CLR_RT_HeapBlock_NativeEventDispatcher
// If the "context" is the same as "this", then we skip the "Push" and record is removed.
if ( testRec->m_context != this )
{
// If it is different from this instance of CLR_RT_HeapBlock_NativeEventDispatcher, thin push it back
GLOBAL_LOCK();
CLR_HW_Hardware::HalInterruptRecord* newRec = g_CLR_HW_Hardware.m_interruptData.m_HalQueue.Push();
CLR_HW_Hardware::HalInterruptRecord* newRec = NULL;
GLOBAL_LOCK();
newRec = g_CLR_HW_Hardware.m_interruptData.m_HalQueue.Push();
GLOBAL_UNLOCK();

newRec->AssignFrom( *testRec );
Expand Down
3 changes: 2 additions & 1 deletion src/PAL/AsyncProcCall/AsyncContinuations.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,9 @@ bool HAL_CONTINUATION::Dequeue_And_Execute()
// helpfull to make the call to release the global mutext happens
bool result;

HAL_CONTINUATION* ptr = NULL;
GLOBAL_LOCK();
HAL_CONTINUATION* ptr = g_HAL_Continuation_List.ExtractFirstNode();
ptr = g_HAL_Continuation_List.ExtractFirstNode();
GLOBAL_UNLOCK();

if(ptr == NULL )
Expand Down
21 changes: 19 additions & 2 deletions targets/CMSIS-OS/ChibiOS/Include/targetHAL.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
#include <nanoHAL_v2.h>
#include <hal.h>

#define GLOBAL_LOCK() chSysLock();
#define GLOBAL_UNLOCK(); chSysUnlock();
// platform dependent delay
#define PLATFORM_DELAY(milliSecs) osDelay(milliSecs);

Expand Down Expand Up @@ -74,3 +72,22 @@ extern uint32_t __deployment_start__;
extern uint32_t __deployment_end__;

#endif //_TARGET_HAL_H_
extern int my_lock_counter;

#define GLOBAL_LOCK() \
{ \
if (port_is_isr_context()) \
chSysLockFromISR(); \
else \
chSysLock();


#define GLOBAL_UNLOCK() \
if (port_is_isr_context()) \
chSysUnlockFromISR(); \
else \
chSysUnlock(); \
}

//#define GLOBAL_LOCK() chSysLock();
//#define GLOBAL_UNLOCK(); chSysUnlock();
3 changes: 3 additions & 0 deletions targets/CMSIS-OS/ChibiOS/Lwip/platform_sys_arch.c
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include <hal.h>
#include <cmsis_os.h>
#include <nanoHAL_v2.h>
#include <targetHAL.h>

#include "lwip/opt.h"
#include "lwip/mem.h"
Expand Down Expand Up @@ -81,6 +82,7 @@ err_t sys_mbox_new(sys_mbox_t *mbox, int size)
void sys_mbox_free(sys_mbox_t *mbox)
{
msg_t msgp;
GLOBAL_LOCK();

// check for messages waiting
// If there are messages still present in the
Expand All @@ -95,6 +97,7 @@ void sys_mbox_free(sys_mbox_t *mbox)
// breaks execution to ensure developer is aware that this function needs a strong implementation at platform level
__asm volatile("BKPT #0\n");
}
GLOBAL_UNLOCK();

// ...free memory from mailbox
platform_free(*mbox);
Expand Down
33 changes: 22 additions & 11 deletions targets/CMSIS-OS/ChibiOS/common/Target_Windows_Storage.c
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,28 @@ static void SdCardDetectCallback(void *arg)
{
BaseBlockDevice* bbdp = (BaseBlockDevice*)arg;

if(chVTIsArmed(&sdCardDebounceTimer))
{
// there is a debounce timer already running so this change in pin value should be discarded
return;
}

// save current status
sdCardPresent = blkIsInserted(bbdp);

// setup timer
chVTSetI(&sdCardDebounceTimer, TIME_MS2I(SDCARD_POLLING_DELAY), SdCardInsertionMonitorCallback, arg);
if (port_is_isr_context()) {
chSysLockFromISR();
if(!chVTIsArmedI(&sdCardDebounceTimer))
{
// save current status
sdCardPresent = blkIsInserted(bbdp);
// setup timer
chVTSetI(&sdCardDebounceTimer, TIME_MS2I(SDCARD_POLLING_DELAY), SdCardInsertionMonitorCallback, arg);
}
chSysUnlockFromISR();

} else {
if(!chVTIsArmed(&sdCardDebounceTimer))
{

// save current status
sdCardPresent = blkIsInserted(bbdp);

// setup timer
chVTSet(&sdCardDebounceTimer, TIME_MS2I(SDCARD_POLLING_DELAY), SdCardInsertionMonitorCallback, arg);
}
}
}

// Card insertion event handler
Expand Down
21 changes: 14 additions & 7 deletions targets/CMSIS-OS/ChibiOS/nanoCLR/Windows.Devices.Gpio/cpu_gpio.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ static void GpioEventCallback(void *arg)
gpio_input_state * pGpio = (gpio_input_state *)arg;

// Ignore any pin changes during debounce
if (pGpio->waitingDebounce) return;
if (pGpio->waitingDebounce)
{
chSysUnlockFromISR();
return;
}

// check if there is a debounce time set
if (pGpio->debounceMs > 0)
Expand Down Expand Up @@ -136,7 +140,7 @@ gpio_input_state * AllocateGpioInputState(GPIO_PIN pinNumber)

void UnlinkInputState(gpio_input_state * pState)
{
chVTResetI(&pState->debounceTimer);
chVTReset(&pState->debounceTimer);

// disable the EXT interrupt channel
// it's OK to do always this, no matter if it's enabled or not
Expand Down Expand Up @@ -189,25 +193,28 @@ bool CPU_GPIO_ReservePin(GPIO_PIN pinNumber, bool fReserve)
if (!IsValidGpioPin(pinNumber)) return false;

int port = pinNumber >> 4, bit = 1 << (pinNumber & 0x0F);
bool ret = true;
GLOBAL_LOCK();

if (fReserve)
{
if (pinReserved[port] & bit)
{
GLOBAL_UNLOCK();
return false; // already reserved
ret = false; // already reserved
}
else
{

pinReserved[port] |= bit;
}
}
else
{
pinReserved[port] &= ~bit;
}

GLOBAL_UNLOCK();
return true;
return ret;
}

// Return if Pin is reserved
Expand Down Expand Up @@ -319,10 +326,10 @@ bool CPU_GPIO_EnableOutputPin(GPIO_PIN pinNumber, GpioPinValue InitialState, Gp

void CPU_GPIO_DisablePin(GPIO_PIN pinNumber, GpioPinDriveMode driveMode, uint32_t alternateFunction)
{
GLOBAL_LOCK();

DeleteInputState(pinNumber);

GLOBAL_LOCK();

CPU_GPIO_SetDriveMode(pinNumber, driveMode);

// get IoLine from pin number
Expand Down
19 changes: 10 additions & 9 deletions targets/CMSIS-OS/ChibiOS/nanoCLR/targetHAL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,13 @@ void SystemState_Clear(SYSTEM_STATE_type state)
}

bool SystemState_Query(SYSTEM_STATE_type state)
{
GLOBAL_LOCK();

bool systemStateCopy = SystemState_QueryNoLock(state);

GLOBAL_UNLOCK();

return systemStateCopy;
}
{
bool systemStateCopy = false;
GLOBAL_LOCK();

systemStateCopy = SystemState_QueryNoLock(state);

GLOBAL_UNLOCK();

return systemStateCopy;
}
9 changes: 7 additions & 2 deletions targets/CMSIS-OS/ChibiOS/nanoCLR/targetPAL_Events.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ bool Events_Uninitialize()
{
NATIVE_PROFILE_PAL_EVENTS();

chVTResetI(&boolEventsTimer);
chVTReset(&boolEventsTimer);

return true;
}
Expand Down Expand Up @@ -99,7 +99,12 @@ void Events_SetBoolTimer( bool* timerCompleteFlag, uint32_t millisecondsFromNow
if(timerCompleteFlag != NULL)
{
// no need to stop the timer even if it's running because the API does it anyway
chVTSetI(&boolEventsTimer, TIME_MS2I(millisecondsFromNow), local_Events_SetBoolTimer_Callback, timerCompleteFlag);
if (port_is_isr_context()){
chVTSetI(&boolEventsTimer, TIME_MS2I(millisecondsFromNow), local_Events_SetBoolTimer_Callback, timerCompleteFlag);
}
else{
chVTSet(&boolEventsTimer, TIME_MS2I(millisecondsFromNow), local_Events_SetBoolTimer_Callback, timerCompleteFlag);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ void rng_lld_stop() {


uint32_t rng_lld_GenerateRandomNumber() {
systime_t start = chVTGetSystemTime();
systime_t start = chVTGetSystemTimeX();
systime_t end = start + TIME_MS2I(RNG_TIMEOUT_VALUE);


Expand Down

0 comments on commit 31f9aa8

Please sign in to comment.