[PATCH v6 0/3] gpu: nova-core: retain the GSP-RM log buffers

[PATCH v6 0/3] gpu: nova-core: retain the GSP-RM log buffers

[Vladislav Zaharov]

The GSP-RM log buffers are exposed through debugfs, but they are DMA
allocations of the device, so they go away as soon as the GPU is unbound,
and in particular when probe() fails. That is the case todo.rst asks
about, and the one where a GSP log is most useful.

Patch 1 moves the debugfs root into the module data. Patch 2 adds a
gsp_keep_logs module parameter: when it is set, whatever the GSP wrote
is copied into module memory and kept under a "retained" directory until
the module is unloaded. Patch 3 drops the completed item from todo.rst.

v5 got no comments, so this version only carries one fix and a rebase.

Changes since v5:
 - keep the "put" pointer read before the barrier in the copy, rather
   than the one the copy reads again, which is not ordered against the
   rest of the data (reported by the Sashiko review bot)
 - rebase onto current drm-rust-next

Tested on a GB203 on top of drm-rust-next with the TLV firmware images:

  - without gsp_keep_logs nothing changes, and no "retained" directory
    is created;
  - with gsp_keep_logs=1, retained/<BDF>/{loginit,logintr,logrm} hold
    the full 64 KiB of each buffer after unbind;
  - binding again does not disturb the copies, and a second unbind
    replaces them, leaving one set;
  - with a failure injected after the GSP has booted, probe() fails with
    -EINVAL and the logs of that attempt are still readable;
  - with a failure injected into the driver registration, the module
    fails to load, leaves no debugfs directory behind, and the next load
    works;
  - repeated load/unload cycles leave nothing behind.

No warnings or oopses in dmesg. Each patch builds on its own, and the
series is clean with CLIPPY=1, rustfmtcheck and checkpatch --strict
(apart from the MAINTAINERS note for the new file, which is already
covered by "F: drivers/gpu/nova-core/").

v5: https://lore.kernel.org/nova-gpu/20260923045551.229259-1-vladazaharova2018@gmail.com/

Vladislav Zaharov (3):
  gpu: nova-core: move the debugfs root into the module data
  gpu: nova-core: gsp: retain the GSP-RM log buffers after unbind
  Documentation: nova: remove completed GSP log buffer task

 Documentation/gpu/nova/core/todo.rst   |  12 --
 drivers/gpu/nova-core/gsp.rs           | 100 ++--------
 drivers/gpu/nova-core/gsp/logbuffer.rs | 255 +++++++++++++++++++++++++
 drivers/gpu/nova-core/nova_core.rs     | 101 ++++++++--
 4 files changed, 356 insertions(+), 112 deletions(-)
 create mode 100644 drivers/gpu/nova-core/gsp/logbuffer.rs


base-commit: 5a1b7121a6c4db6c0239d3ac3fd1c2bb33a40025
-- 
2.56.0

[PATCH v6 1/3] gpu: nova-core: move the debugfs root into the module data

[Vladislav Zaharov]

The debugfs root lives in a static that init() fills in and a guard
field of the module data clears again. That costs a `static mut`, an
unsafe write on each side and a guard type whose only job is to undo
the write.

It also leaks. try_pin_init! drops only the fields it has already
built, and the guard is written after the Registration, so a
registration that fails leaves the guard unbuilt and the static set.
Statics are never dropped, and the module is unloaded right away, so
the directory outlives everything that could remove it. The next load
then finds the name taken: debugfs_create_dir() returns -EEXIST, which
Entry keeps as it would any other pointer, and the driver comes up with
no debugfs at all until the machine is rebooted.

Have the module data own a DebugfsData instead, built before the
registration and dropped after it, and keep only a pointer to it in the
static, for devices that have no other way to reach the data of their
module. What is left is one unsafe read for the users and a single write
when the data is built, with no guard type. A registration that fails
now drops the data that was built before it, and the directory goes with
it.

Assisted-by: LLM
Signed-off-by: Vladislav Zaharov <vladazaharova2018@gmail.com>
---
 drivers/gpu/nova-core/gsp.rs       | 10 +---
 drivers/gpu/nova-core/nova_core.rs | 74 ++++++++++++++++++++++--------
 2 files changed, 56 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
index dda58095f40b..01ed4adffe93 100644
--- a/drivers/gpu/nova-core/gsp.rs
+++ b/drivers/gpu/nova-core/gsp.rs
@@ -199,15 +199,7 @@ pub(crate) fn new(
                         logrm,
                     };
 
