Fix AxiStreamDma stop races and zero-copy lifetime

include/rogue/hardware/axi/AxiStreamDma.h

@@ -23,6 +23,7 @@
 #include <atomic>
 #include <map>
 #include <memory>
+#include <mutex>
 #include <string>
 #include <thread>
 
@@ -84,6 +85,9 @@ typedef std::shared_ptr<rogue::hardware::axi::AxiStreamDmaShared> AxiStreamDmaSh
  * - A background RX thread is started in the constructor and runs until `stop()`
  *   or destruction.
  * - TX operations execute synchronously in caller context of `acceptFrame()`.
+ * - Callers must quiesce public driver operations before calling `stop()`.
+ *   Deferred zero-copy buffer returns may overlap `stop()` and are serialized
+ *   against descriptor closure internally.
  *
  * Zero-copy model:
  * - Enabled by default per device path.
@@ -104,6 +108,9 @@ class AxiStreamDma : public rogue::interfaces::stream::Master, public rogue::int
     // Process-local descriptor for TX/RX operations and dest mask programming.
     int32_t fd_;
 
+    // Serializes fd_ close against deferred zero-copy buffer returns.
+    std::mutex fdMtx_;
+
     // Destination selector used when transmitting frames.
     uint32_t dest_;
 
@@ -204,7 +211,19 @@ class AxiStreamDma : public rogue::interfaces::stream::Master, public rogue::int
     /** @brief Destroys the interface and stops background activity. */
     ~AxiStreamDma();
 
