gpu: nova-core: expose the logging buffers via debugfs

Add a name() method to the Device type, which returns a CStr that contains the device name, typically the BDF address for PCI devices.

Signed-off-by Timur Tabi <[email protected]>

---
 rust/helpers/device.c |  5 +++++
 rust/kernel/device.rs | 16 ++++++++++++++++
 2 files changed, 21 insertions(+)

rust/helpers/device.c +0 -0

index 9a4316bafedf..4609b62f6a06 100644

rust/helpers/device.c +3 -0

+{
+	return dev_name(dev);
+}

rust/kernel/device.rs +0 -0

index c79be2e2bfe3..636c522a8374 100644

rust/kernel/device.rs +9 -0

+    /// - DDDD is the PCI domain (4 hex digits)
+    /// - BB is the bus number (2 hex digits)
+    /// - DD is the device number (2 hex digits)
+    /// - F is the function number (1 hex digit)
+    pub fn name(&self) -> &CStr {
+        // SAFETY: By its type invariant `self.as_raw()` is a valid pointer to a `struct device`.
+        // The returned string is valid for the lifetime of the device.
+        unsafe { CStr::from_char_ptr(bindings::dev_name(self.as_raw())) }
+    }
 }
 
 // SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic

Signature · 1 line

2.52.0

rust/helpers/device.c void rust_helper_dev_set_drvdata(struct device *dev, void *data) · line 29

Quoted 8 lines

rust/helpers/device.c +2 -0

@@ -25,3 +25,8 @@ void rust_helper_dev_set_drvdata(struct device *dev, void *data)
  {
  	dev_set_drvdata(dev, data);
  }
+
+const char *rust_helper_dev_name(const struct device *dev)

Please add the __rust_helper annotation.

Quoted 9 lines

rust/kernel/device.rs +4 -0

+
+    /// Returns the name of the device.
+    ///
+    /// This is the kobject name of the device, or its initial name if the kobject is not

yet
+    /// available.
+    ///
+    /// For PCI devices, the name in the format "DDDD:BB:DD.F" where:

This is driver core code, please drop any details for PCI. Also, the subject of the patch seems wrong.

Quoted 10 lines · Danilo Krummrich

rust/helpers/device.c void rust_helper_dev_set_drvdata(struct device *dev, void *data) · line 29

Quoted 8 lines

rust/helpers/device.c +2 -0

@@ -25,3 +25,8 @@ void rust_helper_dev_set_drvdata(struct device *dev, void *data)
  {
  	dev_set_drvdata(dev, data);
  }
+
+const char *rust_helper_dev_name(const struct device *dev)

Please add the __rust_helper annotation.

Should I add it to all of the functions in this file? None of the existing functions have "__rust_helper".

Add Dir::empty() which returns a &'static reference to a no-op Dir. All file and subdirectory operations on this directory silently succeed without creating actual filesystem entries.

This enables callers to use a single code path regardless of whether debugfs is available at runtime. For example:

    let dir = optional_debugfs_root.unwrap_or_else(|| Dir::empty());
    dir.scope(data, name, |d, s| { ... })

The returned reference has 'static lifetime, allowing initializers returned by scope() to be used outside the immediate scope where the Dir reference was obtained.

Signed-off-by Timur Tabi <[email protected]>

---
 rust/kernel/debugfs.rs | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

rust/kernel/debugfs.rs +6 -0

index facad81e8290..44ff2b37786a 100644
@@ -110,6 +110,24 @@ pub fn new(name: &CStr) -> Self {
         Dir::create(name, None)
     }
 
+    /// Returns a reference to a static no-op directory that doesn't create any debugfs entries.
+    ///
+    /// All file and subdirectory creation operations on this directory will silently succeed
+    /// without creating actual filesystem entries. This is useful when you want to conditionally
+    /// enable debugfs but use the same code path regardless.
+    pub fn empty() -> &'static Self {

rust/kernel/debugfs.rs +2 -0

+    }
+
     /// Creates a subdirectory within this directory.
     ///
     /// # Examples

Add Dir::empty() which returns a &'static reference to a no-op Dir. All file and subdirectory operations on this directory silently succeed without creating actual filesystem entries.

