diff --git a/esphome/core/lwip_fast_select.c b/esphome/core/lwip_fast_select.c index 8684909dc1..b2cf43b914 100644 --- a/esphome/core/lwip_fast_select.c +++ b/esphome/core/lwip_fast_select.c @@ -6,6 +6,52 @@ // 2. The netconn callback is a C function pointer // // defines.h is force-included by the build system (-include flag), providing USE_ESP32 etc. +// +// Thread safety analysis +// ====================== +// Three threads interact with this code: +// 1. Main loop task — calls init, has_data, hook, unhook +// 2. LwIP TCP/IP task — calls event_callback (which reads s_original_callback, writes rcvevent) +// 3. Background tasks — call wake_main_loop +// +// Shared state and safety rationale: +// +// s_main_loop_task (TaskHandle_t, 4 bytes): +// Written once by main loop in init(), before any hook/wake calls. +// Read by TCP/IP thread (in callback) and background tasks (in wake). +// Safe: write-once-then-read pattern. The init() call completes during setup() +// before any sockets are hooked, so all subsequent reads see the final value. +// +// s_original_callback (netconn_callback, 4-byte function pointer): +// Written by main loop in hook_socket() (only when NULL — set once). +// Read by TCP/IP thread in esphome_socket_event_callback(). +// Safe: set-once pattern. The first hook_socket() captures the original callback. +// All subsequent hooks see it already set and skip the write. The TCP/IP thread +// only reads this after the callback pointer has been swapped (which happens after +// the write), so it always sees the initialized value. +// +// sock->conn->callback (netconn_callback, 4-byte function pointer): +// Written by main loop in hook_socket() and unhook_socket(). +// Read by TCP/IP thread when invoking the callback. +// Safe: 32-bit aligned pointer writes are atomic on Xtensa and RISC-V (ESP32). +// The TCP/IP thread will see either the old or new pointer atomically — never a +// torn value. Both the wrapper and original callbacks are valid at all times +// (the wrapper itself calls the original), so either value is correct. +// +// sock->rcvevent (s16_t, 2 bytes): +// Written by TCP/IP thread in event_callback (via SYS_ARCH_INC/DEC under lock). +// Read by main loop in has_data(). +// Safe: aligned 16-bit reads are atomic on Xtensa/RISC-V. We use __atomic_load_n +// with __ATOMIC_RELAXED to prevent compiler reordering. Staleness is acceptable — +// a missed increment means we poll again next loop iteration (~16ms), and the +// task notification provides the real wake signal. +// +// FreeRTOS task notification value: +// Written by TCP/IP thread (xTaskNotifyGive in callback) and background tasks +// (xTaskNotifyGive in wake_main_loop). Read by main loop (ulTaskNotifyTake). +// Safe: FreeRTOS notification APIs are thread-safe by design (use internal +// critical sections). Multiple concurrent xTaskNotifyGive calls are safe — +// the notification count simply increments. #ifdef USE_ESP32 @@ -17,10 +63,30 @@ #include "esphome/core/lwip_fast_select.h" -// Task handle for the main loop — set during setup, read from any thread/ISR +#include + +// Compile-time verification of thread safety assumptions. +// On ESP32 (Xtensa/RISC-V), naturally-aligned reads/writes up to 32 bits are atomic. +// These asserts ensure our cross-thread shared state meets those requirements. + +// Pointer types must fit in a single 32-bit store (atomic write) +_Static_assert(sizeof(TaskHandle_t) <= 4, "TaskHandle_t must be <= 4 bytes for atomic access"); +_Static_assert(sizeof(netconn_callback) <= 4, "netconn_callback must be <= 4 bytes for atomic access"); + +// rcvevent must fit in a single atomic read +_Static_assert(sizeof(((struct lwip_sock *) 0)->rcvevent) <= 4, "rcvevent must be <= 4 bytes for atomic access"); + +// Struct member alignment — natural alignment guarantees atomicity on Xtensa/RISC-V. +// Misaligned access would not be atomic even if the size is <= 4 bytes. +_Static_assert(offsetof(struct netconn, callback) % sizeof(netconn_callback) == 0, + "netconn.callback must be naturally aligned for atomic access"); +_Static_assert(offsetof(struct lwip_sock, rcvevent) % sizeof(((struct lwip_sock *) 0)->rcvevent) == 0, + "lwip_sock.rcvevent must be naturally aligned for atomic access"); + +// Task handle for the main loop — written once in init(), read from TCP/IP and background tasks. static TaskHandle_t s_main_loop_task = NULL; -// Saved original event_callback pointer (same for all LwIP sockets) +// Saved original event_callback pointer — written once in first hook_socket(), read from TCP/IP task. static netconn_callback s_original_callback = NULL; // Wrapper callback: calls original event_callback + notifies main loop task. @@ -33,7 +99,6 @@ static void esphome_socket_event_callback(struct netconn *conn, enum netconn_evt } // Wake the main loop task if sleeping in ulTaskNotifyTake(). // Only notify on receive events to avoid spurious wakeups from send-ready events. - // xTaskNotifyGive is safe from task context (LwIP TCP/IP thread). NOT ISR-safe. if (evt == NETCONN_EVT_RCVPLUS) { TaskHandle_t task = s_main_loop_task; if (task != NULL) { @@ -48,10 +113,8 @@ bool esphome_lwip_socket_has_data(int fd) { struct lwip_sock *sock = lwip_socket_dbg_get_socket(fd); if (sock == NULL || sock->conn == NULL) return false; - // rcvevent is modified by LwIP's TCP/IP thread in event_callback. - // Use atomic load for C11 memory model correctness. On Xtensa/RISC-V (ESP32) - // aligned 16-bit reads are naturally atomic, so this compiles to a plain load - // but prevents compiler reordering. + // Atomic load prevents compiler reordering. On ESP32 this compiles to a plain load + // since aligned 16-bit reads are naturally atomic on Xtensa/RISC-V. return __atomic_load_n(&sock->rcvevent, __ATOMIC_RELAXED) > 0; } @@ -60,15 +123,13 @@ void esphome_lwip_hook_socket(int fd) { if (sock == NULL || sock->conn == NULL) return; - // Save original callback (only once — same event_callback for all LwIP sockets) + // Save original callback once — all LwIP sockets share the same event_callback. if (s_original_callback == NULL) { s_original_callback = sock->conn->callback; } - // Replace with our wrapper. - // Thread safety: pointer writes are atomic on ESP32 (32-bit aligned). - // The TCP/IP thread may read conn->callback concurrently, but it will see - // either the old or new pointer — both are valid (our wrapper calls the original). + // Replace with our wrapper. Atomic on ESP32 (32-bit aligned pointer write). + // TCP/IP thread sees either old or new pointer — both are valid. sock->conn->callback = esphome_socket_event_callback; } @@ -77,14 +138,13 @@ void esphome_lwip_unhook_socket(int fd) { if (sock == NULL || sock->conn == NULL) return; - // Restore original callback. - // Thread safety: same as hook — pointer write is atomic on ESP32. - // TCP/IP thread sees either wrapper or original, both are safe. + // Restore original callback. Atomic on ESP32 (32-bit aligned pointer write). if (s_original_callback != NULL) { sock->conn->callback = s_original_callback; } } +// Wake the main loop from another FreeRTOS task. NOT ISR-safe. void esphome_lwip_wake_main_loop(void) { TaskHandle_t task = s_main_loop_task; if (task != NULL) {