fixup! gnrc/netapi/netnotify: ACK notify event directly

Use mutex, counter and condvar to collect the exact count of
acknowledgements for a notify event. With this, we can now block for all
replies after dispatching to all receivers and after releasing the lock
on netreg. This a) maintains the receivers priority order and b)
prevents possible deadlocks that could happen if we hold the netreg lock
while blocking for an ACK.

Author: elenaf9

sys/include/net/gnrc/netapi/notify.h

@@ -26,10 +26,9 @@
  *
  * @note    Notification events are associated with type-dependent event data.
  *          Receivers should not access the data directly, but instead use the
- *          the dedicated API functions.
- *          If the data is read directly, the receiver is responsible for setting
- *          the @ref NETAPI_NOTIFY_FLAG_ACK flag for the sending thread.
- *
+ *          the dedicated API functions to copy the data from the sender's.
+ *          If the data is read manually, the receiver is responsible for
+ *          calling @ref gnrc_netapi_notify_ack to unblock the sender.
  *
  * @author  Elena Frank <[email protected]>
  */
@@ -39,15 +38,8 @@ extern "C" {
 #endif
 
 #include "net/ipv6/addr.h"
-#include "sched.h"
-
-/**
- * @brief   Thread flag for acknowledging a received notify even.
- *
- * @note    The flag should only be set after all event data was copied
- *          by receiver and can be freed by the sender.
- */
-#define NETAPI_NOTIFY_FLAG_ACK         (1u << 10)
+#include "cond.h"
+#include "mutex.h"
 
 /**
  * @brief   Definition of notification event types in the network stack.
@@ -64,13 +56,23 @@ typedef enum {
     NETAPI_NOTIFY_L3_UNREACHABLE,   /**< Node became unreachable on the network layer. */
 } netapi_notify_t;
 
+/**
+ * @brief   Data structure to acknowledge netapi notification events.
+ */
+typedef struct {
+    int counter;        /**< ACK counter */
+    cond_t cond;        /**< condition variable to signal change in count */
+    mutex_t lock;       /**< lock for counter */
+} gnrc_netapi_notify_ack_t;
+
 /**
  * @brief   Data structure to be sent for netapi notification events.
  */
 typedef struct {
-    netapi_notify_t event;      /**< the type of event */
-    void *_data;                /**< associated event data. */
-    uint16_t _data_len;         /**< size of the event data */
+    netapi_notify_t event;          /**< the type of event */
+    void *_data;                    /**< associated event data. */
+    uint16_t _data_len;             /**< size of the event data */
+    gnrc_netapi_notify_ack_t *ack;  /**< acknowledge event */
 } gnrc_netapi_notify_t;
 
 /**
@@ -83,13 +85,26 @@ typedef struct {
     kernel_pid_t if_pid;        /**< PID of network interface */
 } netapi_notify_l2_connection_t;
 
+/**
+ * @brief   Acknowledge that a notify event was received and its data read.
+ *
+ * @param[in] ack           Pointer to the event's acknowledgment structure.
+ */
+static inline void gnrc_netapi_notify_ack(gnrc_netapi_notify_ack_t *ack)
+{
+    mutex_lock(&ack->lock);
+    ack->counter++;
+    /* Signal that the counter changed. */
+    cond_signal(&ack->cond);
+    mutex_unlock(&ack->lock);
+}
+
 /**
  * @brief   Parse the connection event data associated with @ref NETAPI_NOTIFY_L2_CONNECTED
  *          and @ref NETAPI_NOTIFY_L2_DISCONNECTED events.
  *
  * @note    This will set the @ref NETAPI_NOTIFY_FLAG_ACK for the sending thread.
  *
- * @param[in] sender_pid    PID of the netapi sender.
  * @param[in] notify        Pointer to the received notify event.
  * @param[out] data         Connection data received in the @p notify event.
  *                          data.l2addr_len should be set to the size of the l2addr buffer.
@@ -99,24 +114,21 @@ typedef struct {
  * @retval                  -ENOBUFS if the length of l2addr in @p data is smaller than the
  *                          received l2addr.
  */
