Commit c0018913 authored by Raghu Krishnamurthy's avatar Raghu Krishnamurthy
Browse files

T589: Fix insufficient ordering guarantees in bakery lock



bakery_lock_get() uses DMB LD after lock acquisition and
bakery_lock_release() uses DMB ST before releasing the lock. This is
insufficient in both cases. With just DMB LD, stores in the critical
section can be reordered before the DMB LD which could mean writes in
the critical section completing before the lock has been acquired
successfully. Similarly, with just DMB ST, a load in the critical section
could be reordered after the the DMB ST. DMB is the least expensive
barrier that can provide the required ordering.
Signed-off-by: default avatarRaghu Krishnamurthy <raghu.ncstate@icloud.com>
Change-Id: Ieb74cbf5b76b09e1789331b71f37f7c660221b0e
parent 9054018b
...@@ -137,10 +137,11 @@ void bakery_lock_get(bakery_lock_t *bakery) ...@@ -137,10 +137,11 @@ void bakery_lock_get(bakery_lock_t *bakery)
} }
/* /*
* Lock acquired. Ensure that any reads from a shared resource in the * Lock acquired. Ensure that any reads and writes from a shared
* critical section read values after the lock is acquired. * resource in the critical section read/write values after the lock is
* acquired.
*/ */
dmbld(); dmbish();
} }
...@@ -154,11 +155,14 @@ void bakery_lock_release(bakery_lock_t *bakery) ...@@ -154,11 +155,14 @@ void bakery_lock_release(bakery_lock_t *bakery)
/* /*
* Ensure that other observers see any stores in the critical section * Ensure that other observers see any stores in the critical section
* before releasing the lock. Release the lock by resetting ticket. * before releasing the lock. Also ensure all loads in the critical
* Then signal other waiting contenders. * section are complete before releasing the lock. Release the lock by
* resetting ticket. Then signal other waiting contenders.
*/ */
dmbst(); dmbish();
bakery->lock_data[me] = 0U; bakery->lock_data[me] = 0U;
/* Required to ensure ordering of the following sev */
dsb(); dsb();
sev(); sev();
} }
...@@ -219,10 +219,11 @@ void bakery_lock_get(bakery_lock_t *lock) ...@@ -219,10 +219,11 @@ void bakery_lock_get(bakery_lock_t *lock)
} }
/* /*
* Lock acquired. Ensure that any reads from a shared resource in the * Lock acquired. Ensure that any reads and writes from a shared
* critical section read values after the lock is acquired. * resource in the critical section read/write values after the lock is
* acquired.
*/ */
dmbld(); dmbish();
} }
void bakery_lock_release(bakery_lock_t *lock) void bakery_lock_release(bakery_lock_t *lock)
...@@ -240,11 +241,14 @@ void bakery_lock_release(bakery_lock_t *lock) ...@@ -240,11 +241,14 @@ void bakery_lock_release(bakery_lock_t *lock)
/* /*
* Ensure that other observers see any stores in the critical section * Ensure that other observers see any stores in the critical section
* before releasing the lock. Release the lock by resetting ticket. * before releasing the lock. Also ensure all loads in the critical
* Then signal other waiting contenders. * section are complete before releasing the lock. Release the lock by
* resetting ticket. Then signal other waiting contenders.
*/ */
dmbst(); dmbish();
my_bakery_info->lock_data = 0U; my_bakery_info->lock_data = 0U;
write_cache_op((uintptr_t)my_bakery_info, is_cached); write_cache_op((uintptr_t)my_bakery_info, is_cached);
/* This sev is ordered by the dsbish in write_cahce_op */
sev(); sev();
} }
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment