cleanup: Bluetooth/A2DP code cleanup

kernel/src/Drivers/USB/Bluetooth/A2dp.cpp

@@ -124,14 +124,11 @@ namespace Drivers::USB::Bluetooth::A2dp {
     static uint64_t g_clockBase = 0;     // ms timestamp of the media clock zero
     static uint64_t g_sentSamples = 0;   // per-channel samples sent since reset
     static uint64_t g_lastSendMs = 0;
-    static uint32_t g_underruns = 0;     // ring ran dry while behind schedule
-    static bool g_inUnderrun = false;
 
     static void ResetMediaClock() {
         g_clockBase = Timekeeping::GetMilliseconds();
         g_sentSamples = 0;
         g_lastSendMs = g_clockBase;
-        g_inUnderrun = false;
     }
 
     // Volume
@@ -515,10 +512,6 @@ namespace Drivers::USB::Bluetooth::A2dp {
     // remoteCid=0/connRsp=1 media-setup failure seen on HW even with SCMS-T
     // configured.  So we serve one record: A2DP AudioSource.
 
-    // SDP-HOST-TEST-BEGIN: the host harness (/tmp/sdptest) extracts the region
-    // between these markers verbatim and unit-tests it with libc, so keep it
-    // free of kernel-only dependencies (KernelLogStream and L2cap::SendData
-    // are stubbed there).
     // AudioSource service record: attribute id (uint16 DE) + value, sorted by
     // id, all SDP data elements big-endian.
     static const uint8_t kSourceRecord[] = {
@@ -766,15 +759,6 @@ namespace Drivers::USB::Bluetooth::A2dp {
             // silently (its device interrogation then never completes, and a
             // stack that service-authorizes channels against that
             // interrogation parks them at "authorization pending" forever).
-            {   // Raw request dump: ONE decisive run beats guessing what the
-                // headset asked for.  Queries are once-per-connection rare.
-                KernelLogStream cl(INFO, "BT-A2DP");
-                cl << "SDP req:" << base::hex;
-                for (uint16_t i = 0; i < len && i < 28; i++)
-                    cl << " " << (uint64_t)d[i];
-                cl << base::dec;
-            }
-
             // Field offsets differ: 0x06 has the search pattern at 5, 0x04 a
             // 4-byte record handle at 5.  Both follow with max byte count,
             // AttributeIDList, continuation state.
@@ -916,7 +900,6 @@ namespace Drivers::USB::Bluetooth::A2dp {
             SdpServerSend(cid, 0x01, tid, params, n);
         }
     }
-    // SDP-HOST-TEST-END
 
     // Called by L2CAP when data arrives on ANY SDP channel -- ours (the client
     // query response) or one the headset opened to us (a request to serve).
@@ -945,7 +928,6 @@ namespace Drivers::USB::Bluetooth::A2dp {
         if (!cid || !L2cap::WaitConfigured(cid, timeoutMs)) {
             KernelLogStream(WARNING, "BT-A2DP") << "SDP channel setup failed (connRsp="
                 << base::hex << (uint64_t)L2cap::LastConnRspResult() << base::dec << ")";
-            Hci::DumpAclStats();   // did our CONN_REQ even go out / any reply arrive?
             return false;
         }
         g_sdpCid = cid;
@@ -993,9 +975,9 @@ namespace Drivers::USB::Bluetooth::A2dp {
         constexpr int kMaxAttempts = 4;
 
         // Connection phase, retried.  A sink commonly ignores the very first
-        // L2CAP CONN_REQ that lands right after Encryption Change (the build-39
+        // L2CAP CONN_REQ that lands right after Encryption Change (connRsp stays
-        // HW symptom: connRsp stays 0xFFFF, the headset never answers).  Re-dial
+        // 0xFFFF, the headset never answers).  Re-dial up to kMaxAttempts.  We
-        // up to kMaxAttempts.  We retry ONLY when the remote ignored us
+        // retry ONLY when the remote ignored us
         // (connRsp==0xFFFF -> our dialed channel has RemoteCid==0, so freeing it
         // owes the peer no Disconnect); if it answered but config stalled
         // (connRsp!=0xFFFF) we do NOT loop -- that's the config phase, surfaced
@@ -1038,14 +1020,12 @@ namespace Drivers::USB::Bluetooth::A2dp {
                 << " localCfg=" << (uint64_t)(ch ? ch->LocalConfigDone : 0)
                 << " remoteCfg=" << (uint64_t)(ch ? ch->RemoteConfigDone : 0)
                 << " connRsp=" << base::hex << (uint64_t)L2cap::LastConnRspResult()
-                << base::dec << " incomingReqs="
+                << base::dec << ")";
-                << (uint64_t)L2cap::IncomingAvdtpReqCount() << ")";
 
             // Retry only if the remote IGNORED our CONN_REQ entirely.  If it
             // answered (connRsp != 0xFFFF) the stall is in the config exchange,
             // not the dial -- don't loop; let the config-phase logs speak.
             if (L2cap::LastConnRspResult() != 0xFFFF) {
-                Hci::DumpAclStats();
                 return false;
             }
             // Free our unanswered dialed channel so repeated retries don't leak
@@ -1062,7 +1042,6 @@ namespace Drivers::USB::Bluetooth::A2dp {
 
         if (g_sigCid == 0) {
             KernelLogStream(WARNING, "BT-A2DP") << "AVDTP signaling setup gave up after retries";
-            Hci::DumpAclStats();
             return false;
         }
         KernelLogStream(OK, "BT-A2DP") << "AVDTP signaling channel ready, cid="
@@ -1107,9 +1086,9 @@ namespace Drivers::USB::Bluetooth::A2dp {
         //    sink's open-acceptor window), and HOLD that same channel while
         //    polling.  A CONN_RSP result=1 (PENDING) is NON-FINAL: a conformant
         //    sink follows it with SUCCESS/REFUSED on the SAME channel, so we must
-        //    NOT tear it down and re-dial -- doing so (build 47) only churned CIDs
+        //    NOT tear it down and re-dial -- doing so only churns CIDs and
-        //    and abandoned the very connection the sink was authorizing, while a
+        //    abandons the very connection the sink is authorizing; a single held
-        //    single held dial (build 44) already proved holding alone is harmless.
+        //    dial is harmless even when the sink takes seconds to authorize.
         //    Also accept an inbound transport channel (some sinks open it).
         //    NOTE: Bose service-gates this channel TWICE: SetConfiguration must
         //    include Content Protection (SCMS-T, see AvdtpSetConfiguration), AND
@@ -1145,9 +1124,7 @@ namespace Drivers::USB::Bluetooth::A2dp {
                 << " localCfg=" << (uint64_t)(ch ? ch->LocalConfigDone : 0)
                 << " remoteCfg=" << (uint64_t)(ch ? ch->RemoteConfigDone : 0)
                 << " connRsp=" << base::hex << (uint64_t)L2cap::LastConnRspResult()
-                << base::dec << " incomingReqs="
+                << base::dec << ")";
-                << (uint64_t)L2cap::IncomingAvdtpReqCount() << ")";
-            Hci::DumpAclStats();
             return false;
         }
 
@@ -1155,12 +1132,12 @@ namespace Drivers::USB::Bluetooth::A2dp {
             << base::hex << (uint64_t)g_mediaCid << base::dec << " state=Open";
 
         // 5. AVRCP control channel (PSM 0x17), best effort, AFTER the stream
-        //    exists -- the order every phone uses.  An earlier build dialed it
+        //    exists -- the order every phone uses.  Dialed before the media
-        //    right after the signaling channel; the sink answered PENDING and
+        //    channel, the sink answers PENDING and never completes it, leaving a
-        //    never completed it, so the half-open dial just sat in its
+        //    half-open dial parked in its authorization queue ahead of the media
-        //    authorization queue ahead of the media channel.  Dialing here
+        //    channel.  Dialing here keeps the bring-up clean and still gives the
-        //    keeps the bring-up clean and still gives the headset its absolute
+        //    headset its absolute volume path; an inbound AVCTP connect is
-        //    volume path; an inbound AVCTP connect is accepted at any time.
+        //    accepted at any time.
         {
             uint16_t avrcp = L2cap::Connect(L2cap::PSM_AVCTP);
             if (avrcp && L2cap::WaitConfigured(avrcp, 1500)) {
@@ -1411,15 +1388,7 @@ namespace Drivers::USB::Bluetooth::A2dp {
             uint64_t audioMs = g_sentSamples * 1000 / g_pcmRate;
             uint64_t elapsed = now - g_clockBase;
 
-            if (fill < bytesPerFrame) {
+            if (fill < bytesPerFrame) break;   // ring dry, nothing to send
-                // Ring dry.  If the sink's lead is exhausted too, this is an
-                // audible dropout caused by the app not feeding in time.
-                if (!g_inUnderrun && g_sentSamples != 0 && elapsed > audioMs) {
-                    g_inUnderrun = true;
-                    g_underruns++;
-                }
-                break;
-            }
             if (audioMs >= elapsed + LEAD_MS) break;   // sink lead is full
             if (!Hci::AclTxReady()) {
                 // Normal credit pacing most of the time.  A credit pool stuck
@@ -1501,26 +1470,6 @@ namespace Drivers::USB::Bluetooth::A2dp {
             g_timestamp += samplesPerFrame * nFrames;
             g_sentSamples += samplesPerFrame * nFrames;
             g_lastSendMs = now;
-            g_inUnderrun = false;
-
-            // Bring-up diagnostic: heartbeat every 512 packets (~6.8 s of
-            // audio at 5 frames/packet) proving the pipeline is healthy.
-            if ((g_seqNum & 0x1FF) == 0) {
-                uint32_t left = g_ringHead.load(std::memory_order_acquire)
-                              - g_ringTail.load(std::memory_order_relaxed);
-                uint32_t ringMs = (g_pcmRate && g_sbcEncoder.Channels)
-                    ? (uint32_t)((uint64_t)left * 1000
-                                 / ((uint64_t)g_pcmRate * g_sbcEncoder.Channels * 2))
-                    : 0;
-                KernelLogStream(INFO, "BT-A2DP") << "media: seq="
-                    << (uint64_t)g_seqNum
-                    << " bundle=" << (uint64_t)nFrames
-                    << " ring=" << (uint64_t)ringMs << "ms"
-                    << " lead=" << (uint64_t)(audioMs > elapsed ? audioMs - elapsed : 0) << "ms"
-                    << " inflight=" << (uint64_t)Hci::AclTxInFlight()
-                    << " pending=" << (uint64_t)Hci::AclPendingCount()
-                    << " underruns=" << (uint64_t)g_underruns;
-            }
         }
 
         g_pumpActive.store(false, std::memory_order_release);

kernel/src/Drivers/USB/Bluetooth/Bluetooth.cpp

@@ -292,9 +292,8 @@ namespace Drivers::USB::Bluetooth {
                 << (uint64_t)g_bdAddr[1] << ":" << (uint64_t)g_bdAddr[0] << base::dec;
         }
 
-        // NOTE: an earlier build overrode the BD_ADDR via 0xFC31 to dodge a
+        // NOTE: do NOT override the BD_ADDR via 0xFC31 here.  The BD_ADDR is an
-        // (since disproven) stale-bond theory.  Removed: the BD_ADDR is an input
+        // input to the SSP authentication confirmation, and if the override only
-        // to the SSP authentication confirmation, and if the override only
         // changes the advertised address but not the address the firmware uses
         // in the crypto, the two sides compute different confirmations and
         // pairing fails (Simple Pairing Complete = 0x05).  Use the real address.
@@ -501,8 +500,6 @@ namespace Drivers::USB::Bluetooth {
     int Connect(const uint8_t* bdAddr, uint32_t timeoutMs) {
         if (!g_initialized || !bdAddr) return -1;
 
-        Hci::ResetEventTrace();   // capture the pairing/SSP event sequence
-
         if (!Hci::CreateConnection(bdAddr)) return -1;
 
         // Wait for Connection Complete event
@@ -571,7 +568,6 @@ namespace Drivers::USB::Bluetooth {
                         // the link later dropped, so the disk write never stalls
                         // the nested pairing event handler.
                         Hci::FlushLinkKeys();
-                        Hci::DumpEventTrace();   // show the pairing/SSP sequence
                         return 0;
                     }
                 }
@@ -582,7 +578,6 @@ namespace Drivers::USB::Bluetooth {
             }
         }
 
-        Hci::DumpEventTrace();   // show whatever events did arrive
         return -1; // Timeout
     }
 

kernel/src/Drivers/USB/Bluetooth/Hci.cpp

@@ -82,17 +82,11 @@ namespace Drivers::USB::Bluetooth::Hci {
     // on another core.  int32 so a stray double-NOCP clamps instead of wrapping.
     static std::atomic<int32_t> g_aclPendingCount{0};
 
-    // Diagnostic ACL data-path counters: TX submitted, TX completed (bulk OUT
+    // ACL TX flow-control counters: TX submitted and TX completed (bulk OUT
-    // completion), RX received (bulk IN).  Used to tell whether L2CAP signaling
+    // completion).  tx - txDone is the number of TX DMA ring slots still owned
-    // actually flows over ACL after encryption is enabled.  tx - txDone is also
+    // by the xHCI (see AclTxReady), so the media writer throttles on it.
-    // the number of TX DMA ring slots still owned by the xHCI (see AclTxReady).
     static std::atomic<uint32_t> g_aclTxCount{0};
     static std::atomic<uint32_t> g_aclTxDoneCount{0};
-    static volatile uint32_t g_aclRxCount = 0;
-    // Incremented when an incoming ACL packet is dropped because the RX ring was
-    // full -- a nonzero value means a burst overran the ring (and could have
-    // dropped an L2CAP Config Response).  Surfaced by DumpAclStats().
-    static volatile uint32_t g_aclRxDropCount = 0;
 
     // Inquiry results
     static InquiryDevice g_inquiryResults[MAX_INQUIRY_RESULTS] = {};
@@ -120,19 +114,6 @@ namespace Drivers::USB::Bluetooth::Hci {
     static volatile uint32_t g_ssFragsSent = 0;
     static volatile uint32_t g_lastControlCC = 0;
 
-    // Lockless HCI-event trace for diagnosing the pairing/SSP sequence.
-    // ProcessEvent (which may run from the xHCI IRQ) only does an array write
-    // here -- no lock, no terminal I/O, so it cannot deadlock against g_termLock.
-    // DumpEventTrace() prints it from top-level (process) context.
-    static constexpr int EVT_TRACE_MAX = 48;
-    static volatile uint8_t g_evtTrace[EVT_TRACE_MAX] = {};
-    static volatile uint8_t g_evtTraceCount = 0;
-    // Status byte of the last Simple Pairing Complete (0x36) / Auth Complete
-    // (0x06) / Disconnection (0x05) -- captured to find WHY pairing fails.
-    static volatile uint8_t g_lastSppStatus = 0xEE;
-    static volatile uint8_t g_lastAuthStatus = 0xEE;
-    static volatile uint8_t g_lastDiscReason = 0xEE;
-
     // Pending-command queue.  Pairing replies (IO-cap / user-confirm / link-key)
     // are triggered from event handlers running NESTED under PollEvents, where a
     // command can only be fire-and-forget (the reentrancy guard makes a nested
@@ -327,16 +308,13 @@ namespace Drivers::USB::Bluetooth::Hci {
             // re-arm on every completion below (the bulk IN must keep cycling to
             // absorb the device's ~635 KB cc=4 glitch during firmware download).
             if (data && length >= sizeof(AclHeader) + 4) {
-                g_aclRxCount++;
                 uint8_t next = (uint8_t)((g_aclRxHead + 1) % ACL_RX_SLOTS);
-                if (next != g_aclRxTail) {   // ring not full
+                if (next != g_aclRxTail) {   // ring not full (else drop: overran)
                     uint32_t copyLen = length;
                     if (copyLen > ACL_RX_SLOT_SIZE) copyLen = ACL_RX_SLOT_SIZE;
                     memcpy(g_aclRxRing[g_aclRxHead], data, copyLen);
                     g_aclRxLens[g_aclRxHead] = (uint16_t)copyLen;
                     g_aclRxHead = next;
-                } else {
-                    g_aclRxDropCount++;   // ring overran -> a packet was lost
                 }
             }
 
@@ -409,10 +387,10 @@ namespace Drivers::USB::Bluetooth::Hci {
     // glitches a USB transaction error (cc=4) near ~635 KB of the upload, and an
     // armed bulk IN ABSORBS it (a benign cc=4 on the bulk endpoint) so the
     // interrupt-IN event pipe survives and the download completes.  Deferring the
-    // bulk-IN arm (build 40) moved that cc=4 onto the interrupt IN and wedged the
+    // bulk-IN arm moves that cc=4 onto the interrupt IN and wedges the download at
-    // download at 635 KB -- reverted.  The harmless firmware-phase bulk-IN runts
+    // 635 KB, so the bulk IN must be armed up front.  The harmless firmware-phase
-    // (4-7 byte completions) are filtered at enqueue in TransferCallback so they
+    // bulk-IN runts (4-7 byte completions) are filtered at enqueue in
-    // never pollute the RX ring.
+    // TransferCallback so they never pollute the RX ring.
     void StartEventPipe() {
         if (!g_initialized) return;
 
@@ -648,16 +626,6 @@ namespace Drivers::USB::Bluetooth::Hci {
         g_aclPendingCount.store(0, std::memory_order_relaxed);
     }
 
-    void DumpAclStats() {
-        KernelLogStream(INFO, "BT-HCI") << "ACL stats: tx="
-            << (uint64_t)g_aclTxCount.load(std::memory_order_relaxed)
-            << " txDone=" << (uint64_t)g_aclTxDoneCount.load(std::memory_order_relaxed)
-            << " rx=" << (uint64_t)g_aclRxCount
-            << " rxDrop=" << (uint64_t)g_aclRxDropCount
-            << " pending=" << (uint64_t)AclPendingCount()
-            << " bufNum=" << (uint64_t)g_aclMaxNum;
-    }
-
     // =========================================================================
     // ProcessEvent — handle HCI events
     // =========================================================================
@@ -669,17 +637,6 @@ namespace Drivers::USB::Bluetooth::Hci {
         uint8_t evtParamLen = data[1];
         const uint8_t* params = data + 2;
 
-        // Record into the lockless trace (safe from the IRQ path: array write
-        // only, no lock / no terminal I/O).  DumpEventTrace() prints it later
-        // from top-level context.
-        if (evtCode != EVT_NUM_COMPLETED_PACKETS && g_evtTraceCount < EVT_TRACE_MAX) {
-            g_evtTrace[g_evtTraceCount++] = evtCode;
-        }
-        // status byte = params[0] for these (Disconnection: status,handle,reason)
-        if (evtCode == EVT_SIMPLE_PAIRING_COMPLETE && evtParamLen >= 1) g_lastSppStatus = params[0];
-        if (evtCode == EVT_AUTH_COMPLETE && evtParamLen >= 1) g_lastAuthStatus = params[0];
-        if (evtCode == EVT_DISCONNECTION_COMPLETE && evtParamLen >= 4) g_lastDiscReason = params[3];
-
         switch (evtCode) {
             case EVT_CONNECTION_COMPLETE: {
                 if (evtParamLen >= 11) {
@@ -1124,31 +1081,6 @@ namespace Drivers::USB::Bluetooth::Hci {
         return true;
     }
 
-    void ResetEventTrace() {
-        g_evtTraceCount = 0;
-        g_lastSppStatus = 0xEE;
-        g_lastAuthStatus = 0xEE;
-        g_lastDiscReason = 0xEE;
-    }
-
-    void DumpEventTrace() {
-        // Top-level only (acquires g_termLock).  Shows the HCI-event sequence so
-        // a stalled pairing/SSP flow is visible, e.g. whether an IO Capability
-        // Request (0x31) / User Confirm (0x33) arrive after a link-key reply, or
-        // the link just goes 0x03 (connect) -> 0x17 (link-key req) -> 0x05
-        // (disconnect/auth-fail) with no pairing in between.
-        KernelLogStream s(INFO, "BT-HCI");
-        s << "event trace:";
-        uint8_t n = g_evtTraceCount;
-        if (n > EVT_TRACE_MAX) n = EVT_TRACE_MAX;
-        for (uint8_t i = 0; i < n; i++) {
-            s << " " << base::hex << (uint64_t)g_evtTrace[i] << base::dec;
-        }
-        s << " | spp=" << base::hex << (uint64_t)g_lastSppStatus
-          << " auth=" << (uint64_t)g_lastAuthStatus
-          << " disc=" << (uint64_t)g_lastDiscReason << base::dec;
-    }
-
     void ProcessPendingCommands() {
         // Top-level only: drains queued pairing replies with real, confirmed
         // control transfers.  Must NOT be called from inside PollEvents.

kernel/src/Drivers/USB/Bluetooth/Hci.hpp

@@ -304,19 +304,11 @@ namespace Drivers::USB::Bluetooth::Hci {
     // remote that holds a stale, un-clearable bond to our real address.
     bool SetBdAddr(const uint8_t* addr);
 
-    // Lockless HCI-event trace (diagnostic).  Reset before a connection attempt,
-    // dump afterwards from top-level to see the pairing/SSP event sequence.
-    void ResetEventTrace();
-    void DumpEventTrace();
-
     // Send any queued pairing replies (IO-cap / user-confirm / link-key) with
     // real confirmed transfers.  Call from top-level (e.g. the connect loop),
     // NOT from an event handler -- event handlers only enqueue.
     void ProcessPendingCommands();
 
-    // Diagnostic: print ACL data-path counters (tx / txDone / rx / pending).
-    void DumpAclStats();
-
     // ACL TX flow control: outstanding (un-acked) ACL packets, and the
     // controller's ACL buffer count (Number-Of-Completed-Packets credits).  The
     // media writer throttles on these so it never overruns the controller.

kernel/src/Drivers/USB/Bluetooth/L2cap.cpp

@@ -31,19 +31,10 @@ namespace Drivers::USB::Bluetooth::L2cap {
     // 3=security-block, 4=no-resources).  0xFFFF = none received yet.
     static volatile uint16_t g_lastConnRspResult = 0xFFFF;
 
-    // Diagnostic: how many incoming AVDTP connection requests the remote has
-    // made (tells us whether the headset wants to initiate the audio channel).
-    static volatile uint16_t g_incomingAvdtpReqs = 0;
-
     // Channel table
     static ChannelInfo g_channels[MAX_CHANNELS] = {};
     static uint16_t g_nextCid = CID_DYNAMIC_START;
 
-    // Signaling response tracking
-    static volatile bool g_sigResponseReady = false;
-    static uint8_t g_sigResponseBuf[64] = {};
-    static uint32_t g_sigResponseLen = 0;
-
     // =========================================================================
     // Helpers
     // =========================================================================
@@ -154,7 +145,6 @@ namespace Drivers::USB::Bluetooth::L2cap {
 
                         // Accept connections for AVDTP, SDP, and AVRCP control
                         if (psm == PSM_AVDTP || psm == PSM_SDP || psm == PSM_AVCTP) {
-                            if (psm == PSM_AVDTP) g_incomingAvdtpReqs++;
                             auto* ch = AllocChannel(psm);
                             if (ch) {
                                 ch->RemoteCid = srcCid;
@@ -565,8 +555,4 @@ namespace Drivers::USB::Bluetooth::L2cap {
         return g_lastConnRspResult;
     }
 
-    uint16_t IncomingAvdtpReqCount() {
-        return g_incomingAvdtpReqs;
-    }
-
 }

kernel/src/Drivers/USB/Bluetooth/L2cap.hpp

@@ -125,13 +125,10 @@ namespace Drivers::USB::Bluetooth::L2cap {
     // Get the ACL handle
     uint16_t GetAclHandle();
 
-    // Diagnostic: result code of the most recent outgoing L2CAP Connection
+    // Result code of the most recent outgoing L2CAP Connection Response
-    // Response (0xFFFF if none yet).  0=success, 1=pending, 2=PSM-unsupported,
+    // (0xFFFF if none yet).  0=success, 1=pending, 2=PSM-unsupported,
-    // 3=security-block, 4=no-resources.
+    // 3=security-block, 4=no-resources.  StartSource() retries only when the
+    // remote ignored the dial entirely (0xFFFF).
     uint16_t LastConnRspResult();
 
-    // Diagnostic: number of incoming AVDTP connection requests seen (whether the
-    // remote wants to initiate the audio channel itself).
-    uint16_t IncomingAvdtpReqCount();
-
 }