-uint8_t gnrc_netapi_notify_copy_l2_connection_data(kernel_pid_t sender_pid,
-                                                   gnrc_netapi_notify_t *notify,
+uint8_t gnrc_netapi_notify_copy_l2_connection_data(gnrc_netapi_notify_t *notify,
                                                    netapi_notify_l2_connection_t *data);
 /**
  * @brief   Parse the ipv6 address associated with @ref NETAPI_NOTIFY_L3_DISCOVERED and
  *          @ref NETAPI_NOTIFY_L3_UNREACHABLE events.
  *
  * @note    This will set the @ref NETAPI_NOTIFY_FLAG_ACK for the sending thread.
  *
- * @param[in] sender_pid    PID of the netapi sender.
  * @param[in] notify        Pointer to the received notify event.
  * @param[out] addr         IPv6 address of the remote.
  *
  * @retval                  sizeof(ipv6_addr_t) on success.
  * @retval                  -EINVAL if @p notify is of a wrong @ref netapi_notify_t type.
  */
-int gnrc_netapi_notify_copy_l3_address(kernel_pid_t sender_pid, gnrc_netapi_notify_t *notify,
-                                       ipv6_addr_t *addr);
+int gnrc_netapi_notify_copy_l3_address(gnrc_netapi_notify_t *notify, ipv6_addr_t *addr);
 
 #ifdef __cplusplus
 }

sys/net/gnrc/netapi/gnrc_netapi.c

@@ -27,7 +27,8 @@
 #include "net/gnrc/netapi/notify.h"
 #include "net/gnrc/pktbuf.h"
 #include "net/gnrc/netapi.h"
-#include "thread.h"
+#include "cond.h"
+#include "mutex.h"
 
 #define ENABLE_DEBUG 0
 #include "debug.h"