-                    #[allow(static_mut_refs)]
-                    // SAFETY: `DEBUGFS_ROOT` is created before driver registration and cleared
-                    // after driver unregistration, so no probe() can race with its modification.
-                    //
-                    // PANIC: `DEBUGFS_ROOT` cannot be `None` here.  It is set before driver
-                    // registration and cleared after driver unregistration, so it is always
-                    // `Some` for the entire lifetime that probe() can be called.
-                    let log_parent: &debugfs::Dir = unsafe { crate::DEBUGFS_ROOT.as_ref() }
-                        .expect("DEBUGFS_ROOT not initialized");
+                    let log_parent: &debugfs::Dir = crate::debugfs_data(dev).root();
 
                     log_parent.scope(log_buffers, dev.name(), |logs, dir| {
                         dir.read_binary_file(c"loginit", &logs.loginit.0);
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index a8f6dc22057b..ee3a6600f320 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -4,6 +4,7 @@
 
 use kernel::{
     debugfs,
+    device,
     driver::Registration,
     pci,
     prelude::*,
@@ -32,40 +33,75 @@
 
 pub(crate) const MODULE_NAME: &core::ffi::CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
 
-// TODO: Move this into per-module data once that exists.
-static mut DEBUGFS_ROOT: Option<debugfs::Dir> = None;
+/// Pointer to the [`DebugfsData`] the module owns.
+///
+/// A device has no way to reach the data of its module, so code running on behalf of a bound
+/// device goes through here instead.
+///
+/// Written once, while the module data is built, and never again: what it points at is dropped
+/// only after the driver is unregistered, so it is good for as long as any device is bound, and
+/// is not read outside of that.
+// TODO: Drop this once devices can reach the data of their module.
+static mut DEBUGFS_DATA: *const DebugfsData = core::ptr::null();
 
-/// Guard that clears `DEBUGFS_ROOT` when dropped.
-struct DebugfsRootGuard;
+/// Data the module shares with every GPU it drives.
+///
+/// Reached from a device through [`debugfs_data()`].
+#[pin_data]
+pub(crate) struct DebugfsData {
+    /// Root directory of the driver in debugfs.
+    root: debugfs::Dir,
+}
+
+impl DebugfsData {
+    /// Creates the shared data.
+    fn new() -> impl PinInit<Self> {
+        pin_init!(Self {
+            root: debugfs::Dir::new(c"nova-core"),
+        })
+    }
 
-impl Drop for DebugfsRootGuard {
-    fn drop(&mut self) {
-        // SAFETY: This guard is dropped after `_driver` (due to field order),
-        // so the driver is unregistered and no probe() can be running.
-        unsafe { DEBUGFS_ROOT = None };
+    /// Returns the root directory of the driver in debugfs.
+    pub(crate) fn root(&self) -> &debugfs::Dir {
+        &self.root
     }
 }
 
+/// Returns the data the module shares with its devices.
+///
+/// The bound device is what makes this sound: the data is built before the driver is registered
+/// and dropped after it is unregistered, so it outlives every device that is bound, and the
+/// returned reference cannot be held past the one it is taken for.
+pub(crate) fn debugfs_data<'a>(_dev: &'a device::Device<device::Bound>) -> &'a DebugfsData {
+    // SAFETY: A device can only be bound once the driver is registered, which happens after
+    // `DEBUGFS_DATA` is written, so it points at the data of this module, which lives at least
+    // as long as the caller's device is bound.
+    unsafe { &*DEBUGFS_DATA }
+}
+
 #[pin_data]
 struct NovaCoreModule {
-    // Fields are dropped in declaration order, so `_driver` is dropped first,
-    // then `_debugfs_guard` clears `DEBUGFS_ROOT`.
+    // Fields are dropped in declaration order, so the registration goes first and no probe() can
+    // still be running once the shared data is torn down. `init()` builds them the other way
+    // round, as the data has to be there before the first probe() reaches for it.
     #[pin]
     _driver: Registration<pci::Adapter<driver::NovaCoreDriver>>,
-    _debugfs_guard: DebugfsRootGuard,
+    #[pin]
+    _debugfs: DebugfsData,
 }
 
 impl InPlaceModule for NovaCoreModule {
     fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self, Error> {
-        let dir = debugfs::Dir::new(c"nova-core");
-
-        // SAFETY: We are the only driver code running during init, so there
-        // cannot be any concurrent access to `DEBUGFS_ROOT`.
-        unsafe { DEBUGFS_ROOT = Some(dir) };
-
         try_pin_init!(Self {
+            _debugfs <- DebugfsData::new(),
+            _: {
+                // SAFETY: Nothing reads `DEBUGFS_DATA` before a device is bound, which cannot
+                // happen until the driver is registered below. What it points at is dropped only
+                // once the driver is unregistered, so it is valid for as long as any device is
+                // bound, and nothing reads it outside of that.
+                unsafe { DEBUGFS_DATA = &*_debugfs };
+            },
             _driver <- Registration::new(MODULE_NAME, module),
-            _debugfs_guard: DebugfsRootGuard,
         })
     }
 }
-- 
2.56.0

[PATCH v6 2/3] gpu: nova-core: gsp: retain the GSP-RM log buffers after unbind

[Vladislav Zaharov]

The GSP-RM log buffers are exposed through debugfs, but the Scope that
owns them lives in Gsp, inside GspResources, inside the Gpu built by
probe(). They are DMA allocations of the device and cannot outlive it,
so the entries go away as soon as the GPU is unbound - and, more to the
point, as soon as probe() fails, which is exactly when the log of a GSP
that did not come up is the thing one wants to read.

Add a gsp_keep_logs module parameter. When it is set, dropping the log
buffers copies whatever the GSP wrote into memory owned by the module
and exposes the copies until the module is unloaded. A buffer whose
"put" pointer is still zero was never written to and is skipped.

The GSP has normally been stopped by the time the buffers are dropped,
but a boot that timed out can leave it still appending, so a DMA read
barrier orders the read of the "put" pointer before the copy, and that
value, not the one the copy reads again, is what the copy keeps.

The copies belong to the module data next to the debugfs root, and live
in a "retained" directory created during module init rather than on
first use, which keeps the teardown path of a device from having to
create anything. Keeping them out of the directory used by bound GPUs
also means a device coming back does not find its debugfs name taken by
its own history; nouveau, which recreates the entries under the name of
the GPU that just went away, has that problem.

While at it, move the log buffer code out of gsp.rs into gsp/logbuffer.rs.

Assisted-by: LLM
Signed-off-by: Vladislav Zaharov <vladazaharova2018@gmail.com>
---
 drivers/gpu/nova-core/gsp.rs           |  92 ++-------
 drivers/gpu/nova-core/gsp/logbuffer.rs | 255 +++++++++++++++++++++++++
 drivers/gpu/nova-core/nova_core.rs     |  29 ++-
 3 files changed, 302 insertions(+), 74 deletions(-)
 create mode 100644 drivers/gpu/nova-core/gsp/logbuffer.rs

diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
index 01ed4adffe93..72752c6bd6c0 100644
--- a/drivers/gpu/nova-core/gsp.rs
+++ b/drivers/gpu/nova-core/gsp.rs
@@ -12,11 +12,7 @@
         CoherentView,
         DmaAddress, //
     },
-    io::{
-        io_project,
-        io_write,
-        Io, //
-    },
+    io::io_write,
     pci,
     prelude::*, //
 };
@@ -24,9 +20,13 @@
 pub(crate) mod cmdq;
 pub(crate) mod commands;
 mod fw;
+mod logbuffer;
 mod regs;
 mod sequencer;
 
+use logbuffer::LogBuffers;
+pub(crate) use logbuffer::RetainedLogs;
+
 pub(crate) use fw::{
     GspFmcBootParams,
     GspFwWprMeta,
@@ -77,10 +77,6 @@ pub(crate) fn dev(&self) -> &'gpu device::Device<device::Bound> {
     }
 }
 
-/// Number of GSP pages to use in a RM log buffer.
-const RM_LOG_BUFFER_NUM_PAGES: usize = 0x10;
-const LOG_BUFFER_SIZE: usize = RM_LOG_BUFFER_NUM_PAGES * GSP_PAGE_SIZE;
-
 /// Array of page table entries, as understood by the GSP bootloader.
 #[repr(C)]
 #[derive(FromBytes, IntoBytes)]
@@ -101,49 +97,6 @@ fn init(view: CoherentView<'_, Self>, start: DmaAddress) -> Result<()> {
     }
 }
 
