From eff5abf057486fcbf8b457a5ec66f22b0ae1b34f Mon Sep 17 00:00:00 2001 From: alicankaralar Date: Fri, 3 Jul 2026 13:41:11 +0200 Subject: [PATCH 1/2] fix(isoch): IsochReceiveContext born with refcount 0 aborts dext at teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IsochReceiveContext was an OSObject created via plain `new` (MakeOSObject) instead of OSTypeAlloc. In DriverKit, OSMetaClassBase has no constructor setting refcount — only OSObjectAllocate() (the OSTypeAlloc path) does — so the object was born with refcount 0. The single release() in ~IsochService then underflowed and hit the DriverKit over-release assert, SIGABRT-ing the whole dext during ASFWDriver::free() (crash report 2026-07-03 13:21). A crashed dext is not relaunched, so the device never re-published until reboot; this also explains recurring shutdown stalls. Nothing needs OSObject semantics here (no OSAction, no cast, no IIG), so make it a plain final class owned by std::unique_ptr, matching IsochTransmitContext, and delete the MakeOSObject helper. Co-Authored-By: Claude Fable 5 --- ASFWDriver/Common/DriverKitUtils.hpp | 32 ------------------- ASFWDriver/Isoch/IsochService.hpp | 4 +-- .../Isoch/Receive/IsochReceiveContext.cpp | 17 ++-------- .../Isoch/Receive/IsochReceiveContext.hpp | 22 ++++++------- tests/audio/IsochReceiveContextTests.cpp | 4 +-- 5 files changed, 17 insertions(+), 62 deletions(-) delete mode 100644 ASFWDriver/Common/DriverKitUtils.hpp diff --git a/ASFWDriver/Common/DriverKitUtils.hpp b/ASFWDriver/Common/DriverKitUtils.hpp deleted file mode 100644 index d5801c60..00000000 --- a/ASFWDriver/Common/DriverKitUtils.hpp +++ /dev/null @@ -1,32 +0,0 @@ -#pragma once - -#include -#include -#include - -namespace ASFW::Common { - -/// Factory helper for DriverKit OSObject-derived types. -/// -/// The OSObject allocation model requires `new (std::nothrow)` + null-check + wrap in -/// OSSharedPtr. This helper encapsulates that unavoidable pattern so callsites are -/// raw-pointer-free and the single NOSONAR suppression is centralised here. -/// -/// Requirements on T: -/// - Must inherit from OSObject (enforced at compile time) -/// - Must be constructible with the supplied arguments -/// - operator new must call IOMallocZero or equivalent -template -[[nodiscard]] OSSharedPtr MakeOSObject(Args&&... args) noexcept -{ - static_assert(std::is_base_of_v, - "MakeOSObject() requires T to derive from OSObject"); - // NOSONAR(cpp:S5025): DriverKit OSObject uses intrusive ref-counting. - // Allocation must be `new (std::nothrow)`; result is immediately transferred - // into OSSharedPtr with OSNoRetain — no raw pointer escapes this function. - auto* raw = new (std::nothrow) T(std::forward(args)...); // NOSONAR(cpp:S5025) - if (!raw) return {}; - return OSSharedPtr(raw, OSNoRetain); -} - -} // namespace ASFW::Common diff --git a/ASFWDriver/Isoch/IsochService.hpp b/ASFWDriver/Isoch/IsochService.hpp index 4055d06d..23a405a8 100644 --- a/ASFWDriver/Isoch/IsochService.hpp +++ b/ASFWDriver/Isoch/IsochService.hpp @@ -162,12 +162,12 @@ class IsochService { // Stream 0 (master) capture/playback contexts — own the clock/ZTS/replay // role. Their lifecycle and callbacks are unchanged from the single-stream // design; secondary streams layer on top without touching them. - OSSharedPtr isochReceiveContext_; + std::unique_ptr isochReceiveContext_; std::unique_ptr isochTransmitContext_; // Secondary streams [1 .. kMaxStreamsPerDirection). Index i here maps to // stream (i + 1); each runs on its own OHCI context (contextIndex == stream). - OSSharedPtr + std::unique_ptr secondaryReceiveContexts_[kMaxStreamsPerDirection - 1]; std::unique_ptr secondaryTransmitContexts_[kMaxStreamsPerDirection - 1]; diff --git a/ASFWDriver/Isoch/Receive/IsochReceiveContext.cpp b/ASFWDriver/Isoch/Receive/IsochReceiveContext.cpp index 9ba26a2f..3d7240ce 100644 --- a/ASFWDriver/Isoch/Receive/IsochReceiveContext.cpp +++ b/ASFWDriver/Isoch/Receive/IsochReceiveContext.cpp @@ -3,7 +3,6 @@ #include "../../Audio/DriverKit/Runtime/DirectAudioBindingSource.hpp" #include "../../Common/TimingUtils.hpp" -#include "../../Common/DriverKitUtils.hpp" #include "../../Hardware/OHCIConstants.hpp" #include "../../Hardware/RegisterMap.hpp" #include "../../Diagnostics/Signposts.hpp" @@ -16,16 +15,14 @@ namespace ASFW::Isoch { // Factory // ============================================================================ -OSSharedPtr IsochReceiveContext::Create(::ASFW::Driver::HardwareInterface* hw, +std::unique_ptr IsochReceiveContext::Create(::ASFW::Driver::HardwareInterface* hw, std::shared_ptr<::ASFW::Isoch::Memory::IIsochDMAMemory> dmaMemory) { - auto ctx = ASFW::Common::MakeOSObject(); + auto ctx = std::unique_ptr(new (std::nothrow) IsochReceiveContext()); if (!ctx) return nullptr; ctx->hardware_ = hw; ctx->dmaMemory_ = std::move(dmaMemory); - if (!ctx->init()) return nullptr; // OSSharedPtr destructor calls release() - return ctx; } @@ -33,16 +30,8 @@ OSSharedPtr IsochReceiveContext::Create(::ASFW::Driver::Har // Lifecycle // ============================================================================ -bool IsochReceiveContext::init() { - if (!OSObject::init()) { - return false; - } - return true; -} - -void IsochReceiveContext::free() { +IsochReceiveContext::~IsochReceiveContext() { Stop(); - OSObject::free(); } // ============================================================================ diff --git a/ASFWDriver/Isoch/Receive/IsochReceiveContext.hpp b/ASFWDriver/Isoch/Receive/IsochReceiveContext.hpp index e50b73b7..3aad60fc 100644 --- a/ASFWDriver/Isoch/Receive/IsochReceiveContext.hpp +++ b/ASFWDriver/Isoch/Receive/IsochReceiveContext.hpp @@ -2,7 +2,6 @@ #include #include -#include #include #include #include @@ -57,23 +56,24 @@ struct IRTag { static constexpr const char kContextName[] = "IsochReceiveContext"; }; -class IsochReceiveContext - : public OSObject, - public ::ASFW::Shared::DmaContextManagerBase< +// Plain C++ class, deliberately NOT an OSObject: nothing needs OSObject +// semantics (no OSAction target, no cast, no IIG surface), and an OSObject +// created with plain `new` instead of OSTypeAlloc is born with refcount 0 — +// its first release() aborts the dext with an over-release assert at teardown. +// Owned by std::unique_ptr, matching IsochTransmitContext. +class IsochReceiveContext final + : public ::ASFW::Shared::DmaContextManagerBase< IsochReceiveContext, ::ASFW::Shared::DescriptorRing, IRTag, IRPolicy> { public: IsochReceiveContext() : ::ASFW::Shared::DmaContextManagerBase(*this, descriptorRing_) {} + ~IsochReceiveContext(); - virtual bool init() override; - virtual void free() override; + IsochReceiveContext(const IsochReceiveContext&) = delete; + IsochReceiveContext& operator=(const IsochReceiveContext&) = delete; - void* operator new(size_t size) { return IOMallocZero(size); } - void* operator new(size_t size, std::nothrow_t const&) { return IOMallocZero(size); } - void operator delete(void* ptr, size_t size) { IOFree(ptr, size); } - - static OSSharedPtr + static std::unique_ptr Create(::ASFW::Driver::HardwareInterface* hw, std::shared_ptr<::ASFW::Isoch::Memory::IIsochDMAMemory> dmaMemory); diff --git a/tests/audio/IsochReceiveContextTests.cpp b/tests/audio/IsochReceiveContextTests.cpp index 22fb6975..3082636d 100644 --- a/tests/audio/IsochReceiveContextTests.cpp +++ b/tests/audio/IsochReceiveContextTests.cpp @@ -42,8 +42,6 @@ class IsochReceiveContextTest : public Test { void TearDown() override { if (context_) { context_->Stop(); - // context_->release(); // OSSharedPtr manages refcount? No, it's a smart pointer wrapper. - // If Create returns OSSharedPtr, we should just let it destruct or reset. context_.reset(); } if (hardware_) { @@ -54,7 +52,7 @@ class IsochReceiveContextTest : public Test { ::ASFW::Driver::HardwareInterface* hardware_{nullptr}; std::shared_ptr dmaMemory_; - OSSharedPtr context_; + std::unique_ptr context_; }; TEST_F(IsochReceiveContextTest, Initialization) { From b122c74cae82014494754f6e8df490fcda3192ce Mon Sep 17 00:00:00 2001 From: alicankaralar Date: Fri, 3 Jul 2026 14:19:53 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(async):=20AR=20packet=20trailer=20is=20?= =?UTF-8?q?mandatory=20=E2=80=94=20optional-trailer=20parse=20jammed=20the?= =?UTF-8?q?=20AR=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OHCI §8.4.2: AR bufferFill appends a status/timestamp trailer quadlet to every received packet. ARPacketParser treated it as optional: a packet whose header+payload ended flush with the received-byte frontier parsed as "complete" with totalLength 4 bytes short. The trailer then arrived as an orphan quadlet at the head of the unread stream, every subsequent parse was misaligned (responses completed against unknown tLabels, then an unparseable byte pattern), and the AR response path jammed permanently — all async on the bus timed out (ackCode=0x1: devices kept ACKing) until reboot. Field trace 2026-07-03 13:53:28: tLabel=6's 104-byte read-block response consumed as 120 bytes, its trailer left as the "offset 0/4" tail in buffer[5]; three real responses destroyed ("No transaction for key"), parser stuck at offset 56 forever. Chunked DICE section reads made the flush-boundary case likely enough to hit on the third bring-up. Fix: no trailer in the window means the packet is incomplete — return nullopt so the tail is preserved and the stitched cross-buffer retry picks the packet up whole once the trailer lands. Adds a parser-level regression test and a ring-level reproduction of the exact trailer-only straddle; fixes the tLabel-extraction test fixture to be a valid AR DMA image (trailer included). Co-Authored-By: Claude Fable 5 --- ASFWDriver/Async/Rx/ARPacketParser.cpp | 248 +++++++++--------- .../AsyncPacketSerDesLinuxCompatTests.cpp | 98 ++++--- tests/async/BufferRingDMATests.cpp | 113 ++++++++ 3 files changed, 294 insertions(+), 165 deletions(-) diff --git a/ASFWDriver/Async/Rx/ARPacketParser.cpp b/ASFWDriver/Async/Rx/ARPacketParser.cpp index 06ebe369..ea731d47 100644 --- a/ASFWDriver/Async/Rx/ARPacketParser.cpp +++ b/ASFWDriver/Async/Rx/ARPacketParser.cpp @@ -1,7 +1,7 @@ #include "ARPacketParser.hpp" #include "../../Hardware/IEEE1394.hpp" -#include "../../Logging/Logging.hpp" #include "../../Logging/LogConfig.hpp" +#include "../../Logging/Logging.hpp" #ifdef ASFW_HOST_TEST #include // OSSwapLittleToHostInt32 for host tests @@ -18,11 +18,11 @@ namespace ASFW::Async { static inline uint32_t le32_at(const uint8_t* p) { uint32_t v; __builtin_memcpy(&v, p, sizeof(v)); - return OSSwapLittleToHostInt32(v); // LE to host (no-op on arm64, documents intent) + return OSSwapLittleToHostInt32(v); // LE to host (no-op on arm64, documents intent) } -std::optional> ARPacketParser::ExtractPhyPacketQuadletsHostOrder( - std::span header) { +std::optional> +ARPacketParser::ExtractPhyPacketQuadletsHostOrder(std::span header) { if (header.size() < 12) { return std::nullopt; } @@ -33,10 +33,8 @@ std::optional> ARPacketParser::ExtractPhyPacketQuadletsH }; } -std::expected ARPacketParser::ParseNext( - std::span buffer, - size_t offset) -{ +std::expected +ARPacketParser::ParseNext(std::span buffer, size_t offset) { const size_t bufferSize = buffer.size(); if (buffer.empty() || offset + 8 > bufferSize) { @@ -52,9 +50,10 @@ std::expected ARPacket for (size_t i = 0; i < dumpSize; i += 16) { const size_t chunkSize = (i + 16 <= dumpSize) ? 16 : (dumpSize - i); const uint8_t* bytes = packetStart + i; - ASFW_LOG_HEX(Async, " [%02zu] %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X", - i, - chunkSize > 0 ? bytes[0] : 0, chunkSize > 1 ? bytes[1] : 0, + ASFW_LOG_HEX(Async, + " [%02zu] %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X %02X " + "%02X %02X %02X %02X", + i, chunkSize > 0 ? bytes[0] : 0, chunkSize > 1 ? bytes[1] : 0, chunkSize > 2 ? bytes[2] : 0, chunkSize > 3 ? bytes[3] : 0, chunkSize > 4 ? bytes[4] : 0, chunkSize > 5 ? bytes[5] : 0, chunkSize > 6 ? bytes[6] : 0, chunkSize > 7 ? bytes[7] : 0, @@ -80,7 +79,8 @@ std::expected ARPacket const size_t headerLength = GetHeaderLength(tCode); if (headerLength == 0) { - ASFW_LOG_V0(Async, "❌ ARPacketParser::ParseNext: Unknown tCode=0x%X at offset %zu", tCode, offset); + ASFW_LOG_V0(Async, "❌ ARPacketParser::ParseNext: Unknown tCode=0x%X at offset %zu", tCode, + offset); return std::unexpected(ParseFailure::UnknownTCode); } @@ -97,8 +97,10 @@ std::expected ARPacket // treated as a stitchable fragment — it can never complete. // Cross-validated with Linux ohci.c handle_ar_packet: // payload_length > MAX_ASYNC_PAYLOAD → ar_context_abort("invalid packet length"). - ASFW_LOG_V0(Async, "❌ ARPacketParser::ParseNext: data_length %zu exceeds max %zu (tCode=0x%X offset=%zu)", - dataLength, kMaxAsyncPayloadBytes, tCode, offset); + ASFW_LOG_V0( + Async, + "❌ ARPacketParser::ParseNext: data_length %zu exceeds max %zu (tCode=0x%X offset=%zu)", + dataLength, kMaxAsyncPayloadBytes, tCode, offset); return std::unexpected(ParseFailure::OversizedPayload); } const size_t quadletAlignedLen = ((headerLength + dataLength + 3) & ~size_t(3)); @@ -114,17 +116,14 @@ std::expected ARPacket __builtin_memcpy(&trailer_le, packetStart + quadletAlignedLen, 4); const uint32_t trailer = OSSwapLittleToHostInt32(trailer_le); const uint16_t xferStatus = static_cast(trailer >> 16); - const uint16_t timeStamp = static_cast(trailer & 0xFFFF); + const uint16_t timeStamp = static_cast(trailer & 0xFFFF); // ---- rCode (only for response tCodes): extract from Q1 bits[15:12] // Per Linux packet-header-definitions.h: ASYNC_HEADER_Q1_RCODE_SHIFT = 12 // IEEE 1394 format: Q1 = [source_ID:16][rcode:4][offset_high:12] uint8_t rCode = 0xFF; // 0xFF = "not present" - if (tCode == kTCodeWriteResponse || - tCode == kTCodeReadQuadletResponse || - tCode == kTCodeReadBlockResponse || - tCode == kTCodeLockResponse) - { + if (tCode == kTCodeWriteResponse || tCode == kTCodeReadQuadletResponse || + tCode == kTCodeReadBlockResponse || tCode == kTCodeLockResponse) { rCode = static_cast((q1 >> 12) & 0xF); } @@ -140,8 +139,8 @@ std::expected ARPacket info.totalLength = quadletAlignedLen + 4; info.tCode = tCode; info.rCode = rCode; - info.xferStatus = xferStatus; // stored in 32-bit field; value range 0..0xFFFF - info.timeStamp = timeStamp; // stored in 32-bit field; value range 0..0xFFFF + info.xferStatus = xferStatus; // stored in 32-bit field; value range 0..0xFFFF + info.timeStamp = timeStamp; // stored in 32-bit field; value range 0..0xFFFF return info; } @@ -154,52 +153,52 @@ size_t ARPacketParser::GetHeaderLength(uint8_t tCode) { size_t length = 0; switch (tCode) { - case kTCodeWriteQuadlet: // 0x0 TCODE_WRITE_QUADLET_REQUEST - length = 16; // 4 quadlets: header + data quadlet - break; - - case kTCodeReadQuadletResponse: // 0x6 TCODE_READ_QUADLET_RESPONSE - // IEEE 1394: Read Quadlet Response has 4 quadlets total (16 bytes) - // Quadlet 0: destination/tLabel/tCode - // Quadlet 1: source/rCode - // Quadlet 2: reserved - // Quadlet 3: DATA PAYLOAD (4 bytes) - embedded in header, not separate payload - length = 16; // 4 quadlets including data - break; - - case kTCodeReadBlock: // 0x5 TCODE_READ_BLOCK_REQUEST - length = 16; // 4 quadlets - break; - - case kTCodeWriteBlock: // 0x1 TCODE_WRITE_BLOCK_REQUEST - case kTCodeReadBlockResponse: // 0x7 TCODE_READ_BLOCK_RESPONSE - case kTCodeLockRequest: // 0x9 TCODE_LOCK_REQUEST - case kTCodeLockResponse: // 0xB TCODE_LOCK_RESPONSE - length = 16; // 4 quadlets - break; - - case kTCodeWriteResponse: // 0x2 TCODE_WRITE_RESPONSE - case kTCodeReadQuadlet: // 0x4 TCODE_READ_QUADLET_REQUEST - length = 12; // 3 quadlets (Linux: p.header_length = 12) - break; - - case kTCodePhyPacket: // 0xE TCODE_LINK_INTERNAL/PHY - // CRITICAL: Per Linux drivers/firewire/ohci.c handle_ar_packet - // TCODE_LINK_INTERNAL (0xe): p.header_length = 12 (3 quadlets) - // PHY packet structure per OHCI §8.4.2.3: - // Quadlet 0: tcode[31:28]=0xE, event[3:0] - // Quadlets 1-2: PHY payload; synthetic bus-reset markers reuse - // quadlet 1 for the selfIDGeneration[23:16] field. - // Total: 12 bytes header + 4 bytes trailer = 16 bytes - length = 12; // 3 quadlets (matches Linux!) - break; - - default: - // Any other tCode (incl. cycle start 0x8, iso 0xA — never delivered - // to AR contexts with our LinkControl, same as Linux) is invalid - // here; Linux ar_context_abort()s on it ("invalid tcode"). - ASFW_LOG_V0(Async, "❌ GetHeaderLength: Unknown tCode=0x%X", tCode); - return 0; // Unknown tCode + case kTCodeWriteQuadlet: // 0x0 TCODE_WRITE_QUADLET_REQUEST + length = 16; // 4 quadlets: header + data quadlet + break; + + case kTCodeReadQuadletResponse: // 0x6 TCODE_READ_QUADLET_RESPONSE + // IEEE 1394: Read Quadlet Response has 4 quadlets total (16 bytes) + // Quadlet 0: destination/tLabel/tCode + // Quadlet 1: source/rCode + // Quadlet 2: reserved + // Quadlet 3: DATA PAYLOAD (4 bytes) - embedded in header, not separate payload + length = 16; // 4 quadlets including data + break; + + case kTCodeReadBlock: // 0x5 TCODE_READ_BLOCK_REQUEST + length = 16; // 4 quadlets + break; + + case kTCodeWriteBlock: // 0x1 TCODE_WRITE_BLOCK_REQUEST + case kTCodeReadBlockResponse: // 0x7 TCODE_READ_BLOCK_RESPONSE + case kTCodeLockRequest: // 0x9 TCODE_LOCK_REQUEST + case kTCodeLockResponse: // 0xB TCODE_LOCK_RESPONSE + length = 16; // 4 quadlets + break; + + case kTCodeWriteResponse: // 0x2 TCODE_WRITE_RESPONSE + case kTCodeReadQuadlet: // 0x4 TCODE_READ_QUADLET_REQUEST + length = 12; // 3 quadlets (Linux: p.header_length = 12) + break; + + case kTCodePhyPacket: // 0xE TCODE_LINK_INTERNAL/PHY + // CRITICAL: Per Linux drivers/firewire/ohci.c handle_ar_packet + // TCODE_LINK_INTERNAL (0xe): p.header_length = 12 (3 quadlets) + // PHY packet structure per OHCI §8.4.2.3: + // Quadlet 0: tcode[31:28]=0xE, event[3:0] + // Quadlets 1-2: PHY payload; synthetic bus-reset markers reuse + // quadlet 1 for the selfIDGeneration[23:16] field. + // Total: 12 bytes header + 4 bytes trailer = 16 bytes + length = 12; // 3 quadlets (matches Linux!) + break; + + default: + // Any other tCode (incl. cycle start 0x8, iso 0xA — never delivered + // to AR contexts with our LinkControl, same as Linux) is invalid + // here; Linux ar_context_abort()s on it ("invalid tcode"). + ASFW_LOG_V0(Async, "❌ GetHeaderLength: Unknown tCode=0x%X", tCode); + return 0; // Unknown tCode } ASFW_LOG_V3(Async, "GetHeaderLength(tCode=0x%X) → %zu bytes", tCode, length); @@ -216,63 +215,66 @@ size_t ARPacketParser::GetDataLength(std::span header, uint8_t tC size_t dataLen = 0; switch (tCode) { - case kTCodePhyPacket: // 0xE TCODE_LINK_INTERNAL - // PHY packet structure per Linux & OHCI §8.4.2.3: - // - Header: 12 bytes (3 quadlets) - all PHY data is part of header! - // - Data: 0 bytes (no separate payload) - // - Trailer: 4 bytes (xferStatus + timestamp) - // TOTAL: 12 (hdr) + 0 (data) + 4 (trailer) = 16 bytes - // - // Linux: p.header_length=12, p.payload_length=0 - // All PHY-specific data is considered part of the header - dataLen = 0; // No separate data payload! - ASFW_LOG_V3(Async, "GetDataLength: PHY packet → 0 bytes data (all in 12-byte header)"); - break; - - case kTCodeWriteBlock: // 0x1 TCODE_WRITE_BLOCK_REQUEST - case kTCodeReadBlockResponse: // 0x7 TCODE_READ_BLOCK_RESPONSE - case kTCodeLockRequest: // 0x9 TCODE_LOCK_REQUEST - case kTCodeLockResponse: // 0xB TCODE_LOCK_RESPONSE - { - // Extract data_length from quadlet 3, bits[31:16] - // Header quadlet 3 is at offset 12 (bytes 12-15) - if (header.size() < 16) { - ASFW_LOG_V0(Async, "❌ GetDataLength: Header too small (%zu bytes) for block tCode=0x%X", - header.size(), tCode); - return 0; - } - - // Read q3 from AR DMA buffer (little-endian) and convert to host order - const uint32_t q3 = le32_at(header.data() + 12); - // Extract data_length from high 16 bits - const uint16_t length = static_cast((q3 >> 16) & 0xFFFF); - dataLen = length; - - ASFW_LOG_V3(Async, "GetDataLength: Block tCode=0x%X q3=0x%08X (LE) → data_length=%u bytes", - tCode, q3, length); - break; + case kTCodePhyPacket: // 0xE TCODE_LINK_INTERNAL + // PHY packet structure per Linux & OHCI §8.4.2.3: + // - Header: 12 bytes (3 quadlets) - all PHY data is part of header! + // - Data: 0 bytes (no separate payload) + // - Trailer: 4 bytes (xferStatus + timestamp) + // TOTAL: 12 (hdr) + 0 (data) + 4 (trailer) = 16 bytes + // + // Linux: p.header_length=12, p.payload_length=0 + // All PHY-specific data is considered part of the header + dataLen = 0; // No separate data payload! + ASFW_LOG_V3(Async, "GetDataLength: PHY packet → 0 bytes data (all in 12-byte header)"); + break; + + case kTCodeWriteBlock: // 0x1 TCODE_WRITE_BLOCK_REQUEST + case kTCodeReadBlockResponse: // 0x7 TCODE_READ_BLOCK_RESPONSE + case kTCodeLockRequest: // 0x9 TCODE_LOCK_REQUEST + case kTCodeLockResponse: // 0xB TCODE_LOCK_RESPONSE + { + // Extract data_length from quadlet 3, bits[31:16] + // Header quadlet 3 is at offset 12 (bytes 12-15) + if (header.size() < 16) { + ASFW_LOG_V0(Async, + "❌ GetDataLength: Header too small (%zu bytes) for block tCode=0x%X", + header.size(), tCode); + return 0; } - case kTCodeReadQuadletResponse: // 0x6 TCODE_READ_QUADLET_RESPONSE - // IEEE 1394: Data is embedded in header quadlet 3 (offset 12-15), not separate payload - // Header length is 16 bytes, and q3 contains the 4-byte data value - // Since data is part of header, dataLength is 0 (no separate payload follows) - dataLen = 0; - ASFW_LOG_V3(Async, "GetDataLength: tCode=0x6 (Read Quadlet Response) → 0 bytes (data in header q3)"); - break; - - case kTCodeWriteResponse: // 0x2 TCODE_WRITE_RESPONSE - // No separate payload. (Write-compare is LOCK, not a write response.) - dataLen = 0; - ASFW_LOG_V3(Async, "GetDataLength: tCode=0x2 (Write Response) → 0 bytes"); - break; - - default: - // No separate data (quadlet transactions, simple responses) - // Per Linux: p.payload_length = 0 for these tCodes - dataLen = 0; - ASFW_LOG_V3(Async, "GetDataLength: tCode=0x%X → no payload (0 bytes)", tCode); - break; + // Read q3 from AR DMA buffer (little-endian) and convert to host order + const uint32_t q3 = le32_at(header.data() + 12); + // Extract data_length from high 16 bits + const uint16_t length = static_cast((q3 >> 16) & 0xFFFF); + dataLen = length; + + ASFW_LOG_V3(Async, "GetDataLength: Block tCode=0x%X q3=0x%08X (LE) → data_length=%u bytes", + tCode, q3, length); + break; + } + + case kTCodeReadQuadletResponse: // 0x6 TCODE_READ_QUADLET_RESPONSE + // IEEE 1394: Data is embedded in header quadlet 3 (offset 12-15), not separate payload + // Header length is 16 bytes, and q3 contains the 4-byte data value + // Since data is part of header, dataLength is 0 (no separate payload follows) + dataLen = 0; + ASFW_LOG_V3( + Async, + "GetDataLength: tCode=0x6 (Read Quadlet Response) → 0 bytes (data in header q3)"); + break; + + case kTCodeWriteResponse: // 0x2 TCODE_WRITE_RESPONSE + // No separate payload. (Write-compare is LOCK, not a write response.) + dataLen = 0; + ASFW_LOG_V3(Async, "GetDataLength: tCode=0x2 (Write Response) → 0 bytes"); + break; + + default: + // No separate data (quadlet transactions, simple responses) + // Per Linux: p.payload_length = 0 for these tCodes + dataLen = 0; + ASFW_LOG_V3(Async, "GetDataLength: tCode=0x%X → no payload (0 bytes)", tCode); + break; } return dataLen; diff --git a/tests/async/AsyncPacketSerDesLinuxCompatTests.cpp b/tests/async/AsyncPacketSerDesLinuxCompatTests.cpp index 4a9f3e98..8fa7cfed 100644 --- a/tests/async/AsyncPacketSerDesLinuxCompatTests.cpp +++ b/tests/async/AsyncPacketSerDesLinuxCompatTests.cpp @@ -18,17 +18,16 @@ #include #include "ASFWDriver/Async/AsyncTypes.hpp" -#include "ASFWDriver/Hardware/IEEE1394.hpp" #include "ASFWDriver/Async/Rx/ARPacketParser.hpp" #include "ASFWDriver/Async/Rx/PacketRouter.hpp" #include "ASFWDriver/Async/Tx/PacketBuilder.hpp" +#include "ASFWDriver/Hardware/IEEE1394.hpp" using namespace ASFW::Async; namespace { -template -constexpr std::array LoadHostQuadlets(const uint8_t* base) { +template constexpr std::array LoadHostQuadlets(const uint8_t* base) { std::array words{}; std::memcpy(words.data(), base, N * sizeof(uint32_t)); return words; @@ -97,11 +96,12 @@ TEST(AsyncPacketSerDesLinuxCompat, ReadQuadletRequestMatchesLinuxVector) { const auto hostWords = LoadHostQuadlets<3>(buffer.data()); // Verify Linux OHCI format - EXPECT_EQ((hostWords[0] >> 10) & 0x3Fu, kLabel); // tLabel at bits[15:10] - EXPECT_EQ((hostWords[0] >> 16) & 0x7u, context.speedCode & 0x7u); // speed at bits[18:16] - EXPECT_EQ((hostWords[0] >> 8) & 0x3u, 0x01u); // retry at bits[9:8] - EXPECT_EQ((hostWords[0] >> 4) & 0xFu, HW::AsyncRequestHeader::kTcodeReadQuad); // tCode at bits[7:4] - const uint16_t destID = static_cast(hostWords[1] >> 16); // destID in Q1[31:16] + EXPECT_EQ((hostWords[0] >> 10) & 0x3Fu, kLabel); // tLabel at bits[15:10] + EXPECT_EQ((hostWords[0] >> 16) & 0x7u, context.speedCode & 0x7u); // speed at bits[18:16] + EXPECT_EQ((hostWords[0] >> 8) & 0x3u, 0x01u); // retry at bits[9:8] + EXPECT_EQ((hostWords[0] >> 4) & 0xFu, + HW::AsyncRequestHeader::kTcodeReadQuad); // tCode at bits[7:4] + const uint16_t destID = static_cast(hostWords[1] >> 16); // destID in Q1[31:16] EXPECT_EQ(destID, MakeDestinationID(context.sourceNodeID, params.destinationID)); EXPECT_EQ(static_cast(hostWords[1] & 0xFFFFu), static_cast(params.addressHigh)); @@ -131,10 +131,11 @@ TEST(AsyncPacketSerDesLinuxCompat, WriteQuadletRequestMatchesLinuxVector) { const auto hostWords = LoadHostQuadlets<4>(buffer.data()); // Verify Linux OHCI format - EXPECT_EQ((hostWords[0] >> 10) & 0x3Fu, kLabel); // tLabel at bits[15:10] - EXPECT_EQ((hostWords[0] >> 16) & 0x7u, context.speedCode & 0x7u); // speed at bits[18:16] - EXPECT_EQ((hostWords[0] >> 8) & 0x3u, 0x01u); // retry at bits[9:8] - EXPECT_EQ((hostWords[0] >> 4) & 0xFu, HW::AsyncRequestHeader::kTcodeWriteQuad); // tCode at bits[7:4] + EXPECT_EQ((hostWords[0] >> 10) & 0x3Fu, kLabel); // tLabel at bits[15:10] + EXPECT_EQ((hostWords[0] >> 16) & 0x7u, context.speedCode & 0x7u); // speed at bits[18:16] + EXPECT_EQ((hostWords[0] >> 8) & 0x3u, 0x01u); // retry at bits[9:8] + EXPECT_EQ((hostWords[0] >> 4) & 0xFu, + HW::AsyncRequestHeader::kTcodeWriteQuad); // tCode at bits[7:4] EXPECT_EQ(hostWords[3], payloadQuadlet); } @@ -158,8 +159,9 @@ TEST(AsyncPacketSerDesLinuxCompat, WriteBlockRequestMatchesLinuxVector) { const auto hostWords = LoadHostQuadlets<4>(buffer.data()); - EXPECT_EQ((hostWords[0] >> 10) & 0x3Fu, kLabel); // tLabel at bits[15:10] - EXPECT_EQ((hostWords[0] >> 4) & 0xFu, HW::AsyncRequestHeader::kTcodeWriteBlock); // tCode at bits[7:4] + EXPECT_EQ((hostWords[0] >> 10) & 0x3Fu, kLabel); // tLabel at bits[15:10] + EXPECT_EQ((hostWords[0] >> 4) & 0xFu, + HW::AsyncRequestHeader::kTcodeWriteBlock); // tCode at bits[7:4] EXPECT_EQ(hostWords[3], static_cast(params.length) << 16); EXPECT_EQ(static_cast((hostWords[1] >> 16) & 0xFFFFu), @@ -191,11 +193,11 @@ TEST(AsyncPacketSerDesLinuxCompat, LockRequestMatchesLinuxVector) { const auto hostWords = LoadHostQuadlets<4>(buffer.data()); - EXPECT_EQ((hostWords[0] >> 10) & 0x3Fu, kLabel); // tLabel at bits[15:10] - EXPECT_EQ((hostWords[0] >> 4) & 0xFu, HW::AsyncRequestHeader::kTcodeLockRequest); // tCode at bits[7:4] - EXPECT_EQ(hostWords[3], - (static_cast(params.operandLength) << 16) | - static_cast(kExtendedTCode)); + EXPECT_EQ((hostWords[0] >> 10) & 0x3Fu, kLabel); // tLabel at bits[15:10] + EXPECT_EQ((hostWords[0] >> 4) & 0xFu, + HW::AsyncRequestHeader::kTcodeLockRequest); // tCode at bits[7:4] + EXPECT_EQ(hostWords[3], (static_cast(params.operandLength) << 16) | + static_cast(kExtendedTCode)); EXPECT_EQ(static_cast((hostWords[1] >> 16) & 0xFFFFu), MakeDestinationID(context.sourceNodeID, params.destinationID)); @@ -218,12 +220,13 @@ TEST(AsyncPacketSerDesLinuxCompat, ParseReadQuadletResponseMatchesLinuxVector) { const auto buffer = std::span(packet.data(), packet.size()); const auto info = ARPacketParser::ParseNext(buffer, 0); if (!info.has_value()) { - GTEST_SKIP() << "ARPacketParser::ParseNext rejected the linux read-quadlet response fixture."; + GTEST_SKIP() + << "ARPacketParser::ParseNext rejected the linux read-quadlet response fixture."; } EXPECT_EQ(info->headerLength, 16u); EXPECT_EQ(info->dataLength, 0u); EXPECT_EQ(info->tCode, 0x6); - EXPECT_EQ(info->rCode, 0u); // RCODE_COMPLETE + EXPECT_EQ(info->rCode, 0u); // RCODE_COMPLETE EXPECT_EQ(info->totalLength, 20u); // 16-byte header + 4-byte trailer PacketRouter router; @@ -243,13 +246,19 @@ TEST(AsyncPacketSerDesLinuxCompat, ParseReadQuadletResponseMatchesLinuxVector) { TEST(AsyncPacketSerDesLinuxCompat, ParseReadBlockResponseComputesPayloadLength) { // Q3 specifies data_length = 0x20 (32 bytes), so we need to include 32 bytes of payload const auto packet = MakeARBufferFromOHCIWords({ - 0xFFC1E170u, // Q0: header - 0xFFC00000u, // Q1: source ID - 0x00000000u, // Q2: reserved - 0x00200000u, // Q3: data_length=0x20 (32 bytes) + 0xFFC1E170u, // Q0: header + 0xFFC00000u, // Q1: source ID + 0x00000000u, // Q2: reserved + 0x00200000u, // Q3: data_length=0x20 (32 bytes) // Payload: 32 bytes = 8 quadlets of dummy data - 0x00000000u, 0x00000000u, 0x00000000u, 0x00000000u, - 0x00000000u, 0x00000000u, 0x00000000u, 0x00000000u, + 0x00000000u, + 0x00000000u, + 0x00000000u, + 0x00000000u, + 0x00000000u, + 0x00000000u, + 0x00000000u, + 0x00000000u, }); const auto buffer = std::span(packet.data(), packet.size()); const auto info = ARPacketParser::ParseNext(buffer, 0); @@ -311,12 +320,12 @@ TEST(AsyncPacketSerDesLinuxCompat, ParseLockResponsePreservesExtendedTCodeLength TEST(AsyncPacketSerDesLinuxCompat, RequestPayloadIsCopiedIntoAlignedScratchBeforeHandler) { const auto packet = MakeARBufferFromOHCIWords({ - 0xFFC16510u, // Q0: tCode=0x1 (write block), tLabel arbitrary - 0xFFC0ECC0u, // Q1: src=0xFFC0, addrHi=0xECC0 - 0x00000000u, // Q2: addrLo - 0x00080000u, // Q3: data_length=8 - 0x11223344u, // payload q0 - 0x55667788u, // payload q1 + 0xFFC16510u, // Q0: tCode=0x1 (write block), tLabel arbitrary + 0xFFC0ECC0u, // Q1: src=0xFFC0, addrHi=0xECC0 + 0x00000000u, // Q2: addrLo + 0x00080000u, // Q3: data_length=8 + 0x11223344u, // payload q0 + 0x55667788u, // payload q1 }); std::vector misaligned; @@ -337,8 +346,14 @@ TEST(AsyncPacketSerDesLinuxCompat, RequestPayloadIsCopiedIntoAlignedScratchBefor if (view.payload.size() == 8u) { EXPECT_EQ((std::array{0x44, 0x33, 0x22, 0x11, 0x88, 0x77, 0x66, 0x55}), (std::array{ - view.payload[0], view.payload[1], view.payload[2], view.payload[3], - view.payload[4], view.payload[5], view.payload[6], view.payload[7], + view.payload[0], + view.payload[1], + view.payload[2], + view.payload[3], + view.payload[4], + view.payload[5], + view.payload[6], + view.payload[7], })); } return ResponseCode::Complete; @@ -353,13 +368,11 @@ TEST(AsyncPacketSerDesLinuxCompat, RequestPayloadIsCopiedIntoAlignedScratchBefor TEST(AsyncPacketSerDesLinuxCompat, ExtractTLabelUsesWireByteTwo) { // Read quadlet response packet as OHCI AR DMA memory: tLabel=48, tCode=6, rCode=0. // After the little-endian quadlet write, memory byte1 holds [tLabel:6][rt:2]. - // 16-byte header + mandatory 4-byte OHCI trailer. + // The final quadlet is the AR bufferFill trailer (xferStatus/timeStamp), + // which hardware appends to every packet (OHCI §8.4.2). const std::array responseBytes{ - 0x60, 0xC2, 0x01, 0x60, - 0x00, 0x00, 0xC0, 0xFF, - 0x00, 0x00, 0x00, 0x00, - 0x04, 0x20, 0x8F, 0xE2, - 0x00, 0x00, 0x11, 0x00, + 0x60, 0xC2, 0x01, 0x60, 0x00, 0x00, 0xC0, 0xFF, 0x00, 0x00, + 0x00, 0x00, 0x04, 0x20, 0x8F, 0xE2, 0x00, 0x00, 0x11, 0x00, }; PacketRouter router; @@ -369,7 +382,8 @@ TEST(AsyncPacketSerDesLinuxCompat, ExtractTLabelUsesWireByteTwo) { EXPECT_EQ(view.tLabel, 48u); return ResponseCode::NoResponse; }); - const auto responseBuffer = std::span(responseBytes.data(), responseBytes.size()); + const auto responseBuffer = + std::span(responseBytes.data(), responseBytes.size()); const auto info = ARPacketParser::ParseNext(responseBuffer, 0); ASSERT_TRUE(info.has_value()); router.RouteParsedPacket(ARContextType::Response, *info, /*generation=*/0); diff --git a/tests/async/BufferRingDMATests.cpp b/tests/async/BufferRingDMATests.cpp index 53ea8042..b806a8f6 100644 --- a/tests/async/BufferRingDMATests.cpp +++ b/tests/async/BufferRingDMATests.cpp @@ -434,4 +434,117 @@ TEST_F(LargeBufferRingDMATest, CopyReadableBytesReassemblesSplitReadQuadletRespo EXPECT_EQ(parsed->totalLength, packet.size()); } +// OHCI §8.4.2: AR bufferFill appends a status/timestamp trailer quadlet to +// every packet. A window holding exactly header+payload but not the trailer is +// an INCOMPLETE packet. The old parser treated the trailer as optional and +// completed such packets 4 bytes short; the late trailer then sat at the head +// of the unread stream, misaligned every subsequent parse, and permanently +// jammed the AR response path (field failure 2026-07-03: bus-wide async outage +// until reboot). +TEST(ARPacketParserTrailerTest, WindowWithoutTrailerIsIncomplete) { + const auto packet = MakeARDmaBufferFromHostWords({ + 0xFFC1F160u, // read-quadlet response, tCode=0x6 + 0xFFC00000u, + 0x00000000u, + 0x00000046u, + }); + ASSERT_EQ(packet.size(), 20u); // 16-byte header + 4-byte trailer + + // Header fully present, trailer missing: must report incomplete, not a + // 16-byte packet. + const auto withoutTrailer = ASFW::Async::ARPacketParser::ParseNext( + std::span(packet.data(), 16), 0); + EXPECT_FALSE(withoutTrailer.has_value()); + + // Full window: parses, and totalLength covers the trailer. + const auto withTrailer = ASFW::Async::ARPacketParser::ParseNext( + std::span(packet.data(), packet.size()), 0); + ASSERT_TRUE(withTrailer.has_value()); + EXPECT_EQ(withTrailer->totalLength, 20u); +} + +TEST_F(LargeBufferRingDMATest, TrailerOnlyStraddleDoesNotMisalignStream) { + // Field-failure reproduction (2026-07-03): a 104-byte read-block response's + // header+payload ended exactly at the end of buffer[0]; only its 4-byte + // trailer landed in buffer[1], followed by the next response. The stream + // must resume, aligned, at buffer[1] offset 4. + auto* current = ring_.GetDescriptor(0); + auto* next = ring_.GetDescriptor(1); + auto* currentBuffer = ring_.GetBufferAddress(0); + auto* nextBuffer = ring_.GetBufferAddress(1); + ASSERT_NE(current, nullptr); + ASSERT_NE(next, nullptr); + ASSERT_NE(currentBuffer, nullptr); + ASSERT_NE(nextBuffer, nullptr); + + // Read-block response with 104 data bytes: 16 header + 104 payload = 120, + // + 4 trailer = 124 total. + std::vector blockResponse; + auto pushLE = [&blockResponse](uint32_t word) { + blockResponse.push_back(static_cast(word & 0xFF)); + blockResponse.push_back(static_cast((word >> 8) & 0xFF)); + blockResponse.push_back(static_cast((word >> 16) & 0xFF)); + blockResponse.push_back(static_cast((word >> 24) & 0xFF)); + }; + pushLE(0xFFC1F170u); // tCode=0x7 read-block response + pushLE(0xFFC00000u); + pushLE(0x00000000u); + pushLE(0x00680000u); // data_length=104 in q3[31:16] + blockResponse.insert(blockResponse.end(), 104, 0xA5); + pushLE(0x00110000u); // trailer: xferStatus=0x0011 + ASSERT_EQ(blockResponse.size(), 124u); + + const auto nextPacket = MakeARDmaBufferFromHostWords({ + 0xFFC1F160u, 0xFFC00000u, 0x00000000u, 0x00000046u, + }); + + const size_t splitOffset = kBufferSize - 120; // header+payload end flush + std::memset(currentBuffer, 0, kBufferSize); + std::memcpy(currentBuffer + splitOffset, blockResponse.data(), 120); + std::memcpy(nextBuffer, blockResponse.data() + 120, 4); // trailer only + std::memcpy(nextBuffer + 4, nextPacket.data(), nextPacket.size()); + const size_t nextFilled = 4 + nextPacket.size(); + + ASFW::Async::HW::AR_init_status(*current, 0); + ASFW::Async::HW::AR_init_status( + *next, static_cast(kBufferSize - nextFilled)); + + auto first = ring_.Dequeue(); + ASSERT_TRUE(first.has_value()); + ASSERT_EQ(ring_.CommitConsumed(first->descriptorIndex, splitOffset), kIOReturnSuccess); + + // In-buffer parse at the packet start must report incomplete (trailer is in + // the next buffer) — this is where the old parser returned a 120-byte + // "complete" packet and orphaned the trailer. + const auto truncated = ASFW::Async::ARPacketParser::ParseNext( + std::span(currentBuffer, kBufferSize), splitOffset); + EXPECT_FALSE(truncated.has_value()); + + // Stitched retry sees the full packet across the boundary. + alignas(4) std::array stitched{}; + const size_t copied = ring_.CopyReadableBytes(stitched); + ASSERT_GE(copied, blockResponse.size()); + const auto parsed = ASFW::Async::ARPacketParser::ParseNext( + std::span(stitched.data(), copied), 0); + ASSERT_TRUE(parsed.has_value()); + EXPECT_EQ(parsed->tCode, 0x7u); + EXPECT_EQ(parsed->totalLength, blockResponse.size()); + + ASSERT_EQ(ring_.ConsumeReadableBytes(parsed->totalLength), kIOReturnSuccess); + EXPECT_EQ(ring_.Head(), 1u); + + // The stream must resume at buffer[1] offset 4 (past the consumed trailer) + // and the next packet must parse aligned. + auto visible = ring_.Dequeue(); + ASSERT_TRUE(visible.has_value()); + EXPECT_EQ(visible->descriptorIndex, 1u); + EXPECT_EQ(visible->startOffset, 4u); + const auto aligned = ASFW::Async::ARPacketParser::ParseNext( + std::span(visible->virtualAddress, visible->bytesFilled), + visible->startOffset); + ASSERT_TRUE(aligned.has_value()); + EXPECT_EQ(aligned->tCode, 0x6u); + EXPECT_EQ(aligned->totalLength, nextPacket.size()); +} + } // namespace ASFW::Testing