* [PATCH v6 1/7] rust: device: add device name method
2026-01-29 2:28 [PATCH v6 0/7] gpu: nova-core: expose the logging buffers via debugfs Timur Tabi
@ 2026-01-29 2:28 ` Timur Tabi
2026-01-29 2:28 ` [PATCH v6 2/7] rust: uaccess: add write_dma() for copying from DMA buffers to userspace Timur Tabi
` (5 subsequent siblings)
6 siblings, 0 replies; 25+ messages in thread
From: Timur Tabi @ 2026-01-29 2:28 UTC (permalink / raw)
To: Gary Guo, Alice Ryhl, mmaurer, Danilo Krummrich,
Alexandre Courbot, John Hubbard, Joel Fernandes, nouveau,
rust-for-linux
Add a name() method to the `Device` type, which returns a CStr that
contains the device name.
Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
rust/helpers/device.c | 5 +++++
rust/kernel/device.rs | 10 ++++++++++
2 files changed, 15 insertions(+)
diff --git a/rust/helpers/device.c b/rust/helpers/device.c
index 9a4316bafedf..ae02c199b11a 100644
--- a/rust/helpers/device.c
+++ b/rust/helpers/device.c
@@ -25,3 +25,8 @@ void rust_helper_dev_set_drvdata(struct device *dev, void *data)
{
dev_set_drvdata(dev, data);
}
+
+__rust_helper const char *rust_helper_dev_name(const struct device *dev)
+{
+ return dev_name(dev);
+}
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 71b200df0f40..69665d275e24 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -482,6 +482,16 @@ pub fn fwnode(&self) -> Option<&property::FwNode> {
// defined as a `#[repr(transparent)]` wrapper around `fwnode_handle`.
Some(unsafe { &*fwnode_handle.cast() })
}
+
+ /// 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.
+ 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
--
2.52.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v6 2/7] rust: uaccess: add write_dma() for copying from DMA buffers to userspace
2026-01-29 2:28 [PATCH v6 0/7] gpu: nova-core: expose the logging buffers via debugfs Timur Tabi
2026-01-29 2:28 ` [PATCH v6 1/7] rust: device: add device name method Timur Tabi
@ 2026-01-29 2:28 ` Timur Tabi
2026-01-29 17:24 ` Gary Guo
2026-01-31 0:14 ` Danilo Krummrich
2026-01-29 2:28 ` [PATCH v6 3/7] gpu: nova-core: implement BinaryWriter for CoherentAllocation<u8> Timur Tabi
` (4 subsequent siblings)
6 siblings, 2 replies; 25+ messages in thread
From: Timur Tabi @ 2026-01-29 2:28 UTC (permalink / raw)
To: Gary Guo, Alice Ryhl, mmaurer, Danilo Krummrich,
Alexandre Courbot, John Hubbard, Joel Fernandes, nouveau,
rust-for-linux
Add UserSliceWriter::write_dma() to copy data from a CoherentAllocation<u8>
to userspace. This provides a safe interface for copying DMA buffer
contents to userspace without requiring callers to work with raw pointers.
The method handles bounds checking and offset calculation internally,
wrapping the unsafe copy_to_user() call.
Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
rust/kernel/uaccess.rs | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index f989539a31b4..f9bb88e47408 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -7,6 +7,7 @@
use crate::{
alloc::{Allocator, Flags},
bindings,
+ dma::CoherentAllocation,
error::Result,
ffi::{c_char, c_void},
fs::file,
@@ -481,6 +482,46 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
Ok(())
}
+ /// Writes raw data to this user pointer from a DMA coherent allocation.
+ ///
+ /// Returns error if the offset+count exceeds the allocation 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.
+ ///
+ /// Note: The memory may be concurrently modified by hardware (e.g., DMA). In such cases,
+ /// the copied data may be inconsistent, but this does not cause undefined behavior.
+ pub fn write_dma(
+ &mut self,
+ alloc: &CoherentAllocation<u8>,
+ offset: usize,
+ count: usize,
+ ) -> Result {
+ let len = alloc.count();
+ if offset.checked_add(count).ok_or(EOVERFLOW)? > len {
+ return Err(ERANGE);
+ }
+
+ // SAFETY: `start_ptr()` returns a valid pointer to a memory region of `count()` bytes,
+ // as guaranteed by the `CoherentAllocation` invariants. The check above ensures
+ // `offset + count <= len`.
+ let src_ptr = unsafe { alloc.start_ptr().add(offset) };
+
+ // SAFETY: `src_ptr` is valid for reads of `count` bytes per the above.
+ let res = unsafe {
+ bindings::copy_to_user(self.ptr.as_mut_ptr(), src_ptr.cast::<c_void>(), count)
+ };
+ if res != 0 {
+ return Err(EFAULT);
+ }
+
+ self.ptr = self.ptr.wrapping_byte_add(count);
+ self.length -= count;
+
+ Ok(())
+ }
+
/// Writes raw data to this user pointer from a kernel buffer partially.
///
/// This is the same as [`Self::write_slice`] but considers the given `offset` into `data` and
--
2.52.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v6 2/7] rust: uaccess: add write_dma() for copying from DMA buffers to userspace
2026-01-29 2:28 ` [PATCH v6 2/7] rust: uaccess: add write_dma() for copying from DMA buffers to userspace Timur Tabi
@ 2026-01-29 17:24 ` Gary Guo
2026-01-31 0:14 ` Danilo Krummrich
1 sibling, 0 replies; 25+ messages in thread
From: Gary Guo @ 2026-01-29 17:24 UTC (permalink / raw)
To: Timur Tabi, Gary Guo, Alice Ryhl, mmaurer, Danilo Krummrich,
Alexandre Courbot, John Hubbard, Joel Fernandes, nouveau,
rust-for-linux
On Thu Jan 29, 2026 at 2:28 AM GMT, Timur Tabi wrote:
> Add UserSliceWriter::write_dma() to copy data from a CoherentAllocation<u8>
> to userspace. This provides a safe interface for copying DMA buffer
> contents to userspace without requiring callers to work with raw pointers.
>
> The method handles bounds checking and offset calculation internally,
> wrapping the unsafe copy_to_user() call.
>
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
> rust/kernel/uaccess.rs | 41 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index f989539a31b4..f9bb88e47408 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -7,6 +7,7 @@
> use crate::{
> alloc::{Allocator, Flags},
> bindings,
> + dma::CoherentAllocation,
> error::Result,
> ffi::{c_char, c_void},
> fs::file,
> @@ -481,6 +482,46 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
> Ok(())
> }
>
> + /// Writes raw data to this user pointer from a DMA coherent allocation.
> + ///
> + /// Returns error if the offset+count exceeds the allocation 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.
> + ///
> + /// Note: The memory may be concurrently modified by hardware (e.g., DMA). In such cases,
> + /// the copied data may be inconsistent, but this does not cause undefined behavior.
> + pub fn write_dma(
> + &mut self,
> + alloc: &CoherentAllocation<u8>,
> + offset: usize,
> + count: usize,
> + ) -> Result {
> + let len = alloc.count();
> + if offset.checked_add(count).ok_or(EOVERFLOW)? > len {
> + return Err(ERANGE);
> + }
> +
> + // SAFETY: `start_ptr()` returns a valid pointer to a memory region of `count()` bytes,
> + // as guaranteed by the `CoherentAllocation` invariants. The check above ensures
> + // `offset + count <= len`.
> + let src_ptr = unsafe { alloc.start_ptr().add(offset) };
> +
> + // SAFETY: `src_ptr` is valid for reads of `count` bytes per the above.
> + let res = unsafe {
> + bindings::copy_to_user(self.ptr.as_mut_ptr(), src_ptr.cast::<c_void>(), count)
> + };
You're not checking `count` against `self.length` in this function, so you
perform an OOB write to userspace memory.
Best,
Gary
> + if res != 0 {
> + return Err(EFAULT);
> + }
> +
> + self.ptr = self.ptr.wrapping_byte_add(count);
> + self.length -= count;
> +
> + Ok(())
> + }
> +
> /// Writes raw data to this user pointer from a kernel buffer partially.
> ///
> /// This is the same as [`Self::write_slice`] but considers the given `offset` into `data` and
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v6 2/7] rust: uaccess: add write_dma() for copying from DMA buffers to userspace
2026-01-29 2:28 ` [PATCH v6 2/7] rust: uaccess: add write_dma() for copying from DMA buffers to userspace Timur Tabi
2026-01-29 17:24 ` Gary Guo
@ 2026-01-31 0:14 ` Danilo Krummrich
1 sibling, 0 replies; 25+ messages in thread
From: Danilo Krummrich @ 2026-01-31 0:14 UTC (permalink / raw)
To: Timur Tabi
Cc: Gary Guo, Alice Ryhl, mmaurer, Alexandre Courbot, John Hubbard,
Joel Fernandes, nouveau, rust-for-linux
On Thu Jan 29, 2026 at 3:28 AM CET, Timur Tabi wrote:
> + /// Writes raw data to this user pointer from a DMA coherent allocation.
> + ///
> + /// Returns error if the offset+count exceeds the allocation 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.
> + ///
> + /// Note: The memory may be concurrently modified by hardware (e.g., DMA). In such cases,
> + /// the copied data may be inconsistent, but this does not cause undefined behavior.
> + pub fn write_dma(
> + &mut self,
> + alloc: &CoherentAllocation<u8>,
> + offset: usize,
> + count: usize,
> + ) -> Result {
> + let len = alloc.count();
> + if offset.checked_add(count).ok_or(EOVERFLOW)? > len {
> + return Err(ERANGE);
> + }
> +
> + // SAFETY: `start_ptr()` returns a valid pointer to a memory region of `count()` bytes,
> + // as guaranteed by the `CoherentAllocation` invariants. The check above ensures
> + // `offset + count <= len`.
> + let src_ptr = unsafe { alloc.start_ptr().add(offset) };
> +
> + // SAFETY: `src_ptr` is valid for reads of `count` bytes per the above.
> + let res = unsafe {
> + bindings::copy_to_user(self.ptr.as_mut_ptr(), src_ptr.cast::<c_void>(), count)
> + };
> + if res != 0 {
> + return Err(EFAULT);
> + }
> +
> + self.ptr = self.ptr.wrapping_byte_add(count);
> + self.length -= count;
> +
> + Ok(())
> + }
I think the idea was to have a common read_slice_raw() method that can be shared
between write_dma() and write_slice(). Can you please factor this out?
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v6 3/7] gpu: nova-core: implement BinaryWriter for CoherentAllocation<u8>
2026-01-29 2:28 [PATCH v6 0/7] gpu: nova-core: expose the logging buffers via debugfs Timur Tabi
2026-01-29 2:28 ` [PATCH v6 1/7] rust: device: add device name method Timur Tabi
2026-01-29 2:28 ` [PATCH v6 2/7] rust: uaccess: add write_dma() for copying from DMA buffers to userspace Timur Tabi
@ 2026-01-29 2:28 ` Timur Tabi
2026-01-30 8:31 ` Alice Ryhl
2026-01-29 2:28 ` [PATCH v6 4/7] gpu: nova-core: Replace module_pci_driver! with explicit module init Timur Tabi
` (3 subsequent siblings)
6 siblings, 1 reply; 25+ messages in thread
From: Timur Tabi @ 2026-01-29 2:28 UTC (permalink / raw)
To: Gary Guo, Alice Ryhl, mmaurer, Danilo Krummrich,
Alexandre Courbot, John Hubbard, Joel Fernandes, nouveau,
rust-for-linux
Implement the BinaryWriter trait for CoherentAllocation<u8>, enabling
DMA coherent allocations to be exposed as readable binary files.
The implementation handles offset tracking and bounds checking, copying
data from the coherent allocation to userspace via write_dma().
Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
drivers/gpu/nova-core/gsp.rs | 1 +
rust/kernel/dma.rs | 36 +++++++++++++++++++++++++++++++++++-
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
index 174feaca0a6b..f7134c75b1f2 100644
--- a/drivers/gpu/nova-core/gsp.rs
+++ b/drivers/gpu/nova-core/gsp.rs
@@ -3,6 +3,7 @@
mod boot;
use kernel::{
+ debugfs,
device,
dma::{
CoherentAllocation,
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index acc65b1e0f24..dca16fb5a2fc 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -5,12 +5,14 @@
//! C header: [`include/linux/dma-mapping.h`](srctree/include/linux/dma-mapping.h)
use crate::{
- bindings, build_assert, device,
+ bindings, build_assert, debugfs, device,
device::{Bound, Core},
error::{to_result, Result},
+ fs::file,
prelude::*,
sync::aref::ARef,
transmute::{AsBytes, FromBytes},
+ uaccess::UserSliceWriter,
};
use core::ptr::NonNull;
@@ -651,6 +653,38 @@ fn drop(&mut self) {
// can be sent to another thread.
unsafe impl<T: AsBytes + FromBytes + Send> Send for CoherentAllocation<T> {}
+// SAFETY: Sharing `&CoherentAllocation` across threads is safe if `T` is `Sync`, because all
+// methods that access the buffer contents (`field_read`, `field_write`, `as_slice`,
+// `as_slice_mut`) are `unsafe`, and callers are responsible for ensuring no data races occur.
+// The safe methods only return metadata or raw pointers whose use requires `unsafe`.
+unsafe impl<T: AsBytes + FromBytes + Sync> Sync for CoherentAllocation<T> {}
+
+impl debugfs::BinaryWriter for CoherentAllocation<u8> {
+ fn write_to_slice(
+ &self,
+ writer: &mut UserSliceWriter,
+ offset: &mut file::Offset,
+ ) -> Result<usize> {
+ if offset.is_negative() {
+ return Err(EINVAL);
+ }
+
+ let offset_val: usize = (*offset).try_into().map_err(|_| EINVAL)?;
+ let len = self.count();
+
+ if offset_val >= len {
+ return Ok(0);
+ }
+
+ let count = (len - offset_val).min(writer.len());
+
+ writer.write_dma(self, offset_val, count)?;
+
+ *offset += count as i64;
+ Ok(count)
+ }
+}
+
/// Reads a field of an item from an allocated region of structs.
///
/// # Examples
--
2.52.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v6 3/7] gpu: nova-core: implement BinaryWriter for CoherentAllocation<u8>
2026-01-29 2:28 ` [PATCH v6 3/7] gpu: nova-core: implement BinaryWriter for CoherentAllocation<u8> Timur Tabi
@ 2026-01-30 8:31 ` Alice Ryhl
2026-01-30 18:53 ` Timur Tabi
0 siblings, 1 reply; 25+ messages in thread
From: Alice Ryhl @ 2026-01-30 8:31 UTC (permalink / raw)
To: Timur Tabi
Cc: Gary Guo, mmaurer, Danilo Krummrich, Alexandre Courbot,
John Hubbard, Joel Fernandes, nouveau, rust-for-linux
On Wed, Jan 28, 2026 at 08:28:33PM -0600, Timur Tabi wrote:
> [PATCH v6 3/7] gpu: nova-core: implement BinaryWriter for CoherentAllocation<u8>
The commit title says 'gpu: nova-core:' but this is primarily a change
to rust/kernel/dma.rs, so it should say 'rust: dma:' or similar.
> +// SAFETY: Sharing `&CoherentAllocation` across threads is safe if `T` is `Sync`, because all
> +// methods that access the buffer contents (`field_read`, `field_write`, `as_slice`,
> +// `as_slice_mut`) are `unsafe`, and callers are responsible for ensuring no data races occur.
> +// The safe methods only return metadata or raw pointers whose use requires `unsafe`.
> +unsafe impl<T: AsBytes + FromBytes + Sync> Sync for CoherentAllocation<T> {}
This change is unrelated to implementing BinaryWriter for
CoherentAllocation.
> +impl debugfs::BinaryWriter for CoherentAllocation<u8> {
> + fn write_to_slice(
> + &self,
> + writer: &mut UserSliceWriter,
> + offset: &mut file::Offset,
> + ) -> Result<usize> {
> + if offset.is_negative() {
> + return Err(EINVAL);
> + }
> +
> + let offset_val: usize = (*offset).try_into().map_err(|_| EINVAL)?;
If the user seeks to a large offset, this leads to EINVAL. But the
correct behavior for a file is to simply return Ok(0) in such case.
Alice
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v6 3/7] gpu: nova-core: implement BinaryWriter for CoherentAllocation<u8>
2026-01-30 8:31 ` Alice Ryhl
@ 2026-01-30 18:53 ` Timur Tabi
0 siblings, 0 replies; 25+ messages in thread
From: Timur Tabi @ 2026-01-30 18:53 UTC (permalink / raw)
To: aliceryhl@google.com
Cc: John Hubbard, gary@garyguo.net, mmaurer@google.com,
rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org,
Joel Fernandes, dakr@kernel.org, Alexandre Courbot
On Fri, 2026-01-30 at 08:31 +0000, Alice Ryhl wrote:
> On Wed, Jan 28, 2026 at 08:28:33PM -0600, Timur Tabi wrote:
> > [PATCH v6 3/7] gpu: nova-core: implement BinaryWriter for CoherentAllocation<u8>
>
> The commit title says 'gpu: nova-core:' but this is primarily a change
> to rust/kernel/dma.rs, so it should say 'rust: dma:' or similar.
Ok.
>
> > +// SAFETY: Sharing `&CoherentAllocation` across threads is safe if `T` is `Sync`, because
> > all
> > +// methods that access the buffer contents (`field_read`, `field_write`, `as_slice`,
> > +// `as_slice_mut`) are `unsafe`, and callers are responsible for ensuring no data races
> > occur.
> > +// The safe methods only return metadata or raw pointers whose use requires `unsafe`.
> > +unsafe impl<T: AsBytes + FromBytes + Sync> Sync for CoherentAllocation<T> {}
>
> This change is unrelated to implementing BinaryWriter for
> CoherentAllocation.
Did you intend to reference this diff in gsp.rs?
use kernel::{
+ debugfs,
device,
dma::{
CoherentAllocation,
because that diff was mistakenly added to this commit. However, I'm pretty sure the `Sync for
CoherentAllocation<T>` should be part of 'implement BinaryWriter for CoherentAllocation<u8>'
because debugfs::read_binary_file() requires T: BinaryWriter + Sync.
> > +impl debugfs::BinaryWriter for CoherentAllocation<u8> {
> > + fn write_to_slice(
> > + &self,
> > + writer: &mut UserSliceWriter,
> > + offset: &mut file::Offset,
> > + ) -> Result<usize> {
> > + if offset.is_negative() {
> > + return Err(EINVAL);
> > + }
> > +
> > + let offset_val: usize = (*offset).try_into().map_err(|_| EINVAL)?;
>
> If the user seeks to a large offset, this leads to EINVAL. But the
> correct behavior for a file is to simply return Ok(0) in such case.
Will fix in 7. Thanks.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v6 4/7] gpu: nova-core: Replace module_pci_driver! with explicit module init
2026-01-29 2:28 [PATCH v6 0/7] gpu: nova-core: expose the logging buffers via debugfs Timur Tabi
` (2 preceding siblings ...)
2026-01-29 2:28 ` [PATCH v6 3/7] gpu: nova-core: implement BinaryWriter for CoherentAllocation<u8> Timur Tabi
@ 2026-01-29 2:28 ` Timur Tabi
2026-01-29 2:28 ` [PATCH v6 5/7] gpu: nova-core: use pin projection in method boot() Timur Tabi
` (2 subsequent siblings)
6 siblings, 0 replies; 25+ messages in thread
From: Timur Tabi @ 2026-01-29 2:28 UTC (permalink / raw)
To: Gary Guo, Alice Ryhl, mmaurer, Danilo Krummrich,
Alexandre Courbot, John Hubbard, Joel Fernandes, nouveau,
rust-for-linux
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 <ttabi@nvidia.com>
---
drivers/gpu/nova-core/nova_core.rs | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index c1121e7c64c5..80ecbb50ec82 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -2,6 +2,13 @@
//! Nova Core GPU Driver
+use kernel::{
+ driver::Registration,
+ pci,
+ prelude::*,
+ InPlaceModule, //
+};
+
#[macro_use]
mod bitfield;
@@ -20,8 +27,22 @@
pub(crate) const MODULE_NAME: &kernel::str::CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
-kernel::module_pci_driver! {
- type: driver::NovaCore,
+#[pin_data]
+struct NovaCoreModule {
+ #[pin]
+ _driver: Registration<pci::Adapter<driver::NovaCore>>,
+}
+
+impl InPlaceModule for NovaCoreModule {
+ fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self, Error> {
+ try_pin_init!(Self {
+ _driver <- Registration::new(MODULE_NAME, module),
+ })
+ }
+}
+
+module! {
+ type: NovaCoreModule,
name: "NovaCore",
authors: ["Danilo Krummrich"],
description: "Nova Core GPU driver",
--
2.52.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v6 5/7] gpu: nova-core: use pin projection in method boot()
2026-01-29 2:28 [PATCH v6 0/7] gpu: nova-core: expose the logging buffers via debugfs Timur Tabi
` (3 preceding siblings ...)
2026-01-29 2:28 ` [PATCH v6 4/7] gpu: nova-core: Replace module_pci_driver! with explicit module init Timur Tabi
@ 2026-01-29 2:28 ` Timur Tabi
2026-01-29 2:28 ` [PATCH v6 6/7] gpu: nova-core: create debugfs root in module init Timur Tabi
2026-01-29 2:28 ` [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries Timur Tabi
6 siblings, 0 replies; 25+ messages in thread
From: Timur Tabi @ 2026-01-29 2:28 UTC (permalink / raw)
To: Gary Guo, Alice Ryhl, mmaurer, Danilo Krummrich,
Alexandre Courbot, John Hubbard, Joel Fernandes, nouveau,
rust-for-linux
Struct Gsp in gsp.rs is tagged with #[pin_data], which allows any of its
fields to be pinned (i.e. with #[pin]). When #[pin] is added to any
field in a #[pin_data] struct, fields can no longer be directly accessed
via normal field access. Instead, pin projection must be used to access
those fields.
Currently, no fields are pinned, but that will change. The boot() method
receives self: Pin<&mut Self>. When struct Gsp contains any pinned
fields, direct field access like self.cmdq is not allowed through
Pin<&mut Self>, as Pin prevents obtaining &mut Self to protect pinned
data from being moved.
Use pin projection via self.as_mut().project() to access struct fields.
The project() method, generated by #[pin_data], returns a projection
struct providing &mut references to non-pinned fields, enabling mutable
access while preserving pin invariants.
Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
drivers/gpu/nova-core/gsp/boot.rs | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
index be427fe26a58..527c75feb461 100644
--- a/drivers/gpu/nova-core/gsp/boot.rs
+++ b/drivers/gpu/nova-core/gsp/boot.rs
@@ -159,12 +159,14 @@ pub(crate) fn boot(
CoherentAllocation::<GspFwWprMeta>::alloc_coherent(dev, 1, GFP_KERNEL | __GFP_ZERO)?;
dma_write!(wpr_meta[0] = GspFwWprMeta::new(&gsp_fw, &fb_layout))?;
- self.cmdq
+ let this = self.as_mut().project();
+
+ this.cmdq
.send_command(bar, commands::SetSystemInfo::new(pdev))?;
- self.cmdq.send_command(bar, commands::SetRegistry::new())?;
+ this.cmdq.send_command(bar, commands::SetRegistry::new())?;
gsp_falcon.reset(bar)?;
- let libos_handle = self.libos.dma_handle();
+ let libos_handle = this.libos.dma_handle();
let (mbox0, mbox1) = gsp_falcon.boot(
bar,
Some(libos_handle as u32),
@@ -231,13 +233,13 @@ pub(crate) fn boot(
dev: pdev.as_ref().into(),
bar,
};
- GspSequencer::run(&mut self.cmdq, seq_params)?;
+ GspSequencer::run(this.cmdq, seq_params)?;
// Wait until GSP is fully initialized.
- commands::wait_gsp_init_done(&mut self.cmdq)?;
+ commands::wait_gsp_init_done(this.cmdq)?;
// Obtain and display basic GPU information.
- let info = commands::get_gsp_info(&mut self.cmdq, bar)?;
+ let info = commands::get_gsp_info(this.cmdq, bar)?;
match info.gpu_name() {
Ok(name) => dev_info!(pdev.as_ref(), "GPU name: {}\n", name),
Err(e) => dev_warn!(pdev.as_ref(), "GPU name unavailable: {:?}\n", e),
--
2.52.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v6 6/7] gpu: nova-core: create debugfs root in module init
2026-01-29 2:28 [PATCH v6 0/7] gpu: nova-core: expose the logging buffers via debugfs Timur Tabi
` (4 preceding siblings ...)
2026-01-29 2:28 ` [PATCH v6 5/7] gpu: nova-core: use pin projection in method boot() Timur Tabi
@ 2026-01-29 2:28 ` Timur Tabi
2026-01-30 8:34 ` Alice Ryhl
2026-01-29 2:28 ` [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries Timur Tabi
6 siblings, 1 reply; 25+ messages in thread
From: Timur Tabi @ 2026-01-29 2:28 UTC (permalink / raw)
To: Gary Guo, Alice Ryhl, mmaurer, Danilo Krummrich,
Alexandre Courbot, John Hubbard, Joel Fernandes, nouveau,
rust-for-linux
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 <ttabi@nvidia.com>
---
drivers/gpu/nova-core/Kconfig | 13 +++++++++++++
drivers/gpu/nova-core/nova_core.rs | 28 ++++++++++++++++++++++++++++
2 files changed, 41 insertions(+)
diff --git a/drivers/gpu/nova-core/Kconfig b/drivers/gpu/nova-core/Kconfig
index 527920f9c4d3..974c5d08f6de 100644
--- a/drivers/gpu/nova-core/Kconfig
+++ b/drivers/gpu/nova-core/Kconfig
@@ -14,3 +14,16 @@ config NOVA_CORE
This driver is work in progress and may not be functional.
If M is selected, the module will be called nova_core.
+
+config NOVA_CORE_DEBUGFS
+ bool "Nova Core debugfs support"
+ depends on NOVA_CORE
+ depends on DEBUG_FS
+ default y
+ help
+ Enable debugfs support for the Nova Core driver. This exposes
+ debugging information and log buffers via the debugfs filesystem.
+
+ Each GPU will have an entry under /sys/kernel/debug/nova_core.
+
+ If unsure, say Y.
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index 80ecbb50ec82..582b03013ebc 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -3,6 +3,7 @@
//! Nova Core GPU Driver
use kernel::{
+ debugfs,
driver::Registration,
pci,
prelude::*,
@@ -27,16 +28,43 @@
pub(crate) const MODULE_NAME: &kernel::str::CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
+// FIXME: Move this into per-module data once that exists
+static mut DEBUGFS_ROOT: Option<debugfs::Dir> = None;
+
+/// Guard that clears DEBUGFS_ROOT when dropped.
+struct DebugfsRootGuard;
+
+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 };
+ }
+}
+
#[pin_data]
struct NovaCoreModule {
+ // Fields are dropped in declaration order, so _driver is dropped first,
+ // then _debugfs_guard clears DEBUGFS_ROOT.
#[pin]
_driver: Registration<pci::Adapter<driver::NovaCore>>,
+ _debugfs_guard: DebugfsRootGuard,
}
impl InPlaceModule for NovaCoreModule {
fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self, Error> {
+ #[cfg(CONFIG_NOVA_CORE_DEBUGFS)]
+ {
+ let dir = debugfs::Dir::new(kernel::c_str!("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 {
_driver <- Registration::new(MODULE_NAME, module),
+ _debugfs_guard: DebugfsRootGuard,
})
}
}
--
2.52.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v6 6/7] gpu: nova-core: create debugfs root in module init
2026-01-29 2:28 ` [PATCH v6 6/7] gpu: nova-core: create debugfs root in module init Timur Tabi
@ 2026-01-30 8:34 ` Alice Ryhl
2026-01-30 14:59 ` Timur Tabi
0 siblings, 1 reply; 25+ messages in thread
From: Alice Ryhl @ 2026-01-30 8:34 UTC (permalink / raw)
To: Timur Tabi
Cc: Gary Guo, mmaurer, Danilo Krummrich, Alexandre Courbot,
John Hubbard, Joel Fernandes, nouveau, rust-for-linux
On Wed, Jan 28, 2026 at 08:28:36PM -0600, Timur Tabi wrote:
> +// FIXME: Move this into per-module data once that exists
> +static mut DEBUGFS_ROOT: Option<debugfs::Dir> = None;
Surely debugfs has a solution to avoid this...
Alice
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 6/7] gpu: nova-core: create debugfs root in module init
2026-01-30 8:34 ` Alice Ryhl
@ 2026-01-30 14:59 ` Timur Tabi
0 siblings, 0 replies; 25+ messages in thread
From: Timur Tabi @ 2026-01-30 14:59 UTC (permalink / raw)
To: aliceryhl@google.com
Cc: John Hubbard, gary@garyguo.net, mmaurer@google.com,
rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org,
Joel Fernandes, dakr@kernel.org, Alexandre Courbot
On Fri, 2026-01-30 at 08:34 +0000, Alice Ryhl wrote:
> On Wed, Jan 28, 2026 at 08:28:36PM -0600, Timur Tabi wrote:
> > +// FIXME: Move this into per-module data once that exists
> > +static mut DEBUGFS_ROOT: Option<debugfs::Dir> = None;
>
> Surely debugfs has a solution to avoid this...
Yes, debugfs_lookup(), but that was rejected:
https://lore.kernel.org/all/DF18RFX3IHVP.3GYNJIYAFFJU6@kernel.org/
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-29 2:28 [PATCH v6 0/7] gpu: nova-core: expose the logging buffers via debugfs Timur Tabi
` (5 preceding siblings ...)
2026-01-29 2:28 ` [PATCH v6 6/7] gpu: nova-core: create debugfs root in module init Timur Tabi
@ 2026-01-29 2:28 ` Timur Tabi
2026-01-30 23:36 ` Timur Tabi
6 siblings, 1 reply; 25+ messages in thread
From: Timur Tabi @ 2026-01-29 2:28 UTC (permalink / raw)
To: Gary Guo, Alice Ryhl, mmaurer, Danilo Krummrich,
Alexandre Courbot, John Hubbard, Joel Fernandes, nouveau,
rust-for-linux
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 <acourbot@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
drivers/gpu/nova-core/gsp.rs | 45 +++++++++++++++++++++++++++++-------
1 file changed, 37 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
index f7134c75b1f2..33f9cfd13849 100644
--- a/drivers/gpu/nova-core/gsp.rs
+++ b/drivers/gpu/nova-core/gsp.rs
@@ -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.
@@ -124,15 +132,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::<GspArgumentsPadded>::alloc_coherent(
dev,
@@ -153,6 +163,25 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
dma_write!(rmargs[0].inner = fw::GspArgumentsCached::new(cmdq))?;
dma_write!(libos[3] = LibosMemoryRegionInitArgument::new("RMARGS", rmargs))?;
},
+ logs <- {
+ let log_buffers = LogBuffers {
+ loginit,
+ logintr,
+ 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.
+ let log_parent = unsafe { crate::DEBUGFS_ROOT.as_ref() }
+ .expect("DEBUGFS_ROOT not initialized");
+
+ log_parent.scope(log_buffers, dev.name(), |logs, dir| {
+ dir.read_binary_file(c_str!("loginit"), &logs.loginit.0);
+ dir.read_binary_file(c_str!("logintr"), &logs.logintr.0);
+ dir.read_binary_file(c_str!("logrm"), &logs.logrm.0);
+ })
+ },
}))
})
}
--
2.52.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-29 2:28 ` [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries Timur Tabi
@ 2026-01-30 23:36 ` Timur Tabi
2026-01-30 23:58 ` Gary Guo
0 siblings, 1 reply; 25+ messages in thread
From: Timur Tabi @ 2026-01-30 23:36 UTC (permalink / raw)
To: John Hubbard, gary@garyguo.net, mmaurer@google.com,
rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org,
Joel Fernandes, dakr@kernel.org, aliceryhl@google.com,
Alexandre Courbot
On Wed, 2026-01-28 at 20:28 -0600, Timur Tabi wrote:
> + #[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.
> + let log_parent = unsafe { crate::DEBUGFS_ROOT.as_ref() }
> + .expect("DEBUGFS_ROOT not initialized");
> +
> + log_parent.scope(log_buffers, dev.name(), |logs, dir| {
> + dir.read_binary_file(c_str!("loginit"), &logs.loginit.0);
> + dir.read_binary_file(c_str!("logintr"), &logs.logintr.0);
> + dir.read_binary_file(c_str!("logrm"), &logs.logrm.0);
I think there might be a problem with this code that I don't know how to resolve.
If CONFIG_NOVA_CORE_DEBUGFS=n, then DEBUGFS_ROOT is None, and so the .as_ref() will also be
none, and the .expect will cause a panic. We don't want that.
If I remove the .expect(), then log_parent becomes None, but then the .scope() won't compile.
I could wrap the whole thing in #[cfg(CONFIG_NOVA_CORE_DEBUGFS)], but my understanding is that
the call to .scope() is necessary to ensure that LogBuffers is not dropped at the end of this
function.
It seems like I'm going to need to do something like this in struct Gsp:
#[cfg(CONFIG_NOVA_CORE_DEBUGFS)]
#[pin]
logs: debugfs::Scope<LogBuffers>,
#[cfg(not(CONFIG_NOVA_CORE_DEBUGFS))]
logs: LogBuffers, // Just own them directly, no debugfs
But the design of debugfs is to have it not care if debugfs is disabled.
Any suggestions?
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-30 23:36 ` Timur Tabi
@ 2026-01-30 23:58 ` Gary Guo
2026-01-31 0:07 ` John Hubbard
2026-01-31 0:10 ` Danilo Krummrich
0 siblings, 2 replies; 25+ messages in thread
From: Gary Guo @ 2026-01-30 23:58 UTC (permalink / raw)
To: Timur Tabi, John Hubbard, gary@garyguo.net, mmaurer@google.com,
rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org,
Joel Fernandes, dakr@kernel.org, aliceryhl@google.com,
Alexandre Courbot
On Fri Jan 30, 2026 at 11:36 PM GMT, Timur Tabi wrote:
> On Wed, 2026-01-28 at 20:28 -0600, Timur Tabi wrote:
>> + #[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.
>> + let log_parent = unsafe { crate::DEBUGFS_ROOT.as_ref() }
>> + .expect("DEBUGFS_ROOT not initialized");
>> +
>> + log_parent.scope(log_buffers, dev.name(), |logs, dir| {
>> + dir.read_binary_file(c_str!("loginit"), &logs.loginit.0);
>> + dir.read_binary_file(c_str!("logintr"), &logs.logintr.0);
>> + dir.read_binary_file(c_str!("logrm"), &logs.logrm.0);
>
> I think there might be a problem with this code that I don't know how to resolve.
>
> If CONFIG_NOVA_CORE_DEBUGFS=n, then DEBUGFS_ROOT is None, and so the .as_ref() will also be
> none, and the .expect will cause a panic. We don't want that.
>
> If I remove the .expect(), then log_parent becomes None, but then the .scope() won't compile.
>
> I could wrap the whole thing in #[cfg(CONFIG_NOVA_CORE_DEBUGFS)], but my understanding is that
> the call to .scope() is necessary to ensure that LogBuffers is not dropped at the end of this
> function.
>
> It seems like I'm going to need to do something like this in struct Gsp:
>
> #[cfg(CONFIG_NOVA_CORE_DEBUGFS)]
> #[pin]
> logs: debugfs::Scope<LogBuffers>,
>
> #[cfg(not(CONFIG_NOVA_CORE_DEBUGFS))]
> logs: LogBuffers, // Just own them directly, no debugfs
>
> But the design of debugfs is to have it not care if debugfs is disabled.
>
> Any suggestions?
I think the rationale behind current debugfs design is that when it is disabled
in its entirety, then all of the code are compiled out and you're leaved with
ZST, so code don't have to care at all and you'll still have no codegen in the
end.
However, when debugfs is enabled, but CONFIG_NOVA_CORE_DEBUGFS=n, then using
debugfs functionalities would *not* be compiled out (so, for the `Dir::empty()`
patch in the previous iteration, all of the debugging facility would still be
around with CONFIG_DEBUG_FS=y and CONFIG_NOVA_CORE_DEBUGFS=n, which is not
desirable).
The straightforward solution is thus sprinkle `#[cfg(CONFIG_NOVA_CORE_DEBUGFS)]`
everywhere where debugfs is touched, which is non-ideal.
One idea is to create types that look exactly like `Dir` but always ZST and
no-op regardless whether CONFIG_DEBUG_FS is enabled... But that feel a bit..
weird. Matthew, what do you think?
Best,
Gary
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-30 23:58 ` Gary Guo
@ 2026-01-31 0:07 ` John Hubbard
2026-01-31 0:10 ` Danilo Krummrich
1 sibling, 0 replies; 25+ messages in thread
From: John Hubbard @ 2026-01-31 0:07 UTC (permalink / raw)
To: Gary Guo, Timur Tabi, mmaurer@google.com,
rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org,
Joel Fernandes, dakr@kernel.org, aliceryhl@google.com,
Alexandre Courbot
On 1/30/26 3:58 PM, Gary Guo wrote:
> On Fri Jan 30, 2026 at 11:36 PM GMT, Timur Tabi wrote:
>> On Wed, 2026-01-28 at 20:28 -0600, Timur Tabi wrote:
...
> However, when debugfs is enabled, but CONFIG_NOVA_CORE_DEBUGFS=n, then using
> debugfs functionalities would *not* be compiled out (so, for the `Dir::empty()`
> patch in the previous iteration, all of the debugging facility would still be
> around with CONFIG_DEBUG_FS=y and CONFIG_NOVA_CORE_DEBUGFS=n, which is not
> desirable).
>
> The straightforward solution is thus sprinkle `#[cfg(CONFIG_NOVA_CORE_DEBUGFS)]`
> everywhere where debugfs is touched, which is non-ideal.
Another straightforward solution is to not have CONFIG_NOVA_CORE_DEBUGFS
at all! Just use CONFIG_DEBUG_FS. That cleans up everything nicely.
Furthermore, that appears to be how quite a few other drivers do it,
too: a quick search reports 117 uses of CONFIG_DEBUG_FS across
84 files in drivers/gpu/ alone, and zero major DRM GPU drivers with
a *_DEBUGFS config option.
So I don't see a need for this granular per-driver approach, for
this feature.
thanks,
--
John Hubbard
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-30 23:58 ` Gary Guo
2026-01-31 0:07 ` John Hubbard
@ 2026-01-31 0:10 ` Danilo Krummrich
2026-01-31 0:16 ` John Hubbard
2026-01-31 0:26 ` Timur Tabi
1 sibling, 2 replies; 25+ messages in thread
From: Danilo Krummrich @ 2026-01-31 0:10 UTC (permalink / raw)
To: Gary Guo
Cc: Timur Tabi, John Hubbard, mmaurer@google.com,
rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org,
Joel Fernandes, aliceryhl@google.com, Alexandre Courbot
On Sat Jan 31, 2026 at 12:58 AM CET, Gary Guo wrote:
> On Fri Jan 30, 2026 at 11:36 PM GMT, Timur Tabi wrote:
>> I think there might be a problem with this code that I don't know how to resolve.
>>
>> If CONFIG_NOVA_CORE_DEBUGFS=n, then DEBUGFS_ROOT is None, and so the .as_ref() will also be
>> none, and the .expect will cause a panic. We don't want that.
>>
>> If I remove the .expect(), then log_parent becomes None, but then the .scope() won't compile.
>>
>> I could wrap the whole thing in #[cfg(CONFIG_NOVA_CORE_DEBUGFS)], but my understanding is that
>> the call to .scope() is necessary to ensure that LogBuffers is not dropped at the end of this
>> function.
>>
>> It seems like I'm going to need to do something like this in struct Gsp:
>>
>> #[cfg(CONFIG_NOVA_CORE_DEBUGFS)]
>> #[pin]
>> logs: debugfs::Scope<LogBuffers>,
>>
>> #[cfg(not(CONFIG_NOVA_CORE_DEBUGFS))]
>> logs: LogBuffers, // Just own them directly, no debugfs
>>
>> But the design of debugfs is to have it not care if debugfs is disabled.
>>
>> Any suggestions?
>
> I think the rationale behind current debugfs design is that when it is disabled
> in its entirety, then all of the code are compiled out and you're leaved with
> ZST, so code don't have to care at all and you'll still have no codegen in the
> end.
>
> However, when debugfs is enabled, but CONFIG_NOVA_CORE_DEBUGFS=n, then using
> debugfs functionalities would *not* be compiled out (so, for the `Dir::empty()`
> patch in the previous iteration, all of the debugging facility would still be
> around with CONFIG_DEBUG_FS=y and CONFIG_NOVA_CORE_DEBUGFS=n, which is not
> desirable).
>
> The straightforward solution is thus sprinkle `#[cfg(CONFIG_NOVA_CORE_DEBUGFS)]`
> everywhere where debugfs is touched, which is non-ideal.
>
> One idea is to create types that look exactly like `Dir` but always ZST and
> no-op regardless whether CONFIG_DEBUG_FS is enabled... But that feel a bit..
> weird. Matthew, what do you think?
There is no need for CONFIG_NOVA_CORE_DEBUGFS in the first place. The only
Kconfig we need is for retaining the GSP log buffers after driver unbind.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-31 0:10 ` Danilo Krummrich
@ 2026-01-31 0:16 ` John Hubbard
2026-01-31 0:20 ` Danilo Krummrich
2026-01-31 4:11 ` Timur Tabi
2026-01-31 0:26 ` Timur Tabi
1 sibling, 2 replies; 25+ messages in thread
From: John Hubbard @ 2026-01-31 0:16 UTC (permalink / raw)
To: Danilo Krummrich, Gary Guo
Cc: Timur Tabi, mmaurer@google.com, rust-for-linux@vger.kernel.org,
nouveau@lists.freedesktop.org, Joel Fernandes,
aliceryhl@google.com, Alexandre Courbot
On 1/30/26 4:10 PM, Danilo Krummrich wrote:
> On Sat Jan 31, 2026 at 12:58 AM CET, Gary Guo wrote:
>> On Fri Jan 30, 2026 at 11:36 PM GMT, Timur Tabi wrote:
>>> I think there might be a problem with this code that I don't know how to resolve.
...
> There is no need for CONFIG_NOVA_CORE_DEBUGFS in the first place. The only
> Kconfig we need is for retaining the GSP log buffers after driver unbind.
Interesting. And in fact...I think we can live without that feature,
especially if it complicates things here. If someone is debugging,
and they need logs, they can usually (always?) find a way to leave
the driver bound long enough to get them, or so I believe after
some experience doing this. Yes?
thanks,
--
John Hubbard
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-31 0:16 ` John Hubbard
@ 2026-01-31 0:20 ` Danilo Krummrich
2026-01-31 0:24 ` John Hubbard
2026-01-31 4:11 ` Timur Tabi
1 sibling, 1 reply; 25+ messages in thread
From: Danilo Krummrich @ 2026-01-31 0:20 UTC (permalink / raw)
To: John Hubbard
Cc: Gary Guo, Timur Tabi, mmaurer@google.com,
rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org,
Joel Fernandes, aliceryhl@google.com, Alexandre Courbot
On Sat Jan 31, 2026 at 1:16 AM CET, John Hubbard wrote:
> On 1/30/26 4:10 PM, Danilo Krummrich wrote:
>> On Sat Jan 31, 2026 at 12:58 AM CET, Gary Guo wrote:
>>> On Fri Jan 30, 2026 at 11:36 PM GMT, Timur Tabi wrote:
>>>> I think there might be a problem with this code that I don't know how to resolve.
> ...
>> There is no need for CONFIG_NOVA_CORE_DEBUGFS in the first place. The only
>> Kconfig we need is for retaining the GSP log buffers after driver unbind.
>
> Interesting. And in fact...I think we can live without that feature,
> especially if it complicates things here. If someone is debugging,
> and they need logs, they can usually (always?) find a way to leave
> the driver bound long enough to get them, or so I believe after
> some experience doing this. Yes?
I guess you could put a long enough sleep or just leak the debugfs directory.
Not sure how much it hurts developer ergonimics going for multiple iterations
though.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-31 0:20 ` Danilo Krummrich
@ 2026-01-31 0:24 ` John Hubbard
0 siblings, 0 replies; 25+ messages in thread
From: John Hubbard @ 2026-01-31 0:24 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Gary Guo, Timur Tabi, mmaurer@google.com,
rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org,
Joel Fernandes, aliceryhl@google.com, Alexandre Courbot
On 1/30/26 4:20 PM, Danilo Krummrich wrote:
> On Sat Jan 31, 2026 at 1:16 AM CET, John Hubbard wrote:
>> On 1/30/26 4:10 PM, Danilo Krummrich wrote:
>>> On Sat Jan 31, 2026 at 12:58 AM CET, Gary Guo wrote:
>>>> On Fri Jan 30, 2026 at 11:36 PM GMT, Timur Tabi wrote:
>>>>> I think there might be a problem with this code that I don't know how to resolve.
>> ...
>>> There is no need for CONFIG_NOVA_CORE_DEBUGFS in the first place. The only
>>> Kconfig we need is for retaining the GSP log buffers after driver unbind.
>>
>> Interesting. And in fact...I think we can live without that feature,
>> especially if it complicates things here. If someone is debugging,
>> and they need logs, they can usually (always?) find a way to leave
>> the driver bound long enough to get them, or so I believe after
>> some experience doing this. Yes?
>
> I guess you could put a long enough sleep or just leak the debugfs directory.
> Not sure how much it hurts developer ergonimics going for multiple iterations
> though.
hmmm maybe you are right after all. Because I was thinking that driver
unbind wouldn't happen right away, but actually it would and yes that's
not a happy story. (The hacked up debugfs I have locally doesn't follow
the rules.)
OK never mind, I like your config idea now. :)
thanks,
--
John Hubbard
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-31 0:16 ` John Hubbard
2026-01-31 0:20 ` Danilo Krummrich
@ 2026-01-31 4:11 ` Timur Tabi
1 sibling, 0 replies; 25+ messages in thread
From: Timur Tabi @ 2026-01-31 4:11 UTC (permalink / raw)
To: gary@garyguo.net, dakr@kernel.org, John Hubbard
Cc: nouveau@lists.freedesktop.org, Joel Fernandes,
aliceryhl@google.com, mmaurer@google.com, Alexandre Courbot,
rust-for-linux@vger.kernel.org
On Fri, 2026-01-30 at 16:16 -0800, John Hubbard wrote:
> If someone is debugging,
> and they need logs, they can usually (always?) find a way to leave
> the driver bound long enough to get them, or so I believe after
> some experience doing this. Yes?
I haven't tested this in Nova, but in Nouveau the problem was that if GSP-RM failed to reach
INIT_DONE (for whatever reason), Nouveau would dismantle the entire GSP object and delete
everything associated with it. Rather than try to change that behavior, it was easier to create
"shadow" debugfs entries that were tied to the PCI device.
If we can ensure that NovaCore doesn't behave this way, then we won't have this problem and
won't need a Kconfig (or a command-line option like Nouveau uses).
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-31 0:10 ` Danilo Krummrich
2026-01-31 0:16 ` John Hubbard
@ 2026-01-31 0:26 ` Timur Tabi
2026-01-31 0:35 ` Danilo Krummrich
1 sibling, 1 reply; 25+ messages in thread
From: Timur Tabi @ 2026-01-31 0:26 UTC (permalink / raw)
To: gary@garyguo.net, dakr@kernel.org
Cc: nouveau@lists.freedesktop.org, Joel Fernandes,
aliceryhl@google.com, mmaurer@google.com, Alexandre Courbot,
John Hubbard, rust-for-linux@vger.kernel.org
On Sat, 2026-01-31 at 01:10 +0100, Danilo Krummrich wrote:
> > One idea is to create types that look exactly like `Dir` but always ZST and
> > no-op regardless whether CONFIG_DEBUG_FS is enabled... But that feel a bit..
> > weird. Matthew, what do you think?
>
> There is no need for CONFIG_NOVA_CORE_DEBUGFS in the first place. The only
> Kconfig we need is for retaining the GSP log buffers after driver unbind.
Danilo, I'm confused. You told me to add this Kconfig:
https://lore.kernel.org/rust-for-linux/DFR6RWROWBYA.1Q9JKH8UDSXOX@kernel.org/
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-31 0:26 ` Timur Tabi
@ 2026-01-31 0:35 ` Danilo Krummrich
2026-01-31 0:51 ` Timur Tabi
0 siblings, 1 reply; 25+ messages in thread
From: Danilo Krummrich @ 2026-01-31 0:35 UTC (permalink / raw)
To: Timur Tabi
Cc: gary@garyguo.net, nouveau@lists.freedesktop.org, Joel Fernandes,
aliceryhl@google.com, mmaurer@google.com, Alexandre Courbot,
John Hubbard, rust-for-linux@vger.kernel.org
On Sat Jan 31, 2026 at 1:26 AM CET, Timur Tabi wrote:
> On Sat, 2026-01-31 at 01:10 +0100, Danilo Krummrich wrote:
>> > One idea is to create types that look exactly like `Dir` but always ZST and
>> > no-op regardless whether CONFIG_DEBUG_FS is enabled... But that feel a bit..
>> > weird. Matthew, what do you think?
>>
>> There is no need for CONFIG_NOVA_CORE_DEBUGFS in the first place. The only
>> Kconfig we need is for retaining the GSP log buffers after driver unbind.
>
> Danilo, I'm confused. You told me to add this Kconfig:
>
> https://lore.kernel.org/rust-for-linux/DFR6RWROWBYA.1Q9JKH8UDSXOX@kernel.org/
Maybe a misunderstanding due to my type? I.e.
"I think it's OK to always have the entries on keeping them beyond device unbind
has to be behind a Kconfig option."
vs.
"I think it's OK to always have the entries, only keeping them beyond device unbind
has to be behind a Kconfig option."
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v6 7/7] gpu: nova-core: create GSP-RM logging buffers debugfs entries
2026-01-31 0:35 ` Danilo Krummrich
@ 2026-01-31 0:51 ` Timur Tabi
0 siblings, 0 replies; 25+ messages in thread
From: Timur Tabi @ 2026-01-31 0:51 UTC (permalink / raw)
To: dakr@kernel.org
Cc: John Hubbard, gary@garyguo.net, mmaurer@google.com,
rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org,
Joel Fernandes, aliceryhl@google.com, Alexandre Courbot
On Sat, 2026-01-31 at 01:35 +0100, Danilo Krummrich wrote:
> > Danilo, I'm confused. You told me to add this Kconfig:
> >
> > https://lore.kernel.org/rust-for-linux/DFR6RWROWBYA.1Q9JKH8UDSXOX@kernel.org/
>
> Maybe a misunderstanding due to my type? I.e.
>
> "I think it's OK to always have the entries on keeping them beyond device unbind
> has to be behind a Kconfig option."
>
> vs.
>
> "I think it's OK to always have the entries, only keeping them beyond device unbind
> has to be behind a Kconfig option."
LOL. Ok, I will remove the Kconfig.
I have no idea how to add support for keeping the entries beyond device unbind in NovaCore.
That code in Nouveau took months to work out, and it's still hackish even by C standards.
^ permalink raw reply [flat|nested] 25+ messages in thread