diff --git a/sys/include/net/netopt.h b/sys/include/net/netopt.h index 1580386740..f2258ade82 100644 --- a/sys/include/net/netopt.h +++ b/sys/include/net/netopt.h @@ -391,11 +391,12 @@ typedef enum { NETOPT_CCA_MODE, /** - * @brief (@ref netstats_t*) get statistics about sent and received packets and data of the - * device or protocol + * @brief (@ref netstats_t) get statistics about sent and received packets + * and data of the device or protocol * - * Expects a pointer to a @ref netstats_t struct that will be pointed to - * the corresponding @ref netstats_t of the module. + * A get operation expects a @ref netstats_t and will copy the current + * statistics into it, atomically. A set operation resets the statistics + * (zeros it out) regardless of the parameter given. */ NETOPT_STATS, diff --git a/sys/net/gnrc/netif/gnrc_netif.c b/sys/net/gnrc/netif/gnrc_netif.c index f6bcbf7eec..0aa690e08d 100644 --- a/sys/net/gnrc/netif/gnrc_netif.c +++ b/sys/net/gnrc/netif/gnrc_netif.c @@ -186,25 +186,35 @@ int gnrc_netif_get_from_netdev(gnrc_netif_t *netif, gnrc_netapi_opt_t *opt) res = sizeof(uint8_t); break; case NETOPT_STATS: - /* XXX discussed this with Oleg, it's supposed to be a pointer */ switch ((int16_t)opt->context) { #if IS_USED(MODULE_NETSTATS_IPV6) && IS_USED(MODULE_GNRC_NETIF_IPV6) - case NETSTATS_IPV6: - assert(opt->data_len == sizeof(netstats_t *)); - *((netstats_t **)opt->data) = &netif->ipv6.stats; - res = sizeof(&netif->ipv6.stats); - break; + case NETSTATS_IPV6: + { + assert(opt->data_len == sizeof(netstats_t)); + /* IPv6 thread is updating this, to prevent data + * corruptions, we have to guarantee mutually exclusive + * access */ + unsigned irq_state = irq_disable(); + memcpy(opt->data, &netif->ipv6.stats, + sizeof(netif->ipv6.stats)); + irq_restore(irq_state); + res = sizeof(netif->ipv6.stats); + } + break; #endif #ifdef MODULE_NETSTATS_L2 - case NETSTATS_LAYER2: - assert(opt->data_len == sizeof(netstats_t *)); - *((netstats_t **)opt->data) = &netif->stats; - res = sizeof(&netif->stats); - break; + case NETSTATS_LAYER2: + assert(opt->data_len == sizeof(netstats_t)); + /* this is only accesses from the netif thread (us), so no need + * to lock this */ + memcpy(opt->data, &netif->stats, + sizeof(netif->stats)); + res = sizeof(netif->stats); + break; #endif - default: - /* take from device */ - break; + default: + /* take from device */ + break; } break; #if IS_USED(MODULE_GNRC_NETIF_IPV6) @@ -413,6 +423,33 @@ int gnrc_netif_set_from_netdev(gnrc_netif_t *netif, opt->data_len); res = sizeof(netopt_enable_t); break; + case NETOPT_STATS: + switch ((int16_t)opt->context) { +#if IS_USED(MODULE_NETSTATS_IPV6) && IS_USED(MODULE_GNRC_NETIF_IPV6) + case NETSTATS_IPV6: + { + /* IPv6 thread is updating this, to prevent data + * corruptions, we have to guarantee mutually exclusive + * access */ + unsigned irq_state = irq_disable(); + memset(&netif->ipv6.stats, 0, sizeof(netif->ipv6.stats)); + irq_restore(irq_state); + res = 0; + } + break; +#endif +#ifdef MODULE_NETSTATS_L2 + case NETSTATS_LAYER2: + /* this is only accesses from the netif thread (us), so no need + * to lock this */ + memset(&netif->stats, 0, sizeof(netif->stats)); + res = 0; + break; +#endif + default: + /* take from device */ + break; + } default: break; } diff --git a/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c b/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c index 2de00e89ae..89b03d98b0 100644 --- a/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c +++ b/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c @@ -271,8 +271,12 @@ static void _send_to_iface(gnrc_netif_t *netif, gnrc_pktsnip_t *pkt) ipv6_addr_to_str(addr_str, &hdr->dst, sizeof(addr_str)), hdr->nh, byteorder_ntohs(hdr->len)); #ifdef MODULE_NETSTATS_IPV6 + /* This is read from the netif thread. To prevent data corruptions, we + * have to guarantee mutually exclusive access */ + unsigned irq_state = irq_disable(); netif->ipv6.stats.tx_success++; netif->ipv6.stats.tx_bytes += gnrc_pkt_len(pkt->next); + irq_restore(irq_state); #endif #ifdef MODULE_GNRC_SIXLOWPAN @@ -510,7 +514,11 @@ static void _send_unicast(gnrc_pktsnip_t *pkt, bool prep_hdr, netif->pid); /* and send to interface */ #ifdef MODULE_NETSTATS_IPV6 + /* This is read from the netif thread. To prevent data corruptions, we + * have to guarantee mutually exclusive access */ + unsigned irq_state = irq_disable(); netif->ipv6.stats.tx_unicast_count++; + irq_restore(irq_state); #endif _send_to_iface(netif, pkt); } @@ -533,7 +541,11 @@ static inline void _send_multicast_over_iface(gnrc_pktsnip_t *pkt, } DEBUG("ipv6: send multicast over interface %" PRIkernel_pid "\n", netif->pid); #ifdef MODULE_NETSTATS_IPV6 + /* This is read from the netif thread. To prevent data corruptions, we + * have to guarantee mutually exclusive access */ + unsigned irq_state = irq_disable(); netif->ipv6.stats.tx_mcast_count++; + irq_restore(irq_state); #endif /* and send to interface */ _send_to_iface(netif, pkt); @@ -737,9 +749,13 @@ static void _receive(gnrc_pktsnip_t *pkt) netif = gnrc_netif_hdr_get_netif(netif_hdr->data); #ifdef MODULE_NETSTATS_IPV6 assert(netif != NULL); + /* This is read from the netif thread. To prevent data corruptions, we + * have to guarantee mutually exclusive access */ + unsigned irq_state = irq_disable(); netstats_t *stats = &netif->ipv6.stats; stats->rx_count++; stats->rx_bytes += (gnrc_pkt_len(pkt) - netif_hdr->size); + irq_restore(irq_state); #endif } diff --git a/sys/shell/commands/sc_gnrc_netif.c b/sys/shell/commands/sc_gnrc_netif.c index f9233ca111..94e28ccca8 100644 --- a/sys/shell/commands/sc_gnrc_netif.c +++ b/sys/shell/commands/sc_gnrc_netif.c @@ -152,16 +152,18 @@ static const char *_netstats_module_to_str(uint8_t module) static int _netif_stats(netif_t *iface, unsigned module, bool reset) { - netstats_t *stats; + netstats_t stats; int res = netif_get_opt(iface, NETOPT_STATS, module, &stats, - sizeof(&stats)); + sizeof(stats)); if (res < 0) { puts(" Protocol or device doesn't provide statistics."); } else if (reset) { - memset(stats, 0, sizeof(netstats_t)); - printf("Reset statistics for module %s!\n", _netstats_module_to_str(module)); + res = netif_set_opt(iface, NETOPT_STATS, module, NULL, 0); + printf("Reset statistics for module %s: %s!\n", + _netstats_module_to_str(module), + (res < 0) ? "failed" : "succeeded"); } else { printf(" Statistics for %s\n" @@ -169,13 +171,13 @@ static int _netif_stats(netif_t *iface, unsigned module, bool reset) " TX packets %u (Multicast: %u) bytes %u\n" " TX succeeded %u errors %u\n", _netstats_module_to_str(module), - (unsigned) stats->rx_count, - (unsigned) stats->rx_bytes, - (unsigned) (stats->tx_unicast_count + stats->tx_mcast_count), - (unsigned) stats->tx_mcast_count, - (unsigned) stats->tx_bytes, - (unsigned) stats->tx_success, - (unsigned) stats->tx_failed); + (unsigned)stats.rx_count, + (unsigned)stats.rx_bytes, + (unsigned)(stats.tx_unicast_count + stats.tx_mcast_count), + (unsigned)stats.tx_mcast_count, + (unsigned)stats.tx_bytes, + (unsigned)stats.tx_success, + (unsigned)stats.tx_failed); res = 0; } return res;