[rtems commit] score: Maybe fix _Thread_Lock_acquire()

[Sebastian Huber]

Module:    rtems
Branch:    master
Commit:    c6556e2ecc6b80f981bb210d541544f24b7f59df
Changeset: http://git.rtems.org/rtems/commit/?id=c6556e2ecc6b80f981bb210d541544f24b7f59df

Author:    Sebastian Huber <sebastian.huber at embedded-brains.de>
Date:      Wed Jun  1 14:38:05 2016 +0200

score: Maybe fix _Thread_Lock_acquire()

The approach with the generation number was broken.  The load/store of
the current lock, the thread queue and the thread queue operations were not
properly synchronized.  Under certain conditions on a PowerPC T4240 old
thread queue operations operated on a new thread queue (NULL pointer).

---

 cpukit/score/include/rtems/score/thread.h     |  5 ---
 cpukit/score/include/rtems/score/threadimpl.h | 44 +++++----------------------
 2 files changed, 7 insertions(+), 42 deletions(-)

diff --git a/cpukit/score/include/rtems/score/thread.h b/cpukit/score/include/rtems/score/thread.h
index 4618a40..7491e8f 100644
--- a/cpukit/score/include/rtems/score/thread.h
+++ b/cpukit/score/include/rtems/score/thread.h
@@ -691,11 +691,6 @@ typedef struct {
    */
   SMP_lock_Stats Stats;
 #endif
-
-  /**
-   * @brief Generation number to invalidate stale locks.
-   */
-  Atomic_Uint generation;
 } Thread_Lock_control;
 #endif
 
diff --git a/cpukit/score/include/rtems/score/threadimpl.h b/cpukit/score/include/rtems/score/threadimpl.h
index bd2ec50..164773a 100644
--- a/cpukit/score/include/rtems/score/threadimpl.h
+++ b/cpukit/score/include/rtems/score/threadimpl.h
@@ -1128,21 +1128,16 @@ RTEMS_INLINE_ROUTINE void *_Thread_Lock_acquire(
   SMP_ticket_lock_Control *lock;
 
   while ( true ) {
-    unsigned int my_generation;
-    bool         success;
-
     _ISR_lock_ISR_disable( lock_context );
 
     /*
-     * Ensure that we read our lock generation before we obtain our
-     * current lock.  See _Thread_Lock_set_unprotected().
+     * We assume that a normal load of pointer is identical to a relaxed atomic
+     * load.  Here, we may read an out-of-date lock.  However, only the owner
+     * of this out-of-date lock is allowed to set a new one.  Thus, we read at
+     * least this new lock ...
      */
-    my_generation = _Atomic_Load_uint(
-      &the_thread->Lock.generation,
-      ATOMIC_ORDER_ACQUIRE
-    );
-
     lock = the_thread->Lock.current;
+
     _SMP_ticket_lock_Acquire(
       lock,
       &_Thread_Executing->Lock.Stats,
@@ -1150,18 +1145,9 @@ RTEMS_INLINE_ROUTINE void *_Thread_Lock_acquire(
     );
 
     /*
-     * We must use a read-modify-write operation to observe the last value
-     * written.
+     * ... here, and so on.
      */
-    success = _Atomic_Compare_exchange_uint(
-      &the_thread->Lock.generation,
-      &my_generation,
-      my_generation,
-      ATOMIC_ORDER_RELAXED,
-      ATOMIC_ORDER_RELAXED
-    );
-
-    if ( success ) {
+    if ( lock == the_thread->Lock.current ) {
       return lock;
     }
 
@@ -1185,22 +1171,6 @@ RTEMS_INLINE_ROUTINE void _Thread_Lock_set_unprotected(
 )
 {
   the_thread->Lock.current = new_lock;
-
-  /*
-   * The generation release corresponds to the generation acquire in
-   * _Thread_Lock_acquire() and ensures that the new lock and other fields are
-   * visible to the next thread lock owner.  Otherwise someone would be able to
-   * read an up to date generation number and an old lock.  See
-   * _Thread_Wait_set_queue() and _Thread_Wait_restore_default_operations().
-   *
-   * Since we set a new lock right before, this increment is not protected by a
-   * lock and thus must be an atomic operation.
-   */
-  _Atomic_Fetch_add_uint(
-    &the_thread->Lock.generation,
-    1,
-    ATOMIC_ORDER_RELEASE
-  );
 }
 #endif