-/// The logging buffers are byte queues that contain encoded printf-like
-/// messages from GSP-RM.  They need to be decoded by a special application
-/// that can parse the buffers.
-///
-/// The 'loginit' buffer contains logs from early GSP-RM init and
-/// exception dumps.  The 'logrm' buffer contains the subsequent logs. Both are
-/// written to directly by GSP-RM and can be any multiple of GSP_PAGE_SIZE.
-///
-/// The physical address map for the log buffer is stored in the buffer
-/// itself, starting with offset 1. Offset 0 contains the "put" pointer (pp).
-/// Initially, pp is equal to 0. If the buffer has valid logging data in it,
-/// then pp points to index into the buffer where the next logging entry will
-/// be written. Therefore, the logging data is valid if:
-///   1 <= pp < sizeof(buffer)/sizeof(u64)
-struct LogBuffer<'a>(Coherent<'a, [u8; LOG_BUFFER_SIZE]>);
-
-impl<'a> LogBuffer<'a> {
-    /// Creates a new `LogBuffer` mapped on `dev`.
-    fn new(dev: &'a device::Device<device::Bound>) -> Result<Self> {
-        let obj = Self(Coherent::zeroed(dev, GFP_KERNEL)?);
-
-        let start_addr = obj.0.dma_address();
-
-        let pte_view = io_project!(
-            obj.0,
-            [build: size_of::<u64>()..][build: ..RM_LOG_BUFFER_NUM_PAGES * size_of::<u64>()]
-        )
-        .try_cast::<PteArray<RM_LOG_BUFFER_NUM_PAGES>>()?;
-        PteArray::init(pte_view, start_addr)?;
-
-        Ok(obj)
-    }
-}
-
-struct LogBuffers<'a> {
-    /// Init log buffer.
-    loginit: LogBuffer<'a>,
-    /// Interrupts log buffer.
-    logintr: LogBuffer<'a>,
-    /// RM log buffer.
-    logrm: LogBuffer<'a>,
-}
-
 /// GSP runtime data.
 #[pin_data]
 pub(crate) struct Gsp<'gsp> {
@@ -168,9 +121,7 @@ pub(crate) fn new(
         pin_init::pin_init_scope(move || {
             let dev = pdev.as_ref();
 
-            let loginit = LogBuffer::new(dev)?;
-            let logintr = LogBuffer::new(dev)?;
-            let logrm = LogBuffer::new(dev)?;
+            let log_buffers = LogBuffers::new(dev)?;
 
             // Initialise the logging structures. The OpenRM equivalents are in:
             // _kgspInitLibosLoggingStructures (allocates memory for buffers)
@@ -185,28 +136,23 @@ pub(crate) fn new(
                         GFP_KERNEL,
                     )?;
 
-                    libos.init_at(0, LibosMemoryRegionInitArgument::new("LOGINIT", &loginit.0))?;
-                    libos.init_at(1, LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0))?;
-                    libos.init_at(2, LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0))?;
+                    libos.init_at(
+                        0,
+                        LibosMemoryRegionInitArgument::new("LOGINIT", &log_buffers.loginit.0),
+                    )?;
+                    libos.init_at(
+                        1,
+                        LibosMemoryRegionInitArgument::new("LOGINTR", &log_buffers.logintr.0),
+                    )?;
+                    libos.init_at(
+                        2,
+                        LibosMemoryRegionInitArgument::new("LOGRM", &log_buffers.logrm.0),
+                    )?;
                     libos.init_at(3, LibosMemoryRegionInitArgument::new("RMARGS", rmargs))?;
 
                     libos.into()
                 },
-                logs <- {
-                    let log_buffers = LogBuffers {
-                        loginit,
-                        logintr,
-                        logrm,
-                    };
-
-                    let log_parent: &debugfs::Dir = crate::debugfs_data(dev).root();
-
-                    log_parent.scope(log_buffers, dev.name(), |logs, dir| {
-                        dir.read_binary_file(c"loginit", &logs.loginit.0);
-                        dir.read_binary_file(c"logintr", &logs.logintr.0);
-                        dir.read_binary_file(c"logrm", &logs.logrm.0);
-                    })
-                },
+                logs <- log_buffers.scope(),
             }))
         })
     }