This enables callers to use a single code path regardless of whether debugfs is available at runtime. For example:

    let dir = optional_debugfs_root.unwrap_or_else(|| Dir::empty());
    dir.scope(data, name, |d, s| { ... })

The returned reference has 'static lifetime, allowing initializers returned by scope() to be used outside the immediate scope where the Dir reference was obtained.

Signed-off-by Timur Tabi <[email protected]>

---
 rust/kernel/debugfs.rs | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

rust/kernel/debugfs.rs +18 -1

index facad81e8290..44ff2b37786a 100644
@@ -110,6 +110,24 @@ pub fn new(name: &CStr) -> Self {
         Dir::create(name, None)
     }
 
+    /// Returns a reference to a static no-op directory that doesn't create any debugfs entries.
+    ///
+    /// All file and subdirectory creation operations on this directory will silently succeed
+    /// without creating actual filesystem entries. This is useful when you want to conditionally
+    /// enable debugfs but use the same code path regardless.
+    pub fn empty() -> &'static Self {
+        #[cfg(CONFIG_DEBUG_FS)]
+        {
+            static EMPTY: Dir = Dir(None);
+            &EMPTY
+        }
+        #[cfg(not(CONFIG_DEBUG_FS))]
+        {
+            static EMPTY: Dir = Dir();
+            &EMPTY
+        }
+    }
+
     /// Creates a subdirectory within this directory.
     ///
     /// # Examples
--

2.52.0

I thought the whole point of Greg's admonition to never report errors and to automatically create no-op structures if debugfs was disable was that we *don't* do things like this, and instead you just do something like

```
// Always do this, whether or not debugfs is available.
let debugfs_root = Dir::new("my_thing");
debugfs_root.scope(data, name, |d, s| { ... })
```

If DebugFS is not available, it already returns an empty fake directory. That variant of a fake directory is also size 0, so you aren't paying anything to use it...

Add a new method to UserSliceWriter that copies data from a raw kernel pointer to userspace, without requiring a Rust slice reference.

The method takes:
- data: raw pointer to the source buffer
- len: total size of the source buffer (for bounds checking)
- offset: byte offset into the source buffer to start copying from
- count: number of bytes to copy

The method is marked unsafe because the caller must ensure the pointer is valid for the specified length and that the memory is not mutated during the call.

Signed-off-by Timur Tabi <[email protected]>

---
 rust/kernel/uaccess.rs | 50 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

rust/kernel/uaccess.rs +18 -0

index f989539a31b4..8bbb0084abb1 100644
@@ -481,6 +481,56 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
         Ok(())
     }
 
+    /// Writes raw data to this user pointer from a raw kernel pointer.
+    ///
+    /// This is similar to [`Self::write_slice`] but takes a raw pointer instead of a slice,
+    /// along with a total buffer length, an offset into the that buffer, and a count of bytes
+    /// to copy.
+    ///
+    /// Returns error if the offset+count exceeds the buffer size.
+    ///
+    /// Fails with [`EFAULT`] if the write happens on a bad address, or if the write goes out of
+    /// bounds of this [`UserSliceWriter`]. This call may modify the associated userspace slice
+    /// even if it returns an error.
+    ///
+    /// # Safety
+    ///
+    /// - `data` must point to a valid memory region of at least `len` bytes that remains allocated
+    ///   for the duration of this call.
+    ///
+    /// Note: Unlike [`Self::write_slice`], this method does not require exclusive access to the

(Cc: Alice)

Alice, for context, this is used when exporting a DMA buffer through debugfs, while the DMA buffer may be in use by the device, i.e. no slice can be created.

Quoted 44 lines · Timur Tabi

Add a new method to UserSliceWriter that copies data from a raw kernel pointer to userspace, without requiring a Rust slice reference.

The method takes:
- data: raw pointer to the source buffer
- len: total size of the source buffer (for bounds checking)
- offset: byte offset into the source buffer to start copying from
- count: number of bytes to copy

The method is marked unsafe because the caller must ensure the pointer is valid for the specified length and that the memory is not mutated during the call.

Signed-off-by Timur Tabi <[email protected]>

