Merge #17810 #18348 #19120

17810: drivers/slipdev: implement sleep states r=benpicco a=benpicco



18348: sys/net/gnrc/pktbuf_static: make use of alignas() r=maribu a=maribu

### Contribution description

Since we are now using C11, we can make use of `alignas()` provided by `<stdalign.h>` to make the alignment code easier to read.

### Testing procedure

I didn't expect this to change binaries, but is safes 4 bytes. `elf_diff` shows that the compiler (at least GCC 11.3.0) was not able to detect that `gnrc_pktbuf_static_buf` was just an alias for `_pktbuf_buf`. That makes sense since it would be hard without LTO to rule out external writes to `gnrc_pktbuf_static_buf`, unless one would have added a `const` (to the pointer, not to the data the pointer points to).

The [output of `elf_diff`](https://mari-bu.de/pr_18348_gnrc_pktbuf_static_elf_diff.html) looks otherwise quite unscary.

Also:

```
$ make BOARD=nucleo-f767zi -C tests/unittests/ tests-pktbuf flash test
make: Entering directory '/home/maribu/Repos/software/RIOT/tests/unittests'
Building application "tests_unittests" for "nucleo-f767zi" with MCU "stm32".
[...]
Welcome to pyterm!
Type '/exit' to exit.
READY
s
START
.............................................
OK (45 tests)

make: Leaving directory '/home/maribu/Repos/software/RIOT/tests/unittests'
```

### Issues/PRs references

None

19120: CI: seperate check-labels and check-commits workflows r=maribu a=kaspar030



Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de>
Co-authored-by: Kaspar Schleiser <kaspar@schleiser.de>

.github/workflows/check-commits.yml

@@ -1,18 +1,8 @@
-name: check-pr
+name: check-commits
 on:
   pull_request:
-    types: [opened, reopened, labeled, unlabeled, synchronize]
-  pull_request_review:
-    types: [submitted, dismissed]
+    types: [opened, reopened, synchronize]
 jobs:
-  check-labels:
-    runs-on: ubuntu-latest
-    steps:
-    - uses: RIOT-OS/check-labels-action@v1.0.0
-      with:
-        access_token: ${{ secrets.GITHUB_TOKEN }}
-        unset_labels: 'CI: needs squashing,State: waiting for CI update,State: waiting for other PR,Process: blocked by feature freeze'
-        cond_labels: '(Process: needs >1 ACK,review.approvals>1),(Area: RDM,review.approvals>2)'
   check-commits:
     runs-on: ubuntu-latest
     if: ${{ github.base_ref }}

.github/workflows/check-labels.yml

@@ -0,0 +1,15 @@
+name: check-labels
+on:
+  pull_request:
+    types: [opened, reopened, labeled, unlabeled]
+  pull_request_review:
+    types: [submitted, dismissed]
+jobs:
+  check-labels:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: RIOT-OS/check-labels-action@v1.0.0
+      with:
+        access_token: ${{ secrets.GITHUB_TOKEN }}
+        unset_labels: 'CI: needs squashing,State: waiting for CI update,State: waiting for other PR,Process: blocked by feature freeze'
+        cond_labels: '(Process: needs >1 ACK,review.approvals>1),(Area: RDM,review.approvals>2)'

drivers/include/slipdev.h

@@ -72,6 +72,14 @@ enum {
      * @brief   Device writes received data to stdin
      */
     SLIPDEV_STATE_STDIN,
+    /**
+     * @brief   Device is in standby, will wake up when sending data
+     */
+    SLIPDEV_STATE_STANDBY,
+    /**
+     * @brief   Device is in sleep mode
+     */
+    SLIPDEV_STATE_SLEEP,
 };
 /** @} */
 

drivers/slipdev/slipdev.c

@@ -32,6 +32,8 @@
 #include "mutex.h"
 #include "stdio_uart.h"
 
+static int _check_state(slipdev_t *dev);
+
 static inline void slipdev_lock(void)
 {
     if (IS_USED(MODULE_SLIPDEV_STDIO)) {
@@ -74,6 +76,23 @@ check_end:
     }
 }
 
+static void _poweron(slipdev_t *dev)
+{
+    if ((dev->state != SLIPDEV_STATE_STANDBY) ||
+        (dev->state != SLIPDEV_STATE_SLEEP)) {
+        return;
+    }
+
+    dev->state = 0;
+    uart_init(dev->config.uart, dev->config.baudrate, _slip_rx_cb, dev);
+}
+
+static inline void _poweroff(slipdev_t *dev, uint8_t state)
+{
+    uart_poweroff(dev->config.uart);
+    dev->state = state;
+}
+
 static int _init(netdev_t *netdev)
 {
     slipdev_t *dev = (slipdev_t *)netdev;
@@ -150,10 +169,33 @@ unsigned slipdev_unstuff_readbyte(uint8_t *buf, uint8_t byte, bool *escaped)
     return res;
 }
 
+static int _check_state(slipdev_t *dev)
+{
+    /* power states not supported when multiplexing stdio */
+    if (IS_USED(MODULE_SLIPDEV_STDIO)) {
+        return 0;
+    }
+
+    /* discard data when interface is in SLEEP mode */
+    if (dev->state == SLIPDEV_STATE_SLEEP) {
+        return -ENETDOWN;
+    }
+
+    /* sending data wakes the interface from STANDBY */
+    if (dev->state == SLIPDEV_STATE_STANDBY) {
+        _poweron(dev);
+    }
+
+    return 0;
+}
+
 static int _send(netdev_t *netdev, const iolist_t *iolist)
 {
     slipdev_t *dev = (slipdev_t *)netdev;
-    int bytes = 0;
+    int bytes = _check_state(dev);
+    if (bytes) {
+        return bytes;
+    }
 
     DEBUG("slipdev: sending iolist\n");
     slipdev_lock();
@@ -233,6 +275,45 @@ static void _isr(netdev_t *netdev)
     }
 }
 
