Mbed-os: core_util_atomic_incr_u8() implementation and __EXCLUSIVE_ACCESS

[danny4478]

This issue refers to mbed_critical.c file.

As far as we saw, there are two implementations for core_util_atomic_incr_u8() function:

1. For ARMv7 and newer, uses __LDREX() / __STREX()
2. For older architectures using critical section.

The implementation is selected according to __EXCLUSIVE_ACCESS define from core_cm.h.

We are building mbed_os for K64F which supports LDREX / STREX but the implementation goes to the critical section instead (verified by introducing a compilation error).

Is it a configuration bug or a feature?

Reviewers: @alzix

[pan-]

@danny4478 I suppose it is a bug, __EXCLUSIVE_ACCESS is defined in rtx_core_cm.h but doesn't propagate - for good reasons - to mbed_critical.h.

@c1728p9 Would it be possible to define __EXCLUSIVE_ACCESS at a more global scope ?

[danny4478]

Not sure what you mean...
We would like to use the atomic increment function and expected the __LDREX() / __STREX() to be used.
If __EXCLUSIVE_ACCESS is not propagated (what are the good reasons you have mentioned?) then I don't see how defining it in a more global scope can help here.

[0xc0170]

This was changed 3 months back to use the macro from RTX instead of our own ifdef. This is the last change https://github.com/ARMmbed/mbed-os/pull/4902

cc @deepikabhavnani

[pan-]

@danny4478 Mbed 2 compiles without RTOS support and the platform layer does not exploit explicitly RTOS promitives; those are some good reasons to not include an RTOS header into the platform module.

[deepikabhavnani]

RTX code defines __EXCLUSIVE_ACCESS for specific targets inside RTX code, if we need global access to this macro we can add it to macros section https://github.com/ARMmbed/mbed-os/blob/master/targets/targets.json#L614

[0xc0170]

I remember that we define some core values in tools/targets/__init__.py file, that could contain this macro for architectures that support it ?

[alzix]

how about just adding the include to rtx_core_cm.h from mbed_critical.c? :)
who else would leverage from this define?

[0xc0170]

Can't do that, see the comment above.

@danny4478 Mbed 2 compiles without RTOS support and the platform layer does not exploit explicitly RTOS promitives; those are some good reasons to not include an RTOS header into the platform module.

[deepikabhavnani]

@0xc0170 - Do you mean adding macro to CORE_LABELS section (https://github.com/ARMmbed/mbed-os/blob/master/tools/targets/__init__.py#L35)

@theotherjimmy - Any thoughts on this?

[0xc0170]

@0xc0170 - Do you mean adding macro to CORE_LABELS section (https://github.com/ARMmbed/mbed-os/blob/master/tools/targets/__init__.py#L35)

Yes, that is correct, that is my proposal. It's architecture specific so could be placed there in targets (we got there other core definitions). It might be a good to name it differently not to cause clashes with rtx symbol. What others think?

[deepikabhavnani]

It might be a good to name it differently not to cause clashes with rtx symbol.

👍

[theotherjimmy]

@0xc0170 @deepikabhavnani I'd rather not see magic show up in tools/targets/__init__.py. Could this macro be added to a header file somewhere instead?

[0xc0170]

@0xc0170 @deepikabhavnani I'd rather not see magic show up in tools/targets/__init__.py. Could this macro be added to a header file somewhere instead?

I sent PR referenced above to finalize the fix, please review