Skip to content
This repository has been archived by the owner on Jan 15, 2021. It is now read-only.

Add a C re-entrant critical section API #18

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions mbed-util/critical.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* PackageLicenseDeclared: Apache-2.0
* Copyright (c) 2015 ARM Limited
*
* 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.
*/

#ifndef __MBED_UTIL_CRITICAL_H__
#define __MBED_UTIL_CRITICAL_H__

#ifdef __cplusplus
namespace mbed {
namespace util {
extern "C" {
#endif

void critical_section_enter();
void critical_section_exit();

#ifdef __cplusplus
} // extern "C"
} // namspace util
} // namespace mbed
#endif


#endif // __MBED_UTIL_CRITICAL_H__
46 changes: 46 additions & 0 deletions source/critical.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* PackageLicenseDeclared: Apache-2.0
* Copyright (c) 2015 ARM Limited
*
* 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 <stdint.h> // uint32_t, UINT32_MAX
#include <stddef.h> // NULL
#include "cmsis-core/core_generic.h" //__disable_irq, __enable_irq

// Module include
#include "mbed-util/critical.h"

static volatile uint32_t interruptEnableCounter = 0;
static volatile uint32_t critical_primask = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is not required.

const uint32_t invalid = 0xFFFFFFFEUL;
void critical_section_enter()
{
/* sample the primask */
if (!interruptEnableCounter) {
critical_primask = __get_PRIMASK();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably use atomic_cas

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the primask like this introduces a race condition. Instead, we should check to see that interrupts are currently enabled, and assert if they are not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't atomic_cas c++ and thus not usable in a c based critical section lock function?

}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If interruptEnableCounter is non-zero, we should check the primask. If interrupts are enabled, we should assert that something has enabled them.

__disable_irq();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the interrupts are disabled and you call critical_section_enter and then critical_section_exit, it'll enable interrupts. Wouldn't it be better to save/restore interrupt state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't disable interrupts, call critical_section_enter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't disable interrupts, but library code (for example) might not care if it runs with interrupts enabled or not, but still re-enable interrupts by mistake if it uses these functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, I could preserve the primask if critical_section_enter is called and interruptEnableCounter is 0.

/* not allowed to overflow the interruptEnableCounter. */
if (interruptEnableCounter == UINT32_MAX) {
/* Generate a fault */
((void(*)(void))&invalid)();
}
interruptEnableCounter++;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably use atomic_incr

}

void critical_section_exit()
{
if (interruptEnableCounter) interruptEnableCounter--;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably use atomic_decr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check the primask here and see if someone has enabled interrupts when they shouldn't be.

if (!interruptEnableCounter && !critical_primask) __enable_irq();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use __set_PRIMASK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, let's not.

}
55 changes: 55 additions & 0 deletions test/critical.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* PackageLicenseDeclared: Apache-2.0
* Copyright (c) 2015 ARM Limited
*
* 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 "cmsis-core/core_generic.h" // __get_PRIMASK
#include "mbed/test_env.h" // MBED_HOSTTEST_* definitions
// Module include
#include "mbed-util/critical.h"

void app_start(int argc, char *argv[])
{
MBED_HOSTTEST_TIMEOUT(5);
MBED_HOSTTEST_SELECT(default);
MBED_HOSTTEST_DESCRIPTION(mbed-util critical section test);
MBED_HOSTTEST_START("MBED_UTIL_CRITICAL");
bool pass = false;
do {
// Check the current primask
uint32_t primask;
if ((primask = __get_PRIMASK()) != 0) break;

// Test a single critical section
mbed::util::critical_section_enter();
if ((primask = __get_PRIMASK()) == 0) break;
mbed::util::critical_section_exit();
if ((primask = __get_PRIMASK()) != 0) break;

// Test multiple nested critical sections
for (int i = 0; i < 16; i++) {
mbed::util::critical_section_enter();
}
if ((primask = __get_PRIMASK()) == 0) break;
for (int i = 0; i < 16; i++) {
mbed::util::critical_section_exit();
}
if ((primask = __get_PRIMASK()) != 0) break;

pass = true;
} while (0);

MBED_HOSTTEST_RESULT(pass);
}