sys/suit: Ensure previous thread is stopped before reusing its stack

[chrysn]

Contribution description

Closes: #19195

If the thread has released the mutex but the thread has not terminated (which happens in the situation that would previously have overwritten a still active thread's state), then a warning is shown and the trigger is ignored.

Testing procedure

This should work before and after:

• make -C examples/suit_update BOARD=native all term
• aiocoap-client coap://'[fe80::3c63:beff:fe85:ca96%tapbr0]'/suit/trigger -m POST --payload 'coap://[2001:db8::]/foo'
• In parallel, on the RIOT shell, run suit fetch coap://[2001:db8::]/foo
• After the first download fails, the second one starts right away ("suit_worker: update failed, hdr invalid" / "suit_worker: started").

Run again with the worker thread on low priority:

diff --git a/sys/suit/transport/worker.c b/sys/suit/transport/worker.c
index a54022fb28..e26701a64c 100644
--- a/sys/suit/transport/worker.c
+++ b/sys/suit/transport/worker.c
@@ -70 +70 @@
-#define SUIT_COAP_WORKER_PRIO THREAD_PRIORITY_MAIN - 1
+#define SUIT_COAP_WORKER_PRIO THREAD_PRIORITY_MAIN + 1

Before, this runs the download once silently (no clue why it doesn't run it twice, but then again, I claim there's concurrent memory access from two thread, so who knows what happens). After, it runs a single download and shows an error message for the second one once the first is complete ("Ignoring SUIT trigger: worker is still busy.").

Issues/PRs references

This may be made incrementally easier by #19197 -- that PR as it is now would spare us the zombification (because returning would do that), and having a wait function would allow us to turn the new error case into a success.

[benpicco]

I wonder a bit:
We have XFA auto-init now, so always starting the SUIT thread would be trivial.
The original idea of this approach was to simplify the code, if we have to introduce such bespoke zombiefy patterns now, it might be better to just always spawn the thread and have it wait on a mutex that gets unlocked by suit_worker_trigger().

[chrysn]

That too would be a completely fine solution. There'd probably be a mutex and some other mechanism (a second mutex, an thread flag, whatever), but yes, chances are it's simpler.

It may make me come back to matrix and ask "are there now any valid use cases for a thread terminating and restarting?", but that's not really a concern for this concrete issue.

[riot-ci]

Murdock results

✔️ PASSED

2c716d3 sys/suit: Ensure previous thread is stopped before reusing its stack

Success	Failures	Total	Runtime
6796	0	6796	07m:40s

Artifacts

• Documentation preview

[benpicco]

Thinking about it, one reason to do it that way was that if you don't call suit_worker_trigger() (because you are directly calling suit_handle_url() in a thread with sufficient stack), all the custom suit thread thing should be thrown away by the compiler.

The suit thread is only needed because the suit_flashwrite backed requires to keep 1 flash page on the stack, which can be up to 8k depending on the platform.

So if you already have a thread with sufficient stack you would call suit_handle_url() and the linker should throw away that extra thread stack only used by suit_worker_trigger().

[benpicco]

bors merge

[bors]

Build succeeded:

• Murdock
• static-tests
• tools-build-success