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

2025-05-14T07:00:41-05:00

jwhitham commented

2025-05-14 07:00:41 -05:00

(Migrated from github.com)

This bug can be triggered if the lockout request times out. If the victim CPU core has interrupts disabled for a long time, it will not respond quickly enough to the lockout request, which times out. However, the request is still pending in the FIFO, and when it is eventually handled by the victim CPU core, that core enters an infinite loop, waiting for a LOCKOUT_MAGIC_END message which never arrives.

I considered the solution of always sending a LOCKOUT_MAGIC_END message even on timeout, but this wouldn't work if the FIFO was already full. I could not use a blocking wait, as this would not respect the timeout, and as far as I can tell, there is no hardware support for clearing the FIFO without also resetting the CPU.

In this PR, the lockout state is controlled by a shared variable. The FIFO is used to begin a lockout and acknowledge it as before. But the end of the lockout is now signalled by updating the shared variable. This ensures that the end of the lockout request is recognised reliably by the victim CPU core, regardless of whether the end was caused by a timeout or whether the lockout completed normally. __wfe and __sev are used to signal updates to the shared variable in order to avoid polling.

The semantics of the multicore_lockout_end_... functions change: they will no longer wait for the lockout to end. This is described further in the updated doxygen and in the comments below.

Fixes #2454 This bug can be triggered if the lockout request times out. If the victim CPU core has interrupts disabled for a long time, it will not respond quickly enough to the lockout request, which times out. However, the request is still pending in the FIFO, and when it is eventually handled by the victim CPU core, that core enters an infinite loop, waiting for a LOCKOUT_MAGIC_END message which never arrives. I considered the solution of always sending a LOCKOUT_MAGIC_END message even on timeout, but this wouldn't work if the FIFO was already full. I could not use a blocking wait, as this would not respect the timeout, and as far as I can tell, there is no hardware support for clearing the FIFO without also resetting the CPU. In this PR, the lockout state is controlled by a shared variable. The FIFO is used to begin a lockout and acknowledge it as before. But the end of the lockout is now signalled by updating the shared variable. This ensures that the end of the lockout request is recognised reliably by the victim CPU core, regardless of whether the end was caused by a timeout or whether the lockout completed normally. __wfe and __sev are used to signal updates to the shared variable in order to avoid polling. The semantics of the `multicore_lockout_end_...` functions change: they will no longer wait for the lockout to end. This is described further in the updated doxygen and in the comments below.

lurch (Migrated from github.com) reviewed

2025-05-14 07:36:31 -05:00

src/rp2_common/pico_multicore/multicore.c

AFAICT it looks like lockout_request_id only gets incremented and never gets decremented - does that mean that theoretically it might eventually overflow?

AFAICT it looks like `lockout_request_id` only gets incremented and never gets decremented - does that mean that theoretically it might eventually overflow?

lurch commented

2025-05-14 07:42:09 -05:00

(Migrated from github.com)

Does this PR mean that any of the Doxygen at https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/pico_multicore/include/pico/multicore.h#L418 also needs updating to match, or is that all still "correct"?

jwhitham (Migrated from github.com) reviewed

2025-05-14 12:54:18 -05:00

src/rp2_common/pico_multicore/multicore.c

That's correct, but overflow (with wrap to 0) is ok. The numerical value of the lockout_request_id does not matter, all that is important is that the current lockout request has a different value to any others that may still be pending in the FIFO.

jwhitham commented

2025-05-14 12:54:24 -05:00

(Migrated from github.com)

Does this PR mean that any of the Doxygen at https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/pico_multicore/include/pico/multicore.h#L418 also needs updating to match, or is that all still "correct"?

You're right, this did need updating. And, I didn't realise this until now, but the semantics of multicore_lockout_end_blocking and multicore_lockout_end_timeout_us are different now, because these functions do not wait for the lockout to end. They just guarantee that it will end.

I thought about whether it was a good idea to push ahead with this behaviour change. I think it is a good idea, because it's simpler, and I don't see the value in knowing that the end of the lockout has been acknowledged by the victim core. The new design avoids the possibility for multicore_lockout_end_timeout_us that "a timeout here will leave the "lockout" functionality in a bad state" (as previously stated in doxygen). Timeouts always leave the lockout functionality in a usable state, both for start and end. I think the old blocking behaviour for the multicore_lockout_end_... functions could be preserved but at the cost of a more complex design, and I feel like it wouldn't be justified... however, I may be missing something!