diff --git a/drivers/gpu/nova-core/gsp/logbuffer.rs b/drivers/gpu/nova-core/gsp/logbuffer.rs
new file mode 100644
index 000000000000..3c85d48f3942
--- /dev/null
+++ b/drivers/gpu/nova-core/gsp/logbuffer.rs
@@ -0,0 +1,255 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! GSP-RM log buffers, and the debugfs entries exposing them.
+
+use core::convert::Infallible;
+
+use kernel::{
+    debugfs,
+    device,
+    dma::Coherent,
+    io::{
+        io_project,
+        Io, //
+    },
+    prelude::*,
+    str::CString,
+    sync::barrier::{
+        dma_mb,
+        Read, //
+    }, //
+};
+
+use crate::gsp::{
+    PteArray,
+    GSP_PAGE_SIZE, //
+};
+
+/// Number of GSP pages to use in a RM log buffer.
+const RM_LOG_BUFFER_NUM_PAGES: usize = 0x10;
+const LOG_BUFFER_SIZE: usize = RM_LOG_BUFFER_NUM_PAGES * GSP_PAGE_SIZE;
+
+/// The logging buffers are byte queues that contain encoded printf-like
+/// messages from GSP-RM.  They need to be decoded by a special application
+/// that can parse the buffers.
+///
+/// The 'loginit' buffer contains logs from early GSP-RM init and
+/// exception dumps.  The 'logrm' buffer contains the subsequent logs. Both are
+/// written to directly by GSP-RM and can be any multiple of GSP_PAGE_SIZE.
+///
+/// The physical address map for the log buffer is stored in the buffer
+/// itself, starting with offset 1. Offset 0 contains the "put" pointer (pp).
+/// Initially, pp is equal to 0. If the buffer has valid logging data in it,
+/// then pp points to index into the buffer where the next logging entry will
+/// be written. Therefore, the logging data is valid if:
+///   1 <= pp < sizeof(buffer)/sizeof(u64)
+pub(super) struct LogBuffer<'a>(pub(super) Coherent<'a, [u8; LOG_BUFFER_SIZE]>);
+
+impl<'a> LogBuffer<'a> {
+    /// Creates a new `LogBuffer` mapped on `dev`.
+    fn new(dev: &'a device::Device<device::Bound>) -> Result<Self> {
+        let obj = Self(Coherent::zeroed(dev, GFP_KERNEL)?);
+
+        let start_addr = obj.0.dma_address();
+
+        let pte_view = io_project!(
+            obj.0,
+            [build: size_of::<u64>()..][build: ..RM_LOG_BUFFER_NUM_PAGES * size_of::<u64>()]
+        )
+        .try_cast::<PteArray<RM_LOG_BUFFER_NUM_PAGES>>()?;
+        PteArray::init(pte_view, start_addr)?;
+
+        Ok(obj)
+    }
+
+    /// Copies the contents of this buffer into memory that does not belong to the device.
+    ///
+    /// A buffer the GSP never wrote to yields an empty vector, as it holds nothing worth keeping.
+    fn snapshot(&self) -> Result<VVec<u8>> {
+        // Offset 0 holds the "put" pointer, which the GSP advances as it appends entries. It is
+        // still zero if nothing was ever logged, which is all that is tested here: a buffer that
+        // was written to is copied whole, and making sense of "put" is left to the decoder.
+        let put = io_project!(self.0, [build: ..size_of::<u64>()])
+            .try_cast::<u64>()?
+            .read_val();
+        if put == 0 {
+            return Ok(VVec::new());
+        }
+
+        // ORDERING: LOAD->LOAD ordering needed to order the "put" read before the data read. The
+        // GSP has normally been stopped by the time this runs, but a boot that timed out can leave
+        // it still appending.
+        dma_mb(Read);
+
+        let mut snapshot = VVec::zeroed(LOG_BUFFER_SIZE, GFP_KERNEL)?;
+        io_project!(self.0, [build: ..]).copy_to_slice(&mut snapshot);
+
+        // The copy read "put" once more, and nothing orders that read against the rest of it, so
+        // it may be newer than the data next to it. Keep the value read before the barrier, which
+        // the data is at least as new as. Nothing done here can help a GSP that is still running
+        // and wraps around over the oldest entries while they are copied; only stopping it could.
+        snapshot[..size_of::<u64>()].copy_from_slice(&put.to_ne_bytes());
+
+        Ok(snapshot)
+    }
+}
+
+/// The log buffers of a GPU, for as long as it is bound to the driver.
+pub(super) struct LogBuffers<'a> {
+    /// Device the buffers belong to. Also names their debugfs directory.
+    dev: &'a device::Device<device::Bound>,
+    /// Init log buffer.
+    pub(super) loginit: LogBuffer<'a>,
+    /// Interrupts log buffer.
+    pub(super) logintr: LogBuffer<'a>,
+    /// RM log buffer.
+    pub(super) logrm: LogBuffer<'a>,
+}
+
+impl<'a> LogBuffers<'a> {
+    /// Allocates the three log buffers of `dev`.
+    pub(super) fn new(dev: &'a device::Device<device::Bound>) -> Result<Self> {
+        Ok(Self {
+            dev,
+            loginit: LogBuffer::new(dev)?,
+            logintr: LogBuffer::new(dev)?,
+            logrm: LogBuffer::new(dev)?,
+        })
+    }
+
+    /// Creates an initializer exposing these buffers under a directory named after their device.
+    pub(super) fn scope(self) -> impl PinInit<debugfs::Scope<Self>, Infallible> + 'a {
+        let dev = self.dev;
+
+        let log_parent: &debugfs::Dir = crate::debugfs_data(dev).root();
+
+        log_parent.scope(self, dev.name(), |logs, dir| {
+            dir.read_binary_file(c"loginit", &logs.loginit.0);
+            dir.read_binary_file(c"logintr", &logs.logintr.0);
+            dir.read_binary_file(c"logrm", &logs.logrm.0);
+        })
+    }
+
+    /// Preserves whatever the GSP logged, so it can still be read once the GPU is gone.
+    ///
+    /// The buffers are DMA allocations of the device and cannot outlive it, so their contents are
+    /// copied into memory owned by the module and exposed through fresh debugfs entries. Those
+    /// live until the module is unloaded.
+    ///
+    /// Does nothing if `gsp_keep_logs` was not set when the module was loaded, as there is then
+    /// no directory to put the copies in.
+    fn retain(&self) -> Result {
+        let data = crate::debugfs_data(self.dev);
+
+        // The directory is taken here and the lock dropped again right away: what follows
+        // allocates 64 KiB three times, and no other device should have to wait for that.
+        let Some(dir) = data.retained_logs().lock().dir.clone() else {
+            return Ok(());
+        };
+
+        let logs = RetainedLogBuffers {
+            name: CString::try_from_fmt(fmt!("{}", self.dev.name()))?,
+            loginit: self.loginit.snapshot()?,
+            logintr: self.logintr.snapshot()?,
+            logrm: self.logrm.snapshot()?,
+        };
+
+        // Nothing was ever logged, so there is nothing to keep. A copy from an earlier run of
+        // this device is deliberately left alone: logs from a run that failed are worth more
+        // than the silence of one that did not. A run that did log something does replace it,
+        // even where what it replaces came from a run that failed: nothing here can tell the two
+        // apart, and keeping the older copy would mean never seeing anything newer.
+        if logs.loginit.is_empty() && logs.logintr.is_empty() && logs.logrm.is_empty() {
+            return Ok(());
+        }
+
+        // Make every allocation of our own that can fail before the previous copy of this device
+        // is dropped, so that none of them failing can leave it with no logs at all.
+        let scope = KBox::<debugfs::Scope<RetainedLogBuffers>>::new_uninit(GFP_KERNEL)?;
+
+        let mut retained = data.retained_logs().lock();
+
+        retained.gpus.reserve(1, GFP_KERNEL)?;
+
+        // An earlier run of the same device may have left a copy behind, and its directory
+        // carries the name about to be used again, so it has to go first. Nothing below can
+        // fail, so the replacement is guaranteed to take its place.
+        retained.gpus.retain(|gpu| *gpu.name != *logs.name);
+
+        let scope = scope.write_pin_init(dir.scope(logs, self.dev.name(), |logs, dir| {
+            if !logs.loginit.is_empty() {
+                dir.read_binary_file(c"loginit", &logs.loginit);
+            }
+            if !logs.logintr.is_empty() {
+                dir.read_binary_file(c"logintr", &logs.logintr);
+            }
+            if !logs.logrm.is_empty() {
+                dir.read_binary_file(c"logrm", &logs.logrm);
+            }
+        }))?;
+
+        retained.gpus.push(scope, GFP_KERNEL)?;
+
+        dev_dbg!(self.dev, "GSP-RM log buffers retained\n");
+
+        Ok(())
+    }
+}
+
+impl Drop for LogBuffers<'_> {
+    fn drop(&mut self) {
+        if let Err(e) = self.retain() {
+            dev_warn!(self.dev, "failed to retain GSP-RM log buffers: {:?}\n", e);
+        }
+    }
+}
+
+/// Copies of the log buffers of a GPU that is no longer around.
+struct RetainedLogBuffers {
+    /// Name of the device the buffers came from, which also names their directory.
+    ///
+    /// A copy rather than a reference to the device, so that a GPU that is gone does not stay
+    /// allocated for as long as its logs are kept.
+    name: CString,
+    /// Contents of the init log buffer, empty if it was never written to.
+    loginit: VVec<u8>,
+    /// Contents of the interrupts log buffer, empty if it was never written to.
+    logintr: VVec<u8>,
+    /// Contents of the RM log buffer, empty if it was never written to.
+    logrm: VVec<u8>,
+}
+
+/// Log buffers of GPUs that are gone, and the debugfs entries exposing them.
+///
+/// The copies live under a `retained` directory of their own instead of next to the entries of
+/// the GPUs that are actually bound, so that a device coming back does not find its name taken.
+pub(crate) struct RetainedLogs {
+    /// One entry per GPU.
+    ///
+    /// Declared before `dir`, as the copies live below it.
+    gpus: KVec<Pin<KBox<debugfs::Scope<RetainedLogBuffers>>>>,
+    /// Parent directory of all copies. `None` unless retaining was asked for.
+    dir: Option<debugfs::Dir>,
+}
+
+impl RetainedLogs {
+    /// Creates an empty set of retained log buffers, retaining disabled.
+    pub(crate) const fn new() -> Self {
+        Self {
+            gpus: KVec::new(),
+            dir: None,
+        }
+    }
+
+    /// Creates the directory the copies will live in, enabling retaining.
+    ///
+    /// Does nothing without `CONFIG_DEBUG_FS`, where a [`debugfs::Dir`] is a zero-sized type and
+    /// the copies could never be read back.
+    pub(crate) fn enable(&mut self, parent: &debugfs::Dir) {
+        if !cfg!(CONFIG_DEBUG_FS) {
+            return;
+        }
+
+        self.dir = Some(parent.subdir(c"retained"));
+    }
+}
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index ee3a6600f320..62684631c7fc 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -8,6 +8,7 @@
     driver::Registration,
     pci,
     prelude::*,
