From cfa6d07a3b76017cedfb0def12ad24d811fc6011 Mon Sep 17 00:00:00 2001 From: Kevin Bracey Date: Tue, 28 Nov 2017 11:50:48 +0200 Subject: [PATCH 1/2] Make LDREX/STREX CAS functions strong The LDREX/STREX implementations of the compare-and-swap functions were weak (they could spuriously fail when the value was expected), whereas the critial section implementation was strong, and the documentation has no suggestion that there might be spurious failures. Rationalise by adding a retry loop for STREX failure, so that it only returns false when the value is not expected. Fixes https://github.com/ARMmbed/mbed-os/issues/5556 --- platform/mbed_critical.c | 51 +++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/platform/mbed_critical.c b/platform/mbed_critical.c index db6c05bb2a..d9aa0963ab 100644 --- a/platform/mbed_critical.c +++ b/platform/mbed_critical.c @@ -110,39 +110,42 @@ MBED_WEAK void core_util_critical_section_exit(void) bool core_util_atomic_cas_u8(uint8_t *ptr, uint8_t *expectedCurrentValue, uint8_t desiredValue) { - uint8_t currentValue = __LDREXB((volatile uint8_t*)ptr); - if (currentValue != *expectedCurrentValue) { - *expectedCurrentValue = currentValue; - __CLREX(); - return false; - } - - return !__STREXB(desiredValue, (volatile uint8_t*)ptr); + do { + uint8_t currentValue = __LDREXB((volatile uint8_t*)ptr); + if (currentValue != *expectedCurrentValue) { + *expectedCurrentValue = currentValue; + __CLREX(); + return false; + } + } while (__STREXB(desiredValue, (volatile uint8_t*)ptr)); + return true; } bool core_util_atomic_cas_u16(uint16_t *ptr, uint16_t *expectedCurrentValue, uint16_t desiredValue) { - uint16_t currentValue = __LDREXH((volatile uint16_t*)ptr); - if (currentValue != *expectedCurrentValue) { - *expectedCurrentValue = currentValue; - __CLREX(); - return false; - } - - return !__STREXH(desiredValue, (volatile uint16_t*)ptr); + do { + uint16_t currentValue = __LDREXH((volatile uint16_t*)ptr); + if (currentValue != *expectedCurrentValue) { + *expectedCurrentValue = currentValue; + __CLREX(); + return false; + } + } while (__STREXH(desiredValue, (volatile uint16_t*)ptr)); + return true; } bool core_util_atomic_cas_u32(uint32_t *ptr, uint32_t *expectedCurrentValue, uint32_t desiredValue) { - uint32_t currentValue = __LDREXW((volatile uint32_t*)ptr); - if (currentValue != *expectedCurrentValue) { - *expectedCurrentValue = currentValue; - __CLREX(); - return false; - } - - return !__STREXW(desiredValue, (volatile uint32_t*)ptr); + do { + uint32_t currentValue = __LDREXW((volatile uint32_t*)ptr); + if (currentValue != *expectedCurrentValue) { + *expectedCurrentValue = currentValue; + __CLREX(); + return false; + } + } while (__STREXW(desiredValue, (volatile uint32_t*)ptr)); + return true; } uint8_t core_util_atomic_incr_u8(uint8_t *valuePtr, uint8_t delta) From 59508d46a699675da1b50fa4e2ee34980ba01e6c Mon Sep 17 00:00:00 2001 From: Kevin Bracey Date: Tue, 28 Nov 2017 14:02:28 +0200 Subject: [PATCH 2/2] Explicitly note that CAS is strong in docs Add a note to each CAS making explicit that the functions are strong. Minor wording change to expectedCurrentValue - use of "still updated" about the failure case suggested that it might be written to on success. For some uses it's critically important that expectedCurrentValue only be written on failure, so change wording to "instead updated". --- platform/mbed_critical.h | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/platform/mbed_critical.h b/platform/mbed_critical.h index 0b7cb2a8b1..c1c5689889 100644 --- a/platform/mbed_critical.h +++ b/platform/mbed_critical.h @@ -121,7 +121,7 @@ void core_util_critical_section_exit(void); * } * * @note: In the failure case (where the destination isn't set), the value - * pointed to by expectedCurrentValue is still updated with the current value. + * pointed to by expectedCurrentValue is instead updated with the current value. * This property helps writing concise code for the following incr: * * function incr(p : pointer to int, a : int) returns int { @@ -132,6 +132,10 @@ void core_util_critical_section_exit(void); * } * return value + a * } + * + * @note: This corresponds to the C11 "atomic_compare_exchange_strong" - it + * always succeeds if the current value is expected, as per the pseudocode + * above; it will not spuriously fail as "atomic_compare_exchange_weak" may. */ bool core_util_atomic_cas_u8(uint8_t *ptr, uint8_t *expectedCurrentValue, uint8_t desiredValue); @@ -174,7 +178,7 @@ bool core_util_atomic_cas_u8(uint8_t *ptr, uint8_t *expectedCurrentValue, uint8_ * } * * @note: In the failure case (where the destination isn't set), the value - * pointed to by expectedCurrentValue is still updated with the current value. + * pointed to by expectedCurrentValue is instead updated with the current value. * This property helps writing concise code for the following incr: * * function incr(p : pointer to int, a : int) returns int { @@ -185,6 +189,10 @@ bool core_util_atomic_cas_u8(uint8_t *ptr, uint8_t *expectedCurrentValue, uint8_ * } * return value + a * } + * + * @note: This corresponds to the C11 "atomic_compare_exchange_strong" - it + * always succeeds if the current value is expected, as per the pseudocode + * above; it will not spuriously fail as "atomic_compare_exchange_weak" may. */ bool core_util_atomic_cas_u16(uint16_t *ptr, uint16_t *expectedCurrentValue, uint16_t desiredValue); @@ -227,7 +235,7 @@ bool core_util_atomic_cas_u16(uint16_t *ptr, uint16_t *expectedCurrentValue, uin * } * * @note: In the failure case (where the destination isn't set), the value - * pointed to by expectedCurrentValue is still updated with the current value. + * pointed to by expectedCurrentValue is instead updated with the current value. * This property helps writing concise code for the following incr: * * function incr(p : pointer to int, a : int) returns int { @@ -237,6 +245,10 @@ bool core_util_atomic_cas_u16(uint16_t *ptr, uint16_t *expectedCurrentValue, uin * done = atomic_cas(p, &value, value + a) // *value gets updated automatically until success * } * return value + a + * + * @note: This corresponds to the C11 "atomic_compare_exchange_strong" - it + * always succeeds if the current value is expected, as per the pseudocode + * above; it will not spuriously fail as "atomic_compare_exchange_weak" may. * } */ bool core_util_atomic_cas_u32(uint32_t *ptr, uint32_t *expectedCurrentValue, uint32_t desiredValue); @@ -280,7 +292,7 @@ bool core_util_atomic_cas_u32(uint32_t *ptr, uint32_t *expectedCurrentValue, uin * } * * @note: In the failure case (where the destination isn't set), the value - * pointed to by expectedCurrentValue is still updated with the current value. + * pointed to by expectedCurrentValue is instead updated with the current value. * This property helps writing concise code for the following incr: * * function incr(p : pointer to int, a : int) returns int { @@ -291,6 +303,10 @@ bool core_util_atomic_cas_u32(uint32_t *ptr, uint32_t *expectedCurrentValue, uin * } * return value + a * } + * + * @note: This corresponds to the C11 "atomic_compare_exchange_strong" - it + * always succeeds if the current value is expected, as per the pseudocode + * above; it will not spuriously fail as "atomic_compare_exchange_weak" may. */ bool core_util_atomic_cas_ptr(void **ptr, void **expectedCurrentValue, void *desiredValue);