nimble/phy/nrf5x: fix race conditions in TIMER0 setup

In the nRF5x LL PHY, we use TIMER0 to trigger various Tx/Rx sequencing
tasks.  The PHY attempted to recover from some cases in which it was 'late'
setting up TIMER0; in theory, these ought not have happened, but if
interrupt priorities do not provide absolute priority for the LL (for
instance, in RTOS systems that have other priorities, or scheduling systems
that have incompatible interrupt priority in order to call into RTOS queue
management routines), then the PHY can indeed end up failing to schedule a
transmit / receive window in time.

In some cases, the recovery process for these had race conditions, which
could result in the LL state machine locking up (the end-of-tx interrupt
would never fire).  One race condition was extremely short (a few
instructions wide; if the timer was set, but the timer triggered before the
appropriate PPI could be configured); the other race condition was a
microsecond wide (comparison to see if the timer had already triggered
needed the current time to be greater in order to fail, but if it triggered
within that microsecond, the current time would be equal).  There are also
other potential issues around timer event reset and check that we did not
replicate, but did notice by inspection.

These cases appeared in a few different forms, so we factor out a "safe
reset" routine to ensure that a timer will not trigger (to be used before
setting up a PPI), and then we also factor out a routine to check whether a
timer has been missed.

This change is verified to fix some observed stability issues in PebbleOS
under heavy load.

Signed-off-by: Joshua Wise <[email protected]>

Author: jwise

nimble/drivers/nrf5x/src/ble_phy.c

@@ -583,6 +583,31 @@ ble_phy_rxpdu_copy(uint8_t *dptr, struct os_mbuf *rxpdu)
            sizeof(struct ble_mbuf_hdr));
 }
 
+/**
+ * There is no way on nRF5 to disable a COMPARE event entirely.  So we fake
+ * it, by setting it as far in the future as possible (by triggering a
+ * capture to it), and then clearing the event if it was set.
+ */
+static void
+timer0_safely_reset(int timer)
+{
+    nrf_timer_task_trigger(NRF_TIMER0, nrf_timer_capture_task_get(timer));
+    NRF_TIMER0->EVENTS_COMPARE[timer] = 0;
+}
+
+/**
+ * It is possible that we set up a timer, but we were too late -- it didn't
+ * fire.  Detect when this happens, reliably, by capturing the current time
+ * to a scratch counter and comparing with wraparound.
+ */
+static bool
+timer0_did_miss(int timer, int scratch)
+{
+    nrf_timer_task_trigger(NRF_TIMER0, nrf_timer_capture_task_get(scratch));
+    uint32_t ticks_rem = NRF_TIMER0->CC[timer] - NRF_TIMER0->CC[scratch];
+    return (ticks_rem >= 0x80000000) && !NRF_TIMER0->EVENTS_COMPARE[timer];
+}
+
 /**
  * Called when we want to wait if the radio is in either the rx or tx
  * disable states. We want to wait until that state is over before doing
@@ -885,13 +910,13 @@ ble_phy_wfr_enable(int txrx, uint8_t tx_phy_mode, uint32_t wfr_usecs)
     /* Adjust for delay between actual access address RX and EVENT_ADDRESS */
     end_time += g_ble_phy_t_rxaddrdelay[phy];
 
-    /* wfr_secs is the time from rxen until timeout */
-    nrf_timer_cc_set(NRF_TIMER0, 3, end_time);
-    NRF_TIMER0->EVENTS_COMPARE[3] = 0;
-
     /* Enable wait for response PPI */
+    timer0_safely_reset(3);
     phy_ppi_wfr_enable();
 
+    /* wfr_secs is the time from rxen until timeout */
+    nrf_timer_cc_set(NRF_TIMER0, 3, end_time);
+
     /*
      * It may happen that if CPU is halted for a brief moment (e.g. during flash
      * erase or write), TIMER0 already counted past CC[3] and thus wfr will not
@@ -904,8 +929,7 @@ ble_phy_wfr_enable(int txrx, uint8_t tx_phy_mode, uint32_t wfr_usecs)
      * CC[1] is only used as a reference on RX start, we do not need it here so
      * it can be used to read TIMER0 counter.
      */
