gpu: nova-core: expose the logging buffers via debugfs
Subsystems (6)
v3 cover gpu: nova-core: expose the logging buffers via debugfs
GSP-RM writes its printf message to "logging buffers", which are blocks memory allocated by the driver. The messages are encoded, so exposing the buffers as debugfs entries allows the buffers to be extracted and decoded by a special application.
When the driver loads, a /sys/kernel/debug/nova_core root entry is created. To do this, the normal module_pci_driver! macro call is replaced with an explicit initialization function, as this allows that debugfs entry to be created once for all GPUs.
Then in each GPU's initialization, a subdirectory based on the PCI BDF name is created, and the logging buffer entries are created under that.
Note: the debugfs entry has a file size of 0, because debugfs defaults a 0 size and the Rust abstractions do not adjust it for the same of the object. Nouveau makes this adjustment manually in the driver.
Based on Linus' repository, not drm-rust-next.
Summary of changes:
1. Replace module_pci_driver! with explicit init function.
2. Creates root, per-gpu, and individual buffer debugfs entries.
3. Adds a pci::name() method to generate a PCI device name string.
4. Adds a lookup() method to debugfs, so that the root debugfs entry doesn't need to be a global variable like it is in Nouveau.
5. Makes some updates to debugfs core code.
Alexandre Courbot (1):
gpu: nova-core: implement BinaryWriter for LogBuffer
Timur Tabi (8):
rust: pci: add PCI device name method
rust: debugfs: add lookup contructor
rust: debugfs: add Dir::empty() for no-op directory handle
rust: debugfs: fix Dir::scope() to not borrow self for returned
lifetime
gpu: nova-core: Replace module_pci_driver! with explicit module init
gpu: nova-core: create debugfs root when driver loads
gpu: nova-core: use pin projection in method boot()
gpu: nova-core: create GSP-RM logging buffers debugfs entries
drivers/gpu/nova-core/gsp.rs | 66 +++++++++++++++--- drivers/gpu/nova-core/gsp/boot.rs | 15 ++-- drivers/gpu/nova-core/nova_core.rs | 29 +++++++- rust/kernel/debugfs.rs | 108 ++++++++++++++++++++++------- rust/kernel/debugfs/entry.rs | 49 ++++++++++++- rust/kernel/pci.rs | 37 ++++++++++ 6 files changed, 259 insertions(+), 45 deletions(-)
base-commit: ea1013c1539270e372fc99854bc6e4d94eaeff66
Signature · 1 line
2.52.0
PATCH v3 1/9 rust: pci: add PCI device name method
Add a name() method to the PCI Device type, which returns a CStr that contains the device name, typically the BDF address.
--- rust/kernel/pci.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
rust/kernel/pci.rs +37 -0
@@ -427,6 +427,43 @@ pub fn pci_class(&self) -> Class {// SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`. Class ::from_raw (unsafe { (*self.as_raw ()).class }) } + +/// Returns the PCI device name. +/// +/// This returns the device name in the format "DDDD:BB:DD.F" where: +/// - 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) +/// +/// # Examples +/// +/// ``` +/// # use kernel::{c_str, debugfs::Dir, device::Core, pci, prelude::*}; +/// fn create_debugfs(pdev: &pci::Device<Core>) -> Result { +/// let dir = Dir::new(pdev.name()); +/// Ok(()) +/// } +/// ``` +#[inline] +pub fn name (&self) -> &CStr { +// SAFETY: By its type invariant `self.as_raw` is always a valid pointer to a +// `struct pci_dev`, which contains a `struct device dev` member. +unsafe { +let pci_dev = self.as_raw (); +let dev = &raw const (*pci_dev).dev; + +// If init_name is set, use it; otherwise use the kobject name +let init_name = (*dev).init_name; +let name_ptr =if !init_name.is_null () { + init_name + }else { + (*dev).kobj.name + }; + +CStr ::from_char_ptr (name_ptr) + } + } }impl Device <device::Core > {
Signature · 1 line
2.52.0
Alice Ryhl
maintainerRe: [PATCH v3 1/9] rust: pci: add PCI device name method
Quoted 29 lines · Timur Tabi
Add a
name()method to the PCIDevicetype, which returns a CStr that contains the device name, typically the BDF address.Signed-off-by Timur Tabi <[email protected]>--- rust/kernel/pci.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)rust/kernel/pci.rs +13
-0@@ -427,6 +427,43 @@ pub fn pci_class(&self) -> Class {// SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`. Class ::from_raw (unsafe { (*self.as_raw ()).class }) } + +/// Returns the PCI device name. +/// +/// This returns the device name in the format "DDDD:BB:DD.F" where: +/// - 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) +/// +/// # Examples +/// +/// ``` +/// # use kernel::{c_str, debugfs::Dir, device::Core, pci, prelude::*};
rust/kernel/pci.rs pub fn pci_class(&self) -> Class { · line 452
Quoted 10 lines
rust/kernel/pci.rs +10
-0+/// fn create_debugfs(pdev: &pci::Device<Core>) -> Result { +/// let dir = Dir::new(pdev.name()); +/// Ok(()) +/// } +/// ``` +#[inline] +pub fn name (&self) -> &CStr { +// SAFETY: By its type invariant `self.as_raw` is always a valid pointer to a +// `struct pci_dev`, which contains a `struct device dev` member. +unsafe {
What's the actual unsafe operation in this block? We generally try to limit the scope of unsafe as much as possible.
Alice
Quoted 20 lines
rust/kernel/pci.rs +14
-0+let pci_dev = self.as_raw (); +let dev = &raw const (*pci_dev).dev; + +// If init_name is set, use it; otherwise use the kobject name +let init_name = (*dev).init_name; +let name_ptr =if !init_name.is_null () { + init_name + }else { + (*dev).kobj.name + }; + +CStr ::from_char_ptr (name_ptr) + } + } }impl Device <device::Core > {Signature · 1 line
2.52.0
Danilo Krummrich
maintainerRe: [PATCH v3 1/9] rust: pci: add PCI device name method
In reply to Timur TabiQuoted 10 lines · Timur Tabi
rust/kernel/pci.rs +10
-0+#[inline] +pub fn name (&self) -> &CStr { +// SAFETY: By its type invariant `self.as_raw` is always a valid pointer to a +// `struct pci_dev`, which contains a `struct device dev` member. +unsafe { +let pci_dev = self.as_raw (); +let dev = &raw const (*pci_dev).dev; + +// If init_name is set, use it; otherwise use the kobject name +let init_name = (*dev).init_name;
rust/kernel/pci.rs pub fn pci_class(&self) -> Class { · line 467
Quoted 10 lines
rust/kernel/pci.rs +9
-0+let name_ptr =if !init_name.is_null () { + init_name + }else { + (*dev).kobj.name + }; + +CStr ::from_char_ptr (name_ptr) + } + } }
This does not belong into pci::Device, but device::Device.
Also, please do not open-code the init_name dance, please use dev_name() instead.
Re: [PATCH v3 1/9] rust: pci: add PCI device name method
In reply to Timur Tabirust/kernel/pci.rs pub fn pci_class(&self) -> Class { · line 437
Quoted 8 lines · Timur Tabi
rust/kernel/pci.rs +7
-0+/// Returns the PCI device name. +/// +/// This returns the device name in the format "DDDD:BB:DD.F" where: +/// - 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)
It probably looks better like (i.e. code spans + ending in period):
/// This returns the device name in the format `DDDD:BB:DD.F` where:
/// - `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).Quoted 1 line
+ // If
init_nameis set, use it; otherwise use the kobject name
Similarly:
// If `init_name` is set, use it; otherwise use the kobject name.
Thanks!
Cheers,
Miguel
PATCH v3 2/9 rust: debugfs: add lookup contructor
In reply to Timur TabiAdd a new constructor for Dir that uses the debugfs_lookup() API to obtain a reference to an existing debugfs directory entry.
The key difference from Dir::new() and Dir::subdir() is the cleanup
semantics: when a Dir obtained via lookup() is dropped, it calls dput() to release the reference rather than debugfs_remove() which would delete the directory.
To implement this cleanup distinction, the Entry class now includes an is_lookup boolean that specifies how the entry was created and therefore how it should be dropped.
--- rust/kernel/debugfs.rs | 43 +++++++++++++++++++++++++++++++ rust/kernel/debugfs/entry.rs | 49 +++++++++++++++++++++++++++++++++--- 2 files changed, 89 insertions(+), 3 deletions(-)
rust/kernel/debugfs.rs +43 -0
@@ -110,6 +110,49 @@ pub fn new(name: &CStr) -> Self {Dir ::create (name,None ) } +/// Looks up an existing directory in DebugFS. +/// +/// If `parent` is [`None`], the lookup is performed from the root of the debugfs filesystem. +/// +/// Returns [`Some(Dir)`] representing the looked-up directory if found, or [`None`] if the +/// directory does not exist or if debugfs is not enabled. When dropped, the [`Dir`] will +/// release its reference to the dentry without removing the directory from the filesystem. +/// +/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// // Look up a top-level directory +/// let nova_core = Dir::lookup(c_str!("nova_core"), None); +/// +/// // Look up a subdirectory within a parent +/// let parent = Dir::new(c_str!("parent")); +/// let child = parent.subdir(c_str!("child")); +/// let looked_up = Dir::lookup(c_str!("child"), Some(&parent)); +/// // `looked_up` now refers to the same directory as `child`. +/// // Dropping `looked_up` will not remove the directory. +/// ``` +pub fn lookup (name: &CStr , parent:Option <&Dir >) ->Option <Self > { +#[cfg( CONFIG_DEBUG_FS )] + { +let parent_entry =match parent { +// If the parent couldn't be allocated, just early-return +Some (Dir (None )) =>return None , +Some (Dir (Some (entry))) =>Some (entry.clone ()), +None =>None , + }; +let entry =Entry ::lookup (name, parent_entry)?; +Some (Self ( +// If Arc creation fails, the `Entry` will be dropped, so the reference will be +// released. +Arc ::new (entry,GFP_KERNEL ).ok (), + )) + } +#[cfg(not( CONFIG_DEBUG_FS ))] +None + } +/// Creates a subdirectory within this directory. /// /// # Examples
rust/kernel/debugfs/entry.rs +46 -3
93 lines not on this page · raw message
Signature · 1 line
2.52.0
Danilo Krummrich
maintainerRe: [PATCH v3 2/9] rust: debugfs: add lookup contructor
Quoted 11 lines · Timur Tabi
Add a new constructor for Dir that uses the
debugfs_lookup()API to obtain a reference to an existing debugfs directory entry.The key difference from
Dir::new()andDir::subdir()is the cleanup
semantics: when a Dir obtained vialookup()is dropped, it callsdput()to release the reference rather thandebugfs_remove()which would delete the directory.To implement this cleanup distinction, the Entry class now includes an
is_lookupboolean that specifies how the entry was created and therefore how it should be dropped.
I have two main comments about this.
(1) I think it would be better to wrap "lookup entries" into a new type in order to account for the fact that they do not behave like regular entries. I.e. when a "lookup entry" is dropped, it does not cause the entry to disappear from the file system. Analogously, when the regular entry is dropped it disappears from the file system regardless of whether a "lookup entry" still has a reference count.
(2) The commit message lacks the motiviation for adding this functionality, which is only for the purpose of a workaround of an unrelated limitation with accessing the fields of a module structure (which Gary already works on a solution for).
I understand it might take a while until the solution Gary works on is ready and I'm perfectly fine with a temporary workaround. But, this workaround has to be on nova-core only and not introduce a feature in core code that in the near future becomes dead code.
If we have a use-case for lookup() elsewhere, I'd like to see it first. But if we don't I prefer not to add this feature as a pure workaround for something else.
In fact, I think that debugfs_lookup() rarely is the correct solution and more often indicates some kind of (design) issue, like in this case.
- Danilo
Re: [PATCH v3 2/9] rust: debugfs: add lookup contructor
In reply to Timur TabiQuoted 40 lines · Timur Tabi
Add a new constructor for Dir that uses the
debugfs_lookup()API to obtain a reference to an existing debugfs directory entry.The key difference from
Dir::new()andDir::subdir()is the cleanup
semantics: when a Dir obtained vialookup()is dropped, it callsdput()to release the reference rather thandebugfs_remove()which would delete the directory.To implement this cleanup distinction, the Entry class now includes an
is_lookupboolean that specifies how the entry was created and therefore how it should be dropped.Signed-off-by Timur Tabi <[email protected]>--- rust/kernel/debugfs.rs | 43 +++++++++++++++++++++++++++++++ rust/kernel/debugfs/entry.rs | 49 +++++++++++++++++++++++++++++++++--- 2 files changed, 89 insertions(+), 3 deletions(-)rust/kernel/debugfs.rs +13
-0@@ -110,6 +110,49 @@ pub fn new(name: &CStr) -> Self {Dir ::create (name,None ) } +/// Looks up an existing directory in DebugFS. +/// +/// If `parent` is [`None`], the lookup is performed from the root of the debugfs filesystem. +/// +/// Returns [`Some(Dir)`] representing the looked-up directory if found, or [`None`] if the +/// directory does not exist or if debugfs is not enabled. When dropped, the [`Dir`] will +/// release its reference to the dentry without removing the directory from the filesystem. +/// +/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir;
rust/kernel/debugfs.rs pub fn new(name: &CStr) -> Self { · line 135
Quoted 10 lines
rust/kernel/debugfs.rs +10
-0+/// // Look up a top-level directory +/// let nova_core = Dir::lookup(c_str!("nova_core"), None); +/// +/// // Look up a subdirectory within a parent +/// let parent = Dir::new(c_str!("parent")); +/// let child = parent.subdir(c_str!("child")); +/// let looked_up = Dir::lookup(c_str!("child"), Some(&parent)); +/// // `looked_up` now refers to the same directory as `child`. +/// // Dropping `looked_up` will not remove the directory. +/// ```
This also breaks the assumption people were able to have with Dir previously that if they have a Dir and debugfs is enabled, it's usable for creating subdirs/files.
This might be considered an issue, but at minimum it needs to be documented.
rust/kernel/debugfs.rs pub fn new(name: &CStr) -> Self { · line 136
Quoted 1 line
rust/kernel/debugfs.rs +1
-0+pub fn lookup (name: &CStr , parent:Option <&Dir >) ->Option <Self > {
We were explicitly asked by Greg *not* to expose error conditions in directory constructors. I would expect that to extend to lookup as well, so this would return a Self, not allowing the caller to find out if the file was actually there. This might be a bit of a grey area as the comment he cited on debugfs_create_file and friends has explicit verbiage about it being bad form to check errors here and debugfs_lookup mentions explicitly a null return for the file not being there.
Another point in favor of just returning Self rather than Option<Self> is that in your proposed real-world usage of this, you create a dummy directory in the None case.
rust/kernel/debugfs.rs pub fn new(name: &CStr) -> Self { · line 138
Quoted 2 lines
rust/kernel/debugfs.rs +2
-0+#[cfg( CONFIG_DEBUG_FS )] + {
We shouldn't need to track a parent entry here, and so shouldn't need to do the whole cloning dance.
rust/kernel/debugfs.rs pub fn new(name: &CStr) -> Self { · line 145
Quoted 7 lines
rust/kernel/debugfs.rs +7
-0+let parent_entry =match parent { +// If the parent couldn't be allocated, just early-return +Some (Dir (None )) =>return None , +Some (Dir (Some (entry))) =>Some (entry.clone ()), +None =>None , + }; +let entry =Entry ::lookup (name, parent_entry)?;
There's no guarantee that this lookup has returned a directory - a lookup can return files too afaict
Quoted 18 lines
rust/kernel/debugfs.rs +10
-0+Some (Self ( +// If Arc creation fails, the `Entry` will be dropped, so the reference will be +// released. +Arc ::new (entry,GFP_KERNEL ).ok (), + )) + } +#[cfg(not( CONFIG_DEBUG_FS ))] +None + } +/// Creates a subdirectory within this directory. /// /// # Examples rust/kernel/debugfs/entry.rs +0
-0
rust/kernel/debugfs/entry.rs pub(crate) struct Entry<'a> { · line 23
Quoted 6 lines
rust/kernel/debugfs/entry.rs +3
-0@@ -18,6 +18,9 @@ pub(crate) struct Entry<'a> { _parent: Option<Arc<Entry<'static>>>,// If we were created with a non-owning parent, this prevents us from outliving it _phantom: PhantomData<&'a ()>, +// If true, this entry was obtained via debugfs_lookup and should be released +// with dput() instead of debugfs_remove(). + is_lookup: bool,
I agree with Danilo - I think this would be cleaner as a different type (or at least an enum variant) rather than adding another field (if we need it). Notably:
1. _parent is not required for a lookup entry - the lookup doesn't need to keep it alive, because it's not using the "If I have the directory handle, I can create real files in it" semantics we had before.
2. The _phantom is irrelevant for a lookup entry because they can't be scoped
3. The drop behavior is different (which you're gating with is_lookup).
Basically the only part that is the same is the presence of a dentry pointer under the hood.
Quoted 97 lines
rust/kernel/debugfs/entry.rs +43
-4}// SAFETY: [`Entry`] is just a `dentry` under the hood, which the API promises can be transferred @@ -43,9 +46,38 @@ pub(crate) fn dynamic_dir(name: &CStr, parent: Option<Arc<Self>>) -> Self { entry, _parent: parent, _phantom: PhantomData, + is_lookup: false, } } +/// Looks up an existing entry in debugfs. +/// +/// Returns [`Some(Entry)`] representing the looked-up dentry if the entry exists, or [`None`] +/// if the entry was not found. When dropped, the entry will release its reference via `dput()` +/// instead of removing the directory. +pub (crate )fn lookup (name: &CStr , parent:Option <Arc <Self >>) ->Option <Self > { +let parent_ptr =match &parent { +Some (entry) => entry.as_ptr (), +None => core::ptr::null_mut (), + }; +// SAFETY: The invariants of this function's arguments ensure the safety of this call. +// * `name` is a valid C string by the invariants of `&CStr`. +// * `parent_ptr` is either `NULL` (if `parent` is `None`), or a pointer to a valid +// `dentry` by our invariant. `debugfs_lookup` handles `NULL` pointers correctly. +let entry =unsafe { bindings::debugfs_lookup (name.as_char_ptr (), parent_ptr) }; + +if entry.is_null () { +return None ; + } + +Some (Entry { + entry, + _parent: parent, + _phantom:PhantomData , + is_lookup:true , + }) + } +/// # Safety /// /// * `data` must outlive the returned `Entry`. @@ -76,6 +108,7 @@ pub(crate) unsafe fn dynamic_file<T>( entry, _parent: Some(parent), _phantom: PhantomData, + is_lookup: false, } } }@@ -97,6 +130,7 @@ pub(crate) fn dir(name: &CStr, parent: Option<&'a Entry<'_>>) -> Self { entry, _parent: None, _phantom: PhantomData, + is_lookup: false, } }@@ -129,6 +163,7 @@ pub(crate) fn file<T>( entry, _parent: None, _phantom: PhantomData, + is_lookup: false, } } }@@ -140,6 +175,7 @@ pub(crate) fn empty() -> Self { entry: core::ptr::null_mut(), _parent: None, _phantom: PhantomData, + is_lookup: false, } }@@ -157,8 +193,15 @@ pub(crate) fn as_ptr(&self) -> *mut bindings::dentry {impl Drop for Entry <'_> {fn drop (&mut self) { -// SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries. -// `as_ptr` guarantees that the pointer is of this form. -unsafe { bindings::debugfs_remove (self.as_ptr ()) } +if self.is_lookup { +// SAFETY: `dput` can take `NULL` and legal dentries. +// `as_ptr` guarantees that the pointer is of this form. +// This entry was obtained via `debugfs_lookup`, so we release the reference. +unsafe { bindings::dput (self.as_ptr ()) } + }else { +// SAFETY: `debugfs_remove` can take `NULL`, error values, and legal DebugFS dentries. +// `as_ptr` guarantees that the pointer is of this form. +unsafe { bindings::debugfs_remove (self.as_ptr ()) } + } } } --2.52.0
PATCH v3 3/9 rust: debugfs: add Dir::empty() for no-op directory handle
In reply to Timur TabiAdd a Dir::empty() constructor that returns a Dir representing no directory. Operations on this directory (creating files, subdirectories) become no-ops, but the data is still stored.
This is useful as a fallback when a directory lookup fails but you still need a Dir value to pass to APIs like Dir::scope(). For example:
let dir = Dir::lookup(c_str!("parent"), None)
.unwrap_or_else(Dir::empty);When CONFIG_DEBUG_FS is enabled, returns Dir(None). When disabled, returns the empty tuple struct Dir().
--- rust/kernel/debugfs.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
rust/kernel/debugfs.rs +24 -0
@@ -110,6 +110,30 @@ pub fn new(name: &CStr) -> Self {Dir ::create (name,None ) } +/// Creates an empty [`Dir`] that represents no directory. +/// +/// Operations on this directory (such as creating files or subdirectories) will be no-ops. +/// This is useful as a fallback when a directory lookup fails but you still need a [`Dir`] +/// value. +/// +/// # Examples +/// +/// ``` +/// # use kernel::c_str; +/// # use kernel::debugfs::Dir; +/// let dir = Dir::lookup(c_str!("maybe_exists"), None) +/// .unwrap_or_else(Dir::empty); +/// // If lookup failed, file creation becomes a no-op +/// ``` +pub fn empty () ->Self { +#[cfg( CONFIG_DEBUG_FS )] + { +Self (None ) + } +#[cfg(not( CONFIG_DEBUG_FS ))] +Self () + } +/// Looks up an existing directory in DebugFS. /// /// If `parent` is [`None`], the lookup is performed from the root of the debugfs filesystem.
Signature · 1 line
2.52.0
PATCH v3 4/9 rust: debugfs: fix Dir::scope() to not borrow self for returned lifetime
In reply to Timur TabiDir::scope() was declared as taking &'a self, which tied the returned PinInit's lifetime to the Dir. This prevented using scope() with a locally-created Dir:
let dir = Dir::lookup(...).unwrap_or_else(Dir::empty);
let scope = dir.scope(...); // Error: returns value referencing local
The borrow was unnecessary since scoped_dir() internally clones the Arc<Entry>. Fix this by cloning self.0 before the closure, allowing the closure to capture the cloned value via move instead of borrowing self.
This also removes the now-unused scoped_dir() helper method, inlining its logic directly into scope().
--- rust/kernel/debugfs.rs | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-)
rust/kernel/debugfs.rs +17 -24
@@ -397,27 +397,6 @@ pub fn write_callback_file<'a, T, E: 'a, W>( self.create_file (name, data, file_ops) } -// While this function is safe, it is intentionally not public because it's a bit of a -// footgun. -// -// Unless you also extract the `entry` later and schedule it for `Drop` at the appropriate -// time, a `ScopedDir` with a `Dir` parent will never be deleted. -fn scoped_dir <'data>(&self, name: &CStr ) ->ScopedDir <'data, 'static> { -#[cfg( CONFIG_DEBUG_FS )] - { -let parent_entry =match &self.0 { -None =>return ScopedDir ::empty (), -Some (entry) => entry.clone (), - }; -ScopedDir { - entry:ManuallyDrop ::new (Entry ::dynamic_dir (name,Some (parent_entry))), - _phantom:PhantomData , - } - } -#[cfg(not( CONFIG_DEBUG_FS ))] -ScopedDir ::empty () - } -/// Creates a new scope, which is a directory associated with some data `T`. /// /// The created directory will be a subdirectory of `self`. The `init` closure is called to
@@ -427,7 +406,7 @@ fn scoped_dir<'data>(&self, name: &CStr) -> ScopedDir<'data, 'static> {/// The entire directory tree created within the scope will be removed when the returned /// `Scope` handle is dropped. pub fn scope <'a,T : 'a,E : 'a,F >( - &'a self, + &self, data:impl PinInit <T ,E > + 'a, name: &'aCStr , init: F,
@@ -435,8 +414,22 @@ pub fn scope<'a, T: 'a, E: 'a, F>( where F: for<'data, 'dir> FnOnce(&'data T, &'dir ScopedDir<'data, 'dir>) + 'a, { -Scope ::new (data, |data| { -let scoped = self.scoped_dir (name); +// Clone the parent entry so the closure doesn't need to borrow `self`. +#[cfg( CONFIG_DEBUG_FS )] +let parent_entry = self.0 .clone (); + +Scope ::new (data,move |data| { +#[cfg( CONFIG_DEBUG_FS )] +let scoped =match parent_entry { +None =>ScopedDir ::empty (), +Some (entry) =>ScopedDir { + entry:ManuallyDrop ::new (Entry ::dynamic_dir (name,Some (entry))), + _phantom:PhantomData , + }, + }; +#[cfg(not( CONFIG_DEBUG_FS ))] +let scoped =ScopedDir ::empty (); +init (data, &scoped); scoped.into_entry () })
Signature · 1 line
2.52.0
Re: [PATCH v3 4/9] rust: debugfs: fix Dir::scope() to not borrow self for returned lifetime
Quoted 7 lines · Timur Tabi
Dir::scope()was declared as taking &'a self, which tied the returned PinInit's lifetime to the Dir. This prevented usingscope()with a locally-created Dir:let dir = Dir::lookup(...).unwrap_or_else(Dir::empty);
let scope =dir.scope(...); // Error: returns value referencing local
This looks like you're trying to return an impl PinInit<Scope> from your function (which isn't written in the example, but I assume that's what you eventually do, otherwise this wouldn't be an error).
The generalized solution to this is to use pin_init_scope, i.e.
```
pin_init_scope(|| {
let dir = // Generate a `Dir` somehow here
let scope = dir.scope(/* Scope defining function here */)
// Anything else you wanted to do
Ok(scope)
})
```Quoted 55 lines
The borrow was unnecessary since
scoped_dir()internally clones the Arc<Entry>. Fix this by cloning self.0 before the closure, allowing the closure to capture the cloned value via move instead of borrowing self.This also removes the now-unused
scoped_dir()helper method, inlining its logic directly intoscope().Signed-off-by Timur Tabi <[email protected]>--- rust/kernel/debugfs.rs | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-)rust/kernel/debugfs.rs +1
-22@@ -397,27 +397,6 @@ pub fn write_callback_file<'a, T, E: 'a, W>( self.create_file (name, data, file_ops) } -// While this function is safe, it is intentionally not public because it's a bit of a -// footgun. -// -// Unless you also extract the `entry` later and schedule it for `Drop` at the appropriate -// time, a `ScopedDir` with a `Dir` parent will never be deleted. -fn scoped_dir <'data>(&self, name: &CStr ) ->ScopedDir <'data, 'static> { -#[cfg( CONFIG_DEBUG_FS )] - { -let parent_entry =match &self.0 { -None =>return ScopedDir ::empty (), -Some (entry) => entry.clone (), - }; -ScopedDir { - entry:ManuallyDrop ::new (Entry ::dynamic_dir (name,Some (parent_entry))), - _phantom:PhantomData , - } - } -#[cfg(not( CONFIG_DEBUG_FS ))] -ScopedDir ::empty () - } -/// Creates a new scope, which is a directory associated with some data `T`. /// /// The created directory will be a subdirectory of `self`. The `init` closure is called to @@ -427,7 +406,7 @@ fn scoped_dir<'data>(&self, name: &CStr) -> ScopedDir<'data, 'static> {/// The entire directory tree created within the scope will be removed when the returned /// `Scope` handle is dropped. pub fn scope <'a,T : 'a,E : 'a,F >( - &'a self, + &self, data:impl PinInit <T ,E > + 'a, name: &'aCStr , init: F,
rust/kernel/debugfs.rs pub fn scope<'a, T: 'a, E: 'a, F>( · line 419
Quoted 8 lines
rust/kernel/debugfs.rs +3
-2@@ -435,8 +414,22 @@ pub fn scope<'a, T: 'a, E: 'a, F>( where F: for<'data, 'dir> FnOnce(&'data T, &'dir ScopedDir<'data, 'dir>) + 'a, { -Scope ::new (data, |data| { -let scoped = self.scoped_dir (name); +// Clone the parent entry so the closure doesn't need to borrow `self`. +#[cfg( CONFIG_DEBUG_FS )] +let parent_entry = self.0 .clone ();
This isn't necessarily a show-stopper, but this lets us create a weird intermediate state where from the user's PoV, there are no references to a directory (they don't have the Dir anymore, and a Scope doesn't exist yet, just a PinInit<Scope>), but the directory is held alive. It feels weird, but I suppose users won't usually have a PinInit<Scope> around for long.
The weirdness in debugging would look like:
1. Create dir
2. See dir in filesystem
3. Call scope, but don't actually activate the PinInit yet (either due to coding error or because the target data structure won't be available until later.
4. See no subdirectories under dir in filesystem
5. Drop dir
6. See dir still hanging around
This would also happen with my pin_init_scope suggestion, but at least then user code would be explicitly capturing the Dir right in front of them in the existing-dir case, and in the "create-or-lookup new dir" case like you've got here, would delay directory creation until we're actually instantiating the directory somewhere.
Quoted 20 lines
rust/kernel/debugfs.rs +13
-1+ +Scope ::new (data,move |data| { +#[cfg( CONFIG_DEBUG_FS )] +let scoped =match parent_entry { +None =>ScopedDir ::empty (), +Some (entry) =>ScopedDir { + entry:ManuallyDrop ::new (Entry ::dynamic_dir (name,Some (entry))), + _phantom:PhantomData , + }, + }; +#[cfg(not( CONFIG_DEBUG_FS ))] +let scoped =ScopedDir ::empty (); +init (data, &scoped); scoped.into_entry () }) --2.52.0
Quoted 90 lines · Timur Tabi
Dir::scope()was declared as taking &'a self, which tied the returned PinInit's lifetime to the Dir. This prevented usingscope()with a locally-created Dir:let dir = Dir::lookup(...).unwrap_or_else(Dir::empty);
let scope =dir.scope(...); // Error: returns value referencing localThe borrow was unnecessary since
scoped_dir()internally clones the Arc<Entry>. Fix this by cloning self.0 before the closure, allowing the closure to capture the cloned value via move instead of borrowing self.This also removes the now-unused
scoped_dir()helper method, inlining its logic directly intoscope().Signed-off-by Timur Tabi <[email protected]>--- rust/kernel/debugfs.rs | 41 +++++++++++++++++------------------------ 1 file changed, 17 insertions(+), 24 deletions(-)rust/kernel/debugfs.rs +17
-25@@ -397,27 +397,6 @@ pub fn write_callback_file<'a, T, E: 'a, W>( self.create_file (name, data, file_ops) } -// While this function is safe, it is intentionally not public because it's a bit of a -// footgun. -// -// Unless you also extract the `entry` later and schedule it for `Drop` at the appropriate -// time, a `ScopedDir` with a `Dir` parent will never be deleted. -fn scoped_dir <'data>(&self, name: &CStr ) ->ScopedDir <'data, 'static> { -#[cfg( CONFIG_DEBUG_FS )] - { -let parent_entry =match &self.0 { -None =>return ScopedDir ::empty (), -Some (entry) => entry.clone (), - }; -ScopedDir { - entry:ManuallyDrop ::new (Entry ::dynamic_dir (name,Some (parent_entry))), - _phantom:PhantomData , - } - } -#[cfg(not( CONFIG_DEBUG_FS ))] -ScopedDir ::empty () - } -/// Creates a new scope, which is a directory associated with some data `T`. /// /// The created directory will be a subdirectory of `self`. The `init` closure is called to @@ -427,7 +406,7 @@ fn scoped_dir<'data>(&self, name: &CStr) -> ScopedDir<'data, 'static> {/// The entire directory tree created within the scope will be removed when the returned /// `Scope` handle is dropped. pub fn scope <'a,T : 'a,E : 'a,F >( - &'a self, + &self, data:impl PinInit <T ,E > + 'a, name: &'aCStr , init: F,@@ -435,8 +414,22 @@ pub fn scope<'a, T: 'a, E: 'a, F>( where F: for<'data, 'dir> FnOnce(&'data T, &'dir ScopedDir<'data, 'dir>) + 'a, { -Scope ::new (data, |data| { -let scoped = self.scoped_dir (name); +// Clone the parent entry so the closure doesn't need to borrow `self`. +#[cfg( CONFIG_DEBUG_FS )] +let parent_entry = self.0 .clone (); + +Scope ::new (data,move |data| { +#[cfg( CONFIG_DEBUG_FS )] +let scoped =match parent_entry { +None =>ScopedDir ::empty (), +Some (entry) =>ScopedDir { + entry:ManuallyDrop ::new (Entry ::dynamic_dir (name,Some (entry))), + _phantom:PhantomData , + }, + }; +#[cfg(not( CONFIG_DEBUG_FS ))] +let scoped =ScopedDir ::empty (); +init (data, &scoped); scoped.into_entry () }) --2.52.0
PATCH v3 5/9 gpu: nova-core: Replace module_pci_driver! with explicit module init
In reply to Timur TabiReplace 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.
--- drivers/gpu/nova-core/nova_core.rs | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)
drivers/gpu/nova-core/nova_core.rs +20 -3
@@ -2,6 +2,9 @@//! Nova Core GPU Driver +use kernel::{error::Error , pci, prelude::*,InPlaceModule }; +use pin_init::{PinInit , pinned_drop}; +#[macro_use] mod bitfield;
@@ -21,13 +24,27 @@pub (crate )const MODULE_NAME : &kernel::str::CStr = <LocalModule as kernel::ModuleMetadata >::NAME ; -kernel::module_pci_driver! { -type : driver::NovaCore , +#[pin_data( PinnedDrop )] +struct NovaCoreModule { +#[pin] + _driver: kernel::driver::Registration <pci::Adapter <driver::NovaCore >>, +} + +impl InPlaceModule for NovaCoreModule { +fn init (module: &'static kernel::ThisModule ) ->impl PinInit <Self ,Error > { +pr_info! ("NovaCore GPU driver loaded\n" ); +try_pin_init! (Self { + _driver <- kernel::driver::Registration ::new (MODULE_NAME , module), + }) + } +} + +module! { +type :NovaCoreModule , name:"NovaCore" , authors: ["Danilo Krummrich" ], description:"Nova Core GPU driver" , license:"GPL v2" , - firmware: [], } kernel::module_firmware!(firmware::ModInfoBuilder );
Signature · 1 line
2.52.0
Danilo Krummrich
maintainerRe: [PATCH v3 5/9] gpu: nova-core: Replace module_pci_driver! with explicit module init
drivers/gpu/nova-core/nova_core.rs line 6
Quoted 10 lines · Timur Tabi
drivers/gpu/nova-core/nova_core.rs +2
-0@@ -2,6 +2,9 @@//! Nova Core GPU Driver +use kernel::{error::Error , pci, prelude::*,InPlaceModule }; +use pin_init::{PinInit , pinned_drop};
Please use kernel vertical style [1].
[1] https://docs.kernel.org/rust/coding-guidelines.html#imports
Quoted 9 lines
drivers/gpu/nova-core/nova_core.rs +1
-1+#[macro_use] mod bitfield;@@ -21,13 +24,27 @@pub (crate )const MODULE_NAME : &kernel::str::CStr = <LocalModule as kernel::ModuleMetadata >::NAME ; -kernel::module_pci_driver! {
drivers/gpu/nova-core/nova_core.rs line 35
Quoted 10 lines
drivers/gpu/nova-core/nova_core.rs +9
-1- type: driver::NovaCore, +#[pin_data( PinnedDrop )] +struct NovaCoreModule { +#[pin] + _driver: kernel::driver::Registration <pci::Adapter <driver::NovaCore >>, +} + +impl InPlaceModule for NovaCoreModule { +fn init (module: &'static kernel::ThisModule ) ->impl PinInit <Self ,Error > { +pr_info! ("NovaCore GPU driver loaded\n" );
Please don't add such print statements, drivers should be silent when they work properly.
drivers/gpu/nova-core/nova_core.rs line 30
Quoted 13 lines
drivers/gpu/nova-core/nova_core.rs +8
-1+try_pin_init! (Self { + _driver <- kernel::driver::Registration ::new (MODULE_NAME , module), + }) + } +} + +module! { +type :NovaCoreModule , name:"NovaCore" , authors: ["Danilo Krummrich" ], description:"Nova Core GPU driver" , license:"GPL v2" , - firmware: [],
This change seems unrelated, please send a separate patch for this.
Quoted 5 lines
drivers/gpu/nova-core/nova_core.rs +0
-0} kernel::module_firmware!(firmware::ModInfoBuilder );Signature · 1 line
2.52.0
PATCH v3 6/9 gpu: nova-core: create debugfs root when driver loads
In reply to Timur TabiCreate the 'nova_core' root debugfs entry when the driver loads. The Dir entry is part of the NovaCoreModule, so when the module unloads, the NovaCoreModule object is dropped, and that drops the _debugfs_root, which automatically deletes the debugfs file.
Field order in Rust structs determines drop order - fields are dropped in declaration order. By placing _driver before _debugfs_root:
1. The driver registration is dropped first, which removes all PCI devices and their associated debugfs subdirectories
2. Then _debugfs_root is dropped, removing the now-empty nova_core directory
This ordering is critical because debugfs_remove() performs recursive removal. If the parent directory were removed first, it would free the child dentries, and then the child's drop would attempt to remove an already-freed dentry, causing a use-after-free crash.
--- drivers/gpu/nova-core/nova_core.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/gpu/nova-core/nova_core.rs +9 -3
@@ -2,8 +2,7 @@//! Nova Core GPU Driver -use kernel::{error::Error , pci, prelude::*,InPlaceModule }; -use pin_init::{PinInit , pinned_drop}; +use kernel::{debugfs::Dir , error::Error , pci, prelude::*,InPlaceModule };#[macro_use] mod bitfield;
@@ -24,17 +23,24 @@pub (crate )const MODULE_NAME : &kernel::str::CStr = <LocalModule as kernel::ModuleMetadata >::NAME ; -#[pin_data( PinnedDrop )] +#[pin_data] struct NovaCoreModule { +// Note: field order matters for drop order. The driver must be dropped before +// the debugfs directory so that device subdirectories are removed first. #[pin] _driver: kernel::driver::Registration <pci::Adapter <driver::NovaCore >>, + _debugfs_root:Dir , }impl InPlaceModule for NovaCoreModule {fn init (module: &'static kernel::ThisModule ) ->impl PinInit <Self ,Error > {pr_info! ("NovaCore GPU driver loaded\n" ); + +let debugfs_root =Dir ::new (kernel::c_str!("nova_core" )); +try_pin_init! (Self { _driver <- kernel::driver::Registration ::new (MODULE_NAME , module), + _debugfs_root: debugfs_root, }) } }
Signature · 1 line
2.52.0
Danilo Krummrich
maintainerRe: [PATCH v3 6/9] gpu: nova-core: create debugfs root when driver loads
Quoted 28 lines · Timur Tabi
Create the '
nova_core' root debugfs entry when the driver loads. The Dir entry is part of the NovaCoreModule, so when the module unloads, the NovaCoreModule object is dropped, and that drops the_debugfs_root, which automatically deletes the debugfs file.Field order in Rust structs determines drop order - fields are dropped in declaration order. By placing
_driverbefore_debugfs_root:1. The driver registration is dropped first, which removes all PCI devices and their associated debugfs subdirectories
2. Then_debugfs_rootis dropped, removing the now-emptynova_coredirectoryThis ordering is critical because
debugfs_remove()performs recursive removal. If the parent directory were removed first, it would free the child dentries, and then the child's drop would attempt to remove an already-freed dentry, causing a use-after-free crash.Signed-off-by Timur Tabi <[email protected]>--- drivers/gpu/nova-core/nova_core.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/gpu/nova-core/nova_core.rs +0
-0
drivers/gpu/nova-core/nova_core.rs line 5
Quoted 6 lines
drivers/gpu/nova-core/nova_core.rs +1
-2@@ -2,8 +2,7 @@//! Nova Core GPU Driver -use kernel::{error::Error , pci, prelude::*,InPlaceModule }; -use pin_init::{PinInit , pinned_drop}; +use kernel::{debugfs::Dir , error::Error , pci, prelude::*,InPlaceModule };
Again, please use kernel vertical style.
drivers/gpu/nova-core/nova_core.rs line 29
Quoted 12 lines
drivers/gpu/nova-core/nova_core.rs +3
-1#[macro_use] mod bitfield;@@ -24,17 +23,24 @@pub (crate )const MODULE_NAME : &kernel::str::CStr = <LocalModule as kernel::ModuleMetadata >::NAME ; -#[pin_data( PinnedDrop )] +#[pin_data] struct NovaCoreModule { +// Note: field order matters for drop order. The driver must be dropped before +// the debugfs directory so that device subdirectories are removed first.
This is not correct, subdirectories have a reference count of their parent directory. Hence, the drop order does not matter.
drivers/gpu/nova-core/nova_core.rs line 39
Quoted 10 lines
drivers/gpu/nova-core/nova_core.rs +3
-0#[pin] _driver: kernel::driver::Registration<pci::Adapter<driver::NovaCore>>, + _debugfs_root: Dir, }impl InPlaceModule for NovaCoreModule {fn init (module: &'static kernel::ThisModule ) ->impl PinInit <Self ,Error > {pr_info! ("NovaCore GPU driver loaded\n" ); + +let debugfs_root =Dir ::new (kernel::c_str!("nova_core" ));
I'd just inline this statement below.
Quoted 9 lines
drivers/gpu/nova-core/nova_core.rs +2
-0+try_pin_init! (Self { _driver <- kernel::driver::Registration ::new (MODULE_NAME , module), + _debugfs_root: debugfs_root, }) } }Signature · 1 line
2.52.0
PATCH v3 7/9 gpu: nova-core: implement BinaryWriter for LogBuffer
In reply to Timur TabiFrom: 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.
--- drivers/gpu/nova-core/gsp.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
drivers/gpu/nova-core/gsp.rs +24 -0
@@ -3,6 +3,7 @@mod boot;use kernel::{ + debugfs, device, dma::{CoherentAllocation ,
@@ -117,6 +118,29 @@ 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 > { +// SAFETY: This is a debug log buffer. GSP may write concurrently, so the +// snapshot may contain partially-written entries. This is acceptable for +// debugging purposes - users should be aware logs may be slightly garbled +// if read while GSP is actively logging. +let slice =unsafe { self.0 .as_slice (0 , self.0 .count ()) }?; + + writer.write_slice_file (slice, offset) + } +} + +// 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 {} +impl Gsp {// Creates an in-place initializer for a `Gsp` manager for `pdev`. pub (crate )fn new (pdev: &pci::Device <device::Bound >) ->Result <impl PinInit <Self ,Error >> {
Signature · 1 line
2.52.0
Danilo Krummrich
maintainerRe: [PATCH v3 7/9] gpu: nova-core: implement BinaryWriter for LogBuffer
Quoted 29 lines · Timur Tabi
From: Alexandre Courbot <[email protected]>
LogBufferis the entity we ultimately want to dump through debugfs. Provide a simple implementation ofBinaryWriterfor 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 | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)drivers/gpu/nova-core/gsp.rs +2
-0@@ -3,6 +3,7 @@mod boot;use kernel::{ + debugfs, device, dma::{CoherentAllocation ,@@ -117,6 +118,29 @@ pub(crate) struct Gsp { rmargs: CoherentAllocation<GspArgumentsCached>, } +impl debugfs::BinaryWriter for LogBuffer {
drivers/gpu/nova-core/gsp.rs pub(crate) struct Gsp { · line 131
Quoted 10 lines
drivers/gpu/nova-core/gsp.rs +10
-0+fn write_to_slice ( + &self, + writer: &mut kernel::uaccess::UserSliceWriter , + offset: &mut kernel::fs::file::Offset , + ) ->Result <usize > { +// SAFETY: This is a debug log buffer. GSP may write concurrently, so the +// snapshot may contain partially-written entries. This is acceptable for +// debugging purposes - users should be aware logs may be slightly garbled +// if read while GSP is actively logging. +let slice =unsafe { self.0 .as_slice (0 , self.0 .count ()) }?;
Unfortunately, it's still undefined behavior to create a slice when the backing buffer can change unexpectedly.
You can use UserSliceWriter::write() instead or add an unsafe method to UserSliceWriter that takes a raw pointer, length and offset.
Given the context of a debug log buffer, I suggest the former.
Quoted 16 lines
drivers/gpu/nova-core/gsp.rs +11
-0+ writer.write_slice_file (slice, offset) + } +} + +// 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 {} +impl Gsp {// Creates an in-place initializer for a `Gsp` manager for `pdev`. pub (crate )fn new (pdev: &pci::Device <device::Bound >) ->Result <impl PinInit <Self ,Error >> {Signature · 1 line
2.52.0
Alexandre Courbot
maintainerRe: [PATCH v3 7/9] gpu: nova-core: implement BinaryWriter for LogBuffer
In reply to Timur TabiQuoted 41 lines · Timur Tabi
From: Alexandre Courbot <[email protected]>
LogBufferis the entity we ultimately want to dump through debugfs. Provide a simple implementation ofBinaryWriterfor 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 | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)drivers/gpu/nova-core/gsp.rs +14
-0@@ -3,6 +3,7 @@mod boot;use kernel::{ + debugfs, device, dma::{CoherentAllocation ,@@ -117,6 +118,29 @@ 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 > { +// SAFETY: This is a debug log buffer. GSP may write concurrently, so the +// snapshot may contain partially-written entries. This is acceptable for +// debugging purposes - users should be aware logs may be slightly garbled +// if read while GSP is actively logging. +let slice =unsafe { self.0 .as_slice (0 , self.0 .count ()) }?; + + writer.write_slice_file (slice, offset)
drivers/gpu/nova-core/gsp.rs pub(crate) struct Gsp { · line 143
Quoted 10 lines
drivers/gpu/nova-core/gsp.rs +10
-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 {} +
Did I actually write all this? I don't have any recollection of submitting such a patch (and the safety comments don't look like my style).
PATCH v3 8/9 gpu: nova-core: use pin projection in method boot()
In reply to Timur TabiStruct 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.
--- drivers/gpu/nova-core/gsp/boot.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/gpu/nova-core/gsp/boot.rs +8 -7
@@ -162,12 +162,13 @@ 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 - .send_command (bar, commands::SetSystemInfo ::new (pdev))?; - self.cmdq.send_command (bar, commands::SetRegistry ::new ())?; +let this = self.as_mut ().project (); + + this.cmdq.send_command (bar, commands::SetSystemInfo ::new (pdev))?; + 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),
@@ -234,13 +235,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)?;dev_info! ( pdev.as_ref (),"GPU name: {}\n" ,
Signature · 1 line
2.52.0
PATCH v3 9/9 gpu: nova-core: create GSP-RM logging buffers debugfs entries
In reply to Timur TabiCreate 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.
--- drivers/gpu/nova-core/gsp.rs | 42 +++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 8 deletions(-)
drivers/gpu/nova-core/gsp.rs +34 -8
@@ -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.
@@ -143,7 +151,9 @@ unsafe impl Sync for LogBuffer {}impl Gsp {// Creates an in-place initializer for a `Gsp` manager for `pdev`. -pub (crate )fn new (pdev: &pci::Device <device::Bound >) ->Result <impl PinInit <Self ,Error >> { +pub (crate )fn new <'a>( + pdev: &'a pci::Device <device::Bound >, + ) ->Result <impl PinInit <Self ,Error > + 'a> {let dev = pdev.as_ref ();let libos =CoherentAllocation ::<LibosMemoryRegionInitArgument >::alloc_coherent ( dev,
@@ -173,11 +183,27 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> Result<impl PinInit<Selfdma_write! (rmargs[0 ] = fw::GspArgumentsCached ::new (&cmdq))?;dma_write! (libos[3 ] =LibosMemoryRegionInitArgument ::new ("RMARGS" , &rmargs))?; -Ok (try_pin_init! (Self { - libos, +let log_buffers =LogBuffers { loginit, logintr, logrm, + }; + +// Look up nova_core debugfs directory - only create entries if it exists. +// If nova_core doesn't exist, debugfs_dir will be empty and file creation +// becomes a no-op (data is still stored, just not exposed via debugfs). +let debugfs_dir = debugfs::Dir ::lookup (c_str! ("nova_core" ),None ) + .unwrap_or_else (debugfs::Dir ::empty); + +let logs = debugfs_dir.scope (log_buffers, pdev.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); + }); + +Ok (try_pin_init! (Self { + libos, + logs <- logs, rmargs, cmdq, }))
Signature · 1 line
2.52.0
kernel test robot
build botRe: [PATCH v3 9/9] gpu: nova-core: create GSP-RM logging buffers debugfs entries
kernel test robot noticed the following build errors:
Hi Timur,
kernel test robot noticed the following build errors:
[auto build test ERROR on ea1013c1539270e372fc99854bc6e4d94eaeff66]
url: https://github.com/intel-lab-lkp/linux/commits/Timur-Tabi/rust-pci-add-PCI-device-name-method/20251218-121537 base: ea1013c1539270e372fc99854bc6e4d94eaeff66 patch link: https://lore.kernel.org/r/20251218013910.459045-10-ttabi%40nvidia.com patch subject: [PATCH v3 9/9] gpu: nova-core: create GSP-RM logging buffers debugfs entries config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20251221/[email protected]/config) compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261) rustc: rustc 1.88.0 (6b00bc388 2025-06-23) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251221/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
PATH=/opt/cross/clang-20/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin INFO PATH=/opt/cross/rustc-1.88.0-bindgen-0.72.1/cargo/bin:/opt/cross/clang-20/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin /usr/bin/timeout -k 100 12h /usr/bin/make KCFLAGS= -fno-crash-diagnostics -Wno-error=return-type -Wreturn-type -funsigned-char -Wundef W=1 --keep-going LLVM=1 -j32 -C source O=/kbuild/obj/consumer/x86_64-rhel-9.4-rust ARCH=x86_64 SHELL=/bin/bash rustfmtcheck make: Entering directory '/kbuild/src/consumer' make[1]: Entering directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust' Diff in drivers/gpu/nova-core/gsp/boot.rs:164:
let this = self.as_mut().project();
- this.cmdq.send_command(bar, commands::SetSystemInfo::new(pdev))?;
+ this.cmdq
+ .send_command(bar, commands::SetSystemInfo::new(pdev))?;
this.cmdq.send_command(bar, commands::SetRegistry::new())?;
gsp_falcon.reset(bar)?; Diff in drivers/gpu/nova-core/gsp/boot.rs:164:
let this = self.as_mut().project();
- this.cmdq.send_command(bar, commands::SetSystemInfo::new(pdev))?;
+ this.cmdq
+ .send_command(bar, commands::SetSystemInfo::new(pdev))?;
this.cmdq.send_command(bar, commands::SetRegistry::new())?;
gsp_falcon.reset(bar)?;
>> Diff in drivers/gpu/nova-core/gsp.rs:192:
// Look up nova_core debugfs directory - only create entries if it exists.
// If nova_core doesn't exist, debugfs_dir will be empty and file creation
// becomes a no-op (data is still stored, just not exposed via debugfs).
- let debugfs_dir = debugfs::Dir::lookup(c_str!("nova_core"), None)
- .unwrap_or_else(debugfs::Dir::empty);
+ let debugfs_dir =
+ debugfs::Dir::lookup(c_str!("nova_core"), None).unwrap_or_else(debugfs::Dir::empty);
let logs = debugfs_dir.scope(log_buffers, pdev.name(), |logs, dir| {
dir.read_binary_file(c_str!("loginit"), &logs.loginit); Diff in drivers/gpu/nova-core/gsp/boot.rs:164:
let this = self.as_mut().project();
- this.cmdq.send_command(bar, commands::SetSystemInfo::new(pdev))?;
+ this.cmdq
+ .send_command(bar, commands::SetSystemInfo::new(pdev))?;
this.cmdq.send_command(bar, commands::SetRegistry::new())?;
gsp_falcon.reset(bar)?;
>> Diff in drivers/gpu/nova-core/gsp.rs:192:
// Look up nova_core debugfs directory - only create entries if it exists.
// If nova_core doesn't exist, debugfs_dir will be empty and file creation
// becomes a no-op (data is still stored, just not exposed via debugfs).
- let debugfs_dir = debugfs::Dir::lookup(c_str!("nova_core"), None)
- .unwrap_or_else(debugfs::Dir::empty);
+ let debugfs_dir =
+ debugfs::Dir::lookup(c_str!("nova_core"), None).unwrap_or_else(debugfs::Dir::empty);
let logs = debugfs_dir.scope(log_buffers, pdev.name(), |logs, dir| {
dir.read_binary_file(c_str!("loginit"), &logs.loginit); make[1]: Leaving directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust' make[2]: *** [Makefile:1871: rustfmt] Error 123
make[2]: Target 'rustfmtcheck' not remade because of errors. make[1]: *** [Makefile:248: __sub-make] Error 2
make[1]: Target 'rustfmtcheck' not remade because of errors.
make: *** [Makefile:248: __sub-make] Error 2
make: Target 'rustfmtcheck' not remade because of errors.
make: Leaving directory '/kbuild/src/consumer'
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Alice Ryhl
maintainerQuoted 43 lines · Timur Tabi
GSP-RM writes its printf message to "logging buffers", which are blocks memory allocated by the driver. The messages are encoded, so exposing the buffers as debugfs entries allows the buffers to be extracted and decoded by a special application.
When the driver loads, a
/sys/kernel/debug/nova_coreroot entry is created. To do this, the normalmodule_pci_driver! macro call is replaced with an explicit initialization function, as this allows that debugfs entry to be created once for all GPUs.Then in each GPU's initialization, a subdirectory based on the PCI BDF name is created, and the logging buffer entries are created under that.
Note: the debugfs entry has a file size of 0, because debugfs defaults a 0 size and the Rust abstractions do not adjust it for the same of the object. Nouveau makes this adjustment manually in the driver.
Based on Linus' repository, not drm-rust-next.
Summary of changes:
1. Replace
module_pci_driver! with explicit init function.
2. Creates root, per-gpu, and individual buffer debugfs entries.
3. Adds apci::name()method to generate a PCI device name string.
4. Adds alookup()method to debugfs, so that the root debugfs entry doesn't need to be a global variable like it is in Nouveau.
5. Makes some updates to debugfs core code.Alexandre Courbot (1):
gpu: nova-core: implement BinaryWriter for LogBufferTimur Tabi (8):
rust: pci: add PCI device name method
rust: debugfs: add lookup contructor
rust: debugfs: addDir::empty()for no-op directory handle
rust: debugfs: fixDir::scope()to not borrow self for returned
lifetime
gpu: nova-core: Replacemodule_pci_driver! with explicit module init
gpu: nova-core: create debugfs root when driver loads
gpu: nova-core: use pin projection in methodboot()
gpu: nova-core: create GSP-RM logging buffers debugfs entries
Quoted 7 lines
drivers/gpu/nova-core/gsp.rs | 66 +++++++++++++++--- drivers/gpu/nova-core/gsp/boot.rs | 15 ++-- drivers/gpu/nova-core/nova_core.rs | 29 +++++++- rust/kernel/debugfs.rs | 108 ++++++++++++++++++++++------- rust/kernel/debugfs/entry.rs | 49 ++++++++++++- rust/kernel/pci.rs | 37 ++++++++++ 6 files changed, 259 insertions(+), 45 deletions(-)
Adding Matthew as this touches debugfs.
Alice
Danilo Krummrich
maintainerQuoted 8 lines · Timur Tabi
Summary of changes:
1. Replace
module_pci_driver! with explicit init function.
2. Creates root, per-gpu, and individual buffer debugfs entries.
3. Adds apci::name()method to generate a PCI device name string.
4. Adds alookup()method to debugfs, so that the root debugfs entry doesn't need to be a global variable like it is in Nouveau.
5. Makes some updates to debugfs core code.
This is nice, but given that this is a v3, where is the changelog?