From c8d8ed6ebe480fcd04aa91c738638eae26c072ef Mon Sep 17 00:00:00 2001 From: Daniel Hammer Date: Sun, 21 Jun 2026 15:01:37 +0200 Subject: [PATCH] cleanup: Bluetooth/A2DP code cleanup --- kernel/src/Drivers/USB/Bluetooth/A2dp.cpp | 81 ++++-------------- .../src/Drivers/USB/Bluetooth/Bluetooth.cpp | 9 +- kernel/src/Drivers/USB/Bluetooth/Hci.cpp | 84 ++----------------- kernel/src/Drivers/USB/Bluetooth/Hci.hpp | 8 -- kernel/src/Drivers/USB/Bluetooth/L2cap.cpp | 14 ---- kernel/src/Drivers/USB/Bluetooth/L2cap.hpp | 11 +-- 6 files changed, 29 insertions(+), 178 deletions(-) diff --git a/kernel/src/Drivers/USB/Bluetooth/A2dp.cpp b/kernel/src/Drivers/USB/Bluetooth/A2dp.cpp index 8879b73..e736f00 100644 --- a/kernel/src/Drivers/USB/Bluetooth/A2dp.cpp +++ b/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 - // HW symptom: connRsp stays 0xFFFF, the headset never answers). Re-dial - // up to kMaxAttempts. We retry ONLY when the remote ignored us + // L2CAP CONN_REQ that lands right after Encryption Change (connRsp stays + // 0xFFFF, the headset never answers). Re-dial up to kMaxAttempts. We + // 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=" - << (uint64_t)L2cap::IncomingAvdtpReqCount() << ")"; + << base::dec << ")"; // 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 - // and abandoned the very connection the sink was authorizing, while a - // single held dial (build 44) already proved holding alone is harmless. + // NOT tear it down and re-dial -- doing so only churns CIDs and + // abandons the very connection the sink is authorizing; a single held + // 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=" - << (uint64_t)L2cap::IncomingAvdtpReqCount() << ")"; - Hci::DumpAclStats(); + << base::dec << ")"; 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 - // right after the signaling channel; the sink answered PENDING and - // never completed it, so the half-open dial just sat in its - // authorization queue ahead of the media channel. Dialing here - // keeps the bring-up clean and still gives the headset its absolute - // volume path; an inbound AVCTP connect is accepted at any time. + // exists -- the order every phone uses. Dialed before the media + // channel, the sink answers PENDING and never completes it, leaving a + // half-open dial parked in its authorization queue ahead of the media + // channel. Dialing here keeps the bring-up clean and still gives the + // headset its absolute volume path; an inbound AVCTP connect is + // 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) { - // 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 (fill < bytesPerFrame) break; // ring dry, nothing to send 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); diff --git a/kernel/src/Drivers/USB/Bluetooth/Bluetooth.cpp b/kernel/src/Drivers/USB/Bluetooth/Bluetooth.cpp index 649e12b..c0cb3ff 100644 --- a/kernel/src/Drivers/USB/Bluetooth/Bluetooth.cpp +++ b/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 - // (since disproven) stale-bond theory. Removed: the BD_ADDR is an input - // to the SSP authentication confirmation, and if the override only + // NOTE: do NOT override the BD_ADDR via 0xFC31 here. The BD_ADDR is an + // input 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 } diff --git a/kernel/src/Drivers/USB/Bluetooth/Hci.cpp b/kernel/src/Drivers/USB/Bluetooth/Hci.cpp index a7bd571..9ff5697 100644 --- a/kernel/src/Drivers/USB/Bluetooth/Hci.cpp +++ b/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 g_aclPendingCount{0}; - // Diagnostic ACL data-path counters: TX submitted, TX completed (bulk OUT - // completion), RX received (bulk IN). Used to tell whether L2CAP signaling - // actually flows over ACL after encryption is enabled. tx - txDone is also - // the number of TX DMA ring slots still owned by the xHCI (see AclTxReady). + // ACL TX flow-control counters: TX submitted and TX completed (bulk OUT + // completion). tx - txDone is the number of TX DMA ring slots still owned + // by the xHCI (see AclTxReady), so the media writer throttles on it. static std::atomic g_aclTxCount{0}; static std::atomic 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 - // download at 635 KB -- reverted. The harmless firmware-phase bulk-IN runts - // (4-7 byte completions) are filtered at enqueue in TransferCallback so they - // never pollute the RX ring. + // bulk-IN arm moves that cc=4 onto the interrupt IN and wedges the download at + // 635 KB, so the bulk IN must be armed up front. The harmless firmware-phase + // bulk-IN runts (4-7 byte completions) are filtered at enqueue in + // 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. diff --git a/kernel/src/Drivers/USB/Bluetooth/Hci.hpp b/kernel/src/Drivers/USB/Bluetooth/Hci.hpp index 6abbf87..928443f 100644 --- a/kernel/src/Drivers/USB/Bluetooth/Hci.hpp +++ b/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. diff --git a/kernel/src/Drivers/USB/Bluetooth/L2cap.cpp b/kernel/src/Drivers/USB/Bluetooth/L2cap.cpp index 8630bc2..b3e0f73 100644 --- a/kernel/src/Drivers/USB/Bluetooth/L2cap.cpp +++ b/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; - } - } diff --git a/kernel/src/Drivers/USB/Bluetooth/L2cap.hpp b/kernel/src/Drivers/USB/Bluetooth/L2cap.hpp index 5fba285..09c7494 100644 --- a/kernel/src/Drivers/USB/Bluetooth/L2cap.hpp +++ b/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 - // Response (0xFFFF if none yet). 0=success, 1=pending, 2=PSM-unsupported, - // 3=security-block, 4=no-resources. + // Result code of the most recent outgoing L2CAP Connection Response + // (0xFFFF if none yet). 0=success, 1=pending, 2=PSM-unsupported, + // 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(); - }