Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
19199: sys/suit: Ensure previous thread is stopped before reusing its stack r=benpicco a=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:

```patch
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.

19205: boards/common: add common timer config for GD32VF103 boards r=benpicco a=benpicco



19207: examples/gnrc_border_router: static: use router from advertisements by default r=benpicco a=benpicco




Co-authored-by: chrysn <[email protected]>
Co-authored-by: Benjamin Valentin <[email protected]>
Co-authored-by: Benjamin Valentin <[email protected]>
  • Loading branch information
4 people authored Jan 27, 2023
4 parents 31fe47c + 2c716d3 + 162d06a + ff52d35 commit c6c84cc
Show file tree
Hide file tree
Showing 7 changed files with 66 additions and 4 deletions.
40 changes: 40 additions & 0 deletions boards/common/gd32v/include/board_common.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright (C) 2020 Koen Zandberg <[email protected]>
* 2023 Gunar Schorcht <[email protected]>
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/

/**
* @ingroup boards_common_gd32v
* @{
*
* @file
* @brief Common board definitions for GD32VF103 boards
*
* @author Koen Zandberg <[email protected]>
* @author Gunar Schorcht <[email protected]>
*/

#ifndef BOARD_COMMON_H
#define BOARD_COMMON_H

#ifdef __cplusplus
extern "C" {
#endif

/**
* @name Xtimer configuration
* @{
*/
#define XTIMER_WIDTH (16)
/** @} */

#ifdef __cplusplus
}
#endif

#endif /* BOARD_COMMON_H */
/** @} */
2 changes: 2 additions & 0 deletions boards/seeedstudio-gd32/include/board.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
#ifndef BOARD_H
#define BOARD_H

#include "board_common.h"

#ifdef __cplusplus
extern "C" {
#endif
Expand Down
2 changes: 2 additions & 0 deletions boards/sipeed-longan-nano/include/board.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
#ifndef BOARD_H
#define BOARD_H

#include "board_common.h"

#ifdef __cplusplus
extern "C" {
#endif
Expand Down
8 changes: 7 additions & 1 deletion examples/gnrc_border_router/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,14 @@ else ifeq (auto_subnets,$(PREFIX_CONF))
USEMODULE += gnrc_ipv6_auto_subnets_simple
else ifeq (static,$(PREFIX_CONF))
IPV6_ADDR ?= 2001:db8:1::1
IPV6_DEFAULT_ROUTER ?= fe80::1
# Only set this if the default router does not send router advertisements
# IPV6_DEFAULT_ROUTER ?= fe80::1
USEMODULE += gnrc_ipv6_static_addr

# configure static DNS server
ifneq (,$(filter sock_dns,$(USEMODULE)))
USEMODULE += auto_init_sock_dns
endif
endif

# Comment this out to disable code in RIOT that does safety checking
Expand Down
3 changes: 2 additions & 1 deletion sys/net/gnrc/network_layer/ipv6/nib/_nib-router.c
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,8 @@ static gnrc_pktsnip_t *_build_ext_opts(gnrc_netif_t *netif,
uint32_t rdnss_ltime = _evtimer_lookup(&sock_dns_server,
GNRC_IPV6_NIB_RDNSS_TIMEOUT);

if ((rdnss_ltime < UINT32_MAX) &&
/* with auto_init_sock_dns we always have a valid (static) DNS server */
if (((rdnss_ltime < UINT32_MAX) || IS_USED(MODULE_AUTO_INIT_SOCK_DNS)) &&
(!ipv6_addr_is_link_local((ipv6_addr_t *)sock_dns_server.addr.ipv6))) {
gnrc_pktsnip_t *rdnsso = gnrc_ndp_opt_rdnss_build(
rdnss_ltime * MS_PER_SEC,
Expand Down
1 change: 0 additions & 1 deletion sys/net/gnrc/network_layer/ipv6/static_addr/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,5 @@ config GNRC_IPV6_STATIC_ADDR_DOWNSTREAM

config GNRC_IPV6_STATIC_DEFAULT_ROUTER
string "Static IPv6 address of the default router"
default "2001:db8::1"

endif # KCONFIG_USEMODULE_GNRC_IPV6_STATIC_ADDR
14 changes: 13 additions & 1 deletion sys/suit/transport/worker.c
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ static char _url[CONFIG_SOCK_URLPATH_MAXLEN];
static uint8_t _manifest_buf[SUIT_MANIFEST_BUFSIZE];

static mutex_t _worker_lock;
/* PID of the worker thread, guarded by */
static kernel_pid_t _worker_pid = KERNEL_PID_UNDEF;

int suit_handle_url(const char *url)
{
Expand Down Expand Up @@ -154,17 +156,27 @@ static void *_suit_worker_thread(void *arg)
}

mutex_unlock(&_worker_lock);
thread_zombify();
/* Actually unreachable, given we're in a thread */
return NULL;
}

void suit_worker_trigger(const char *url, size_t len)
{
mutex_lock(&_worker_lock);
if (_worker_pid != KERNEL_PID_UNDEF) {
if (thread_kill_zombie(_worker_pid) != 1) {
/* This will only happen if the SUIT thread runs on a lower
* priority than the caller */
LOG_WARNING("Ignoring SUIT trigger: worker is still busy.\n");
return;
}
}

memcpy(_url, url, len);
_url[len] = '\0';

thread_create(_stack, SUIT_WORKER_STACKSIZE, SUIT_COAP_WORKER_PRIO,
_worker_pid = thread_create(_stack, SUIT_WORKER_STACKSIZE, SUIT_COAP_WORKER_PRIO,
THREAD_CREATE_STACKTEST,
_suit_worker_thread, NULL, "suit worker");
}

0 comments on commit c6c84cc

Please sign in to comment.