-    /** @brief Stops RX thread and closes DMA file descriptors. */
+    /**
+     * @brief Stops RX thread and closes the per-instance DMA file descriptor.
+     *
+     * @details
+     * The shared zero-copy DMA mapping remains valid until destruction so
+     * downstream Rogue buffers retained after `stop()` do not reference
+     * unmapped memory.
+     *
+     * Before calling `stop()`, callers must ensure that stream operations and
+     * driver controls/accessors have completed and that no new ones can begin.
+     * Deferred returns from previously issued zero-copy buffers may overlap
+     * `stop()`; descriptor closure is synchronized with those callbacks.
+     */
     void stop();
 
     /**

src/rogue/hardware/axi/AxiStreamDma.cpp

@@ -294,6 +294,11 @@ rha::AxiStreamDma::AxiStreamDma(std::string path, uint32_t dest, bool ssiEnable)
 //! Close the device
 rha::AxiStreamDma::~AxiStreamDma() {
     this->stop();
+
+    if (desc_) {
+        closeShared(desc_);
+        desc_.reset();
+    }
 }
 
 void rha::AxiStreamDma::stop() {
@@ -314,19 +319,15 @@ void rha::AxiStreamDma::stop() {
     }
     thread_.reset();
 
-    // Release the shared descriptor exactly once even if stop() is called
-    // again (or runThread() self-exited and the user called stop() before
-    // ~AxiStreamDma()): clearing desc_ after closeShared() prevents a
-    // double-decrement of openCount.
-    if (desc_) {
-        closeShared(desc_);
-        desc_.reset();
-    }
-
-    // Close the per-instance fd exactly once.
-    if (fd_ >= 0) {
-        ::close(fd_);
-        fd_ = -1;
+    // Close the per-instance fd exactly once.  The shared DMA mapping remains
+    // open until ~AxiStreamDma() because zero-copy Rogue buffers can outlive a
+    // user-initiated stop() and still point into desc_->rawBuff.
+    {
+        std::lock_guard<std::mutex> lock(fdMtx_);
+        if (fd_ >= 0) {
+            ::close(fd_);
+            fd_ = -1;
+        }
     }
 }
 
@@ -358,10 +359,8 @@ ris::FramePtr rha::AxiStreamDma::acceptReq(uint32_t size, bool zeroCopyEn) {
     ris::FramePtr frame;
     uint32_t buffSize;
 
-    // Reject use after stop()/teardown.  stop() resets the shared-descriptor
-    // shared_ptr to nullptr and closes the per-instance fd, so a post-stop
-    // call would otherwise segfault on the buffer-size read below instead
-    // of throwing a clean GeneralError.
+    // Reject use after stop()/teardown.  stop() closes the per-instance fd,
+    // and destruction releases the shared descriptor.
     if (!desc_ || fd_ < 0)
         throw rogue::GeneralError("AxiStreamDma::acceptReq",
                                   "instance has been stopped or did not finish construction");
@@ -389,7 +388,7 @@ ris::FramePtr rha::AxiStreamDma::acceptReq(uint32_t size, bool zeroCopyEn) {
             // Keep trying since poll call can fire
             // but getIndex fails because we did not win the buffer lock
             do {
-                // Re-check fd validity: a concurrent stop() can toggle fd_ to -1.
+                // Re-check fd validity before using it in poll() and the driver call.
                 // poll() imposes no FD_SETSIZE ceiling, so large fd values are fine.
                 if (fd_ < 0)
                     throw rogue::GeneralError::create(
@@ -442,10 +441,8 @@ void rha::AxiStreamDma::acceptFrame(ris::FramePtr frame) {
     uint32_t cont;
     bool emptyFrame;
 
-    // Reject use after stop()/teardown.  stop() releases the shared descriptor
-    // and closes fd_, so the inner-loop fd_ guard would still catch this, but
-    // that fires only after frame->lock() and buffer iteration have already
-    // run.  Fail-fast at entry for a cleaner error path, mirroring acceptReq().
+    // Reject use after stop()/teardown before locking or iterating the frame,
+    // mirroring acceptReq().
     if (!desc_ || fd_ < 0)
         throw rogue::GeneralError("AxiStreamDma::acceptFrame",
                                   "instance has been stopped or did not finish construction");
@@ -512,7 +509,7 @@ void rha::AxiStreamDma::acceptFrame(ris::FramePtr frame) {
             // Keep trying since poll call can fire
             // but write fails because we did not win the (*it)er lock
             do {
-                // Re-check fd validity: a concurrent stop() can toggle fd_ to -1.
+                // Re-check fd validity before using it in poll() and the driver call.
                 // poll() imposes no FD_SETSIZE ceiling, so large fd values are fine.
                 if (fd_ < 0)
                     throw rogue::GeneralError::create(
@@ -557,16 +554,14 @@ void rha::AxiStreamDma::acceptFrame(ris::FramePtr frame) {
 //! Return a buffer
 void rha::AxiStreamDma::retBuffer(uint8_t* data, uint32_t meta, uint32_t size) {
     rogue::GilRelease noGil;
-    uint32_t ret[100];
-    uint32_t count;
-    uint32_t x;
 
     // Buffer is zero copy as indicated by bit 31
     if ((meta & 0x80000000) != 0) {
         // Device is open and buffer is not stale
         // Bit 30 indicates buffer has already been returned to hardware
-        if ((fd_ >= 0) && ((meta & 0x40000000) == 0)) {
-            if (dmaRetIndex(fd_, meta & 0x3FFFFFFF) < 0)
+        if ((meta & 0x40000000) == 0) {
+            std::lock_guard<std::mutex> lock(fdMtx_);
+            if (fd_ >= 0 && dmaRetIndex(fd_, meta & 0x3FFFFFFF) < 0)
                 throw(rogue::GeneralError("AxiStreamDma::retBuffer", "AXIS Return Buffer Call Failed!!!!"));
         }
         decCounter(size);

tests/cpp/CMakeLists.txt

@@ -53,6 +53,7 @@ function(rogue_add_cpp_test target)
 endfunction()
 
 add_subdirectory(core)
+add_subdirectory(hardware)
 add_subdirectory(memory)
 add_subdirectory(stream)
 add_subdirectory(protocols)

tests/cpp/hardware/CMakeLists.txt

@@ -0,0 +1,43 @@
+# Hardware-class tests that run AxiStreamDma against an LD_PRELOAD shim for
+# the aes-stream-driver device. ELF symbol interposition is Linux-only; other
+# platforms skip this subtree while still building rogue-core.
+if (NOT CMAKE_SYSTEM_NAME STREQUAL "Linux")
+   return()
+endif()
+
+add_library(rogue-cpp-fake-dma-preload MODULE support/fake_dma_preload.c)
+target_link_libraries(rogue-cpp-fake-dma-preload PRIVATE ${CMAKE_DL_LIBS} pthread)
+
+set(_rogue_fake_dma_env
+   "MPLCONFIGDIR=${CMAKE_BINARY_DIR}/tests/cpp/mplconfig;LD_PRELOAD=$<TARGET_FILE:rogue-cpp-fake-dma-preload>"
+)
+
+rogue_add_cpp_test(rogue-cpp-hardware-axi-stream-dma-zerocopy-lifetime
+   SOURCES
+      test_axi_stream_dma_zerocopy_lifetime.cpp
+   LABELS
+      cpp-hardware
+      no-python
+   LIBRARIES
+      ${CMAKE_DL_LIBS}
+)
+add_dependencies(rogue-cpp-hardware-axi-stream-dma-zerocopy-lifetime rogue-cpp-fake-dma-preload)
+set_tests_properties(rogue-cpp-hardware-axi-stream-dma-zerocopy-lifetime PROPERTIES
+   ENVIRONMENT "${_rogue_fake_dma_env}"
+   TIMEOUT 10
+)
+
+rogue_add_cpp_test(rogue-cpp-hardware-axi-stream-dma-lifecycle
+   SOURCES
+      test_axi_stream_dma_lifecycle.cpp
+   LABELS
+      cpp-hardware
+      no-python
+   LIBRARIES
+      ${CMAKE_DL_LIBS}
+)
+add_dependencies(rogue-cpp-hardware-axi-stream-dma-lifecycle rogue-cpp-fake-dma-preload)
+set_tests_properties(rogue-cpp-hardware-axi-stream-dma-lifecycle PROPERTIES
+   ENVIRONMENT "${_rogue_fake_dma_env}"
+   TIMEOUT 10
+)

tests/cpp/hardware/support/fake_dma_hooks.h

@@ -0,0 +1,100 @@
+/**
+ * ----------------------------------------------------------------------------
+ * Company    : SLAC National Accelerator Laboratory
+ * ----------------------------------------------------------------------------
+ * Description:
+ * Shared C++ accessors for the fake_dma_preload LD_PRELOAD test shim.
+ * ----------------------------------------------------------------------------
+ * This file is part of the rogue software platform. It is subject to
+ * the license terms in the LICENSE.txt file found in the top-level directory
+ * of this distribution and at:
+ *    https://confluence.slac.stanford.edu/display/ppareg/LICENSE.html.
+ * No part of the rogue software platform, including this file, may be
+ * copied, modified, propagated, or distributed except according to the terms
+ * contained in the LICENSE.txt file.
+ * ----------------------------------------------------------------------------
+ **/
+#ifndef ROGUE_TEST_CPP_HARDWARE_SUPPORT_FAKE_DMA_HOOKS_H
+#define ROGUE_TEST_CPP_HARDWARE_SUPPORT_FAKE_DMA_HOOKS_H
+
+#include <dlfcn.h>
+#include <stdint.h>
+
+#include <stdexcept>
+
+namespace rogue_test {
+
+class FakeDma {
+  public:
+    using PathFn        = const char* (*)();
+    using ResetFn       = void (*)();
+    using BlockNextFn   = void (*)();
+    using WaitBlockedFn = int (*)(uint32_t);
+    using ReleaseFn     = void (*)();
+    using CountFn       = int (*)();
+
+    FakeDma() {
+        path_            = load<PathFn>("fakedma_path");
+        reset_           = load<ResetFn>("fakedma_reset");
+        blockNext_       = load<BlockNextFn>("fakedma_block_next_ret_index");
+        waitBlocked_     = load<WaitBlockedFn>("fakedma_wait_blocked");
+        release_         = load<ReleaseFn>("fakedma_release_blocked");
+        mappedCount_     = load<CountFn>("fakedma_mapped_count");
+        fdCount_         = load<CountFn>("fakedma_fd_count");
+        regionCount_     = load<CountFn>("fakedma_region_count");
+        activeCalls_     = load<CountFn>("fakedma_active_call_count");
+        overflowCount_   = load<CountFn>("fakedma_tracking_overflow_count");
+        isClean_         = load<CountFn>("fakedma_is_clean");
+        retIndexCount_     = load<CountFn>("fakedma_ret_index_count");
+        closeDuringCall_   = load<CountFn>("fakedma_close_during_driver_call");
+    }
+
+    const char* path() const { return path_(); }
+    void reset() const { reset_(); }
+    void blockNextRetIndex() const { blockNext_(); }
+    bool waitBlocked(uint32_t timeoutMs) const { return waitBlocked_(timeoutMs) != 0; }
+    void releaseBlocked() const { release_(); }
+    int mappedCount() const { return mappedCount_(); }
+    int fdCount() const { return fdCount_(); }
+    int regionCount() const { return regionCount_(); }
+    int activeCallCount() const { return activeCalls_(); }
+    int overflowCount() const { return overflowCount_(); }
+    bool isClean() const { return isClean_() != 0; }
+    int retIndexCount() const { return retIndexCount_(); }
+    bool closeDuringDriverCall() const { return closeDuringCall_() != 0; }
+
+  private:
+    template <typename Func>
+    Func load(const char* name) {
+        dlerror();
+        void* sym = dlsym(RTLD_DEFAULT, name);
+        if (sym == nullptr) {
+            const char* err = dlerror();
+            throw std::runtime_error(err != nullptr ? err : name);
+        }
+        return reinterpret_cast<Func>(sym);
+    }
+
+    PathFn path_;
+    ResetFn reset_;
+    BlockNextFn blockNext_;
+    WaitBlockedFn waitBlocked_;
+    ReleaseFn release_;
+    CountFn mappedCount_;
+    CountFn fdCount_;
+    CountFn regionCount_;
+    CountFn activeCalls_;
+    CountFn overflowCount_;
+    CountFn isClean_;
+    CountFn retIndexCount_;
+    CountFn closeDuringCall_;
+};
+
+inline FakeDma& fakeDma() {
+    static FakeDma fake;
+    return fake;
+}
+
+}  // namespace rogue_test
+
+#endif