+#if !IS_USED(MODULE_SLIPDEV_STDIO)
+static int _set_state(slipdev_t *dev, netopt_state_t state)
+{
+    if (IS_USED(MODULE_SLIPDEV_STDIO)) {
+        return -ENOTSUP;
+    }
+
+    switch (state) {
+    case NETOPT_STATE_STANDBY:
+        _poweroff(dev, SLIPDEV_STATE_STANDBY);
+        break;
+    case NETOPT_STATE_SLEEP:
+        _poweroff(dev, SLIPDEV_STATE_SLEEP);
+        break;
+    case NETOPT_STATE_IDLE:
+        _poweron(dev);
+        break;
+    default:
+        return -ENOTSUP;
+    }
+
+    return sizeof(netopt_state_t);
+}
+
+static int _set(netdev_t *netdev, netopt_t opt, const void *value, size_t max_len)
+{
+    (void)max_len;
+
+    slipdev_t *dev = (slipdev_t *)netdev;
+    switch (opt) {
+    case NETOPT_STATE:
+        assert(max_len <= sizeof(netopt_state_t));
+        return _set_state(dev, *((const netopt_state_t *)value));
+    default:
+        return -ENOTSUP;
+    }
+}
+#endif /* !MODULE_SLIPDEV_STDIO */
+
 static int _get(netdev_t *netdev, netopt_t opt, void *value, size_t max_len)
 {
     (void)netdev;
@@ -262,7 +343,11 @@ static const netdev_driver_t slip_driver = {
     .init = _init,
     .isr = _isr,
     .get = _get,
+#if IS_USED(MODULE_SLIPDEV_STDIO)
     .set = netdev_set_notsup,
+#else
+    .set = _set,
+#endif
 };
 
 void slipdev_setup(slipdev_t *dev, const slipdev_params_t *params, uint8_t index)

sys/net/gnrc/pktbuf/include/pktbuf_internal.h

@@ -41,15 +41,6 @@ extern "C" {
  */
 extern mutex_t gnrc_pktbuf_mutex;
 
-#if IS_USED(MODULE_GNRC_PKTBUF_STATIC) || DOXYGEN
-/**
- * @brief   The actual static buffer used when module gnrc_pktbuf_static is used
- *
- * @warning This is an internal buffer and should not be touched by external code
- */
-extern uint8_t *gnrc_pktbuf_static_buf;
-#endif
-
 /**
  * @brief   Check if the given pointer is indeed part of the packet buffer
  *
@@ -61,16 +52,7 @@ extern uint8_t *gnrc_pktbuf_static_buf;
  *                  the packet buffer
  * @retval  false   @p ptr does not point to data in the packet buffer
  */
-static inline bool gnrc_pktbuf_contains(void *ptr)
-{
-#if IS_USED(MODULE_GNRC_PKTBUF_STATIC)
-    return ((uintptr_t)ptr >= (uintptr_t)gnrc_pktbuf_static_buf) &&
-           ((uintptr_t)ptr < (uintptr_t)gnrc_pktbuf_static_buf + CONFIG_GNRC_PKTBUF_SIZE);
-#else
-    (void)ptr;
-    return true;
-#endif
-}
+bool gnrc_pktbuf_contains(void *ptr);
 
 /**
  * @brief   Release an internal buffer

sys/net/gnrc/pktbuf_malloc/gnrc_pktbuf_malloc.c

@@ -288,4 +288,12 @@ void gnrc_pktbuf_free_internal(void *data, size_t size)
     _free(data);
 }
 
+bool gnrc_pktbuf_contains(void *ptr)
+{
+    (void)ptr;
+    /* tracking the memory areas malloced is to expensive, so this function
+     * only is useful with gnrc_pktbuf_static */
+    return true;
+}
+
 /** @} */

sys/net/gnrc/pktbuf_static/gnrc_pktbuf_static.c

@@ -18,9 +18,10 @@
 #include <assert.h>
 #include <errno.h>
 #include <inttypes.h>
+#include <stdalign.h>
 #include <stdbool.h>
-#include <string.h>
 #include <stdio.h>
+#include <string.h>
 #include <sys/types.h>
 
 #include "mutex.h"
@@ -45,11 +46,9 @@
 
 #define CANARY 0x55
 
-/* The static buffer needs to be aligned to word size, so that its start
- * address can be casted to `_unused_t *` safely. Just allocating an array of
- * (word sized) uintptr_t is a trivial way to do this */
-static uintptr_t _pktbuf_buf[CONFIG_GNRC_PKTBUF_SIZE / sizeof(uintptr_t)];
-uint8_t *gnrc_pktbuf_static_buf = (uint8_t *)_pktbuf_buf;
+static alignas(sizeof(_unused_t)) uint8_t _static_buf[CONFIG_GNRC_PKTBUF_SIZE];
+static_assert((CONFIG_GNRC_PKTBUF_SIZE % sizeof(_unused_t)) == 0,
+              "CONFIG_GNRC_PKTBUF_SIZE has to be a multiple of 8");
 static _unused_t *_first_unused;
 
 #ifdef DEVELHELP
@@ -91,11 +90,13 @@ void gnrc_pktbuf_init(void)
 {
     mutex_lock(&gnrc_pktbuf_mutex);
     if (CONFIG_GNRC_PKTBUF_CHECK_USE_AFTER_FREE) {
-        memset(_pktbuf_buf, CANARY, sizeof(_pktbuf_buf));
+        memset(_static_buf, CANARY, sizeof(_static_buf));
     }
-    _first_unused = (_unused_t *)_pktbuf_buf;
+    /* Silence false -Wcast-align: _static_buf has qualifier
+     * `alignas(_unused_t)`, so it is guaranteed to be safe */
+    _first_unused = (_unused_t *)(uintptr_t)_static_buf;
     _first_unused->next = NULL;
-    _first_unused->size = sizeof(_pktbuf_buf);
+    _first_unused->size = sizeof(_static_buf);
     mutex_unlock(&gnrc_pktbuf_mutex);
 }
 
@@ -282,12 +283,12 @@ void gnrc_pktbuf_stats(void)
 {
 #ifdef MODULE_OD
     _unused_t *ptr = _first_unused;
-    uint8_t *chunk = &gnrc_pktbuf_static_buf[0];
+    uint8_t *chunk = &_static_buf[0];
     int count = 0;
 
     printf("packet buffer: first byte: %p, last byte: %p (size: %u)\n",
-           (void *)&gnrc_pktbuf_static_buf[0],
-           (void *)&gnrc_pktbuf_static_buf[CONFIG_GNRC_PKTBUF_SIZE],
+           (void *)&_static_buf[0],
+           (void *)&_static_buf[CONFIG_GNRC_PKTBUF_SIZE],
            CONFIG_GNRC_PKTBUF_SIZE);
     printf("  position of last byte used: %" PRIu16 "\n", max_byte_count);
     if (ptr == NULL) {  /* packet buffer is completely full */
@@ -313,8 +314,8 @@ void gnrc_pktbuf_stats(void)
         ptr = ptr->next;
     }
 
-    if (chunk <= &gnrc_pktbuf_static_buf[CONFIG_GNRC_PKTBUF_SIZE - 1]) {
-        _print_chunk(chunk, &gnrc_pktbuf_static_buf[CONFIG_GNRC_PKTBUF_SIZE] - chunk, count);
+    if (chunk <= &_static_buf[CONFIG_GNRC_PKTBUF_SIZE - 1]) {
+        _print_chunk(chunk, &_static_buf[CONFIG_GNRC_PKTBUF_SIZE] - chunk, count);
     }
 #else
     DEBUG("pktbuf: needs od module\n");
@@ -325,8 +326,8 @@ void gnrc_pktbuf_stats(void)
 #ifdef TEST_SUITES
 bool gnrc_pktbuf_is_empty(void)
 {
-    return ((uintptr_t)_first_unused == (uintptr_t)gnrc_pktbuf_static_buf) &&
-           (_first_unused->size == sizeof(_pktbuf_buf));
+    return ((uintptr_t)_first_unused == (uintptr_t)_static_buf) &&
+           (_first_unused->size == sizeof(_static_buf));
 }
 
 bool gnrc_pktbuf_is_sane(void)
@@ -336,8 +337,8 @@ bool gnrc_pktbuf_is_sane(void)
     /* Invariants of this implementation:
      *  - the head of _unused_t list is _first_unused
      *  - if _unused_t list is empty the packet buffer is full and _first_unused is NULL
-     *  - forall ptr_in _unused_t list: &gnrc_pktbuf_static_buf[0] < ptr
-     *                                  && ptr < &gnrc_pktbuf_static_buf[CONFIG_GNRC_PKTBUF_SIZE]
+     *  - forall ptr_in _unused_t list: &_static_buf[0] < ptr
+     *                                  && ptr < &_static_buf[CONFIG_GNRC_PKTBUF_SIZE]
      *  - forall ptr in _unused_t list: ptr->next == NULL || ptr < ptr->next
      *  - forall ptr in _unused_t list: (ptr->next != NULL && ptr->size <= (ptr->next - ptr)) ||
      *                                  (ptr->next == NULL
@@ -345,14 +346,14 @@ bool gnrc_pktbuf_is_sane(void)
      */
 
     while (ptr) {
-        if ((&gnrc_pktbuf_static_buf[0] >= (uint8_t *)ptr)
-            && ((uint8_t *)ptr >= &gnrc_pktbuf_static_buf[CONFIG_GNRC_PKTBUF_SIZE])) {
+        if ((&_static_buf[0] >= (uint8_t *)ptr)
+            && ((uint8_t *)ptr >= &_static_buf[CONFIG_GNRC_PKTBUF_SIZE])) {
             return false;
         }
         if ((ptr->next != NULL) && (ptr >= ptr->next)) {
             return false;
         }
-        size_t pos_in_buf = (uint8_t *)ptr - &gnrc_pktbuf_static_buf[0];
+        size_t pos_in_buf = (uint8_t *)ptr - &_static_buf[0];
         if (((ptr->next == NULL) || (ptr->size > (size_t)((uint8_t *)(ptr->next) - (uint8_t *)ptr)))
             && ((ptr->next != NULL) || (ptr->size != CONFIG_GNRC_PKTBUF_SIZE - pos_in_buf))) {
             return false;
@@ -416,7 +417,7 @@ static void *_pktbuf_alloc(size_t size)
          * We cast to uintptr_t as intermediate step to silence -Wcast-align */
         _unused_t *new = (_unused_t *)((uintptr_t)ptr + size);
 
-        if (((((uint8_t *)new) - &(gnrc_pktbuf_static_buf[0])) + sizeof(_unused_t))
+        if (((((uint8_t *)new) - &(_static_buf[0])) + sizeof(_unused_t))
             > CONFIG_GNRC_PKTBUF_SIZE) {
             /* content of new would exceed packet buffer size so set to NULL */
             _first_unused = NULL;
@@ -431,7 +432,7 @@ static void *_pktbuf_alloc(size_t size)
         new->size = ptr->size - size;
     }
 #ifdef DEVELHELP
-    uint16_t last_byte = (uint16_t)((((uint8_t *)ptr) + size) - &(gnrc_pktbuf_static_buf[0]));
+    uint16_t last_byte = (uint16_t)((((uint8_t *)ptr) + size) - &(_static_buf[0]));
     if (last_byte > max_byte_count) {
         max_byte_count = last_byte;
     }
@@ -489,7 +490,7 @@ void gnrc_pktbuf_free_internal(void *data, size_t size)
     new->size = _align(size);
     /* calculate number of bytes between new _unused_t chunk and end of packet
      * buffer */
-    bytes_at_end = ((&gnrc_pktbuf_static_buf[0] + CONFIG_GNRC_PKTBUF_SIZE)
+    bytes_at_end = ((&_static_buf[0] + CONFIG_GNRC_PKTBUF_SIZE)
                    - (((uint8_t *)new) + new->size));
     if (bytes_at_end < sizeof(_unused_t)) {
         /* new is very last segment and there is a little bit of memory left
@@ -510,4 +511,12 @@ void gnrc_pktbuf_free_internal(void *data, size_t size)
     }
 }
 
+bool gnrc_pktbuf_contains(void *ptr)
+{
+    const uintptr_t start = (uintptr_t)_static_buf;
+    const uintptr_t end = start + sizeof(_static_buf);
+    uintptr_t pos = (uintptr_t)ptr;
+    return ((pos >= start) && (pos < end));
+}
+
 /** @} */