From 45f128dbe3d7da603d67239759b2f181710e7ac0 Mon Sep 17 00:00:00 2001 From: Bruno Batarelo Date: Thu, 22 Oct 2026 13:58:24 +0200 Subject: [PATCH] fix(isoch): enforce RX completion ordering and clear stopped events Wait for transfer status before consuming RX descriptors and order payload reads before descriptor rearming. Clear only each quiesced RX/TX context's pending event. Cover incomplete transfers, ring wraps and stop acknowledgement ordering. --- ASFWDriver/Common/BarrierUtils.hpp | 13 ++ ASFWDriver/Isoch/Memory/IIsochDMAMemory.hpp | 6 + .../Isoch/Receive/IsochReceiveContext.cpp | 9 + ASFWDriver/Isoch/Receive/IsochRxDmaRing.hpp | 22 ++- .../Isoch/Transmit/IsochTransmitContext.cpp | 8 + tests/audio/IsochReceiveContextTests.cpp | 28 +++ tests/audio/IsochRxDmaRingTests.cpp | 177 +++++++++++++++++- .../audio/IsochServiceTxPreparationTests.cpp | 6 + 8 files changed, 263 insertions(+), 6 deletions(-) diff --git a/ASFWDriver/Common/BarrierUtils.hpp b/ASFWDriver/Common/BarrierUtils.hpp index 0444ab125..a1aaf9f71 100644 --- a/ASFWDriver/Common/BarrierUtils.hpp +++ b/ASFWDriver/Common/BarrierUtils.hpp @@ -16,6 +16,19 @@ void FullBarrier(); // full fence for rare cases // MMIO barrier for programming device registers inline void IoBarrier() { OSSynchronizeIO(); } +// DriverKit 27 IOLib.h:235-239 documents OSSynchronizeIO as DMB ST on +// Apple Silicon (also verified in the installed DriverKit library). It does +// not order reads. RX needs completion -> payload reads -> status rearm. +// Use the full system domain for device DMA, not a C++ inner-shareable fence. +// This orders accesses; it is not cache maintenance or a DMA completion wait. +inline void DmaOwnershipBarrier() { +#if defined(__aarch64__) + __builtin_arm_dmb(0xf); // DMB SY: all loads/stores, full system domain. +#else + OSSynchronizeIO(); // DriverKit x86_64: MFENCE. +#endif +} + // Convenience wrappers for raw MMIO 32-bit accesses inline void Write32(volatile uint32_t* addr, uint32_t value) { *addr = value; diff --git a/ASFWDriver/Isoch/Memory/IIsochDMAMemory.hpp b/ASFWDriver/Isoch/Memory/IIsochDMAMemory.hpp index e8a4cd3de..161ed5614 100644 --- a/ASFWDriver/Isoch/Memory/IIsochDMAMemory.hpp +++ b/ASFWDriver/Isoch/Memory/IIsochDMAMemory.hpp @@ -20,6 +20,12 @@ class IIsochDMAMemory : public Shared::IDMAMemory { // Explicitly allocate a payload buffer region (from payload slab) virtual std::optional AllocatePayloadBuffer(size_t size) = 0; + /// Orders completion observation, payload reads, and completion rearming. + /// Separate from publication: receive ownership also needs load ordering. + virtual void ReceiveOwnershipBarrier() const noexcept { + ASFW::Driver::DmaOwnershipBarrier(); + } + /// Orders payload publication before descriptor publication. Kept virtual /// so host tests can record the exact DMA visibility sequence. virtual void PublishBarrier() const noexcept { diff --git a/ASFWDriver/Isoch/Receive/IsochReceiveContext.cpp b/ASFWDriver/Isoch/Receive/IsochReceiveContext.cpp index d4379b28b..1ef7d34f8 100644 --- a/ASFWDriver/Isoch/Receive/IsochReceiveContext.cpp +++ b/ASFWDriver/Isoch/Receive/IsochReceiveContext.cpp @@ -210,6 +210,15 @@ kern_return_t IsochReceiveContext::Stop() { return failure; } + if (control != UINT32_MAX) { + if (auto access = hardware_->TryBeginAccess()) { + // Masked context events are invisible to the interrupt drain + // (Linux ohci.c:2094-2107). Once ACTIVE is clear, retire only our + // stopped context's pending event before a later restart. + access.WriteAndFlush(Driver::Register32::kIsoRecvIntEventClear, contextMask); + } + } + IOLockLock(lock_); Transition(IRPolicy::State::Stopped, 0, "Stop"); IOLockUnlock(lock_); diff --git a/ASFWDriver/Isoch/Receive/IsochRxDmaRing.hpp b/ASFWDriver/Isoch/Receive/IsochRxDmaRing.hpp index b8e6eacbb..18bf3de02 100644 --- a/ASFWDriver/Isoch/Receive/IsochRxDmaRing.hpp +++ b/ASFWDriver/Isoch/Receive/IsochRxDmaRing.hpp @@ -54,14 +54,21 @@ class IsochRxDmaRing final { dma.FetchFromDevice(desc, sizeof(*desc)); - const uint16_t xferStatus = Async::HW::AR_xferStatus(*desc); - const uint16_t resCount = Async::HW::AR_resCount(*desc); - const bool done = (xferStatus != 0) || (resCount != reqCount); - - if (!done) { + const uint32_t status = desc->statusWord; + const uint16_t xferStatus = static_cast(status >> 16); + const uint16_t resCount = static_cast(status); + // A residual byte count is progress, not ownership transfer. + // Cross-checked with Linux ohci.c:2781-2813: packet-per-buffer + // retirement waits for transfer_status before accessing payload. + if (xferStatus == 0) { break; } + // Completion observation must precede payload reads. Linux + // ohci.c:2781-2813 synchronizes RX payload for the CPU here. + // FetchFromDevice's OSSynchronizeIO alone is DMB ST on arm64, + // even for cache-inhibited mappings; it does not order loads. + dma.ReceiveOwnershipBarrier(); const uint16_t actualLength = (resCount <= reqCount) ? static_cast(reqCount - resCount) : 0; auto* payloadVA = bufferRing_.GetElementVA(idx); if (payloadVA && actualLength > 0) { @@ -77,6 +84,11 @@ class IsochRxDmaRing final { }; onPacket(packet); + // Finish payload reads before clearing the completion marker. + // Linux queues payload back for the device only after consumption + // (amdtp-stream.c:1343-1356; ohci.c:3370-3392). This orders CPU + // accesses; the circular program still has no hardware stop fence. + dma.ReceiveOwnershipBarrier(); Async::HW::AR_init_status(*desc, reqCount); dma.PublishToDevice(desc, sizeof(*desc)); diff --git a/ASFWDriver/Isoch/Transmit/IsochTransmitContext.cpp b/ASFWDriver/Isoch/Transmit/IsochTransmitContext.cpp index 7e01a2404..e06c59f21 100644 --- a/ASFWDriver/Isoch/Transmit/IsochTransmitContext.cpp +++ b/ASFWDriver/Isoch/Transmit/IsochTransmitContext.cpp @@ -461,6 +461,14 @@ kern_return_t IsochTransmitContext::Stop() noexcept { return failure; } + if (control != UINT32_MAX) { + if (auto stoppedAccess = hardware_->TryBeginAccess()) { + // As for RX: retire only this quiesced context's masked event + // (Linux ohci.c:2105-2107), leaving other contexts untouched. + stoppedAccess.WriteAndFlush(Register32::kIsoXmitIntEventClear, 1u << contextIndex_); + } + } + if (controlBlock_) { controlBlock_->statusWord.store(IsochTxQueueStatus::kStopped, std::memory_order_release); } diff --git a/tests/audio/IsochReceiveContextTests.cpp b/tests/audio/IsochReceiveContextTests.cpp index 9049aaa0d..ea199ba39 100644 --- a/tests/audio/IsochReceiveContextTests.cpp +++ b/tests/audio/IsochReceiveContextTests.cpp @@ -171,9 +171,11 @@ TEST_F(IsochReceiveContextTest, StopRetainsBindingStateWhenActiveNeverClears) { const auto controlSet = static_cast<::ASFW::Driver::Register32>( ::DMAContextHelpers::IsoRcvContextControlSet(0)); hardware_->SetTestRegister(controlSet, ::ASFW::Driver::ContextControl::kActive); + hardware_->SetTestRegister(::ASFW::Driver::Register32::kIsoRecvIntEventClear, 0); EXPECT_EQ(context_->Stop(), kIOReturnTimeout); EXPECT_EQ(context_->GetState(), IRPolicy::State::Running); + EXPECT_EQ(hardware_->GetTestRegister(::ASFW::Driver::Register32::kIsoRecvIntEventClear), 0U); } TEST_F(IsochReceiveContextTest, PollProcessesPackets) { @@ -227,3 +229,29 @@ TEST_F(IsochReceiveContextTest, BusyPollRetainsBindingUntilStopCanAcquireOwnersh EXPECT_EQ(consumer.quiesceCount, 1U); context_->SetReceiveConsumer(nullptr); } + +TEST_F(IsochReceiveContextTest, StopAcknowledgesOnlyItsQuiescedContext) { + ASSERT_EQ(context_->Configure(0, 3), kIOReturnSuccess); + ASSERT_EQ(context_->Start(), kIOReturnSuccess); + const auto controlSet = static_cast<::ASFW::Driver::Register32>( + ::DMAContextHelpers::IsoRcvContextControlSet(3)); + hardware_->SetTestRegister(controlSet, 0); + hardware_->SetTestRegister(::ASFW::Driver::Register32::kIsoRecvIntEventClear, 0); + hardware_->SetTestRegister(::ASFW::Driver::Register32::kIsoRecvIntEventSet, 0xf); + + EXPECT_EQ(context_->Stop(), kIOReturnSuccess); + EXPECT_EQ(hardware_->GetTestRegister(::ASFW::Driver::Register32::kIsoRecvIntEventClear), 8U); + EXPECT_EQ(context_->GetState(), IRPolicy::State::Stopped); +} + +TEST_F(IsochReceiveContextTest, StopDoesNotAcknowledgeWhenControllerReadsAllOnes) { + ASSERT_EQ(context_->Configure(0, 0), kIOReturnSuccess); + ASSERT_EQ(context_->Start(), kIOReturnSuccess); + const auto controlSet = static_cast<::ASFW::Driver::Register32>( + ::DMAContextHelpers::IsoRcvContextControlSet(0)); + hardware_->SetTestRegister(controlSet, UINT32_MAX); + hardware_->SetTestRegister(::ASFW::Driver::Register32::kIsoRecvIntEventClear, 0); + + EXPECT_EQ(context_->Stop(), kIOReturnSuccess); + EXPECT_EQ(hardware_->GetTestRegister(::ASFW::Driver::Register32::kIsoRecvIntEventClear), 0U); +} diff --git a/tests/audio/IsochRxDmaRingTests.cpp b/tests/audio/IsochRxDmaRingTests.cpp index 39a958776..9370dabe4 100644 --- a/tests/audio/IsochRxDmaRingTests.cpp +++ b/tests/audio/IsochRxDmaRingTests.cpp @@ -1,4 +1,5 @@ #include +#include #include "Isoch/Receive/IsochRxDmaRing.hpp" #include "Isoch/Memory/IsochDMAMemoryManager.hpp" @@ -27,8 +28,128 @@ std::shared_ptr MakeTestIsochMemory(::ASFW::Driver::HardwareInt return concreteMgr; } +// Keep the production allocator/IOVA layout; intercept only visibility events. +// This models ordering obligations, not ARM caches or a physical DMA engine. +class ObservedReceiveMemory final : public IIsochDMAMemory { +public: + explicit ObservedReceiveMemory(IIsochDMAMemory& backing) : backing_(backing) {} + std::function onOwnership; + std::function onFetch; + std::function onPublish; + + std::optional AllocateDescriptor(size_t size) override { + return backing_.AllocateDescriptor(size); + } + std::optional AllocatePayloadBuffer(size_t size) override { + return backing_.AllocatePayloadBuffer(size); + } + std::optional AllocateRegion(size_t size, size_t alignment) override { + return backing_.AllocateRegion(size, alignment); + } + uint64_t VirtToIOVA(const std::byte* p) const noexcept override { + return backing_.VirtToIOVA(p); + } + std::byte* IOVAToVirt(uint64_t address) const noexcept override { + return backing_.IOVAToVirt(address); + } + void FetchFromDevice(const std::byte* p, size_t size) const noexcept override { + if (onFetch) onFetch(p, size); + backing_.FetchFromDevice(p, size); + } + void PublishToDevice(const std::byte* p, size_t size) const noexcept override { + if (onPublish) onPublish(p, size); + backing_.PublishToDevice(p, size); + } + void ReceiveOwnershipBarrier() const noexcept override { + if (onOwnership) onOwnership(); + backing_.ReceiveOwnershipBarrier(); + } + size_t TotalSize() const noexcept override { return backing_.TotalSize(); } + size_t AvailableSize() const noexcept override { return backing_.AvailableSize(); } +private: + IIsochDMAMemory& backing_; +}; + } // namespace +TEST(IsochRxDmaRingTests, OrdersCompletedPayloadReadsAndRearmAcrossWraps) { + ::ASFW::Driver::HardwareInterface hw; + auto backing = MakeTestIsochMemory(hw, 8, 64); + ASSERT_TRUE(backing); + ObservedReceiveMemory mem(*backing); + IsochRxDmaRing ring; + ASSERT_EQ(ring.SetupRings(mem, 8, 64), kIOReturnSuccess); + + // Follow the descriptor program built by SetupRings, including its wrap. + // Fixed-size INPUT_LAST slots have no allocator padding to skip. + uint32_t command = ring.InitialCommandPtrWord(); + for (uint32_t packet = 0; packet < 40; ++packet) { + const uint32_t index = packet % 8; + auto* desc = backing->IOVAToPtr(command & ~0xfu); + ASSERT_EQ(desc, ring.DescriptorAt(index)); + ASSERT_EQ(command & 0xfu, 1u); + auto* payload = ring.PayloadVA(index); + // Delay modeled payload visibility until the ownership acquire. The + // production FetchFromDevice currently supplies only a store barrier. + payload[0] = 0xee; + desc->statusWord = (0x8451u << 16) | 48u; + uint32_t barriers = 0; + bool consumed = false; + bool published = false; + mem.onOwnership = [&] { + EXPECT_EQ(desc->statusWord, (0x8451u << 16) | 48u); + if (barriers == 0) { + EXPECT_FALSE(consumed); + payload[0] = static_cast(packet); + } else { + EXPECT_TRUE(consumed); + } + ++barriers; + }; + mem.onFetch = [&](const std::byte* p, size_t) { + if (p == reinterpret_cast(payload)) EXPECT_EQ(barriers, 1u); + }; + mem.onPublish = [&](const std::byte* p, size_t) { + ASSERT_EQ(p, reinterpret_cast(desc)); + EXPECT_TRUE(consumed); + EXPECT_EQ(barriers, 2u); + EXPECT_EQ(desc->statusWord, 64u); + published = true; + }; + EXPECT_EQ(ring.DrainCompleted(mem, [&](const auto& completion) { + EXPECT_EQ(completion.descriptorIndex, index); + EXPECT_EQ(completion.payload[0], static_cast(packet)); + EXPECT_EQ(desc->statusWord, (0x8451u << 16) | 48u); + EXPECT_FALSE(published); + consumed = true; + }), 1u); + EXPECT_EQ(barriers, 2u); + EXPECT_TRUE(published); + command = desc->branchWord; + mem.onOwnership = {}; + mem.onFetch = {}; + mem.onPublish = {}; + EXPECT_EQ(ring.DrainCompleted(mem, [](const auto&) { ADD_FAILURE(); }), 0u); + } +} + +TEST(IsochRxDmaRingTests, DoesNotAcquireOrReuseWhileOnlyResidualHasChanged) { + ::ASFW::Driver::HardwareInterface hw; + auto backing = MakeTestIsochMemory(hw, 8, 64); + ObservedReceiveMemory mem(*backing); + IsochRxDmaRing ring; + ASSERT_EQ(ring.SetupRings(mem, 8, 64), kIOReturnSuccess); + ring.DescriptorAt(0)->statusWord = 48; + mem.onOwnership = [] { ADD_FAILURE() << "incomplete packet has no ownership transfer"; }; + mem.onPublish = [](const auto*, auto) { ADD_FAILURE() << "must not rearm in-flight DMA"; }; + mem.onFetch = [&](const std::byte* p, size_t size) { + EXPECT_EQ(p, reinterpret_cast(ring.DescriptorAt(0))); + EXPECT_EQ(size, sizeof(ASFW::Async::HW::OHCIDescriptor)); + }; + EXPECT_EQ(ring.DrainCompleted(mem, [](const auto&) { ADD_FAILURE(); }), 0u); + EXPECT_EQ(ring.DescriptorAt(0)->statusWord, 48u); +} + TEST(IsochRxDmaRingTests, InitialCommandPtrWord_SetsZBitAndPointsToDesc0) { ::ASFW::Driver::HardwareInterface hw; auto mem = MakeTestIsochMemory(hw, 8, 64); @@ -64,7 +185,7 @@ TEST(IsochRxDmaRingTests, DrainCompleted_ProcessesOneDescriptorAndRearms) { constexpr uint16_t reqCount = 64; constexpr uint16_t actualLength = 16; - d0->statusWord = (0x0000u << 16) | static_cast(reqCount - actualLength); + d0->statusWord = (0x8451u << 16) | static_cast(reqCount - actualLength); uint32_t calls = 0; const uint32_t processed = ring.DrainCompleted(*mem, [&calls, actualLength](const IsochRxDmaRing::CompletedPacket& pkt) { @@ -86,3 +207,57 @@ TEST(IsochRxDmaRingTests, DrainCompleted_ProcessesOneDescriptorAndRearms) { EXPECT_EQ(ASFW::Async::HW::AR_xferStatus(*d0), 0u); EXPECT_EQ(ASFW::Async::HW::AR_resCount(*d0), reqCount); } + +TEST(IsochRxDmaRingTests, ResidualProgressDoesNotRetireAnIncompletePacket) { + ::ASFW::Driver::HardwareInterface hw; + auto mem = MakeTestIsochMemory(hw, 8, 64); + IsochRxDmaRing ring; + ASSERT_EQ(ring.SetupRings(*mem, 8, 64), kIOReturnSuccess); + auto* first = ring.DescriptorAt(0); + first->statusWord = 48; + ring.DescriptorAt(1)->statusWord = (0x8451u << 16) | 48u; + uint32_t calls = 0; + EXPECT_EQ(ring.DrainCompleted(*mem, [&](const auto&) { ++calls; }), 0u); + EXPECT_EQ(calls, 0u); + EXPECT_EQ(first->statusWord, 48u); + EXPECT_EQ(ring.DrainCompleted(*mem, [&](const auto&) { ++calls; }), 0u); + EXPECT_EQ(first->statusWord, 48u); + first->statusWord = (0x8451u << 16) | 48u; + EXPECT_EQ(ring.DrainCompleted(*mem, [&](const auto& packet) { + EXPECT_EQ(packet.descriptorIndex, calls++); + EXPECT_EQ(packet.actualLength, 16u); + EXPECT_EQ(packet.xferStatus, 0x8451u); + }), 2u); + EXPECT_EQ(calls, 2u); +} + +TEST(IsochRxDmaRingTests, FullRingWrapEmptyTailAndHotAppendPreserveOrder) { + ::ASFW::Driver::HardwareInterface hw; + auto mem = MakeTestIsochMemory(hw, 8, 64); + IsochRxDmaRing ring; + ASSERT_EQ(ring.SetupRings(*mem, 8, 64), kIOReturnSuccess); + // This production receive allocator has fixed-size slots and no wrap + // padding. Exercise its branch from the tail back to descriptor zero. + uint32_t expected = 0; + auto consume = [&](const auto& packet) { + EXPECT_EQ(packet.descriptorIndex, expected++ % 8); + EXPECT_EQ(packet.actualLength, 16u); + }; + for (unsigned lap = 0; lap < 4; ++lap) { + for (unsigned i = 0; i < 8; ++i) + ring.DescriptorAt(i)->statusWord = (0x8451u << 16) | 48u; + EXPECT_EQ(ring.DrainCompleted(*mem, consume), 8u); + EXPECT_EQ(ring.DrainCompleted(*mem, consume), 0u); + } + for (unsigned i = 0; i < 7; ++i) + ring.DescriptorAt(i)->statusWord = (0x8451u << 16) | 48u; + EXPECT_EQ(ring.DrainCompleted(*mem, consume), 7u); + EXPECT_EQ(ring.DrainCompleted(*mem, consume), 0u); // empty at tail + ring.DescriptorAt(7)->statusWord = (0x8451u << 16) | 48u; + EXPECT_EQ(ring.DrainCompleted(*mem, [&](const auto& packet) { + consume(packet); + if (packet.descriptorIndex == 7) + ring.DescriptorAt(0)->statusWord = (0x8451u << 16) | 48u; + }), 2u); // completion arrives across the wrap while draining + EXPECT_EQ(expected, 41u); +} diff --git a/tests/audio/IsochServiceTxPreparationTests.cpp b/tests/audio/IsochServiceTxPreparationTests.cpp index ad9800331..8f16f2671 100644 --- a/tests/audio/IsochServiceTxPreparationTests.cpp +++ b/tests/audio/IsochServiceTxPreparationTests.cpp @@ -248,13 +248,19 @@ TEST(IsochServiceTxPreparation, FaultedTransmitIsQuiescedByStopBeforeReuse) { controlDescriptor, AudioTimingGeometry::kTxPacketsPerGroup), kIOReturnBusy); + // A pending sibling event must survive our stop. Neither event may be + // acknowledged while this context still owns its DMA program. + hardware.SetTestRegister(Register32::kIsoXmitIntEventSet, 0x5u); + hardware.SetTestRegister(Register32::kIsoXmitIntEventClear, 0u); EXPECT_EQ(service.StopTransmit(), kIOReturnTimeout); EXPECT_EQ(context->GetState(), ASFW::Isoch::ITState::Faulted); + EXPECT_EQ(hardware.GetTestRegister(Register32::kIsoXmitIntEventClear), 0u); hardware.SetTestRegister(controlSet, 0); EXPECT_EQ(service.StopTransmit(), kIOReturnSuccess); EXPECT_EQ(context->GetState(), ASFW::Isoch::ITState::Stopped); EXPECT_FALSE(context->NeedsQuiesce()); + EXPECT_EQ(hardware.GetTestRegister(Register32::kIsoXmitIntEventClear), 1u); } // Start clears every ContextControl bit: cycleMatchEnable (bit 31) has an