-    nrf_timer_task_trigger(NRF_TIMER0, NRF_TIMER_TASK_CAPTURE1);
-    if (NRF_TIMER0->CC[1] > NRF_TIMER0->CC[3]) {
+    if (timer0_did_miss(3, 1 /* scratch */)) {
         phy_ppi_wfr_disable();
         nrf_radio_task_trigger(NRF_RADIO, NRF_RADIO_TASK_DISABLE);
     }
@@ -1096,15 +1120,15 @@ ble_phy_tx_end_isr(void)
 
 #if PHY_USE_FEM_LNA
         fem_time = rx_time - MYNEWT_VAL(BLE_FEM_LNA_TURN_ON_US);
-        nrf_timer_cc_set(NRF_TIMER0, 2, fem_time);
-        NRF_TIMER0->EVENTS_COMPARE[2] = 0;
+        timer0_safely_reset(2);
         phy_fem_enable_lna();
+        nrf_timer_cc_set(NRF_TIMER0, 2, fem_time);
 #endif
 
         radio_time = rx_time - BLE_PHY_T_RXENFAST;
-        nrf_timer_cc_set(NRF_TIMER0, 0, radio_time);
-        NRF_TIMER0->EVENTS_COMPARE[0] = 0;
+        timer0_safely_reset(0);
         phy_ppi_timer0_compare0_to_radio_rxen_enable();
+        nrf_timer_cc_set(NRF_TIMER0, 0, radio_time);
 
         /* In case TIMER0 did already count past CC[0] and/or CC[2], radio
          * and/or LNA may not be enabled. In any case we won't be stuck since
@@ -1142,18 +1166,17 @@ ble_phy_tx_end_isr(void)
         tx_time -= g_ble_phy_t_txdelay[g_ble_phy_data.phy_cur_phy_mode];
 
         radio_time = tx_time - BLE_PHY_T_TXENFAST;
-        nrf_timer_cc_set(NRF_TIMER0, 0, radio_time);
-        NRF_TIMER0->EVENTS_COMPARE[0] = 0;
+        timer0_safely_reset(0);
         phy_ppi_timer0_compare0_to_radio_txen_enable();
+        nrf_timer_cc_set(NRF_TIMER0, 0, radio_time);
 
 #if PHY_USE_FEM_PA
-        nrf_timer_cc_set(NRF_TIMER0, 2, fem_time);
-        NRF_TIMER0->EVENTS_COMPARE[2] = 0;
+        timer0_safely_reset(2);
         phy_fem_enable_pa();
+        nrf_timer_cc_set(NRF_TIMER0, 2, fem_time);
 #endif
 
-        nrf_timer_task_trigger(NRF_TIMER0, NRF_TIMER_TASK_CAPTURE3);
-        if (NRF_TIMER0->CC[3] > NRF_TIMER0->CC[0]) {
+        if (timer0_did_miss(0, 3 /* scratch */)) {
             phy_ppi_timer0_compare0_to_radio_txen_disable();
             g_ble_phy_data.phy_transition_late = 1;
         }
@@ -1292,14 +1315,14 @@ ble_phy_rx_end_isr(void)
     tx_time -= g_ble_phy_t_txdelay[g_ble_phy_data.phy_cur_phy_mode];
 
     radio_time = tx_time - BLE_PHY_T_TXENFAST;
-    nrf_timer_cc_set(NRF_TIMER0, 0, radio_time);
-    NRF_TIMER0->EVENTS_COMPARE[0] = 0;
+    timer0_safely_reset(0);
     phy_ppi_timer0_compare0_to_radio_txen_enable();
+    nrf_timer_cc_set(NRF_TIMER0, 0, radio_time);
 
 #if PHY_USE_FEM_PA
-    nrf_timer_cc_set(NRF_TIMER0, 2, fem_time);
-    NRF_TIMER0->EVENTS_COMPARE[2] = 0;
+    timer0_safely_reset(2);
     phy_fem_enable_pa();
+    nrf_timer_cc_set(NRF_TIMER0, 2, fem_time);
 #endif
 
     /* Need to check if TIMER0 did not already count past CC[0] and/or CC[2], so
@@ -1308,11 +1331,9 @@ ble_phy_rx_end_isr(void)
      *
      * Note: CC[3] is used only for wfr which we do not need here.
      */
-    nrf_timer_task_trigger(NRF_TIMER0, NRF_TIMER_TASK_CAPTURE3);
-    is_late = (NRF_TIMER0->CC[3] > radio_time) && !NRF_TIMER0->EVENTS_COMPARE[0];
+    is_late = timer0_did_miss(0, 3 /* scratch */);
 #if PHY_USE_FEM_PA
-    is_late = is_late ||
-              ((NRF_TIMER0->CC[3] > fem_time) && !NRF_TIMER0->EVENTS_COMPARE[2]);
+    is_late = is_late || timer0_did_miss(2, 3 /* scratch */);
 #endif
     if (is_late) {
         phy_ppi_timer0_compare0_to_radio_txen_disable();