From 7aa86235a3c3a4368e2d6156d1cb5160941d1564 Mon Sep 17 00:00:00 2001 From: Lephe Date: Fri, 7 May 2021 09:59:05 +0200 Subject: [PATCH] cpu: fix a data race occurring on the cpu_atomic lock The function was designed with multi-threaded concurrency in mind, where threads can take over while the lock is held and simply block trying to acquire it, which allows the lock holder to proceed. However interrupt handlers are different; they have priority, so once they start they must complete immediately. The cannot afford to block on the lock as the program would simply freeze. In exchange, they clean up before they leave, so there are some guarantees on the execution state even when interrupted. The correct protection is therefore not a lock but a temporary block on interrupts. There is no data race on the value of the saved IMASK because it is preserved during interrupt handling. --- src/cpu/atomic.c | 32 ++++++++++++-------------------- 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/src/cpu/atomic.c b/src/cpu/atomic.c index 9ae24e2..f500995 100644 --- a/src/cpu/atomic.c +++ b/src/cpu/atomic.c @@ -6,41 +6,33 @@ /* Value of IMASK when atomic mode is entered */ static int saved_IMASK = 0; -/* Number of atomic mode levels (sort of mutex) */ +/* Number of atomic mode levels */ static unsigned int atomic_level = 0; -/* Lock on (atomic_level) */ -static char atomic_level_lock = 0; void cpu_atomic_start(void) { - /* Get the lock on (atomic_level) */ - while(__atomic_test_and_set(&atomic_level_lock, __ATOMIC_RELAXED)) {} - - if(atomic_level == 0) { - cpu_sr_t SR = cpu_getSR(); - saved_IMASK = SR.IMASK; - SR.IMASK = 15; - cpu_setSR(SR); - } + /* There is no access problem to IMASK here because interrupts must + preserve and restore it */ + cpu_sr_t SR = cpu_getSR(); + cpu_sr_t SR2 = SR; + SR2.IMASK = 15; + cpu_setSR(SR2); + /* Now that we're alone, atomically update the atomic level */ + if(atomic_level == 0) saved_IMASK = SR.IMASK; atomic_level++; - - /* Release the lock */ - __atomic_clear(&atomic_level_lock, __ATOMIC_RELAXED); } void cpu_atomic_end(void) { - while(__atomic_test_and_set(&atomic_level_lock, __ATOMIC_RELAXED)) {} + cpu_sr_t SR = cpu_getSR(); + /* Update atomic_level before restoring interrupts */ atomic_level--; - if(atomic_level == 0) { - cpu_sr_t SR = cpu_getSR(); SR.IMASK = saved_IMASK; saved_IMASK = 0; - cpu_setSR(SR); } - __atomic_clear(&atomic_level_lock, __ATOMIC_RELAXED); + cpu_setSR(SR); }