> Does this PR mean that any of the Doxygen at https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/pico_multicore/include/pico/multicore.h#L418 also needs updating to match, or is that all still "correct"? You're right, this did need updating. And, I didn't realise this until now, but the semantics of `multicore_lockout_end_blocking` and `multicore_lockout_end_timeout_us` are different now, because these functions do not wait for the lockout to end. They just guarantee that it will end. I thought about whether it was a good idea to push ahead with this behaviour change. I think it is a good idea, because it's simpler, and I don't see the value in knowing that the end of the lockout has been acknowledged by the victim core. The new design avoids the possibility for `multicore_lockout_end_timeout_us` that "a timeout here will leave the "lockout" functionality in a bad state" (as previously stated in doxygen). Timeouts always leave the lockout functionality in a usable state, both for start and end. I think the old blocking behaviour for the `multicore_lockout_end_...` functions could be preserved but at the cost of a more complex design, and I feel like it wouldn't be justified... however, I may be missing something!

lurch (Migrated from github.com) reviewed

2025-05-14 18:05:19 -05:00

src/rp2_common/pico_multicore/multicore.c

In that case, perhaps the LOCKOUT_MAGIC_START is a bit of a red herring, and it'd be better to remove it entirely and just initialise lockout_request_id with 0 ?

In that case, perhaps the `LOCKOUT_MAGIC_START` is a bit of a red herring, and it'd be better to remove it entirely and just initialise `lockout_request_id` with `0` ?

jwhitham (Migrated from github.com) reviewed

2025-05-15 13:04:37 -05:00

src/rp2_common/pico_multicore/multicore.c

Agree, done.

kilograham (Migrated from github.com) reviewed

2025-05-16 11:27:55 -05:00

src/rp2_common/pico_multicore/multicore.c

					
				@ -202,25 +202,27 @@ void multicore_launch_core1_raw(void (*entry)(void), uint32_t *sp, uint32_t vect

				    irq_set_enabled(irq_num, enabled);

Small style point, we would probably use volatile uint32_t explicitly here vs io_rw_32 which is really intended for memory mapped IO

Small style point, we would probably use `volatile uint32_t` explicitly here vs `io_rw_32` which is really intended for memory mapped IO

kilograham (Migrated from github.com) reviewed

2025-05-16 11:28:28 -05:00

src/rp2_common/pico_multicore/multicore.c

					
				@ -271,3 +272,4 @@

				        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;

can you add a (void) i'm pretty sure without this it makes at least one of the many GCC or Clang compiler versions unhappy

can you add a `(void)` i'm pretty sure without this it makes at least one of the many GCC or Clang compiler versions unhappy

kilograham (Migrated from github.com) requested changes

2025-05-16 11:28:48 -05:00

kilograham (Migrated from github.com) left a comment

Thanks, this looks great except a few nits

jwhitham (Migrated from github.com) reviewed

2025-05-16 13:38:45 -05:00

src/rp2_common/pico_multicore/multicore.c

					
				@ -202,25 +202,27 @@ void multicore_launch_core1_raw(void (*entry)(void), uint32_t *sp, uint32_t vect

				    irq_set_enabled(irq_num, enabled);

Thanks, replaced.

jwhitham (Migrated from github.com) reviewed

2025-05-16 13:38:47 -05:00

src/rp2_common/pico_multicore/multicore.c

					
				@ -271,3 +272,4 @@

				        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;

Thanks, fixed.

jwhitham (Migrated from github.com) reviewed

2025-05-17 16:10:45 -05:00

src/rp2_common/pico_multicore/multicore.c

After further testing I found that beginning at 0 causes a problem as a 0-value word is received by CPU core 0 via the FIFO after reset (on RP2350). Core 0 becomes stuck in multicore_lockout_victim_init until core 1 requests a lockout. To avoid this the number should begin at a non-zero value. I restored the original "magic" number and added a comment to explain this.

After further testing I found that beginning at 0 causes a problem as a 0-value word is received by CPU core 0 via the FIFO after reset (on RP2350). Core 0 becomes stuck in `multicore_lockout_victim_init` until core 1 requests a lockout. To avoid this the number should begin at a non-zero value. I restored the original "magic" number and added a comment to explain this.