---
 rust/kernel/uaccess.rs | 50 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

rust/kernel/uaccess.rs +18 -0

index f989539a31b4..8bbb0084abb1 100644
@@ -481,6 +481,56 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
         Ok(())
     }
 
+    /// Writes raw data to this user pointer from a raw kernel pointer.
+    ///
+    /// This is similar to [`Self::write_slice`] but takes a raw pointer instead of a slice,
+    /// along with a total buffer length, an offset into the that buffer, and a count of bytes
+    /// to copy.
+    ///
+    /// Returns error if the offset+count exceeds the buffer size.
+    ///
+    /// Fails with [`EFAULT`] if the write happens on a bad address, or if the write goes out of
+    /// bounds of this [`UserSliceWriter`]. This call may modify the associated userspace slice
+    /// even if it returns an error.
+    ///
+    /// # Safety
+    ///
+    /// - `data` must point to a valid memory region of at least `len` bytes that remains allocated
+    ///   for the duration of this call.
+    ///
+    /// Note: Unlike [`Self::write_slice`], this method does not require exclusive access to the

rust/kernel/uaccess.rs pub fn write_slice(&mut self, data: &[u8]) -> Result { · line 511

Quoted 10 lines

rust/kernel/uaccess.rs +10 -0

+    /// source memory. The memory may be concurrently modified by other threads or hardware (e.g.,
+    /// DMA buffers). In such cases, the copied data may be inconsistent, but this does not cause
+    /// undefined behavior.
+    pub unsafe fn write_buffer(
+        &mut self,
+        data: *const u8,
+        len: usize,
+        offset: usize,
+        count: usize,
+    ) -> Result {

Instead of this we could probably also add a safe method write_dma() that takes a dma::CoherentAllocation<u8> instead. Once we have generic I/O in place, this could be replaced with a generic write_io() method.

Add a new method to UserSliceWriter that copies data from a raw kernel pointer to userspace, without requiring a Rust slice reference.

The method takes:
- data: raw pointer to the source buffer
- len: total size of the source buffer (for bounds checking)
- offset: byte offset into the source buffer to start copying from
- count: number of bytes to copy

The method is marked unsafe because the caller must ensure the pointer is valid for the specified length and that the memory is not mutated during the call.

Signed-off-by Timur Tabi <[email protected]>

---
 rust/kernel/uaccess.rs | 50 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

rust/kernel/uaccess.rs +18 -0

index f989539a31b4..8bbb0084abb1 100644
@@ -481,6 +481,56 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
         Ok(())
     }
 
+    /// Writes raw data to this user pointer from a raw kernel pointer.
+    ///
+    /// This is similar to [`Self::write_slice`] but takes a raw pointer instead of a slice,
+    /// along with a total buffer length, an offset into the that buffer, and a count of bytes
+    /// to copy.
+    ///
+    /// Returns error if the offset+count exceeds the buffer size.
+    ///
+    /// Fails with [`EFAULT`] if the write happens on a bad address, or if the write goes out of
+    /// bounds of this [`UserSliceWriter`]. This call may modify the associated userspace slice
+    /// even if it returns an error.
+    ///
+    /// # Safety
+    ///
+    /// - `data` must point to a valid memory region of at least `len` bytes that remains allocated
+    ///   for the duration of this call.
+    ///
+    /// Note: Unlike [`Self::write_slice`], this method does not require exclusive access to the

Quoted 44 lines · Timur Tabi

Add a new method to UserSliceWriter that copies data from a raw kernel pointer to userspace, without requiring a Rust slice reference.

The method takes:
- data: raw pointer to the source buffer
- len: total size of the source buffer (for bounds checking)
- offset: byte offset into the source buffer to start copying from
- count: number of bytes to copy

The method is marked unsafe because the caller must ensure the pointer is valid for the specified length and that the memory is not mutated during the call.

Signed-off-by Timur Tabi <[email protected]>

---
 rust/kernel/uaccess.rs | 50 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

rust/kernel/uaccess.rs +18 -0

index f989539a31b4..8bbb0084abb1 100644
@@ -481,6 +481,56 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
         Ok(())
     }
 
