fix: fix race in IwxProcessEvents; fix WiFi bugs

kernel/src/Api/BuildNo.hpp

@@ -12,4 +12,4 @@
 
 #pragma once
 
-#define MONTAUK_BUILD_NUMBER 279
+#define MONTAUK_BUILD_NUMBER 283

kernel/src/Drivers/Net/Wifi/Iwx.hpp

@@ -173,6 +173,10 @@ namespace Drivers::Net::Wifi {
 
         // Interrupt-to-idle-loop deferral
         volatile bool WorkPending = false;
+        // When IwxProcessEvents() last ran to completion.  The idle loop
+        // sweeps the ring on a timer as well as on the interrupt, so a single
+        // lost interrupt cannot silence the adapter for good.
+        volatile uint64_t LastPumpMs = 0;
 
         IwxFwInfo Fw;
 
@@ -321,6 +325,13 @@ namespace Drivers::Net::Wifi {
     // from any process/idle context; self-guarded against reentry.
     void IwxProcessEvents();
 
+    // A firmware error seen inside IwxProcessEvents() is only latched there:
+    // reading the error table can mean a long wait for the MAC to wake, and
+    // the pump runs on the BSP.  The dump happens here instead, from the
+    // service path, and at the latest before IwxStopDevice() wipes it.
+    bool IwxFwErrorDumpPending();
+    void IwxFlushFwErrorDump();
+
     // Look up the firmware-advertised version of a command/notification.
     int IwxLookupCmdVer(uint8_t group, uint8_t cmd);
     int IwxLookupNotifVer(uint8_t group, uint8_t cmd);
@@ -419,6 +430,9 @@ namespace Drivers::Net::Wifi {
     void IwxConnectRxData(const uint8_t* frame, uint32_t len);
     // Apply state changes the RX path queued (it cannot send commands itself).
     void IwxConnectService();
+    // Whether IwxConnectService() has anything to do: queued RX-path work, or
+    // a join in progress whose retransmission/give-up timers need watching.
+    bool IwxConnectHasWork();
     int  IwxConnectState();
 
     // Link status for the network stack.

kernel/src/Drivers/Net/Wifi/IwxConnect.cpp

@@ -1091,6 +1091,18 @@ namespace Drivers::Net::Wifi {
         __atomic_clear(&g_inService, __ATOMIC_RELEASE);
     }
 
+    bool IwxConnectHasWork() {
+        if (g_teardownPending || g_linkLostPending
+            || g_sendAssocPending || g_postAssocPending)
+            return true;
+        if (g_eapolTail != g_eapolHead) return true;
+        // Every state between "contexts up" and "connected" runs on timers
+        // (retransmission, give-up, the handshake's own retries).
+        return g_state != ConnState::Idle
+            && g_state != ConnState::Connected
+            && g_state != ConnState::Failed;
+    }
+
     bool IwxConnectRefusedForSecurity() { return g_refusedForSecurity; }
 
     int IwxConnectState() {

kernel/src/Drivers/Net/Wifi/IwxTrans.cpp

@@ -711,6 +711,9 @@ namespace Drivers::Net::Wifi {
     }
 
     void IwxStopDevice() {
+        // Last chance to read the firmware's error table before the reset
+        // below wipes it.
+        IwxFlushFwErrorDump();
         IwxDisableInterrupts();
 
         if (IwxNicLock()) {
@@ -1647,15 +1650,63 @@ namespace Drivers::Net::Wifi {
         IwxWrite32(IWX_RFH_Q0_FRBDCB_WIDX_TRG, widx & ~7u);
     }
 
+    // Latched by IwxProcessEvents(), performed by IwxFlushFwErrorDump().
+    static volatile bool g_fwDumpPending = false;
+
+    bool IwxFwErrorDumpPending() { return g_fwDumpPending; }
+
+    void IwxFlushFwErrorDump() {
+        if (!__atomic_exchange_n(&g_fwDumpPending, false, __ATOMIC_ACQ_REL)) return;
+        IwxDumpFwError();
+    }
+
+    // The first fatal error moves the adapter to Error; repeats of the same
+    // cause before the reset lands are not news and must not re-log.
+    static void IwxNoteFatal(const char* what, bool dump) {
+        if (g_iwx.State == IwxFwState::Error) return;
+        KernelLogStream(ERROR, "WiFi") << what;
+        g_iwx.State = IwxFwState::Error;
+        g_iwx.FwErrors++;
+        if (dump) g_fwDumpPending = true;
+    }
+
     void IwxProcessEvents() {
         if (!g_iwx.Mmio) return;
         // Never nest.  With interrupts enabled during command waits this is a
         // genuine multi-core race, so it has to be an atomic test-and-set.
         if (g_iwx.InProcessEvents.test_and_set(std::memory_order_acquire)) return;
 
+        // Consume the wakeup BEFORE looking at the hardware, never after.
+        // This used to be cleared on the way out, after the automask had
+        // already been re-armed below and after the ring sweep -- which hands
+        // received frames to the network stack and so is anything but short.
+        // An interrupt landing in that window set the flag only for it to be
+        // wiped a moment later: the work was lost, and because MSI-X masks
+        // the vector each time it fires until the driver re-arms it here, no
+        // further interrupt could ever arrive to replace it.  The adapter went
+        // permanently silent -- no RX, no TX completions, so not even the link
+        // supervision noticed -- while the link was still reported up.
+        // Clearing first means an interrupt at any point after this line
+        // leaves the flag set and earns another pass.
+        g_iwx.WorkPending = false;
+        __atomic_thread_fence(__ATOMIC_SEQ_CST);
+
         if (g_msix) {
             uint32_t fh = IwxRead32(IWX_CSR_MSIX_FH_INT_CAUSES_AD);
             uint32_t hwc = IwxRead32(IWX_CSR_MSIX_HW_INT_CAUSES_AD);
+            // All-ones is not a cause word, it is what a read returns when the
+            // device has dropped off the bus (or its link is down).  Taken at
+            // face value it reads as every cause at once, firmware errors
+            // included, and would be "handled" -- dump and all -- on every
+            // single pass.  Mid-bring-up a reset can produce the same read
+            // transiently, so only a running adapter is declared gone.
+            if (fh == 0xffffffff && hwc == 0xffffffff) {
+                if (g_iwx.State == IwxFwState::Running)
+                    IwxNoteFatal("Adapter is not responding on the bus", false);
+                g_iwx.LastPumpMs = Timekeeping::GetMilliseconds();
+                g_iwx.InProcessEvents.clear(std::memory_order_release);
+                return;
+            }
             IwxWrite32(IWX_CSR_MSIX_FH_INT_CAUSES_AD, fh);
             IwxWrite32(IWX_CSR_MSIX_HW_INT_CAUSES_AD, hwc);
             fh &= g_fhMask;
@@ -1673,15 +1724,10 @@ namespace Drivers::Net::Wifi {
             if ((fh & IWX_MSIX_FH_INT_CAUSES_FH_ERR)
                 || (hwc & IWX_MSIX_HW_INT_CAUSES_REG_SW_ERR)
                 || (hwc & IWX_MSIX_HW_INT_CAUSES_REG_SW_ERR_V2)) {
-                KernelLogStream(ERROR, "WiFi") << "Fatal firmware error";
+                IwxNoteFatal("Fatal firmware error", true);
-                g_iwx.State = IwxFwState::Error;
-                g_iwx.FwErrors++;
-                IwxDumpFwError();
             }
             if (hwc & IWX_MSIX_HW_INT_CAUSES_REG_HW_ERR) {
-                KernelLogStream(ERROR, "WiFi") << "Hardware error";
+                IwxNoteFatal("Hardware error", false);
-                g_iwx.State = IwxFwState::Error;
-                g_iwx.FwErrors++;
             }
             if (hwc & IWX_MSIX_HW_INT_CAUSES_REG_RF_KILL) {
                 if (IwxCheckRfKill()) {
@@ -1703,10 +1749,7 @@ namespace Drivers::Net::Wifi {
                     IwxWrite32(IWX_RFH_Q0_FRBDCB_WIDX_TRG, 8);
                 }
                 if (r1 & (1u << 25)) {        // SW_ERR
-                    KernelLogStream(ERROR, "WiFi") << "Fatal firmware error";
+                    IwxNoteFatal("Fatal firmware error", true);
-                    g_iwx.State = IwxFwState::Error;
-                    g_iwx.FwErrors++;
-                    IwxDumpFwError();
                 }
                 if (r1 & ((1u << 31) | (1u << 3) | (1u << 28)))   // FH_RX/SW_RX/periodic
                     IwxNotifIntr();
@@ -1717,7 +1760,7 @@ namespace Drivers::Net::Wifi {
         // been consumed by another poller, so always sweep the ring.
         IwxNotifIntr();
 
-        g_iwx.WorkPending = false;
+        g_iwx.LastPumpMs = Timekeeping::GetMilliseconds();
         g_iwx.InProcessEvents.clear(std::memory_order_release);
     }
 

kernel/src/Drivers/Net/Wifi/Wifi.cpp

@@ -21,6 +21,8 @@
 #include <CppLib/Spinlock.hpp>
 #include <Timekeeping/ApicTimer.hpp>
 #include <Hal/SmpBoot.hpp>
+#include <Hal/Apic/Apic.hpp>
+#include <Hal/Apic/Interrupts.hpp>
 #include <Sched/Scheduler.hpp>
 #include <atomic>
 
@@ -417,9 +419,52 @@ namespace Drivers::Net::Wifi {
         }
     }
 
+    // At most one core inside the blocking half at a time.
+    static volatile bool     g_blockingBusy = false;
+    static volatile uint64_t g_lastApKickMs = 0;
+    static constexpr uint64_t AP_KICK_INTERVAL_MS = 10;
+
+    // The adapter is swept on a timer as well as on its interrupt whenever
+    // anything is in flight.  Cheap (two register reads when idle), and it
+    // means no single lost interrupt can leave the link silently dead.
+    static constexpr uint64_t PUMP_POLL_MS = 100;
+
+    // True when the caller should leave the blocking work alone because an AP
+    // has it (or has just been asked to take it).
+    static bool LeaveToAp() {
+        auto* cpu = Smp::GetCurrentCpuData();
+        if (!cpu || cpu->cpuIndex != 0) return false;
+        if (g_blockingBusy) return true;
+
+        uint64_t now = Timekeeping::GetMilliseconds();
+        uint64_t last = g_lastApKickMs;
+        if (last && now >= last && now - last < AP_KICK_INTERVAL_MS) return true;
+
+        for (int i = 1; i < Smp::GetCpuCount(); i++) {
+            auto* t = Smp::GetCpuData(i);
+            if (!t || !t->started) continue;
+            if (t->currentSlot >= 0 || t->reservedForKernelWork) continue;
+            if (Hal::LocalApic::SendFixedIpi(t->lapicId,
+                    Hal::IRQ_VECTOR_BASE + Hal::IRQ_RESCHEDULE)) {
+                g_lastApKickMs = now;
+                return true;
+            }
+        }
+        return false;
+    }
+
+    static bool InitDue() {
+        return g_initPending.load(std::memory_order_acquire) && !g_initialized
+            && Fs::Vfs::IsDriveRegistered(0);
+    }
+
     void ServiceDeferredInit() {
-        if (!g_initPending.load(std::memory_order_relaxed) || g_initialized) return;
+        if (!InitDue()) return;
-        if (!Fs::Vfs::IsDriveRegistered(0)) return;    // ramdisk not mounted yet
+        if (LeaveToAp()) return;
+        if (__atomic_test_and_set(&g_blockingBusy, __ATOMIC_ACQUIRE)) return;
+        struct Guard {
+            ~Guard() { __atomic_clear(&g_blockingBusy, __ATOMIC_RELEASE); }
+        } guard;
 
         bool expected = true;
         if (!g_initPending.compare_exchange_strong(expected, false,
@@ -440,20 +485,30 @@ namespace Drivers::Net::Wifi {
     static void ServiceAsync();
     static void ServiceRecovery();
 
-    bool HasDeferredWork() {
+    // Whether the RX ring is due a sweep regardless of the interrupt.
-        if (g_initPending.load(std::memory_order_acquire) &&
+    static bool PumpDue(uint64_t now) {
-            !g_initialized && Fs::Vfs::IsDriveRegistered(0)) {
+        if (g_iwx.State != IwxFwState::Running) return false;
-            return true;
+        if (IwxConnectState() == (int)IwxConnStateId::Idle && !g_iwx.ScanActive)
-        }
+            return false;
-        if (!g_iwx.Mmio) return false;
+        uint64_t last = g_iwx.LastPumpMs;
-        if (g_iwx.WorkPending) return true;
+        return now > last && now - last >= PUMP_POLL_MS;
+    }
 
-        uint64_t now = Timekeeping::GetMilliseconds();
+    // Work that may send firmware commands, i.e. wait on the adapter.
-        if (g_scanDeadline != 0 && now >= g_scanDeadline) return true;
+    static bool HasBlockingWork(uint64_t now) {
+        // A finished sweep only needs its generation bumped; an overrunning
+        // one needs aborting.  Both live in ServiceAsync().
+        if (g_scanDeadline != 0 && (now >= g_scanDeadline || !g_iwx.ScanActive))
+            return true;
         // The MLME/WPA state machine owns sub-second retransmission timers in
         // addition to the overall async deadline. Service it until ServiceAsync
         // observes Connected/Failed/Idle and clears this flag.
         if (g_asyncConnect) return true;
+        // Work the RX path queued for IwxConnectService().  This used to ride
+        // on WorkPending, but the pump that clears that flag may now run on a
+        // different core from the one that services the connection.
+        if (IwxConnectHasWork()) return true;
+        if (IwxFwErrorDumpPending()) return true;
         if (g_iwx.State == IwxFwState::Error && g_initialized &&
             !g_recoveryGaveUp &&
             (g_lastRecoveryMs == 0 ||
@@ -463,9 +518,30 @@ namespace Drivers::Net::Wifi {
         return false;
     }
 
+    bool HasDeferredWork() {
+        if (InitDue()) return true;
+        if (!g_iwx.Mmio) return false;
+        if (g_iwx.WorkPending) return true;
+
+        uint64_t now = Timekeeping::GetMilliseconds();
+        if (PumpDue(now)) return true;
+        return HasBlockingWork(now);
+    }
+
     void ServiceEvents() {
         if (!g_iwx.Mmio) return;
-        if (g_iwx.WorkPending) IwxProcessEvents();
+
+        // The non-blocking half: any core, the BSP included.
+        uint64_t now = Timekeeping::GetMilliseconds();
+        if (g_iwx.WorkPending || PumpDue(now)) IwxProcessEvents();
+
+        now = Timekeeping::GetMilliseconds();
+        if (!HasBlockingWork(now)) return;
+        if (LeaveToAp()) return;
+        if (__atomic_test_and_set(&g_blockingBusy, __ATOMIC_ACQUIRE)) return;
+        struct Guard {
+            ~Guard() { __atomic_clear(&g_blockingBusy, __ATOMIC_RELEASE); }
+        } guard;
 
         // Everything below can send firmware commands, and each one waits by
         // busy-spinning.  This runs from ServiceDeferredWork(), which has set
@@ -487,6 +563,11 @@ namespace Drivers::Net::Wifi {
 
         if (ownsPass) IwxEndServicePass();
 
+        // A firmware error the pump latched.  Normally the reset below dumps
+        // it on its way through IwxStopDevice(); this catches the cases where
+        // no reset is coming (recovery exhausted, or the adapter never came up).
+        IwxFlushFwErrorDump();
+
         // Deliberately outside the budget: a reset is a bring-up, not a pass of
         // routine servicing, and it has its own wall-clock handshakes to run.
         ServiceRecovery();

tests/wifi/ap_mlme.py

@@ -27,6 +27,10 @@ def link_of(result):
     f = result.split()
     return int(f[f.index('LINK') + 1]) if 'LINK' in f else -1
 
+def work_of(result):
+    f = result.split()
+    return int(f[f.index('WORK') + 1]) if 'WORK' in f else -1
+
 def check(cond, what):
     print(f"  {'PASS' if cond else 'FAIL'}  {what}")
     if not cond:
@@ -656,6 +660,9 @@ d8.cmd("RXMGMT " + mgmt(0x10, STA, AP, AP,
                         struct.pack('<HHH', 0x0421, 0, 7 | 0xc000)).hex())
 ev = d8.cmd("SERVICE")
 check(link_of(ev['result']) == 1, "the open network is associated and the link is up")
+# The idle loop only hands a connection to a core for servicing when this
+# says there is something to do, so a settled link must report nothing.
+check(work_of(ev['result']) == 0, "a settled link has no service work pending")
 
 # Well short of the threshold: a few unacknowledged frames are ordinary.
 ev = d8.cmd("TXSTATUS 0 15")
@@ -682,6 +689,10 @@ check(link_of(ev['result']) == 1,
       "the transmit path itself does not tear anything down")
 check(not ev['CMD'],
       "no firmware command is sent from the completion path")
+# The pump that noticed this may run on a different core from the one that
+# services the connection; the pending teardown has to be visible by itself.
+check(work_of(ev['result']) == 1,
+      "the pending teardown is reported as service work")
 
 ev = d8.cmd("SERVICE")
 check(state_of(ev['result']) == 0 and link_of(ev['result']) == 0,
@@ -689,6 +700,7 @@ check(state_of(ev['result']) == 0 and link_of(ev['result']) == 0,
 check(any(c == MAC_CONFIG and struct.unpack_from('<I', p, 4)[0] == 3
           for c, p in ev['CMD']),
       "and unwinds the firmware contexts")
+check(work_of(ev['result']) == 0, "and leaves no service work behind")
 d8.p.stdin.close()
 
 print()

tests/wifi/mlme_harness.cpp

@@ -204,10 +204,12 @@ int main() {
             unsigned acked, count;
             sscanf(line.c_str(), "TXSTATUS %u %u", &acked, &count);
             for (unsigned i = 0; i < count; i++) IwxConnectNoteTx(acked != 0);
-            printf("DONE STATE %d LINK %d\n", IwxConnectState(), IwxLinkUp() ? 1 : 0);
+            printf("DONE STATE %d LINK %d WORK %d\n", IwxConnectState(),
+                   IwxLinkUp() ? 1 : 0, IwxConnectHasWork() ? 1 : 0);
         } else if (line.rfind("SERVICE", 0) == 0) {
             IwxConnectService();
-            printf("DONE STATE %d LINK %d\n", IwxConnectState(), IwxLinkUp() ? 1 : 0);
+            printf("DONE STATE %d LINK %d WORK %d\n", IwxConnectState(),
+                   IwxLinkUp() ? 1 : 0, IwxConnectHasWork() ? 1 : 0);
         } else if (line.rfind("TICK ", 0) == 0) {
             unsigned ms; sscanf(line.c_str(), "TICK %u", &ms);
             Timekeeping::g_ms += ms;