proc_ra: protect RA procedure from concurrent thread access

that patch addreses issue #2199 by defering RA-related calls that
are executed from within PHY workers.

The time-critical bits of more complex functions such as tb_decoded()
are still executed in the PHY worker thread but the state machine
manipulation is defered.

srsue/hdr/stack/mac/proc_ra.h

@@ -13,6 +13,7 @@
 #ifndef SRSUE_PROC_RA_H
 #define SRSUE_PROC_RA_H
 
+#include <atomic>
 #include <mutex>
 #include <stdint.h>
 
@@ -30,30 +31,7 @@ namespace srsue {
 class ra_proc : public srslte::timer_callback
 {
 public:
-  ra_proc() : rar_pdu_msg(20)
-  {
-    bzero(&softbuffer_rar, sizeof(srslte_softbuffer_rx_t));
-    pcap                      = NULL;
-    backoff_interval_start    = 0;
-    backoff_interval          = 0;
-    received_target_power_dbm = 0;
-    ra_rnti                   = 0;
-    current_ta                = 0;
-    state                     = IDLE;
-    last_msg3_group           = RA_GROUP_A;
-    phy_h                     = NULL;
-    mux_unit                  = NULL;
-    rrc                       = NULL;
-    transmitted_contention_id = 0;
-    transmitted_crnti         = 0;
-    started_by_pdcch          = false;
-    rar_grant_nbytes          = 0;
-
-    noncontention_enabled = false;
-    next_preamble_idx     = 0;
-    next_prach_mask       = 0;
-    current_task_id       = 0;
-  };
+  ra_proc() : rar_pdu_msg(20){};
 
   ~ra_proc();
 
@@ -82,8 +60,8 @@ public:
   void timer_expired(uint32_t timer_id);
   void new_grant_dl(mac_interface_phy_lte::mac_grant_dl_t grant, mac_interface_phy_lte::tb_action_dl_t* action);
   void tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti);
+  bool contention_resolution_id_received(uint64_t rx_contention_id);
 
-  bool contention_resolution_id_received(uint64_t uecri);
   void start_pcap(srslte::mac_pcap* pcap);
 
   void notify_ra_completed(uint32_t task_id);
@@ -104,40 +82,42 @@ private:
   void response_error();
   void complete();
 
+  bool contention_resolution_id_received_unsafe(uint64_t rx_contention_id);
+
   //  Buffer to receive RAR PDU
   static const uint32_t MAX_RAR_PDU_LEN                 = 2048;
-  uint8_t               rar_pdu_buffer[MAX_RAR_PDU_LEN];
+  uint8_t               rar_pdu_buffer[MAX_RAR_PDU_LEN] = {};
   srslte::rar_pdu       rar_pdu_msg;
 
   // Random Access parameters provided by higher layers defined in 5.1.1
-  srslte::rach_cfg_t rach_cfg, new_cfg;
+  srslte::rach_cfg_t rach_cfg = {};
 
-  int      delta_preamble_db;
-  uint32_t maskIndex;
-  int      preambleIndex;
-  uint32_t new_ra_msg_len;
+  int      delta_preamble_db = 0;
+  uint32_t maskIndex         = 0;
+  int      preambleIndex     = 0;
+  uint32_t new_ra_msg_len    = 0;
 
-  bool     noncontention_enabled;
-  uint32_t next_preamble_idx;
-  uint32_t next_prach_mask;
+  bool     noncontention_enabled = false;
+  uint32_t next_preamble_idx     = 0;
+  uint32_t next_prach_mask       = 0;
 
   // Internal variables
-  uint32_t preambleTransmissionCounter;
-  uint32_t backoff_param_ms;
-  uint32_t sel_maskIndex;
-  uint32_t sel_preamble;
-  int      backoff_interval_start;
-  uint32_t backoff_interval;
-  int      received_target_power_dbm;
-  uint32_t ra_rnti;
-  uint32_t ra_tti;
-  uint32_t current_ta;
+  uint32_t              preambleTransmissionCounter = 0;
+  uint32_t              backoff_param_ms            = 0;
+  uint32_t              sel_maskIndex               = 0;
+  std::atomic<uint32_t> sel_preamble                = {0};
+  int                   backoff_interval_start      = 0;
+  uint32_t              backoff_interval            = 0;
+  int                   received_target_power_dbm   = 0;
+  uint32_t              ra_rnti                     = SRSLTE_INVALID_RNTI;
+  uint32_t              ra_tti                      = 0;
+  uint32_t              current_ta                  = 0;
   // The task_id is a unique number associated with each RA procedure used to track background tasks
-  uint32_t current_task_id;
+  uint32_t current_task_id = 0;
 
-  srslte_softbuffer_rx_t softbuffer_rar;
+  srslte_softbuffer_rx_t softbuffer_rar = {};
 
-  enum {
+  enum ra_state_t {
     IDLE = 0,
     PDCCH_SETUP,
     RESPONSE_RECEPTION,
@@ -145,36 +125,36 @@ private:
     CONTENTION_RESOLUTION,
     START_WAIT_COMPLETION,
     WAITING_COMPLETION
-  } state;
+  };
+  std::atomic<ra_state_t> state = {IDLE};
 
   typedef enum { RA_GROUP_A, RA_GROUP_B } ra_group_t;
 
-  ra_group_t last_msg3_group;
+  ra_group_t last_msg3_group = RA_GROUP_A;
 
-  uint32_t rar_window_st;
+  uint32_t rar_window_st = 0;
 
   void read_params();
 
-  phy_interface_mac_lte*         phy_h;
+  phy_interface_mac_lte*                phy_h = nullptr;
   srslte::log_ref                       log_h;
-  mux*                           mux_unit;
-  srslte::mac_pcap*              pcap;
-  rrc_interface_mac*             rrc;
+  mux*                                  mux_unit   = nullptr;
+  srslte::mac_pcap*                     pcap       = nullptr;
+  rrc_interface_mac*                    rrc        = nullptr;
   srslte::ext_task_sched_handle*        task_sched = nullptr;
+  srslte::task_multiqueue::queue_handle task_queue;
 
   srslte::timer_handler::unique_timer* time_alignment_timer = nullptr;
   srslte::timer_handler::unique_timer  contention_resolution_timer;
 
-  mac_interface_rrc::ue_rnti_t* rntis;
-
-  uint64_t transmitted_contention_id;
-  uint16_t transmitted_crnti;
+  mac_interface_rrc::ue_rnti_t* rntis = nullptr;
 
-  std::mutex mutex;
+  std::atomic<uint64_t> transmitted_contention_id = {0};
+  std::atomic<uint16_t> transmitted_crnti         = {0};
 
-  bool     started_by_pdcch;
-  uint32_t rar_grant_nbytes;
-  bool     rar_received;
+  bool     started_by_pdcch = false;
+  uint32_t rar_grant_nbytes = 0;
+  bool     rar_received     = false;
 };
 
 } // namespace srsue