+    /// Writes raw data to this user pointer from a raw kernel pointer.
+    ///
+    /// This is similar to [`Self::write_slice`] but takes a raw pointer instead of a slice,
+    /// along with a total buffer length, an offset into the that buffer, and a count of bytes
+    /// to copy.
+    ///
+    /// Returns error if the offset+count exceeds the buffer size.
+    ///
+    /// Fails with [`EFAULT`] if the write happens on a bad address, or if the write goes out of
+    /// bounds of this [`UserSliceWriter`]. This call may modify the associated userspace slice
+    /// even if it returns an error.
+    ///
+    /// # Safety
+    ///
+    /// - `data` must point to a valid memory region of at least `len` bytes that remains allocated
+    ///   for the duration of this call.
+    ///
+    /// Note: Unlike [`Self::write_slice`], this method does not require exclusive access to the

rust/kernel/uaccess.rs pub fn write_slice(&mut self, data: &[u8]) -> Result { · line 511

Quoted 10 lines

rust/kernel/uaccess.rs +10 -0

+    /// source memory. The memory may be concurrently modified by other threads or hardware (e.g.,
+    /// DMA buffers). In such cases, the copied data may be inconsistent, but this does not cause
+    /// undefined behavior.
+    pub unsafe fn write_buffer(
+        &mut self,
+        data: *const u8,
+        len: usize,
+        offset: usize,
+        count: usize,
+    ) -> Result {

Why not this signature?

	unsafe fn write_raw_slice(&mut self, data: *const [u8]) -> Result;

You can implement write_slice in terms of it.

Quoted 11 lines · Alice Ryhl

rust/kernel/uaccess.rs pub fn write_slice(&mut self, data: &[u8]) -> Result { · line 511

Quoted 7 lines · Timur Tabi

rust/kernel/uaccess.rs +7 -0

+    pub unsafe fn write_buffer(
+        &mut self,
+        data: *const u8,
+        len: usize,
+        offset: usize,
+        count: usize,
+    ) -> Result {

Why not this signature?

Quoted 3 lines

	unsafe fn write_raw_slice(&mut self, data: *const [u8]) -> Result;

You can implement write_slice in terms of it.

To clarify, I think this would be a simpler signature for write_buffer(). And write_raw_slice() can be used both for DMA and to simplify the existing write_slice.

Quoted 12 lines · Alice Ryhl

Quoted 11 lines · Alice Ryhl

rust/kernel/uaccess.rs pub fn write_slice(&mut self, data: &[u8]) -> Result { · line 511

Quoted 7 lines · Timur Tabi

rust/kernel/uaccess.rs +7 -0

+    pub unsafe fn write_buffer(
+        &mut self,
+        data: *const u8,
+        len: usize,
+        offset: usize,
+        count: usize,
+    ) -> Result {

Why not this signature?

Quoted 7 lines

Quoted 3 lines

	unsafe fn write_raw_slice(&mut self, data: *const [u8]) -> Result;

You can implement write_slice in terms of it.

To clarify, I think this would be a simpler signature for write_buffer(). And write_raw_slice() can be used both for DMA and to simplify the existing write_slice.

I.e. you can use it also to create a safe helper for DMA:

	fn write_dma(
	    &mut self,
	    data: &dma::CoherentAllocation<u8>,
	    offset: usize,
	    count: usize
	) -> Result;

Quoted 21 lines · Danilo Krummrich

Quoted 12 lines · Alice Ryhl

Quoted 11 lines · Alice Ryhl

rust/kernel/uaccess.rs pub fn write_slice(&mut self, data: &[u8]) -> Result { · line 511

Quoted 7 lines · Timur Tabi

rust/kernel/uaccess.rs +7 -0

+    pub unsafe fn write_buffer(
+        &mut self,
+        data: *const u8,
+        len: usize,
+        offset: usize,
+        count: usize,
+    ) -> Result {

Why not this signature?

Quoted 7 lines

Quoted 3 lines

	unsafe fn write_raw_slice(&mut self, data: *const [u8]) -> Result;

You can implement write_slice in terms of it.

To clarify, I think this would be a simpler signature for write_buffer(). And write_raw_slice() can be used both for DMA and to simplify the existing write_slice.

Quoted 8 lines

I.e. you can use it also to create a safe helper for DMA:

	fn write_dma(
	    &mut self,
	    data: &dma::CoherentAllocation<u8>,
	    offset: usize,
	    count: usize
	) -> Result;

Would it make sense to expose a &CoherentAllocation<u8> -> &[Atomic<u8>] conversion method and have a write_slice(&mut self, data: &[Atomic<[u8]>) for UserSliceWriter?

From: Alexandre Courbot <[email protected]>

LogBuffer is the entity we ultimately want to dump through debugfs. Provide a simple implementation of BinaryWriter for it, albeit it might not cut the safety requirements.

Signed-off-by Alexandre Courbot <[email protected]>

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/gsp.rs | 40 ++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

drivers/gpu/nova-core/gsp.rs +1 -0

index 766fd9905358..273327c33aa7 100644
@@ -3,6 +3,7 @@
 mod boot;
 
 use kernel::{
+    debugfs,
     device,
     dma::{
         CoherentAllocation,

From: Alexandre Courbot <[email protected]>

LogBuffer is the entity we ultimately want to dump through debugfs. Provide a simple implementation of BinaryWriter for it, albeit it might not cut the safety requirements.

Signed-off-by Alexandre Courbot <[email protected]>

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/gsp.rs | 40 ++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

drivers/gpu/nova-core/gsp.rs +29 -0

index 766fd9905358..273327c33aa7 100644
@@ -3,6 +3,7 @@
 mod boot;
 
 use kernel::{
+    debugfs,
     device,
     dma::{
         CoherentAllocation,

@@ -117,6 +118,45 @@ pub(crate) struct Gsp {
     rmargs: CoherentAllocation<GspArgumentsCached>,
 }
 
+impl debugfs::BinaryWriter for LogBuffer {
+    fn write_to_slice(
+        &self,
+        writer: &mut kernel::uaccess::UserSliceWriter,
+        offset: &mut kernel::fs::file::Offset,
+    ) -> Result<usize> {
+        if offset.is_negative() {
+            return Err(EINVAL);
+        }
+
+        let offset_val: usize = (*offset).try_into().map_err(|_| EINVAL)?;
+        let len = self.0.count();
+
+        if offset_val >= len {
+            return Ok(0);
+        }
+
+        let count = (len - offset_val).min(writer.len());
+
+        // SAFETY:
+        // - `start_ptr()` returns a valid pointer to a memory region of `count()` bytes,
+        //   as guaranteed by the `CoherentAllocation` invariants.
+        // - `len` equals `self.0.count()`, so the pointer is valid for `len` bytes.
+        // - `offset_val < len` is guaranteed by the check above.
+        // - `count = (len - offset_val).min(writer.len())`, so `offset_val + count <= len`.
+        unsafe { writer.write_buffer(self.0.start_ptr(), len, offset_val, count)? };
+
+        *offset += count as i64;

drivers/gpu/nova-core/gsp.rs +1 -0

+
 impl Gsp {
     // Creates an in-place initializer for a `Gsp` manager for `pdev`.
     pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error> + '_ {

drivers/gpu/nova-core/gsp.rs pub(crate) struct Gsp { · line 158

Quoted 6 lines

drivers/gpu/nova-core/gsp.rs +6 -0

+// SAFETY: `LogBuffer` only provides shared access to the underlying `CoherentAllocation`.
+// GSP may write to the buffer concurrently regardless of CPU access, so concurrent reads
+// from multiple CPU threads do not introduce any additional races beyond what already
+// exists with the device. Reads may observe partially-written log entries, which is
+// acceptable for debug logging purposes.
+unsafe impl Sync for LogBuffer {}

Can we just implement Sync on CoherentAllocation?

Replace the module_pci_driver! macro with an explicit module initialization using the standard module! macro and InPlaceModule trait implementation. No functional change intended, with the exception that the driver now prints a message when loaded.

This change is necessary so that we can create a top-level "nova_core" debugfs entry when the driver is loaded.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/nova_core.rs | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

drivers/gpu/nova-core/nova_core.rs +0 -0

index c1121e7c64c5..d0df8db3693d 100644

drivers/gpu/nova-core/nova_core.rs +5 -0

+    pci,
+    prelude::*,
+    InPlaceModule, //
+};
+
 #[macro_use]
 mod bitfield;
 

drivers/gpu/nova-core/nova_core.rs line 33

Quoted 6 lines

drivers/gpu/nova-core/nova_core.rs +4 -2

-kernel::module_pci_driver! {
-    type: driver::NovaCore,
+#[pin_data]
+struct NovaCoreModule {
+    #[pin]
+    _driver: kernel::driver::Registration<pci::Adapter<driver::NovaCore>>,

Let's import driver, such that this becomes driver::Registration<_>.

Quoted 5 lines

+        })
+    }

drivers/gpu/nova-core/nova_core.rs +3 -0

+}
+
+module! {

Quoted 6 lines

+    type: NovaCoreModule,
      name: "NovaCore",
      authors: ["Danilo Krummrich"],
      description: "Nova Core GPU driver",
      license: "GPL v2",
-    firmware: [],

Should be a separate patch.

Create the 'nova_core' root debugfs entry when the driver loads.

Normally, non-const global variables need to be protected by a mutex. Instead, we use unsafe code, as we know the entry is never modified after the driver is loaded. This solves the lifetime issue of the mutex guard, which would otherwise have required the use of pin_init_scope.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/nova_core.rs | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

drivers/gpu/nova-core/nova_core.rs +1 -0

index d0df8db3693d..eceefa41cddc 100644
@@ -3,6 +3,7 @@
 //! Nova Core GPU Driver
 
 use kernel::{
+    debugfs::Dir,
     error::Error,
     pci,
     prelude::*,

drivers/gpu/nova-core/nova_core.rs +8 -0

+
+#[pin_data(PinnedDrop)]
 struct NovaCoreModule {
     #[pin]
     _driver: kernel::driver::Registration<pci::Adapter<driver::NovaCore>>,

@@ -35,12 +38,27 @@ struct NovaCoreModule {
 
 impl InPlaceModule for NovaCoreModule {
     fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self, Error> {
+        let dir = Dir::new(kernel::c_str!("nova_core"));
+
+        // SAFETY: we are the only driver code running, so there cannot be any concurrent access to
+        // `DEBUGFS_ROOT`.
+        unsafe { DEBUGFS_ROOT = Some(dir) };
+
         try_pin_init!(Self {

drivers/gpu/nova-core/nova_core.rs +3 -0

+    }
+}
+
 module! {
     type: NovaCoreModule,
     name: "NovaCore",

Signature · 1 line

2.52.0

Quoted 8 lines · Timur Tabi

  impl InPlaceModule for NovaCoreModule {
      fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self, Error> { +        let dir = Dir::new(kernel::c_str!("nova_core"));
+
+        // SAFETY: we are the only driver code running, so there cannot be any concurrent access to
+        // DEBUGFS_ROOT.
+        unsafe { DEBUGFS_ROOT = Some(dir) };

I think you forgot to add a new Kconfig to nova-core to enable this and in case not set leave it as None.

Quoted 10 lines · Danilo Krummrich

Quoted 8 lines · Timur Tabi

  impl InPlaceModule for NovaCoreModule {
      fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self, Error> { +        let dir = Dir::new(kernel::c_str!("nova_core"));
+
+        // SAFETY: we are the only driver code running, so there cannot be any concurrent access to
+        // DEBUGFS_ROOT.
+        unsafe { DEBUGFS_ROOT = Some(dir) };

Quoted 2 lines

I think you forgot to add a new Kconfig to nova-core to enable this and in case not set leave it as None.

You don't want the debugfs entries created by default? That's how it is in Nouveau.

Quoted 1 line

You don't want the debugfs entries created by default?  That's how it is in Nouveau.

I think it's OK to always have the entries on keeping them beyond device unbind has to be behind a Kconfig option.

I thought you want this behind a Kconfig in general, otherwise what is debugfs::Dir::empty() for?

Quoted 7 lines · Danilo Krummrich

Quoted 1 line

You don't want the debugfs entries created by default?  That's how it is in Nouveau.

I think it's OK to always have the entries on keeping them beyond device unbind has to be behind a Kconfig option.

I thought you want this behind a Kconfig in general, otherwise what is debugfs::Dir::empty() for?

Oh, I see what you're saying. Dir::empty() is for when CONFIGFS_DEBUG_FS is disabled. So obviously, I should not default DEBUGFS_ROOT to Some(dir) if CONFIGFS_DEBUG_FS is disabled.

Create read-only debugfs entries for LOGINIT, LOGRM, and LOGINTR, which are the three primary printf logging buffers from GSP-RM. LOGPMU will be added at a later date, as it requires it support for its RPC message first.

This patch uses the pin_init_scope feature to create the entries. pin_init_scope solves the lifetime issue over the DEBUGFS_ROOT reference by delaying its acquisition until the time the entry is actually initialized.

Co-developed-by Alexandre Courbot <[email protected]>

Signed-off-by Alexandre Courbot <[email protected]>

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/gpu/nova-core/gsp.rs | 45 +++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 8 deletions(-)

drivers/gpu/nova-core/gsp.rs +18 -8

index 273327c33aa7..4b194bb805b4 100644
@@ -3,6 +3,7 @@
 mod boot;
 
 use kernel::{
+    c_str,
     debugfs,
     device,
     dma::{

@@ -101,17 +102,24 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
     }
 }
 
-/// GSP runtime data.
-#[pin_data]
-pub(crate) struct Gsp {
-    /// Libos arguments.
-    pub(crate) libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
+/// Log buffers used by GSP-RM for debug logging.
+struct LogBuffers {
     /// Init log buffer.
     loginit: LogBuffer,
     /// Interrupts log buffer.
     logintr: LogBuffer,
     /// RM log buffer.
     logrm: LogBuffer,
+}
+
+/// GSP runtime data.
+#[pin_data]
+pub(crate) struct Gsp {
+    /// Libos arguments.
+    pub(crate) libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
+    /// Log buffers, optionally exposed via debugfs.
+    #[pin]
+    logs: debugfs::Scope<LogBuffers>,
     /// Command queue.
     pub(crate) cmdq: Cmdq,
     /// RM arguments.

@@ -163,15 +171,17 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
         pin_init::pin_init_scope(move || {
             let dev = pdev.as_ref();
 
+            // Create log buffers before try_pin_init! so they're accessible throughout
+            let loginit = LogBuffer::new(dev)?;
+            let logintr = LogBuffer::new(dev)?;
+            let logrm = LogBuffer::new(dev)?;
+
             Ok(try_pin_init!(Self {
                 libos: CoherentAllocation::<LibosMemoryRegionInitArgument>::alloc_coherent(
                     dev,
                     GSP_PAGE_SIZE / size_of::<LibosMemoryRegionInitArgument>(),
                     GFP_KERNEL | __GFP_ZERO,
                 )?,
-                loginit: LogBuffer::new(dev)?,
-                logintr: LogBuffer::new(dev)?,
-                logrm: LogBuffer::new(dev)?,
                 cmdq: Cmdq::new(dev)?,
                 rmargs: CoherentAllocation::<GspArgumentsCached>::alloc_coherent(
                     dev,

@@ -192,6 +202,25 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
                     dma_write!(rmargs[0] = fw::GspArgumentsCached::new(cmdq))?;
                     dma_write!(libos[3] = LibosMemoryRegionInitArgument::new("RMARGS", rmargs))?;
                 },

drivers/gpu/nova-core/gsp.rs +9 -0

+                    let debugfs_root = unsafe { crate::DEBUGFS_ROOT.as_ref() }
+                        .unwrap_or_else(|| debugfs::Dir::empty());
+
+                    debugfs_root.scope(log_buffers, dev.name(), |logs, dir| {
+                        dir.read_binary_file(c_str!("loginit"), &logs.loginit);
+                        dir.read_binary_file(c_str!("logintr"), &logs.logintr);
+                        dir.read_binary_file(c_str!("logrm"), &logs.logrm);
+                    })
+                },
             }))
         })
     }

Signature · 1 line

2.52.0