+    sync::Mutex,
     InPlaceModule, //
 };
 
@@ -49,6 +50,11 @@
 /// Reached from a device through [`debugfs_data()`].
 #[pin_data]
 pub(crate) struct DebugfsData {
+    /// Copies of the log buffers of GPUs that are gone.
+    ///
+    /// Declared before `root`, as the copies live below it.
+    #[pin]
+    retained_logs: Mutex<gsp::RetainedLogs>,
     /// Root directory of the driver in debugfs.
     root: debugfs::Dir,
 }
@@ -56,8 +62,18 @@ pub(crate) struct DebugfsData {
 impl DebugfsData {
     /// Creates the shared data.
     fn new() -> impl PinInit<Self> {
+        let root = debugfs::Dir::new(c"nova-core");
+
+        // Deciding here, rather than when the first GPU goes away, keeps the teardown path of a
+        // device from having to create anything.
+        let mut retained_logs = gsp::RetainedLogs::new();
+        if module_parameters::gsp_keep_logs.value() {
+            retained_logs.enable(&root);
+        }
+
         pin_init!(Self {
-            root: debugfs::Dir::new(c"nova-core"),
+            retained_logs <- kernel::new_mutex!(retained_logs),
+            root,
         })
     }
 
@@ -65,6 +81,11 @@ fn new() -> impl PinInit<Self> {
     pub(crate) fn root(&self) -> &debugfs::Dir {
         &self.root
     }
+
+    /// Returns the copies of the log buffers of GPUs that are gone.
+    pub(crate) fn retained_logs(&self) -> &Mutex<gsp::RetainedLogs> {
+        &self.retained_logs
+    }
 }
 
 /// Returns the data the module shares with its devices.
@@ -113,6 +134,12 @@ fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self, Error> {
     description: "Nova Core GPU driver",
     license: "GPL v2",
     firmware: [],
+    params: {
+        gsp_keep_logs: bool {
+            default: false,
+            description: "Keep the GSP-RM log buffers in debugfs after their GPU is gone",
+        },
+    },
 }
 
 kernel::module_firmware!(firmware::ModInfoBuilder);
-- 
2.56.0

[PATCH v6 3/3] Documentation: nova: remove completed GSP log buffer task

[Vladislav Zaharov]

Exposing the GSP-RM log buffers through debugfs is implemented, and with
the gsp_keep_logs module parameter they now also survive a failed probe,
which was the part of the task that was still missing.

Assisted-by: LLM
Signed-off-by: Vladislav Zaharov <vladazaharova2018@gmail.com>
---
 Documentation/gpu/nova/core/todo.rst | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/Documentation/gpu/nova/core/todo.rst b/Documentation/gpu/nova/core/todo.rst
index a01c362b1be0..28fc7329d56b 100644
--- a/Documentation/gpu/nova/core/todo.rst
+++ b/Documentation/gpu/nova/core/todo.rst
@@ -141,18 +141,6 @@ Implement support for instmem (bar2) used to store page tables.
 GPU System Processor (GSP)
 ==========================
 
-Export GSP log buffers
-----------------------
-
-Recent patches from Timur Tabi [1] added support to expose GSP-RM log buffers
-(even after failure to probe the driver) through debugfs.
-
-This is also an interesting feature for nova-core, especially in the early days.
-
-| Link: https://lore.kernel.org/nouveau/20241030202952.694055-2-ttabi@nvidia.com/ [1]
-| Reference: Debugfs abstractions
-| Complexity: Intermediate
-
 GSP firmware abstraction
 ------------------------
 
-- 
2.56.0

Re: [PATCH v6 0/3] gpu: nova-core: retain the GSP-RM log buffers

[Timur Tabi]

On Wed, 2026-10-14 at 22:45 +0700, Vladislav Zaharov wrote:

> Changes since v5:
>  - keep the "put" pointer read before the barrier in the copy, rather
>    than the one the copy reads again, which is not ordered against the
>    rest of the data (reported by the Sashiko review bot)
>  - rebase onto current drm-rust-next

Can you rebase this onto https://github.com/Gnurou/linux/tree/r000_rebase_all

This tree is the future of rust-drm-next, and there have been some significant changes to
debugfs that result in a lot of merge conflicts.

Re: [PATCH v6 0/3] gpu: nova-core: retain the GSP-RM log buffers

[Danilo Krummrich]

On Fri Nov 6, 2026 at 9:51 PM CET, Timur Tabi wrote:
> This tree is the future of rust-drm-next, and there have been some significant changes to
> debugfs that result in a lot of merge conflicts.

I know what you mean, but just to avoid confusion for people new to the
community: It's not exactly the future of drm-rust-next, but work that is
targeting to land in drm-rust-next in the future.

As for this patch series, it sits on my list of patches to deal with (for
probably too long). It also has been on the list for a couple of month now and I
don't think it should be delayed further, so I'll pick this up after some smoke
testing.

Thanks,
Danilo

Re: [PATCH v6 0/3] gpu: nova-core: retain the GSP-RM log buffers

[Alexandre Courbot]

On Sat Nov 7, 2026 at 5:51 AM JST, Timur Tabi wrote:
> On Wed, 2026-10-14 at 22:45 +0700, Vladislav Zaharov wrote:
>
>> Changes since v5:
>>  - keep the "put" pointer read before the barrier in the copy, rather
>>    than the one the copy reads again, which is not ordered against the
>>    rest of the data (reported by the Sashiko review bot)
>>  - rebase onto current drm-rust-next
>
> Can you rebase this onto https://github.com/Gnurou/linux/tree/r000_rebase_all
>
> This tree is the future of rust-drm-next, and there have been some significant changes to
> debugfs that result in a lot of merge conflicts.

This tree is also a moving target; let's not ask contributors to use
obscure bases that are weeks from reconciling with drm-rust-next.

The current v5 applies (almost) cleanly on today's drm-rust-next, so it
is fair game to review and merge if deemed ready. We can rebase our r000
code on top of it should it lands first.

Re: [PATCH v6 0/3] gpu: nova-core: retain the GSP-RM log buffers

[Alexandre Courbot]

On Sat Nov 7, 2026 at 5:51 AM JST, Timur Tabi wrote:
> On Wed, 2026-10-14 at 22:45 +0700, Vladislav Zaharov wrote:
>
>> Changes since v5:
>>  - keep the "put" pointer read before the barrier in the copy, rather
>>    than the one the copy reads again, which is not ordered against the
>>    rest of the data (reported by the Sashiko review bot)
>>  - rebase onto current drm-rust-next
>
> Can you rebase this onto https://github.com/Gnurou/linux/tree/r000_rebase_all
>
> This tree is the future of rust-drm-next, and there have been some significant changes to
> debugfs that result in a lot of merge conflicts.

This tree is also a moving target; let's not ask contributors to use
obscure bases that are weeks from reconciling with drm-rust-next.

The current v5 applies (almost) cleanly on today's drm-rust-next, so it
is fair game to review and merge if deemed ready. We can rebase our r000
code on top of it should it lands first.

Re: [PATCH v6 0/3] gpu: nova-core: retain the GSP-RM log buffers

[John Hubbard]

On 11/6/26 2:52 PM, Danilo Krummrich wrote:
> On Fri Nov 6, 2026 at 9:51 PM CET, Timur Tabi wrote:
>> This tree is the future of rust-drm-next, and there have been some significant changes to
>> debugfs that result in a lot of merge conflicts.
> 
> I know what you mean, but just to avoid confusion for people new to the
> community: It's not exactly the future of drm-rust-next, but work that is
> targeting to land in drm-rust-next in the future.
> 
> As for this patch series, it sits on my list of patches to deal with (for
> probably too long). It also has been on the list for a couple of month now and I
> don't think it should be delayed further, so I'll pick this up after some smoke
> testing.
> 
This is great timing. I'm debugging something with Timur Tabi, and need
this functionality. Although "fortunately", I've also broken GSP booter
unload too, so logs are accidentally retained at the moment, haha. :)

Anyway, I've give this a quick test on that setup, and maybe can
provide a Tested-by, let's see.

thanks,
-- 
John Hubbard

Re: [PATCH v6 0/3] gpu: nova-core: retain the GSP-RM log buffers

[John Hubbard]

On 10/14/26 8:45 AM, Vladislav Zaharov wrote:
> The GSP-RM log buffers are exposed through debugfs, but they are DMA
> allocations of the device, so they go away as soon as the GPU is unbound,
> and in particular when probe() fails. That is the case todo.rst asks
> about, and the one where a GSP log is most useful.
> 
> Patch 1 moves the debugfs root into the module data. Patch 2 adds a
> gsp_keep_logs module parameter: when it is set, whatever the GSP wrote
> is copied into module memory and kept under a "retained" directory until
> the module is unloaded. Patch 3 drops the completed item from todo.rst.
> 
> v5 got no comments, so this version only carries one fix and a rebase.
> 
> Changes since v5:
>  - keep the "put" pointer read before the barrier in the copy, rather
>    than the one the copy reads again, which is not ordered against the
>    rest of the data (reported by the Sashiko review bot)
>  - rebase onto current drm-rust-next
> 
> Tested on a GB203 on top of drm-rust-next with the TLV firmware images:
> 
>   - without gsp_keep_logs nothing changes, and no "retained" directory
>     is created;
>   - with gsp_keep_logs=1, retained/<BDF>/{loginit,logintr,logrm} hold
>     the full 64 KiB of each buffer after unbind;

Although this "works", it has a significant usability issue that I think
we need not accept. We should fix it instead.

Specifically, (as you mention above), I had to add "retained/" to the
directory path in order to retrieve the logs. That is awkward, because
that means that if I ask for gsp_keep_logs=1, now I have to know/remember
that they are in:

    /sys/kernel/debug/nova-core/retained/<BDF>/

instead of:

    /sys/kernel/debug/nova-core/<BDF>

We should try hard to avoid such complications, especially given that
debugging is often done when one has a thousand other complicated things
going on at the same time, late at night, too.

I did a very quick LLM-powered fix on top of your series, and it seems to
work just fine without the "retained/" subdir.

Can you please do that? I can look more closely and provide details and/or
help, if necessary, I have the PoC diffs here.

Let me know how I can help.

thanks,
-- 
John Hubbard


>   - binding again does not disturb the copies, and a second unbind
>     replaces them, leaving one set;
>   - with a failure injected after the GSP has booted, probe() fails with
>     -EINVAL and the logs of that attempt are still readable;
>   - with a failure injected into the driver registration, the module
>     fails to load, leaves no debugfs directory behind, and the next load
>     works;
>   - repeated load/unload cycles leave nothing behind.
> 
> No warnings or oopses in dmesg. Each patch builds on its own, and the
> series is clean with CLIPPY=1, rustfmtcheck and checkpatch --strict
> (apart from the MAINTAINERS note for the new file, which is already
> covered by "F: drivers/gpu/nova-core/").
> 
> v5: https://lore.kernel.org/nova-gpu/20260923045551.229259-1-vladazaharova2018@gmail.com/
> 
> Vladislav Zaharov (3):
>   gpu: nova-core: move the debugfs root into the module data
>   gpu: nova-core: gsp: retain the GSP-RM log buffers after unbind
>   Documentation: nova: remove completed GSP log buffer task
> 
>  Documentation/gpu/nova/core/todo.rst   |  12 --
>  drivers/gpu/nova-core/gsp.rs           | 100 ++--------
>  drivers/gpu/nova-core/gsp/logbuffer.rs | 255 +++++++++++++++++++++++++
>  drivers/gpu/nova-core/nova_core.rs     | 101 ++++++++--
>  4 files changed, 356 insertions(+), 112 deletions(-)
>  create mode 100644 drivers/gpu/nova-core/gsp/logbuffer.rs
> 
> 
> base-commit: 5a1b7121a6c4db6c0239d3ac3fd1c2bb33a40025

Re: [PATCH v6 1/3] gpu: nova-core: move the debugfs root into the module data

[Alexandre Courbot]

On Thu Oct 15, 2026 at 12:45 AM JST, Vladislav Zaharov wrote:
> The debugfs root lives in a static that init() fills in and a guard
> field of the module data clears again. That costs a `static mut`, an
> unsafe write on each side and a guard type whose only job is to undo
> the write.
>
> It also leaks. try_pin_init! drops only the fields it has already
> built, and the guard is written after the Registration, so a
> registration that fails leaves the guard unbuilt and the static set.
> Statics are never dropped, and the module is unloaded right away, so
> the directory outlives everything that could remove it. The next load
> then finds the name taken: debugfs_create_dir() returns -EEXIST, which
> Entry keeps as it would any other pointer, and the driver comes up with
> no debugfs at all until the machine is rebooted.
>
> Have the module data own a DebugfsData instead, built before the
> registration and dropped after it, and keep only a pointer to it in the
> static, for devices that have no other way to reach the data of their
> module. What is left is one unsafe read for the users and a single write
> when the data is built, with no guard type. A registration that fails
> now drops the data that was built before it, and the directory goes with
> it.
>
> Assisted-by: LLM
> Signed-off-by: Vladislav Zaharov <vladazaharova2018@gmail.com>

Fixes: 09691f5d8070 ("gpu: nova-core: create debugfs root in module init")

> ---
>  drivers/gpu/nova-core/gsp.rs       | 10 +---
>  drivers/gpu/nova-core/nova_core.rs | 74 ++++++++++++++++++++++--------
>  2 files changed, 56 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
> index dda58095f40b..01ed4adffe93 100644
> --- a/drivers/gpu/nova-core/gsp.rs
> +++ b/drivers/gpu/nova-core/gsp.rs
> @@ -199,15 +199,7 @@ pub(crate) fn new(
>                          logrm,
>                      };
>  
> -                    #[allow(static_mut_refs)]
> -                    // SAFETY: `DEBUGFS_ROOT` is created before driver registration and cleared
> -                    // after driver unregistration, so no probe() can race with its modification.
> -                    //
> -                    // PANIC: `DEBUGFS_ROOT` cannot be `None` here.  It is set before driver
> -                    // registration and cleared after driver unregistration, so it is always
> -                    // `Some` for the entire lifetime that probe() can be called.
> -                    let log_parent: &debugfs::Dir = unsafe { crate::DEBUGFS_ROOT.as_ref() }
> -                        .expect("DEBUGFS_ROOT not initialized");
> +                    let log_parent: &debugfs::Dir = crate::debugfs_data(dev).root();
>  
>                      log_parent.scope(log_buffers, dev.name(), |logs, dir| {
>                          dir.read_binary_file(c"loginit", &logs.loginit.0);
> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index a8f6dc22057b..ee3a6600f320 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -4,6 +4,7 @@
>  
>  use kernel::{
>      debugfs,
> +    device,
>      driver::Registration,
>      pci,
>      prelude::*,
> @@ -32,40 +33,75 @@
>  
>  pub(crate) const MODULE_NAME: &core::ffi::CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
>  
> -// TODO: Move this into per-module data once that exists.
> -static mut DEBUGFS_ROOT: Option<debugfs::Dir> = None;
> +/// Pointer to the [`DebugfsData`] the module owns.
> +///
> +/// A device has no way to reach the data of its module, so code running on behalf of a bound

nit: "A device currently has no way", this is a temporary limitation
(hopefully).

> +/// device goes through here instead.
> +///
> +/// Written once, while the module data is built, and never again: what it points at is dropped
> +/// only after the driver is unregistered, so it is good for as long as any device is bound, and
> +/// is not read outside of that.

This is some top-class Claudespeak. Let's rephrase it into invariants
that we can refer to later:

/// # Invariants
///
/// - Only written once before the driver is registered.
/// - Points to a valid [`DebugfsData`] for as long as the driver is registered.

> +// TODO: Drop this once devices can reach the data of their module.
> +static mut DEBUGFS_DATA: *const DebugfsData = core::ptr::null();
>  
> -/// Guard that clears `DEBUGFS_ROOT` when dropped.
> -struct DebugfsRootGuard;
> +/// Data the module shares with every GPU it drives.
> +///
> +/// Reached from a device through [`debugfs_data()`].
> +#[pin_data]
> +pub(crate) struct DebugfsData {
> +    /// Root directory of the driver in debugfs.
> +    root: debugfs::Dir,
> +}
> +
> +impl DebugfsData {
> +    /// Creates the shared data.
> +    fn new() -> impl PinInit<Self> {
> +        pin_init!(Self {
> +            root: debugfs::Dir::new(c"nova-core"),
> +        })
> +    }
>  
> -impl Drop for DebugfsRootGuard {
> -    fn drop(&mut self) {
> -        // SAFETY: This guard is dropped after `_driver` (due to field order),
> -        // so the driver is unregistered and no probe() can be running.
> -        unsafe { DEBUGFS_ROOT = None };
> +    /// Returns the root directory of the driver in debugfs.
> +    pub(crate) fn root(&self) -> &debugfs::Dir {
> +        &self.root
>      }
>  }
>  
> +/// Returns the data the module shares with its devices.
> +///
> +/// The bound device is what makes this sound: the data is built before the driver is registered
> +/// and dropped after it is unregistered, so it outlives every device that is bound, and the
> +/// returned reference cannot be held past the one it is taken for.

You don't need to explain why this method is sound, the method is not
marked "unsafe", thus it is safe and that's all callers care about.
Anything more is unneeded cognitive burden and readers will wonder why
they need to know that.

The SAFETY comment below, which gives the same justification, is the
correct place to explain that and already does it.

> +pub(crate) fn debugfs_data<'a>(_dev: &'a device::Device<device::Bound>) -> &'a DebugfsData {
> +    // SAFETY: A device can only be bound once the driver is registered, which happens after
> +    // `DEBUGFS_DATA` is written, so it points at the data of this module, which lives at least
> +    // as long as the caller's device is bound.

... then here you can just invoke the invariant we defined above:

// SAFETY: `_dev` is bound, so the driver is registered. Per the
// invariants of `DEBUGFS_DATA`, the pointer is thus valid and not being
// written to. The `'a` lifetime ensures that the returned reference
// cannot outlive the bound device.

(technically one could pass another `Device` that is not bound to this
driver, but I guess we can overlook that for this private, temporary
interface).

> +    unsafe { &*DEBUGFS_DATA }
> +}
> +
>  #[pin_data]
>  struct NovaCoreModule {
> -    // Fields are dropped in declaration order, so `_driver` is dropped first,
> -    // then `_debugfs_guard` clears `DEBUGFS_ROOT`.
> +    // Fields are dropped in declaration order, so the registration goes first and no probe() can
> +    // still be running once the shared data is torn down. `init()` builds them the other way
> +    // round, as the data has to be there before the first probe() reaches for it.

Here again we are not concerned with what `init` does, we only use this
comment to prevent someone from breaking the drop order we rely on:

// Fields are dropped in declaration order. `_driver` must be dropped
// before `_debugfs` to uphold the invariant of `DEBUGFS_DATA` that it
// remains valid for as long as the driver is registered.

>      #[pin]
>      _driver: Registration<pci::Adapter<driver::NovaCoreDriver>>,
> -    _debugfs_guard: DebugfsRootGuard,
> +    #[pin]
> +    _debugfs: DebugfsData,
>  }
>  
>  impl InPlaceModule for NovaCoreModule {
>      fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self, Error> {
> -        let dir = debugfs::Dir::new(c"nova-core");
> -
> -        // SAFETY: We are the only driver code running during init, so there
> -        // cannot be any concurrent access to `DEBUGFS_ROOT`.
> -        unsafe { DEBUGFS_ROOT = Some(dir) };
> -
>          try_pin_init!(Self {
> +            _debugfs <- DebugfsData::new(),
> +            _: {
> +                // SAFETY: Nothing reads `DEBUGFS_DATA` before a device is bound, which cannot
> +                // happen until the driver is registered below. What it points at is dropped only
> +                // once the driver is unregistered, so it is valid for as long as any device is
> +                // bound, and nothing reads it outside of that.

This can just be:

// SAFETY: The driver is not registered yet, meaning that no device can
// be probed and thus nothing can access `DEBUGFS_DATA` concurrently.

And since this is also where we enforce our invariant:

// INVARIANT: `DEBUGFS_DATA` is written before the driver is registered
// below and points to `_debugfs`, which is pinned and dropped after
// `_driver`.

Re: [PATCH v6 2/3] gpu: nova-core: gsp: retain the GSP-RM log buffers after unbind

[Alexandre Courbot]

On Thu Oct 15, 2026 at 12:45 AM JST, Vladislav Zaharov wrote:
> The GSP-RM log buffers are exposed through debugfs, but the Scope that
> owns them lives in Gsp, inside GspResources, inside the Gpu built by
> probe(). They are DMA allocations of the device and cannot outlive it,
> so the entries go away as soon as the GPU is unbound - and, more to the
> point, as soon as probe() fails, which is exactly when the log of a GSP
> that did not come up is the thing one wants to read.
>
> Add a gsp_keep_logs module parameter. When it is set, dropping the log
> buffers copies whatever the GSP wrote into memory owned by the module
> and exposes the copies until the module is unloaded. A buffer whose
> "put" pointer is still zero was never written to and is skipped.
>
> The GSP has normally been stopped by the time the buffers are dropped,
> but a boot that timed out can leave it still appending, so a DMA read
> barrier orders the read of the "put" pointer before the copy, and that
> value, not the one the copy reads again, is what the copy keeps.

That paragraph is nice as a code comment, but that's an implementation
detail that doesn't belong to a commit log. The commit log should tell
us *why* you are doing this, not go into excruciating details of *how*
you did it - only the big lines to make review easier.

I'd appreciate if you could do a pass on the commit logs and code
comments to deslop them and make them shorter and to the point
(similarly to what I tried to do on patch 1). Right now this patch
is pretty heavy to read.

>
> The copies belong to the module data next to the debugfs root, and live
> in a "retained" directory created during module init rather than on
> first use, which keeps the teardown path of a device from having to
> create anything. Keeping them out of the directory used by bound GPUs
> also means a device coming back does not find its debugfs name taken by
> its own history; nouveau, which recreates the entries under the name of
> the GPU that just went away, has that problem.

What Nouveau does is irrevelant - that's the kind of unneeeded
information I'd like us to avoid.

>
> While at it, move the log buffer code out of gsp.rs into gsp/logbuffer.rs.

"While at it" means this should be two patches: a first one that moves
the code into `gsp/logbuffer.rs`, a second one that performs the changes
you need there.

Right now this moves things while changing them, making it more
difficult to review than it could be.

Re: [PATCH v6 0/3] gpu: nova-core: retain the GSP-RM log buffers

[Danilo Krummrich]

On Sat Nov 7, 2026 at 1:39 AM CET, John Hubbard wrote:
> Specifically, (as you mention above), I had to add "retained/" to the
> directory path in order to retrieve the logs. That is awkward, because
> that means that if I ask for gsp_keep_logs=1, now I have to know/remember
> that they are in:
>
>     /sys/kernel/debug/nova-core/retained/<BDF>/
>
> instead of:
>
>     /sys/kernel/debug/nova-core/<BDF>
>
> We should try hard to avoid such complications, especially given that
> debugging is often done when one has a thousand other complicated things
> going on at the same time, late at night, too.

It doesn't bother me too much, but I think the concern is fair.

The retained directory keeps a clear separation between live buffers and
retained buffers, but I don't think that's a strong reason to keep it.

In nouveau we don't have that separation either, but nouveau also keeps the
retained buffers until module unload. I.e. after the first retention nouveau
never exposes new buffers (including the live ones) unless you reload the
module. This works, but can be rather unexpected when e.g. doing multiple
rebinds via sysfs.

Rebinding via sysfs can be especially useful in multi-GPU setups, where you
don't want to unbind all GPUs by unloading the module.

So, getting rid of retained/ is fine, but let's not follow what nouveau does.
Instead, let's make the same files point to the live buffers when the device is
bound and otherwise let them point at the retained buffers.

Thanks,
Danilo

Re: [PATCH v6 0/3] gpu: nova-core: retain the GSP-RM log buffers

[John Hubbard]

On 11/6/26 5:38 PM, Danilo Krummrich wrote:
> On Sat Nov 7, 2026 at 1:39 AM CET, John Hubbard wrote:
>> Specifically, (as you mention above), I had to add "retained/" to the
>> directory path in order to retrieve the logs. That is awkward, because
>> that means that if I ask for gsp_keep_logs=1, now I have to know/remember
>> that they are in:
>>
>>     /sys/kernel/debug/nova-core/retained/<BDF>/
>>
>> instead of:
>>
>>     /sys/kernel/debug/nova-core/<BDF>
>>
>> We should try hard to avoid such complications, especially given that
>> debugging is often done when one has a thousand other complicated things
>> going on at the same time, late at night, too.
> 
> It doesn't bother me too much, but I think the concern is fair.
> 
> The retained directory keeps a clear separation between live buffers and
> retained buffers, but I don't think that's a strong reason to keep it.
> 
> In nouveau we don't have that separation either, but nouveau also keeps the
> retained buffers until module unload. I.e. after the first retention nouveau
> never exposes new buffers (including the live ones) unless you reload the
> module. This works, but can be rather unexpected when e.g. doing multiple
> rebinds via sysfs.
> 
> Rebinding via sysfs can be especially useful in multi-GPU setups, where you
> don't want to unbind all GPUs by unloading the module.
> 
> So, getting rid of retained/ is fine, but let's not follow what nouveau does.
> Instead, let's make the same files point to the live buffers when the device is
> bound and otherwise let them point at the retained buffers.

This sounds right and proper, yes. :)


thanks,
-- 
John Hubbard