@@ -152,10 +153,16 @@ int gnrc_netapi_dispatch(gnrc_nettype_t type, uint32_t demux_ctx,
 int gnrc_netapi_notify(gnrc_nettype_t type, uint32_t demux_ctx, netapi_notify_t event,
                        void *data, size_t data_len)
 {
+    gnrc_netapi_notify_ack_t ack = {
+        .counter = 0,
+        .cond = COND_INIT,
+        .lock = MUTEX_INIT,
+    };
     gnrc_netapi_notify_t notify = {
         .event = event,
         ._data = data,
         ._data_len = data_len,
+        .ack = &ack
     };
 
     gnrc_netreg_acquire_shared();
@@ -166,24 +173,27 @@ int gnrc_netapi_notify(gnrc_nettype_t type, uint32_t demux_ctx, netapi_notify_t
         /* Look up the registered threads for this message type. */
         gnrc_netreg_entry_t *sendto = gnrc_netreg_lookup(type, demux_ctx);
 
-        /* Make sure the ACK flag isn't already set. */
-        thread_flags_clear(NETAPI_NOTIFY_FLAG_ACK);
-
         /* Dispatch to all registered threads sequentially. */
         while (sendto) {
-            uint32_t status = _dispatch_single(sendto, GNRC_NETAPI_MSG_TYPE_NOTIFY, &notify);
-
-            /* Need to wait for an ACK to ensure that the data was read and that
-               there won't be any dangling pointers after the function returned. */
-            if (data != NULL && status == 0) {
-                thread_flags_wait_all(NETAPI_NOTIFY_FLAG_ACK);
+            int status = _dispatch_single(sendto, GNRC_NETAPI_MSG_TYPE_NOTIFY, &notify);
+            if (status < 0) {
+                numof--;
             }
-
             sendto = gnrc_netreg_getnext(sendto);
         }
     }
 
     gnrc_netreg_release_shared();
 
+    if (data != NULL) {
+        /* Wait for ACK from all receivers to ensure that the data was read and
+           that there won't be any dangling pointers after the function returned. */
+        mutex_lock(&ack.lock);
+        while (ack.counter < numof) {
+            cond_wait(&ack.cond, &ack.lock);
+        }
+        mutex_unlock(&ack.lock);
+    }
+
     return numof;
 }

sys/net/gnrc/netapi/notify/gnrc_netapi_notify.c

@@ -16,12 +16,9 @@
 
 #include "net/gnrc/netapi/notify.h"
 #include "net/ipv6/addr.h"
-#include "sched.h"
-#include "thread.h"
 #include <sys/errno.h>
 
-uint8_t gnrc_netapi_notify_copy_l2_connection_data(kernel_pid_t sender_pid,
-                                                   gnrc_netapi_notify_t *notify,
+uint8_t gnrc_netapi_notify_copy_l2_connection_data(gnrc_netapi_notify_t *notify,
                                                    netapi_notify_l2_connection_t *data)
 {
     int data_len;
@@ -51,14 +48,13 @@ uint8_t gnrc_netapi_notify_copy_l2_connection_data(kernel_pid_t sender_pid,
         break;
     }
 
-    /* Unblock the sending thread now that all data was copied over. */
-    thread_flags_set(thread_get(sender_pid), NETAPI_NOTIFY_FLAG_ACK);
+    /* Acknowledge the read data */
+    gnrc_netapi_notify_ack(notify->ack);
 
     return data_len;
 }
 
-int gnrc_netapi_notify_copy_l3_address(kernel_pid_t sender_pid, gnrc_netapi_notify_t *notify,
-                                       ipv6_addr_t *addr)
+int gnrc_netapi_notify_copy_l3_address(gnrc_netapi_notify_t *notify, ipv6_addr_t *addr)
 {
     int data_len;
 
@@ -77,8 +73,8 @@ int gnrc_netapi_notify_copy_l3_address(kernel_pid_t sender_pid, gnrc_netapi_noti
         break;
     }
 
-    /* Unblock the sending thread now that all data was copied over. */
-    thread_flags_set(thread_get(sender_pid), NETAPI_NOTIFY_FLAG_ACK);
+    /* Acknowledge the read data */
+    gnrc_netapi_notify_ack(notify->ack);
 
     return data_len;
 }

sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c

@@ -241,7 +241,7 @@ static inline void _on_l2_disconnected(kernel_pid_t if_pid, uint8_t *l2addr, uin
  *
  * @param[in] notify    The type of notification.
  */
-static inline void _netapi_notify_event(kernel_pid_t sender_pid, gnrc_netapi_notify_t *notify)
+static inline void _netapi_notify_event(gnrc_netapi_notify_t *notify)
 {
     uint8_t l2addr[CONFIG_GNRC_IPV6_NIB_L2ADDR_MAX_LEN];
     netapi_notify_l2_connection_t data = {
@@ -251,7 +251,7 @@ static inline void _netapi_notify_event(kernel_pid_t sender_pid, gnrc_netapi_not
     };
     netapi_notify_t type = notify->event;
 
-    if (gnrc_netapi_notify_copy_l2_connection_data(sender_pid, notify, &data) <= 0) {
+    if (gnrc_netapi_notify_copy_l2_connection_data(notify, &data) <= 0) {
         DEBUG("ipv6: invalid data on netapi notify event.\n");
         return;
     }
@@ -320,7 +320,7 @@ static void *_event_loop(void *args)
                 break;
             case GNRC_NETAPI_MSG_TYPE_NOTIFY:
                 DEBUG("ipv6: GNRC_NETAPI_MSG_TYPE_NOTIFY received\n");
-                _netapi_notify_event(msg.sender_pid, msg.content.ptr);
+                _netapi_notify_event(msg.content.ptr);
                 break;
 
 #ifdef MODULE_GNRC_IPV6_EXT_FRAG

sys/net/gnrc/routing/rpl/gnrc_rpl.c

@@ -314,13 +314,12 @@ static inline void _handle_unreachable_neighbor(ipv6_addr_t *addr)
  *
  * @param[in] notify    The type of notification.
  */
-static inline void _netapi_notify_event(kernel_pid_t sender_pid, gnrc_netapi_notify_t *notify)
+static inline void _netapi_notify_event(gnrc_netapi_notify_t *notify)
 {
     ipv6_addr_t neigh_addr;
     netapi_notify_t type = notify->event;
 
-    if (gnrc_netapi_notify_copy_l3_address(sender_pid, notify,
-                                           &neigh_addr) != sizeof(ipv6_addr_t)) {
+    if (gnrc_netapi_notify_copy_l3_address(notify, &neigh_addr) != sizeof(ipv6_addr_t)) {
         DEBUG("RPL: Received invalid data for netapi notify event.\n");
         return;
     }
@@ -408,7 +407,7 @@ static void *_event_loop(void *args)
                 break;
             case GNRC_NETAPI_MSG_TYPE_NOTIFY:
                 DEBUG("RPL: GNRC_NETAPI_MSG_TYPE_NOTIFY received\n");
-                _netapi_notify_event(msg.sender_pid, msg.content.ptr);
+                _netapi_notify_event(msg.content.ptr);
                 break;
             default:
                 break;