From e91da361fef77ebd31d15de51e06d5f46b3040f6 Mon Sep 17 00:00:00 2001 From: Jack Whitham Date: Tue, 13 May 2025 19:11:08 +0100 Subject: [PATCH 1/5] 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. --- src/rp2_common/pico_multicore/multicore.c | 53 ++++++++++++++--------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/src/rp2_common/pico_multicore/multicore.c b/src/rp2_common/pico_multicore/multicore.c index f8f5660db..5bf0e9aaa 100644 --- a/src/rp2_common/pico_multicore/multicore.c +++ b/src/rp2_common/pico_multicore/multicore.c @@ -203,24 +203,26 @@ void multicore_launch_core1_raw(void (*entry)(void), uint32_t *sp, uint32_t vect } #define LOCKOUT_MAGIC_START 0x73a8831eu -#define LOCKOUT_MAGIC_END (~LOCKOUT_MAGIC_START) static mutex_t lockout_mutex; -static bool lockout_in_progress; +static io_rw_32 lockout_request_id = LOCKOUT_MAGIC_START; // 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); } } } @@ -257,7 +259,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); @@ -267,7 +269,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; @@ -276,7 +278,7 @@ 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); @@ -284,14 +286,28 @@ static bool multicore_lockout_handshake(uint32_t magic, absolute_time_t until) { return rc; } +static uint32_t update_lockout_request_id() { + // 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; } @@ -309,13 +325,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) { @@ -402,4 +415,4 @@ void multicore_doorbell_unclaim(uint doorbell_num, uint core_mask) { } -#endif \ No newline at end of file +#endif From eadf08bc55c5a4d995009df50f9cbfaa4ee8842e Mon Sep 17 00:00:00 2001 From: Jack Whitham Date: Wed, 14 May 2025 18:39:05 +0100 Subject: [PATCH 2/5] Update documentation for multicore_lockout_end functions --- .../pico_multicore/include/pico/multicore.h | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/rp2_common/pico_multicore/include/pico/multicore.h b/src/rp2_common/pico_multicore/include/pico/multicore.h index 9b4622a98..fe198aed6 100644 --- a/src/rp2_common/pico_multicore/include/pico/multicore.h +++ b/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 cannot 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); From 5551101486721f49e83da1ded7391c6927224981 Mon Sep 17 00:00:00 2001 From: Jack Whitham Date: Thu, 15 May 2025 19:03:29 +0100 Subject: [PATCH 3/5] Simplification, remove magic number (not required) --- src/rp2_common/pico_multicore/multicore.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/rp2_common/pico_multicore/multicore.c b/src/rp2_common/pico_multicore/multicore.c index 5bf0e9aaa..d6e6aa030 100644 --- a/src/rp2_common/pico_multicore/multicore.c +++ b/src/rp2_common/pico_multicore/multicore.c @@ -202,10 +202,8 @@ 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 - static mutex_t lockout_mutex; -static io_rw_32 lockout_request_id = LOCKOUT_MAGIC_START; +static io_rw_32 lockout_request_id = 0; // note this method is in RAM because lockout is used when writing to flash // it only makes inline calls From d928e453a1ad031528d7be312aea5dc4a6db5dac Mon Sep 17 00:00:00 2001 From: Jack Whitham Date: Fri, 16 May 2025 19:20:37 +0100 Subject: [PATCH 4/5] Review improvements --- src/rp2_common/pico_multicore/multicore.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rp2_common/pico_multicore/multicore.c b/src/rp2_common/pico_multicore/multicore.c index d6e6aa030..3c5e1989f 100644 --- a/src/rp2_common/pico_multicore/multicore.c +++ b/src/rp2_common/pico_multicore/multicore.c @@ -203,7 +203,7 @@ void multicore_launch_core1_raw(void (*entry)(void), uint32_t *sp, uint32_t vect } static mutex_t lockout_mutex; -static io_rw_32 lockout_request_id = 0; +static volatile uint32_t lockout_request_id = 0; // note this method is in RAM because lockout is used when writing to flash // it only makes inline calls @@ -284,7 +284,7 @@ static bool multicore_lockout_handshake(uint32_t request_id, absolute_time_t unt return rc; } -static uint32_t update_lockout_request_id() { +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; From 38de1271f8000d2944fdec87f6e50d58b93bd363 Mon Sep 17 00:00:00 2001 From: Jack Whitham Date: Sat, 17 May 2025 22:01:15 +0100 Subject: [PATCH 5/5] Restore use of non-zero magic number --- src/rp2_common/pico_multicore/multicore.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/rp2_common/pico_multicore/multicore.c b/src/rp2_common/pico_multicore/multicore.c index 3c5e1989f..3e397c74e 100644 --- a/src/rp2_common/pico_multicore/multicore.c +++ b/src/rp2_common/pico_multicore/multicore.c @@ -202,8 +202,10 @@ void multicore_launch_core1_raw(void (*entry)(void), uint32_t *sp, uint32_t vect irq_set_enabled(irq_num, enabled); } +// 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 volatile uint32_t lockout_request_id = 0; // note this method is in RAM because lockout is used when writing to flash // it only makes inline calls