Fix AxiStreamDma stop races and zero-copy lifetime

[bengineerd]

Description

Fix two concrete AxiStreamDma shutdown hazards:

• Keep the shared zero-copy DMA mapping alive until ~AxiStreamDma(), so Rogue frames retained after stop() do not point at unmapped memory.
• Serialize deferred zero-copy dmaRetIndex() buffer returns against closure of the per-instance fd_, preventing stop() from closing or allowing reuse of the descriptor while a return is in flight.

This intentionally does not make every public driver operation safe to run concurrently with stop(). Callers must quiesce stream operations and driver controls/accessors before calling stop(). Deferred returns from zero-copy buffers already issued by the interface are the one operation explicitly allowed to overlap shutdown.

Details

• Move shared descriptor cleanup from stop() to ~AxiStreamDma(). Each Rogue Buffer retains its allocating Pool, so outstanding DMA-backed buffers keep the AxiStreamDma object—and therefore the shared mapping—alive until those buffers are released.
• Add a direct std::mutex used only by retBuffer() and the descriptor-close block in stop().
• Hold that mutex across both the fd_ validity check and dmaRetIndex(), while stop() holds it across close(fd_) and setting fd_ = -1.
• Keep stop() idempotent and join the RX thread based on joinable().
• Document the narrowed shutdown contract in the public C++ API.
• Add Linux-only C++ regression tests using an LD_PRELOAD fake aes-stream-driver shim:
  • retain a zero-copy frame across stop() and verify the shared mapping remains present until destruction;
  • block an in-flight dmaRetIndex() and verify stop() waits rather than closing the fd during the driver call.

Performance consideration

One concern remains partially open: every non-stale zero-copy return now performs an additional mutex lock/unlock before the existing decCounter() lock. That is far better than locking every allocation, transmit, getter, and control operation, but the PR contains no targeted DMA-return throughput measurement. The general Performance Tests passed, but they do not exercise this fake-driver return path. If high-rate impact is the deciding issue for @slacrherbst, a hardware measurement or focused benchmark would close it properly.

Validation

• The current GitHub Actions Full Build, Small Build, External Build, macOS arm64 Build, and Performance Tests pass.
• The Linux Full Build ran and passed both new AxiStreamDma lifecycle tests.
• Reverting the mapping-lifetime change causes the retained-mapping regression to fail.
• Removing the retBuffer()/stop() exclusion allows descriptor close to overlap the blocked driver return and causes the lifecycle regression to fail.
• git diff --check passes.

JIRA

N/A

Related

Related to #1276 teardown review.

[Copilot]

Pull request overview

This PR adjusts rogue::hardware::axi::AxiStreamDma teardown semantics for zero-copy so that stop() closes only the per-instance DMA file descriptor while keeping the shared mapped DMA buffers alive until ~AxiStreamDma(), preventing downstream zero-copy frames retained past stop() from referencing unmapped memory.

Changes:

• Move shared descriptor (desc_) cleanup from stop() into ~AxiStreamDma().
• Add fdMtx_ and use it to serialize deferred dmaRetIndex() calls against fd_ closure.
• Remove unused locals in retBuffer() and update stop()/API comments to reflect the new lifetime model.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/rogue/hardware/axi/AxiStreamDma.cpp	Moves shared mapping cleanup to destructor; adds mutex-guarded fd close and mutex-guarded zero-copy buffer return.
include/rogue/hardware/axi/AxiStreamDma.h	Adds <mutex>, introduces fdMtx_, and updates stop() documentation to reflect new lifetime behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

[slacrherbst]

I have a few concerns with this. I guess which is the error this is really solving. Since the fd_ value is only written twice and open and close and all other blocks read, I would suspect we would be fine without a lock. Were there actual cases where we were seeing segfaults on close because accept_frame was called? Also, I can understand the issue with the allocated buffer returns; that seems like a more real problem. Anyway, we should fully understand what is being solved. My main problem is the solution creates a huge amount of object creation and destruction to implement the lock. I would have preferred a more direct, but maybe less "layered and python" like approach that limited the memory allocations at high rate. The target high rate is also why I am apprehensive about even adding locks so I want to make sure this is really necessary. If we do have to add locks around the fd access, we need to use a clean mutex without the fancy class that wraps it.

[bengineerd]

I have reworked the change locally to focus on two cases:

• Keep the shared zero-copy DMA mapping alive until AxiStreamDma destruction so retained frames do not point at unmapped memory after stop().
• Serialize deferred zero-copy dmaRetIndex() buffer returns against closing the per-instance file descriptor.

The FdGuard, stopped_ state, shared_timed_mutex, and locks around allocation, transmit, getters, debug setup, and DMA ack have been removed. The remaining synchronization is a direct std::mutex used only by retBuffer() and the descriptor-close block in stop().

The public documentation now requires callers to quiesce stream operations and driver controls/accessors before calling stop(). Deferred returns from zero-copy buffers already issued by the interface are the one operation explicitly allowed to overlap shutdown.

The regression coverage has also been narrowed to the two behaviors above: retained mapping lifetime and dmaRetIndex() versus descriptor close. The local core build and source checks pass; the runtime fake-driver regressions are Linux-only and will run in CI after the revised code is pushed.