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/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/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 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) {