Changed busy wait to wfe

[Gip-Gip]

Didn't know why read_blocking slept the processor but write_blocking just looped, changed it to where both sleep the processor. Tested on my hardware and everything seems to function normally.

[jannic]

There have been questions regarding this code when it was introduced, https://github.com/rp-rs/rp-hal/pull/204/files/e9694bbb91c66e67df7150fa900a82d9fad928ff#r756965837

It seems like the current solution was chosen because the C API works the same way. Which might or might not be a good reason.
@jonathanpallant, do you remember more details?

[ithinuel]

If the reading core uses the non-blocking read, sev does no harm, so I guess it would make sense to keep the sev in the write but add a wfe in the blocking read.

Using wfe and sev for the other direction as done here seems like a great improvement.

[jannic]

I may have found one issue: write_blocking is used when launching core1, and in that case the other side of the FIFO is controlled by the bootrom, not by our own code. And the bootrom only issues sev after writing, not after reading / draining the FIFO.

From a quick look I'd say that it should still work. While the bootrom doesn't explicitly sev after draining the FIFO, as far as I can tell it always does a write after draining the fifo, and then a sev will be issued, which should unblock the other core even if that one was actually waiting in its own write loop.

But analyzing this multi-core interaction between rust and assembly code is tricky. I might be completely wrong, or I might miss some race condition which could lead to occasional deadlocks.

So, while I agree that the wfe looks like an improvement in general, I'm not confident enough that it's really safe for the specific case of communicating with the bootrom. And the fact that the C SDK doesn't use wfe at that point makes me even more cautions.

[Gip-Gip]

Is it possible you can link to the source code in the RP2040 rom? Perhaps ask a contributor why there's a distinction?

[jonathanpallant]

As I understand it, this is not set up as a high-performance interface, so you should prioritise reliability over performance. If you need to move a lot of data from one core to the other, put it in RAM and send a pointer or some kind of message over the FIFO.

Given that, I would err on the side of caution and just do what the C SDK does rather than trying anything fancy.

[jannic]

Is it possible you can link to the source code in the RP2040 rom? Perhaps ask a contributor why there's a distinction?

Sure: https://github.com/raspberrypi/pico-bootrom/blob/ef22cd8ede5bc007f81d7f2416b48db90f313434/bootrom/bootrom_rt0.S#LL329C4-L329C4

And the C SDK code doing the FIFO write is here: https://github.com/raspberrypi/pico-sdk/blob/f396d05f8252d4670d4ea05c8b7ac938ef0cd381/src/rp2_common/pico_multicore/multicore.c#L18

As I understand it, this is not set up as a high-performance interface, so you should prioritise reliability over performance.

It's not a performance question. If anything, replacing the nop by a wfe will be slightly slower. It would improve power efficiency. However, I'd argue that a fifo write blocking for more than a few cycles shouldn't ever happen. That's completely different from the fifo read: If the second core is not in use, it'll spend all of the time waiting for a command sent through the fifo, so there the wfe is essential.

[Gip-Gip]

It's not a performance question. If anything, replacing the nop by a wfe will be slightly slower. It would improve power efficiency. However, I'd argue that a fifo write blocking for more than a few cycles shouldn't ever happen. That's completely different from the fifo read: If the second core is not in use, it'll spend all of the time waiting for a command sent through the fifo, so there the wfe is essential.

I'm more concerned about the edge case where one core calls a writing block while the other one is panicked, which is the primary reason I made this change. I went ahead and asked the initial committer of the C code as to why, I'm curious myself and if there is a good reason why we need to busywait we should probably know about it.

[jannic]

I went ahead and asked the initial committer of the C code as to why, I'm curious myself and if there is a good reason why we need to busywait we should probably know about it.

👍
If we can't add the wfe, the busy loop should have a comment explaining the reason.

[ithinuel]

@Gip-Gip Did you get any reply from the initial committer of the C code?

[Gip-Gip]

Unfortunately not, the dev only has a twitter linked and that twitter is mildly inactive lol

[9names]

Why would you ask on twitter though? The natural place to ask questions about code on GitHub is through issues. I certainly don't want people DM'ing me or tagging me on social media for stuff like this.

[Gip-Gip]

I'm not the most professionally minded person was just the first thing that came to mind lol, my bad. I can open up a discussion issue or something along those lines too. I don't mind people reaching out to me tbh but I guess that might be a me thing.