srsue/src/stack/mac/mac.cc

@@ -306,7 +306,8 @@ uint16_t mac::get_dl_sched_rnti(uint32_t tti)
       return SRSLTE_SIRNTI;
     }
   }
-  if (ra_window_start > 0 && ra_window_length > 0 && is_in_window(tti, &ra_window_start, &ra_window_length)) {
+  if (uernti.rar_rnti && ra_window_start > 0 && ra_window_length > 0 &&
+      is_in_window(tti, &ra_window_start, &ra_window_length)) {
     Debug("SCHED: Searching RAR-RNTI=0x%x, tti=%d\n", uernti.rar_rnti, tti);
     return uernti.rar_rnti;
   }

srsue/src/stack/mac/proc_ra.cc

@@ -55,6 +55,8 @@ void ra_proc::init(phy_interface_mac_lte*               phy_h_,
   rrc        = rrc_;
   task_sched = task_sched_;
 
+  task_queue = task_sched->make_task_queue();
+
   time_alignment_timer        = time_alignment_timer_;
   contention_resolution_timer = task_sched->get_unique_timer();
 
@@ -80,16 +82,16 @@ void ra_proc::start_pcap(srslte::mac_pcap* pcap_)
   pcap = pcap_;
 }
 
-/* Sets a new configuration. The configuration is applied by initialization() function */
+// RRC calls to set a new PRACH configuration.
+// The configuration is applied by initialization() function.
 void ra_proc::set_config(srslte::rach_cfg_t& rach_cfg_)
 {
-  std::unique_lock<std::mutex> ul(mutex);
-  new_cfg = rach_cfg_;
+  rach_cfg = rach_cfg_;
 }
 
+// RRC might also call this to set additional params during mobility
 void ra_proc::set_config_ded(uint32_t preamble_index, uint32_t prach_mask)
 {
-  std::unique_lock<std::mutex> ul(mutex);
   next_preamble_idx     = preamble_index;
   next_prach_mask       = prach_mask;
   noncontention_enabled = true;
@@ -98,10 +100,6 @@ void ra_proc::set_config_ded(uint32_t preamble_index, uint32_t prach_mask)
 /* Reads the configuration and configures internal variables */
 void ra_proc::read_params()
 {
-  mutex.lock();
-  rach_cfg = new_cfg;
-  mutex.unlock();
-
   // Read initialization parameters
   if (noncontention_enabled) {
     preambleIndex         = next_preamble_idx;
@@ -128,7 +126,7 @@ void ra_proc::read_params()
  */
 void ra_proc::step(uint32_t tti_)
 {
-  switch (state) {
+  switch (state.load()) {
     case IDLE:
       break;
     case PDCCH_SETUP:
@@ -160,8 +158,9 @@ void ra_proc::state_pdcch_setup()
   if (info.is_transmitted) {
     ra_tti  = info.tti_ra;
     ra_rnti = 1 + (ra_tti % 10) + (10 * info.f_id);
-    rInfo("seq=%d, ra-rnti=0x%x, ra-tti=%d, f_id=%d\n", sel_preamble, ra_rnti, info.tti_ra, info.f_id);
-    srslte::console("Random Access Transmission: seq=%d, tti=%d, ra-rnti=0x%x\n", sel_preamble, info.tti_ra, ra_rnti);
+    rInfo("seq=%d, ra-rnti=0x%x, ra-tti=%d, f_id=%d\n", sel_preamble.load(), ra_rnti, info.tti_ra, info.f_id);
+    srslte::console(
+        "Random Access Transmission: seq=%d, tti=%d, ra-rnti=0x%x\n", sel_preamble.load(), info.tti_ra, ra_rnti);
     rar_window_st   = ra_tti + 3;
     rntis->rar_rnti = ra_rnti;
     state           = RESPONSE_RECEPTION;
@@ -314,7 +313,7 @@ void ra_proc::resource_selection()
   }
 
   rDebug("Selected preambleIndex=%d maskIndex=%d GroupA=%d, GroupB=%d\n",
-         sel_preamble,
+         sel_preamble.load(),
          sel_maskIndex,
          rach_cfg.nof_groupA_preambles,
          nof_groupB_preambles);
@@ -326,12 +325,11 @@ void ra_proc::resource_selection()
 /* Preamble transmission as defined in 5.1.3 */
 void ra_proc::preamble_transmission()
 {
-
   received_target_power_dbm = rach_cfg.iniReceivedTargetPower + delta_preamble_db +
                               (preambleTransmissionCounter - 1) * rach_cfg.powerRampingStep;
 
   phy_h->prach_send(sel_preamble, sel_maskIndex - 1, received_target_power_dbm);
-  rntis->rar_rnti        = 0;
+  rntis->rar_rnti        = SRSLTE_INVALID_RNTI;
   ra_tti                 = 0;
   rar_received           = false;
   backoff_interval_start = -1;
@@ -385,8 +383,10 @@ void ra_proc::new_grant_dl(mac_interface_phy_lte::mac_grant_dl_t grant, mac_inte
   }
 }
 
-/* Called upon the successful decoding of a TB addressed to RA-RNTI.
- * Processes the reception of a RAR as defined in 5.1.4
+/* Called from PHY worker upon the successful decoding of a TB addressed to RA-RNTI.
+ * We extract the most relevant and time critical details from the RAR PDU,
+ * as definied in 5.1.4 and then defer the handling of the RA state machine to be
+ * executed on the Stack thread.
  */
 void ra_proc::tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti)
 {
@@ -394,10 +394,12 @@ void ra_proc::tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti)
     pcap->write_dl_ranti(rar_pdu_buffer, rar_grant_nbytes, ra_rnti, true, tti, cc_idx);
   }
 
-  rDebug("RAR decoded successfully TBS=%d\n", rar_grant_nbytes);
-
   rar_pdu_msg.init_rx(rar_grant_nbytes);
-  rar_pdu_msg.parse_packet(rar_pdu_buffer);
+  if (rar_pdu_msg.parse_packet(rar_pdu_buffer) != SRSLTE_SUCCESS) {
+    rError("Error decoding RAR PDU\n");
+  }
+
+  rDebug("RAR decoded successfully TBS=%d\n", rar_grant_nbytes);
 
   // Set Backoff parameter
   if (rar_pdu_msg.has_backoff()) {
@@ -417,24 +419,21 @@ void ra_proc::tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti)
       // TODO: Indicate received target power
       // phy_h->set_target_power_rar(iniReceivedTargetPower, (preambleTransmissionCounter-1)*powerRampingStep);
 
-      uint8_t grant[srslte::rar_subh::RAR_GRANT_LEN];
+      uint8_t grant[srslte::rar_subh::RAR_GRANT_LEN] = {};
       rar_pdu_msg.get()->get_sched_grant(grant);
 
-      rntis->rar_rnti = 0;
+      rntis->rar_rnti = SRSLTE_INVALID_RNTI;
       phy_h->set_rar_grant(grant, rar_pdu_msg.get()->get_temp_crnti());
 
       current_ta = rar_pdu_msg.get()->get_ta_cmd();
 
       rInfo("RAPID=%d, TA=%d, T-CRNTI=0x%x\n",
-            sel_preamble,
+            sel_preamble.load(),
             rar_pdu_msg.get()->get_ta_cmd(),
             rar_pdu_msg.get()->get_temp_crnti());
 
-      if (preambleIndex > 0) {
-        // Preamble selected by Network
-        complete();
-      } else {
-        // Preamble selected by UE MAC
+      // Perform actions when preamble was selected by UE MAC
+      if (preambleIndex <= 0) {
         mux_unit->msg3_prepare();
         rntis->temp_rnti = rar_pdu_msg.get()->get_temp_crnti();
 
@@ -446,7 +445,7 @@ void ra_proc::tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti)
 
           // If we have a C-RNTI, tell Mux unit to append C-RNTI CE if no CCCH SDU transmission
           if (transmitted_crnti) {
-            rInfo("Appending C-RNTI MAC CE 0x%x in next transmission\n", transmitted_crnti);
+            rInfo("Appending C-RNTI MAC CE 0x%x in next transmission\n", transmitted_crnti.load());
             mux_unit->append_crnti_ce_next_tx(transmitted_crnti);
           }
         }
@@ -454,8 +453,13 @@ void ra_proc::tb_decoded_ok(const uint8_t cc_idx, const uint32_t tti)
         // Save transmitted UE contention id, as defined by higher layers
         transmitted_contention_id = rntis->contention_id;
 
+        task_queue.push([this]() {
           rDebug("Waiting for Contention Resolution\n");
           state = CONTENTION_RESOLUTION;
+        });
+      } else {
+        // Preamble selected by Network, defer result handling
+        task_queue.push([this]() { complete(); });
       }
     } else {
       if (rar_pdu_msg.get()->has_rapid()) {
@@ -549,9 +553,22 @@ void ra_proc::timer_expired(uint32_t timer_id)
 }
 
 /* Function called by MAC when a Contention Resolution ID CE is received.
- * Performs the actions defined in 5.1.5 for Temporal C-RNTI Contention Resolution
+ * Since this is called from within a PHY worker thread, we enqueue the handling,
+ * check that the contention resolution IDs match and return so the DL TB can be acked.
+ *
+ * The RA-related actions are scheduled to be executed on the Stack thread,
+ * even if we realize later that we have received that in a wrong state.
  */
 bool ra_proc::contention_resolution_id_received(uint64_t rx_contention_id)
+{
+  task_queue.push([this, rx_contention_id]() { contention_resolution_id_received_unsafe(rx_contention_id); });
+  return (transmitted_contention_id == rx_contention_id);
+}
+
+/*
+ * Performs the actions defined in 5.1.5 for Temporal C-RNTI Contention Resolution
+ */
+bool ra_proc::contention_resolution_id_received_unsafe(uint64_t rx_contention_id)
 {
   bool uecri_successful = false;
 
@@ -571,7 +588,7 @@ bool ra_proc::contention_resolution_id_received(uint64_t rx_contention_id)
     complete();
   } else {
     rInfo("Transmitted UE Contention Id differs from received Contention ID (0x%" PRIx64 " != 0x%" PRIx64 ")\n",
-          transmitted_contention_id,
+          transmitted_contention_id.load(),
           rx_contention_id);
 
     // Discard MAC PDU
@@ -584,8 +601,10 @@ bool ra_proc::contention_resolution_id_received(uint64_t rx_contention_id)
   return uecri_successful;
 }
 
+// Called from PHY worker context, defer actions therefore.
 void ra_proc::pdcch_to_crnti(bool is_new_uplink_transmission)
 {
+  task_queue.push([this, is_new_uplink_transmission]() {
     // TS 36.321 Section 5.1.5
     rDebug("PDCCH to C-RNTI received %s new UL transmission\n", is_new_uplink_transmission ? "with" : "without");
     if ((!started_by_pdcch && is_new_uplink_transmission) || started_by_pdcch) {
@@ -593,8 +612,10 @@ void ra_proc::pdcch_to_crnti(bool is_new_uplink_transmission)
       contention_resolution_timer.stop();
       complete();
     }
+  });
 }
 
+// Called from the Stack thread
 void ra_proc::update_rar_window(int& rar_window_start, int& rar_window_length)
 {
   if (state != RESPONSE_RECEPTION) {
@@ -611,13 +632,19 @@ void ra_proc::update_rar_window(int& rar_window_start, int& rar_window_length)
 // Restart timer at each Msg3 HARQ retransmission (5.1.5)
 void ra_proc::harq_retx()
 {
+  task_queue.push([this]() {
     rInfo("Restarting ContentionResolutionTimer=%d ms\n", contention_resolution_timer.duration());
     contention_resolution_timer.run();
+  });
 }
 
+// Called from PHY worker thread
 void ra_proc::harq_max_retx()
 {
+  task_queue.push([this]() {
     Warning("Contention Resolution is considered not successful. Stopping PDCCH Search and going to Response Error\n");
     response_error();
+  });
 }
+
 } // namespace srsue

srsue/test/mac_test.cc

@@ -2097,7 +2097,7 @@ int run_mac_ra_test(struct ra_test test, mac* mac, phy_dummy* phy, uint32_t* tti
 
         // Step to contention resolution. Make sure timer does not start until Msg3 is transmitted
         // and restarts on every retx
-        for (int k = 0; k < test.rach_cfg.ra_supervision_info.mac_contention_resolution_timer.to_number() - 1; k++) {
+        for (int k = 0; k < test.rach_cfg.ra_supervision_info.mac_contention_resolution_timer.to_number() - 2; k++) {
           stack->run_tti(tti);
           TESTASSERT(mac->get_dl_sched_rnti(tti) == (test.crnti ? test.crnti : test.temp_rnti));
           tti++;
@@ -2115,6 +2115,7 @@ int run_mac_ra_test(struct ra_test test, mac* mac, phy_dummy* phy, uint32_t* tti
       }
 
       if (test.nof_msg3_retx == test.rach_cfg.max_harq_msg3_tx) {
+        stack->run_tti(tti); // RNTI will be reset for next TTI
         TESTASSERT(mac->get_dl_sched_rnti(tti) != temp_rnti);
         break;
       }
@@ -2164,6 +2165,7 @@ int run_mac_ra_test(struct ra_test test, mac* mac, phy_dummy* phy, uint32_t* tti
 
   // RA procedure should be completed here
   if (test.check_ra_successful) {
+    stack->run_tti(tti);
     stack->run_tti(tti);
     TESTASSERT(phy->get_crnti() == (test.crnti ? test.crnti : test.temp_rnti));
     TESTASSERT(mac->get_dl_sched_rnti(tti) == (test.crnti ? test.crnti : test.temp_rnti));
@@ -2218,6 +2220,9 @@ int mac_random_access_test()
   set_mac_cfg_t_rach_cfg_common(&mac_cfg, rach_cfg);
   mac.set_config(mac_cfg);
 
+  uint32 tti = 0;
+  stack.run_tti(tti++); // make sure MAC/PRACH config is applied
+
   // generate config for LCIDs in different LCGs than CCCH
   std::vector<logical_channel_config_t> lcids;
   logical_channel_config_t              config = {};
@@ -2239,8 +2244,6 @@ int mac_random_access_test()
   rlc.write_sdu(0, 6);   // UL-CCCH with Msg3
   rlc.write_sdu(3, 100); // DRB data on other LCG
 
-  uint32 tti = 0;
-
   // Structure that defines the test to be executed
   struct ra_test my_test           = {};
   uint32_t       test_id           = 1;
@@ -2339,6 +2342,7 @@ int mac_random_access_test()
   my_test.check_ra_successful      = false;
   my_test.send_valid_ul_grant      = false;
   TESTASSERT(!run_mac_ra_test(my_test, &mac, &phy, &tti, &stack));
+  stack.run_tti(tti++); // need to wait until complete RA result is signalled
   TESTASSERT(!rrc.ho_finish_successful);
   TESTASSERT(rrc.rach_problem == 2);
 
@@ -2358,6 +2362,7 @@ int mac_random_access_test()
   my_test.msg4_enable         = true;
   my_test.send_valid_ul_grant = true;
   TESTASSERT(!run_mac_ra_test(my_test, &mac, &phy, &tti, &stack));
+  stack.run_tti(tti++); // need to wait until complete RA result is signalled
   TESTASSERT(rrc.ho_finish_successful);
 
   // Test 9: Test non-Contention based HO. Used in HO but preamble is given by the network. In addition to checking