From 4b50d14496896181d261a3c8a9dc5b4d4a062bda Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 9 Mar 2026 21:10:03 -1000 Subject: [PATCH 1/5] [serial_proxy] Reduce loop() overhead by disabling when idle and splitting read path (#14673) --- .../components/serial_proxy/serial_proxy.cpp | 29 ++++++++++++++----- .../components/serial_proxy/serial_proxy.h | 5 ++++ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/esphome/components/serial_proxy/serial_proxy.cpp b/esphome/components/serial_proxy/serial_proxy.cpp index 340f9b0cb85..00d822b75cb 100644 --- a/esphome/components/serial_proxy/serial_proxy.cpp +++ b/esphome/components/serial_proxy/serial_proxy.cpp @@ -28,25 +28,38 @@ void SerialProxy::setup() { // instance_index_ is fixed at registration time; pre-set it so loop() only needs to update data this->outgoing_msg_.instance = this->instance_index_; #endif + // No subscriber at startup; disable loop until a client subscribes + this->disable_loop(); } void SerialProxy::loop() { #ifdef USE_API - // Detect subscriber disconnect - if (this->api_connection_ != nullptr && (this->api_connection_->is_marked_for_removal() || - !this->api_connection_->is_connection_setup() || !api_is_connected())) { - ESP_LOGW(TAG, "Subscriber disconnected"); - this->api_connection_ = nullptr; + // Safety check — loop should only run when subscribed, but guard against races + if (this->api_connection_ == nullptr) [[unlikely]] { + this->disable_loop(); + return; } - if (this->api_connection_ == nullptr) + // Detect subscriber disconnect + if (this->api_connection_->is_marked_for_removal() || !this->api_connection_->is_connection_setup() || + !api_is_connected()) { + ESP_LOGW(TAG, "Subscriber disconnected"); + this->api_connection_ = nullptr; + this->disable_loop(); return; + } // Read available data from UART and forward to subscribed client size_t available = this->available(); if (available == 0) return; + this->read_and_send_(available); +#endif +} + +#ifdef USE_API +void __attribute__((noinline)) SerialProxy::read_and_send_(size_t available) { // Read in chunks up to SERIAL_PROXY_MAX_READ_SIZE uint8_t buffer[SERIAL_PROXY_MAX_READ_SIZE]; size_t to_read = std::min(available, sizeof(buffer)); @@ -56,8 +69,8 @@ void SerialProxy::loop() { this->outgoing_msg_.set_data(buffer, to_read); this->api_connection_->send_serial_proxy_data(this->outgoing_msg_); -#endif } +#endif void SerialProxy::dump_config() { ESP_LOGCONFIG(TAG, @@ -166,6 +179,7 @@ void SerialProxy::serial_proxy_request(api::APIConnection *api_connection, api:: return; } this->api_connection_ = api_connection; + this->enable_loop(); ESP_LOGV(TAG, "API connection subscribed to serial proxy [%u]", this->instance_index_); break; case api::enums::SERIAL_PROXY_REQUEST_TYPE_UNSUBSCRIBE: @@ -174,6 +188,7 @@ void SerialProxy::serial_proxy_request(api::APIConnection *api_connection, api:: return; } this->api_connection_ = nullptr; + this->disable_loop(); ESP_LOGV(TAG, "API connection unsubscribed from serial proxy [%u]", this->instance_index_); break; default: diff --git a/esphome/components/serial_proxy/serial_proxy.h b/esphome/components/serial_proxy/serial_proxy.h index 62f942b19d8..5adfa4fe53b 100644 --- a/esphome/components/serial_proxy/serial_proxy.h +++ b/esphome/components/serial_proxy/serial_proxy.h @@ -101,6 +101,11 @@ class SerialProxy : public uart::UARTDevice, public Component { void set_dtr_pin(GPIOPin *pin) { this->dtr_pin_ = pin; } protected: +#ifdef USE_API + /// Read from UART and send to API client (slow path with 256-byte stack buffer) + void read_and_send_(size_t available); +#endif + /// Instance index for identifying this proxy in API messages uint32_t instance_index_{0}; From a88e9b814663c83f1206668c9dc8b973e164f4f4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 10 Mar 2026 00:02:26 -1000 Subject: [PATCH 2/5] [socket] Fix RP2040 TCP race condition between lwip callbacks and main loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On RP2040 (Pico W), arduino-pico sets PICO_CYW43_ARCH_THREADSAFE_BACKGROUND=1, which means lwip callbacks (recv_fn, accept_fn, err_fn) run from a PendSV interrupt — not the main loop. This allows them to preempt read(), write(), close(), and accept() at any point, causing race conditions on shared state like the rx_buf_ pbuf chain. The most critical race: recv_fn calls pbuf_cat(rx_buf_, pb) while read() is freeing nodes in the same chain, leading to use-after-free and lwip's "Creating an infinite loop" assertion panic. This is the root cause of #10681. Fix: implement RP2040's LwIPLock (previously a no-op) to call cyw43_arch_lwip_begin/end, which acquires the pico-sdk async_context recursive mutex. Add LWIP_LOCK() guards to all main-loop lwip API call sites in the socket layer. On ESP8266, lwip callbacks run cooperatively from the main loop, so LwIPLock remains a no-op. Closes #10681 --- esphome/components/rp2040/helpers.cpp | 16 ++++++- .../components/socket/lwip_raw_tcp_impl.cpp | 47 +++++++++++++++++++ esphome/components/socket/lwip_raw_tcp_impl.h | 6 +++ 3 files changed, 68 insertions(+), 1 deletion(-) diff --git a/esphome/components/rp2040/helpers.cpp b/esphome/components/rp2040/helpers.cpp index 30b40a723a3..4191c2164ad 100644 --- a/esphome/components/rp2040/helpers.cpp +++ b/esphome/components/rp2040/helpers.cpp @@ -7,6 +7,7 @@ #if defined(USE_WIFI) #include +#include // For cyw43_arch_lwip_begin/end (LwIPLock) #endif #include #include @@ -44,9 +45,22 @@ void Mutex::unlock() {} IRAM_ATTR InterruptLock::InterruptLock() { state_ = save_and_disable_interrupts(); } IRAM_ATTR InterruptLock::~InterruptLock() { restore_interrupts(state_); } -// RP2040 doesn't support lwIP core locking, so this is a no-op +// On RP2040 (Pico W), arduino-pico sets PICO_CYW43_ARCH_THREADSAFE_BACKGROUND=1. +// This means lwip callbacks run from a low-priority user IRQ context, not the +// main loop (see low_priority_irq_handler() in pico-sdk +// async_context_threadsafe_background.c). cyw43_arch_lwip_begin/end acquires the +// async_context recursive mutex to prevent IRQ callbacks from firing during +// critical sections. See esphome#10681. +// +// When CYW43 is not available (non-WiFi RP2040 boards), this is a no-op since +// there's no network stack and no lwip callbacks to race with. +#if defined(USE_WIFI) +LwIPLock::LwIPLock() { cyw43_arch_lwip_begin(); } +LwIPLock::~LwIPLock() { cyw43_arch_lwip_end(); } +#else LwIPLock::LwIPLock() {} LwIPLock::~LwIPLock() {} +#endif void get_mac_address_raw(uint8_t *mac) { // NOLINT(readability-non-const-parameter) #ifdef USE_WIFI diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index 445a57809d2..b1ea45b82a5 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -111,6 +111,24 @@ void socket_wake() { } #endif +// ---- LWIP thread safety ---- +// +// On RP2040 (Pico W), arduino-pico sets PICO_CYW43_ARCH_THREADSAFE_BACKGROUND=1. +// This means lwip callbacks (recv_fn, accept_fn, err_fn) run from a low-priority +// user IRQ context, not the main loop (see low_priority_irq_handler() in pico-sdk +// async_context_threadsafe_background.c). They can preempt main-loop code at any point. +// +// Without locking, this causes race conditions between recv_fn and read() on the +// shared rx_buf_ pbuf chain — recv_fn calls pbuf_cat() while read() is freeing +// nodes, leading to use-after-free and infinite-loop crashes. See esphome#10681. +// +// On ESP8266, lwip callbacks run from the SYS context which cooperates with user +// code (CONT context) — they never preempt each other, so no locking is needed. +// +// esphome::LwIPLock is the platform-provided RAII guard (see helpers.h/helpers.cpp). +// On RP2040, it acquires cyw43_arch_lwip_begin/end. On ESP8266, it's a no-op. +#define LWIP_LOCK() esphome::LwIPLock lwip_lock_guard // NOLINT + static const char *const TAG = "socket.lwip"; // set to 1 to enable verbose lwip logging @@ -123,6 +141,7 @@ static const char *const TAG = "socket.lwip"; // ---- LWIPRawCommon methods ---- LWIPRawCommon::~LWIPRawCommon() { + LWIP_LOCK(); if (this->pcb_ != nullptr) { LWIP_LOG("tcp_abort(%p)", this->pcb_); tcp_abort(this->pcb_); @@ -131,6 +150,7 @@ LWIPRawCommon::~LWIPRawCommon() { } int LWIPRawCommon::bind(const struct sockaddr *name, socklen_t addrlen) { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = EBADF; return -1; @@ -196,6 +216,7 @@ int LWIPRawCommon::bind(const struct sockaddr *name, socklen_t addrlen) { } int LWIPRawCommon::close() { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = ECONNRESET; return -1; @@ -214,6 +235,7 @@ int LWIPRawCommon::close() { } int LWIPRawCommon::shutdown(int how) { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = ECONNRESET; return -1; @@ -240,6 +262,7 @@ int LWIPRawCommon::shutdown(int how) { } int LWIPRawCommon::getpeername(struct sockaddr *name, socklen_t *addrlen) { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = ECONNRESET; return -1; @@ -252,6 +275,7 @@ int LWIPRawCommon::getpeername(struct sockaddr *name, socklen_t *addrlen) { } int LWIPRawCommon::getsockname(struct sockaddr *name, socklen_t *addrlen) { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = ECONNRESET; return -1; @@ -284,6 +308,7 @@ size_t LWIPRawCommon::getsockname_to(std::span buf) { } int LWIPRawCommon::getsockopt(int level, int optname, void *optval, socklen_t *optlen) { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = ECONNRESET; return -1; @@ -318,6 +343,7 @@ int LWIPRawCommon::getsockopt(int level, int optname, void *optval, socklen_t *o } int LWIPRawCommon::setsockopt(int level, int optname, const void *optval, socklen_t optlen) { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = ECONNRESET; return -1; @@ -388,6 +414,7 @@ int LWIPRawCommon::ip2sockaddr_(ip_addr_t *ip, uint16_t port, struct sockaddr *n // ---- LWIPRawImpl methods ---- LWIPRawImpl::~LWIPRawImpl() { + LWIP_LOCK(); // Free any received pbufs that LWIP transferred ownership of via recv_fn. // tcp_abort() in the base destructor won't free these since LWIP considers // ownership transferred once the recv callback accepts them. @@ -399,6 +426,7 @@ LWIPRawImpl::~LWIPRawImpl() { } void LWIPRawImpl::init() { + LWIP_LOCK(); LWIP_LOG("init(%p)", this->pcb_); tcp_arg(this->pcb_, this); tcp_recv(this->pcb_, LWIPRawImpl::s_recv_fn); @@ -406,6 +434,9 @@ void LWIPRawImpl::init() { } void LWIPRawImpl::s_err_fn(void *arg, err_t err) { + // Called by lwip core which already holds the async_context lock on RP2040. + // No LWIP_LOCK() needed — acquiring it would be redundant (recursive mutex). + // // "If a connection is aborted because of an error, the application is alerted of this event by // the err callback." // pcb is already freed when this callback is called @@ -422,6 +453,7 @@ err_t LWIPRawImpl::s_recv_fn(void *arg, struct tcp_pcb *pcb, struct pbuf *pb, er } err_t LWIPRawImpl::recv_fn(struct pbuf *pb, err_t err) { + // Called by lwip core which already holds the async_context lock on RP2040. LWIP_LOG("recv(pb=%p err=%d)", pb, err); if (err != 0) { // "An error code if there has been an error receiving Only return ERR_ABRT if you have @@ -448,6 +480,7 @@ err_t LWIPRawImpl::recv_fn(struct pbuf *pb, err_t err) { } ssize_t LWIPRawImpl::read(void *buf, size_t len) { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = ECONNRESET; return -1; @@ -525,6 +558,7 @@ ssize_t LWIPRawImpl::readv(const struct iovec *iov, int iovcnt) { } ssize_t LWIPRawImpl::internal_write_(const void *buf, size_t len) { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = ECONNRESET; return -1; @@ -557,6 +591,11 @@ ssize_t LWIPRawImpl::internal_write_(const void *buf, size_t len) { } int LWIPRawImpl::internal_output_() { + LWIP_LOCK(); + if (this->pcb_ == nullptr) { + errno = ECONNRESET; + return -1; + } LWIP_LOG("tcp_output(%p)", this->pcb_); err_t err = tcp_output(this->pcb_); if (err == ERR_ABRT) { @@ -621,6 +660,7 @@ ssize_t LWIPRawImpl::writev(const struct iovec *iov, int iovcnt) { // ---- LWIPRawListenImpl methods ---- LWIPRawListenImpl::~LWIPRawListenImpl() { + LWIP_LOCK(); // Listen PCBs must use tcp_close(), not tcp_abort(). // tcp_abandon() asserts pcb->state != LISTEN and would access // fields that don't exist in the smaller tcp_pcb_listen struct. @@ -632,6 +672,7 @@ LWIPRawListenImpl::~LWIPRawListenImpl() { } void LWIPRawListenImpl::init() { + LWIP_LOCK(); LWIP_LOG("init(%p)", this->pcb_); tcp_arg(this->pcb_, this); tcp_accept(this->pcb_, LWIPRawListenImpl::s_accept_fn); @@ -639,6 +680,7 @@ void LWIPRawListenImpl::init() { } void LWIPRawListenImpl::s_err_fn(void *arg, err_t err) { + // Called by lwip core which already holds the async_context lock on RP2040. auto *arg_this = reinterpret_cast(arg); ESP_LOGVV(TAG, "socket %p: err(err=%d)", arg_this, err); arg_this->pcb_ = nullptr; @@ -650,6 +692,7 @@ err_t LWIPRawListenImpl::s_accept_fn(void *arg, struct tcp_pcb *newpcb, err_t er } std::unique_ptr LWIPRawListenImpl::accept(struct sockaddr *addr, socklen_t *addrlen) { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = EBADF; return nullptr; @@ -674,6 +717,7 @@ std::unique_ptr LWIPRawListenImpl::accept(struct sockaddr *addr, so } int LWIPRawListenImpl::listen(int backlog) { + LWIP_LOCK(); if (this->pcb_ == nullptr) { errno = EBADF; return -1; @@ -699,6 +743,7 @@ int LWIPRawListenImpl::listen(int backlog) { } err_t LWIPRawListenImpl::accept_fn_(struct tcp_pcb *newpcb, err_t err) { + // Called by lwip core which already holds the async_context lock on RP2040. LWIP_LOG("accept(newpcb=%p err=%d)", newpcb, err); if (err != ERR_OK || newpcb == nullptr) { // "An error code if there has been an error accepting. Only return ERR_ABRT if you have @@ -766,6 +811,8 @@ std::unique_ptr socket_listen_loop_monitored(int domain, int type, return socket_listen(domain, type, protocol); } +#undef LWIP_LOCK + } // namespace esphome::socket #endif // USE_SOCKET_IMPL_LWIP_TCP diff --git a/esphome/components/socket/lwip_raw_tcp_impl.h b/esphome/components/socket/lwip_raw_tcp_impl.h index c171e0537f3..5b2c11cfe2c 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.h +++ b/esphome/components/socket/lwip_raw_tcp_impl.h @@ -95,8 +95,13 @@ class LWIPRawImpl : public LWIPRawCommon { errno = ENOSYS; return -1; } + // Intentionally unlocked — this is a polling check called every loop iteration. + // A stale read at worst delays processing by one loop tick; the actual I/O in + // read() holds the lwip lock and re-checks properly. See esphome#10681. bool ready() const { return this->rx_buf_ != nullptr || this->rx_closed_ || this->pcb_ == nullptr; } + // No lock needed — only called during setup before callbacks are registered. + // A stale pcb_ read is benign (returns ECONNRESET, which the caller handles). int setblocking(bool blocking) { if (this->pcb_ == nullptr) { errno = ECONNRESET; @@ -134,6 +139,7 @@ class LWIPRawListenImpl : public LWIPRawCommon { void init(); + // Intentionally unlocked — polling check, see LWIPRawImpl::ready() comment. bool ready() const { return this->accepted_socket_count_ > 0; } std::unique_ptr accept(struct sockaddr *addr, socklen_t *addrlen); From c182c0c74f549eeccaf34355b8e319e0ca031851 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 10 Mar 2026 00:53:01 -1000 Subject: [PATCH 3/5] [socket] Hold lwip lock for entire readv/writev scatter-gather operation Avoid repeated lock acquire/release cycles per iovec element. The recursive mutex re-entry in inner calls is nearly free (counter bump), while the outer lock prevents the expensive IRQ disable/enable on each iteration. --- esphome/components/socket/lwip_raw_tcp_impl.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index b1ea45b82a5..cabf546a27d 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -540,6 +540,7 @@ ssize_t LWIPRawImpl::read(void *buf, size_t len) { } ssize_t LWIPRawImpl::readv(const struct iovec *iov, int iovcnt) { + LWIP_LOCK(); // Hold for entire scatter-gather operation ssize_t ret = 0; for (int i = 0; i < iovcnt; i++) { ssize_t err = this->read(reinterpret_cast(iov[i].iov_base), iov[i].iov_len); @@ -631,6 +632,7 @@ ssize_t LWIPRawImpl::write(const void *buf, size_t len) { } ssize_t LWIPRawImpl::writev(const struct iovec *iov, int iovcnt) { + LWIP_LOCK(); // Hold for entire scatter-gather operation ssize_t written = 0; for (int i = 0; i < iovcnt; i++) { ssize_t err = this->internal_write_(reinterpret_cast(iov[i].iov_base), iov[i].iov_len); From cc05bf3ed22decdc82bcaf24ae6b9224634a8963 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 10 Mar 2026 00:55:34 -1000 Subject: [PATCH 4/5] [socket] Add LWIP_LOCK to socket factory functions tcp_new() is an lwip core API call that must be bracketed with the lwip lock on RP2040 per pico-sdk docs. Add LWIP_LOCK() to socket() and socket_listen() factory functions. --- esphome/components/socket/lwip_raw_tcp_impl.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index cabf546a27d..0c0d64d1987 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -781,6 +781,7 @@ std::unique_ptr socket(int domain, int type, int protocol) { errno = EPROTOTYPE; return nullptr; } + LWIP_LOCK(); auto *pcb = tcp_new(); if (pcb == nullptr) return nullptr; @@ -800,6 +801,7 @@ std::unique_ptr socket_listen(int domain, int type, int protocol) errno = EPROTOTYPE; return nullptr; } + LWIP_LOCK(); auto *pcb = tcp_new(); if (pcb == nullptr) return nullptr; From 81d12fd14ae95ebe2ab9bc0935d307bda7cebd9e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 10 Mar 2026 00:57:36 -1000 Subject: [PATCH 5/5] [socket] Hold lwip lock for entire write() operation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same pattern as writev — write() calls internal_write_() then internal_output_(), each acquiring the lock separately. Hold the lock at the outer scope so inner calls just bump the recursion counter. --- esphome/components/socket/lwip_raw_tcp_impl.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index 0c0d64d1987..d7fa6a26945 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -616,6 +616,7 @@ int LWIPRawImpl::internal_output_() { } ssize_t LWIPRawImpl::write(const void *buf, size_t len) { + LWIP_LOCK(); // Hold for write + optional output ssize_t written = this->internal_write_(buf, len); if (written == -1) return -1;