Fix multicore_lockout features so that the victim core cannot become stuck in an infinite loop if the lockout attempt times out (earlephilhower#2467)

* Fix issue 2454 flash safe execute lockout

The lockout state is controlled by a shared variable. The FIFO
is used to begin a lockout and acknowledge it (i.e.
multicore_lockout_handshake works as before) but the end
of the lockout is now signalled by updating the shared variable.
This ensures that timeouts are recognised reliably by the victim core.
__wfe and __sev are used to signal updates to the shared variable
in order to avoid polling.

* Update documentation for multicore_lockout_end functions

* Simplification, remove magic number (not required)

* Review improvements

* Restore use of non-zero magic number

Author: jwhitham

src/rp2_common/pico_multicore/include/pico/multicore.h

@@ -437,7 +437,7 @@ static inline uint multicore_doorbell_irq_num(uint doorbell_num) {
  * The core which wishes to lockout the other core calls \ref multicore_lockout_start_blocking or
  * \ref multicore_lockout_start_timeout_us to interrupt the other "victim" core and wait for it to be in a
  * "locked out" state. Once the lockout is no longer needed it calls \ref multicore_lockout_end_blocking or
- * \ref multicore_lockout_end_timeout_us to release the lockout and wait for confirmation.
+ * \ref multicore_lockout_end_timeout_us to release the lockout.
  *
  * \note Because multicore lockout uses the intercore FIFOs, the FIFOs <b>cannot</b> be used for any other purpose
  */
@@ -491,27 +491,31 @@ void multicore_lockout_start_blocking(void);
  */
 bool multicore_lockout_start_timeout_us(uint64_t timeout_us);
 
-/*! \brief Release the other core from a locked out state amd wait for it to acknowledge
+/*! \brief Release the other core from a locked out state
  *  \ingroup multicore_lockout
  *
- * \note The other core must previously have been "locked out" by calling a `multicore_lockout_start_` function
- * from this core
+ * The other core must previously have been "locked out" by calling a `multicore_lockout_start_` function
+ * from this core.
+ *
+ * \note The other core will leave the lockout state if this function is called.
+ * The function only blocks for access to a lockout mutex, it does not wait for the other core
+ * to leave the lockout state.
  */
 void multicore_lockout_end_blocking(void);
 
-/*! \brief Release the other core from a locked out state amd wait up to a time limit for it to acknowledge
+/*! \brief Release the other core from a locked out state
  *  \ingroup multicore_lockout
  *
  * The other core must previously have been "locked out" by calling a `multicore_lockout_start_` function
  * from this core
  *
- * \note be very careful using small timeout values, as a timeout here will leave the "lockout" functionality
- * in a bad state. It is probably preferable to use \ref multicore_lockout_end_blocking anyway as if you have
- * already waited for the victim core to enter the lockout state, then the victim core will be ready to exit
- * the lockout state very quickly.
+ * \note The other core will leave the lockout state if this function returns true.
+ * The function only blocks for access to a lockout mutex, it does not wait for the other core
+ * to leave the lockout state. If the lockout mutex could not be acquired, the function returns
+ * false and no action is taken.
  *
  * \param timeout_us the timeout in microseconds
- * \return true if the other core successfully exited locked out state within the timeout, false otherwise
+ * \return true if the other core will leave the lockout state, false otherwise
  */
 bool multicore_lockout_end_timeout_us(uint64_t timeout_us);
 

src/rp2_common/pico_multicore/multicore.c

@@ -207,25 +207,27 @@ void multicore_launch_core1_raw(void (*entry)(void), uint32_t *sp, uint32_t vect
     irq_set_enabled(irq_num, enabled);
 }
 
-#define LOCKOUT_MAGIC_START 0x73a8831eu
-#define LOCKOUT_MAGIC_END (~LOCKOUT_MAGIC_START)
-
+// A non-zero initialisation value is used in order to reduce the chance of
+// entering the lock handler on bootup due to a 0-word being present in the FIFO
+static volatile uint32_t lockout_request_id = 0x73a8831eu;
 static mutex_t lockout_mutex;
-static bool lockout_in_progress;
 
 // note this method is in RAM because lockout is used when writing to flash
 // it only makes inline calls
 static void __isr __not_in_flash_func(multicore_lockout_handler)(void) {
     multicore_fifo_clear_irq();
     while (multicore_fifo_rvalid()) {
-        if (sio_hw->fifo_rd == LOCKOUT_MAGIC_START) {
+        uint32_t request_id = sio_hw->fifo_rd;
+        if (request_id == lockout_request_id) {
+            // valid lockout request received
             uint32_t save = save_and_disable_interrupts();
-            multicore_fifo_push_blocking_inline(LOCKOUT_MAGIC_START);
-            while (multicore_fifo_pop_blocking_inline() != LOCKOUT_MAGIC_END) {
-                tight_loop_contents(); // not tight but endless potentially
+            multicore_fifo_push_blocking_inline(request_id);
+            // wait for the lockout to expire
+            while (request_id == lockout_request_id) {
+                // when lockout_request_id is updated, the other CPU core calls __sev
+                __wfe();
             }
             restore_interrupts_from_disabled(save);
-            multicore_fifo_push_blocking_inline(LOCKOUT_MAGIC_END);
         }
     }
 }
@@ -262,7 +264,7 @@ void multicore_lockout_victim_deinit(void) {
     }
 }
 
-static bool multicore_lockout_handshake(uint32_t magic, absolute_time_t until) {
+static bool multicore_lockout_handshake(uint32_t request_id, absolute_time_t until) {
     uint irq_num = SIO_FIFO_IRQ_NUM(get_core_num());
     bool enabled = irq_is_enabled(irq_num);
     if (enabled) irq_set_enabled(irq_num, false);
@@ -272,7 +274,7 @@ static bool multicore_lockout_handshake(uint32_t magic, absolute_time_t until) {
         if (next_timeout_us < 0) {
             break;
         }
-        multicore_fifo_push_timeout_us(magic, (uint64_t)next_timeout_us);
+        multicore_fifo_push_timeout_us(request_id, (uint64_t)next_timeout_us);
         next_timeout_us = absolute_time_diff_us(get_absolute_time(), until);
         if (next_timeout_us < 0) {
             break;
@@ -281,22 +283,36 @@ static bool multicore_lockout_handshake(uint32_t magic, absolute_time_t until) {
         if (!multicore_fifo_pop_timeout_us((uint64_t)next_timeout_us, &word)) {
             break;
         }
-        if (word == magic) {
+        if (word == request_id) {
             rc = true;
         }
     } while (!rc);
     if (enabled) irq_set_enabled(irq_num, true);
     return rc;
 }
 
+static uint32_t update_lockout_request_id(void) {
+    // generate new number and then update shared variable
+    uint32_t new_request_id = lockout_request_id + 1;
+    lockout_request_id = new_request_id;
+    // notify other core
+    __sev();
+    return new_request_id;
+}
+
 static bool multicore_lockout_start_block_until(absolute_time_t until) {
     check_lockout_mutex_init();
     if (!mutex_enter_block_until(&lockout_mutex, until)) {
         return false;
     }
-    hard_assert(!lockout_in_progress);
-    bool rc = multicore_lockout_handshake(LOCKOUT_MAGIC_START, until);
-    lockout_in_progress = rc;
+    // generate a new request_id number
+    uint32_t request_id = update_lockout_request_id();
+    // attempt to lock out
+    bool rc = multicore_lockout_handshake(request_id, until);
+    if (!rc) {
+        // lockout failed - cancel it
+        update_lockout_request_id();
+    }
     mutex_exit(&lockout_mutex);
     return rc;
 }
@@ -314,13 +330,10 @@ static bool multicore_lockout_end_block_until(absolute_time_t until) {
     if (!mutex_enter_block_until(&lockout_mutex, until)) {
         return false;
     }
-    assert(lockout_in_progress);
-    bool rc = multicore_lockout_handshake(LOCKOUT_MAGIC_END, until);
-    if (rc) {
-        lockout_in_progress = false;
-    }
+    // lockout finished - cancel it
+    update_lockout_request_id();
     mutex_exit(&lockout_mutex);
-    return rc;
+    return true;
 }
 
 bool multicore_lockout_end_timeout_us(uint64_t timeout_us) {
@@ -407,4 +420,4 @@ void multicore_doorbell_unclaim(uint doorbell_num, uint core_mask) {
 }
 
 
-#endif
+#endif