[PATCH v8 0/6] rust: DebugFS Bindings

[PATCH v8 0/6] rust: DebugFS Bindings

[Matthew Maurer]

This series provides safe DebugFS bindings for Rust, with a sample
module using them.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
Changes in v8:
- Switched from casts to `core::from_{ref, mut}` in type change
- Link to v7: https://lore.kernel.org/r/20250624-debugfs-rust-v7-0-9c8835a7a20f@google.com

Changes in v7:
- Rewrote `entry::Entry` -> `Entry`
- Use `c_int` and `c_void` from kernel prelude rather than core
- Removed unnecessary `display_open` cast
- Switched from `Deref` + an explicit box to `ForeignOwnable` for
  attaching owned data.
- Made `&'static` and `&'static mut` implement `ForeignOwnable`
- Swapped "driver" to "module" in sample code
- Link to v6: https://lore.kernel.org/r/20250618-debugfs-rust-v6-0-72cae211b133@google.com

Changes in v6:
- Replaced explicit lifetimes with children keeping their parents alive.
- Added support for attaching owned data.
- Removed recomendation to only keep root handles and handles you want
  to delete around.
- Refactored some code into separate files to improve clarity.
- Link to v5: https://lore.kernel.org/r/20250505-debugfs-rust-v5-0-3e93ce7bb76e@google.com

Changes in v5:
- Made Dir + File wrappers around Entry
- All functions return owning handles. To discard without drop, use
  `forget`. To keep a handle without drop, use `ManuallyDrop`.
- Fixed bugs around `not(CONFIG_DEBUG_FS)`
- Removed unnecessary `addr_of!`
- Link to v4: https://lore.kernel.org/r/20250502-debugfs-rust-v4-0-788a9c6c2e77@google.com

Changes in v4:
- Remove SubDir, replace with type-level constant.
- Add lifetime to Dir to prevent subdirectories and files from outliving
  their parents and triggering an Oops when accessed.
- Split unsafe blocks with two calls into two blocks
- Access `private` field through direct pointer dereference, avoiding
  creation of a reference to it.
- Notably not changed - owning/non-owning handle defaults. The best read
  I had from the thread was to continue with this mode, but I'm willing
  to change if need be.
- Comment changes
  - More comment markdown
  - Remove scopes from examples
  - Put `as_ptr` properties into a `# Guarantees` section.
- Link to v3: https://lore.kernel.org/r/20250501-debugfs-rust-v3-0-850869fab672@google.com

Changes in v3:
- Split `Dir` into `Dir`/`SubDir`/`File` to improve API.
- Add "." to end of all comments.
- Convert INVARIANT to # Invariants on types.
- Add backticks everywhere I found variables/types in my comments.
- Promoted invariant comment to doc comment.
- Extended sample commenting to make it clearer what is happening.
- Link to v2: https://lore.kernel.org/r/20250430-debugfs-rust-v2-0-2e8d3985812b@google.com

Changes in v2:
- Drop support for builder / pinned bindings in initial series
- Remove `ARef` usage to abstract the dentry nature of handles
- Remove error handling to discourage users from caring whether DebugFS
  is enabled.
- Support CONFIG_DEBUG_FS=n while leaving the API available
- Fixed mistaken decimal/octal mixup
- Doc/comment cleanup
- Link to v1: https://lore.kernel.org/r/20250429-debugfs-rust-v1-0-6b6e7cb7929f@google.com

---
Matthew Maurer (6):
      rust: debugfs: Bind DebugFS directory creation
      rust: debugfs: Bind file creation for long-lived Display
      rust: types: Support &'static and &'static mut ForeignOwnable
      rust: debugfs: Support arbitrary owned backing for File
      rust: debugfs: Support format hooks
      rust: samples: Add debugfs sample

 MAINTAINERS                         |   3 +
 rust/bindings/bindings_helper.h     |   1 +
 rust/kernel/debugfs.rs              | 268 ++++++++++++++++++++++++++++++++++++
 rust/kernel/debugfs/display_file.rs | 155 +++++++++++++++++++++
 rust/kernel/debugfs/entry.rs        |  66 +++++++++
 rust/kernel/lib.rs                  |   1 +
 rust/kernel/types.rs                |  58 ++++++++
 samples/rust/Kconfig                |  11 ++
 samples/rust/Makefile               |   1 +
 samples/rust/rust_debugfs.rs        |  76 ++++++++++
 10 files changed, 640 insertions(+)
---
base-commit: bd243cb5d922cc6343007a3f3918c8d1970541d2
change-id: 20250428-debugfs-rust-3cd5c97eb7d1

Best regards,
-- 
Matthew Maurer <mmaurer@google.com>

[PATCH v8 1/6] rust: debugfs: Bind DebugFS directory creation

[Matthew Maurer]

Support creating DebugFS directories and subdirectories. Similar to the
original DebugFS API, errors are hidden.

Directories persist until their handle and the handles of any child
objects have been dropped.

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 MAINTAINERS                     |  2 +
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/debugfs.rs          | 90 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/debugfs/entry.rs    | 58 ++++++++++++++++++++++++++
 rust/kernel/lib.rs              |  1 +
 5 files changed, 152 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c3f7fbd0d67afe8376ea84bc17d70e9fa4b89bf6..551e3a6a16d9051a2d58a77614c458287da2bdcc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7366,6 +7366,8 @@ F:	include/linux/kobj*
 F:	include/linux/property.h
 F:	include/linux/sysfs.h
 F:	lib/kobj*
+F:	rust/kernel/debugfs.rs
+F:	rust/kernel/debugfs/
 F:	rust/kernel/device.rs
 F:	rust/kernel/device_id.rs
 F:	rust/kernel/devres.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 8cbb660e2ec218021d16e6e0144acf6f4d7cca13..655d88b8e7fe717190ccfb7b8173e84213bf8331 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -45,6 +45,7 @@
 #include <linux/cpufreq.h>
 #include <linux/cpumask.h>
 #include <linux/cred.h>
+#include <linux/debugfs.h>
 #include <linux/device/faux.h>
 #include <linux/dma-mapping.h>
 #include <linux/errname.h>
diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
new file mode 100644
index 0000000000000000000000000000000000000000..2359bd11cd664fb9f7206f8fe38f758dc43d2cb8
--- /dev/null
+++ b/rust/kernel/debugfs.rs
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2025 Google LLC.
+
+//! DebugFS Abstraction
+//!
+//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
+
+#[cfg(CONFIG_DEBUG_FS)]
+use crate::prelude::GFP_KERNEL;
+use crate::str::CStr;
+#[cfg(CONFIG_DEBUG_FS)]
+use crate::sync::Arc;
+
+#[cfg(CONFIG_DEBUG_FS)]
+mod entry;
+#[cfg(CONFIG_DEBUG_FS)]
+use entry::Entry;
+
+/// Owning handle to a DebugFS directory.
+///
+/// This directory will be cleaned up when the handle and all child directory/file handles have
+/// been dropped.
+// We hold a reference to our parent if it exists to prevent the dentry we point to from being
+// cleaned up when our parent is removed.
+pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] Option<Arc<Entry>>);
+
+impl Dir {
+    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
+    #[cfg(CONFIG_DEBUG_FS)]
+    fn create(name: &CStr, parent: Option<&Dir>) -> Self {
+        let parent_ptr = match parent {
+            // If the parent couldn't be allocated, just early-return
+            Some(Dir(None)) => return Self(None),
+            Some(Dir(Some(entry))) => entry.as_ptr(),
+            None => core::ptr::null_mut(),
+        };
+        // SAFETY:
+        // * `name` argument points to a NUL-terminated string that lives across the call, by
+        //   invariants of `&CStr`.
+        // * If `parent` is `None`, `parent_ptr` is null to mean create at root.
+        // * If `parent` is `Some`, `parent_ptr` is a live dentry debugfs pointer.
+        let dir = unsafe { bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr) };
+
+        Self(
+            // If Arc creation fails, the `Entry` will be dropped, so the directory will be cleaned
+            // up.
+            Arc::new(
+                // SAFETY: `debugfs_create_dir` either returns an error code or a legal `dentry`
+                // pointer, and the parent is the same one passed to `debugfs_create_dir`
+                unsafe { Entry::new(dir, parent.and_then(|dir| dir.0.clone())) },
+                GFP_KERNEL,
+            )
+            .ok(),
+        )
+    }
+
+    #[cfg(not(CONFIG_DEBUG_FS))]
+    fn create(_name: &CStr, _parent: Option<&Dir>) -> Self {
+        Self()
+    }
+
+    /// Create a DebugFS subdirectory.
+    ///
+    /// Subdirectory handles cannot outlive the directory handle they were created from.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let parent = Dir::new(c_str!("parent"));
+    /// let child = parent.subdir(c_str!("child"));
+    /// ```
+    pub fn subdir(&self, name: &CStr) -> Self {
+        Dir::create(name, Some(self))
+    }
+
+    /// Create a new directory in DebugFS at the root.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let debugfs = Dir::new(c_str!("parent"));
+    /// ```
+    pub fn new(name: &CStr) -> Self {
+        Dir::create(name, None)
+    }
+}
diff --git a/rust/kernel/debugfs/entry.rs b/rust/kernel/debugfs/entry.rs
new file mode 100644
index 0000000000000000000000000000000000000000..ae0e2c4e1d58e878ebb081a71e4ac0f4a7d99b91
--- /dev/null
+++ b/rust/kernel/debugfs/entry.rs
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2025 Google LLC.
+
+use crate::sync::Arc;
+
+/// Owning handle to a DebugFS entry.
+///
+/// # Invariants
+///
+/// The wrapped pointer will always be `NULL`, an error, or an owned DebugFS `dentry`.
+pub(crate) struct Entry {
+    entry: *mut bindings::dentry,
+    _parent: Option<Arc<Entry>>,
+}
+
+// SAFETY: [`Entry`] is just a `dentry` under the hood, which the API promises can be transferred
+// between threads.
+unsafe impl Send for Entry {}
+
+// SAFETY: All the native functions we re-export use interior locking, and the contents of the
+// struct are opaque to Rust.
+unsafe impl Sync for Entry {}
+
+impl Entry {
+    /// Constructs a new DebugFS [`Entry`] from the underlying pointer.
+    ///
+    /// # Safety
+    ///
+    /// The pointer must either be an error code, `NULL`, or represent a transfer of ownership of a
+    /// live DebugFS directory. If this is a child directory or file, `'a` must be less than the
+    /// lifetime of the parent directory.
+    ///
+    /// If the dentry has a parent, it must be provided as the parent argument.
+    pub(crate) unsafe fn new(entry: *mut bindings::dentry, parent: Option<Arc<Entry>>) -> Self {
+        Self {
+            entry,
+            _parent: parent,
+        }
+    }
+
+    /// Returns the pointer representation of the DebugFS directory.
+    ///
+    /// # Guarantees
+    ///
+    /// Due to the type invariant, the value returned from this function will always be an error
+    /// code, NULL, or a live DebugFS directory.
+    pub(crate) fn as_ptr(&self) -> *mut bindings::dentry {
+        self.entry
+    }
+}
+
+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()) }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c37f4da1866e993be6230bc6715841..43f40b1baa9717ea71e4586042e9e6979491ad37 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -66,6 +66,7 @@
 pub mod cpufreq;
 pub mod cpumask;
 pub mod cred;
+pub mod debugfs;
 pub mod device;
 pub mod device_id;
 pub mod devres;

-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v8 2/6] rust: debugfs: Bind file creation for long-lived Display

[Matthew Maurer]

Allows creation of files for references that live forever and lack
metadata through the `Display` implementation.

The `Display` implementation is used because `seq_printf` needs to route
through `%pA`, which in turn routes through Arguments.

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 rust/kernel/debugfs.rs              | 62 +++++++++++++++++++++++++++++++++++++
 rust/kernel/debugfs/display_file.rs | 61 ++++++++++++++++++++++++++++++++++++
 rust/kernel/debugfs/entry.rs        |  8 +++++
 3 files changed, 131 insertions(+)

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 2359bd11cd664fb9f7206f8fe38f758dc43d2cb8..1f20d85da56fcb89476552feefc9d97fab43cc04 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -10,7 +10,10 @@
 use crate::str::CStr;
 #[cfg(CONFIG_DEBUG_FS)]
 use crate::sync::Arc;
+use core::fmt::Display;
 
+#[cfg(CONFIG_DEBUG_FS)]
+mod display_file;
 #[cfg(CONFIG_DEBUG_FS)]
 mod entry;
 #[cfg(CONFIG_DEBUG_FS)]
@@ -59,6 +62,43 @@ fn create(_name: &CStr, _parent: Option<&Dir>) -> Self {
         Self()
     }
 
+    #[cfg(CONFIG_DEBUG_FS)]
+    fn create_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> File {
+        let Some(parent) = &self.0 else {
+            return File {
+                _entry: Entry::empty(),
+            };
+        };
+        // SAFETY:
+        // * `name` is a NUL-terminated C string, living across the call, by `CStr` invariant.
+        // * `parent` is a live `dentry` since we have a reference to it.
+        // * `vtable` is all stock `seq_file` implementations except for `open`.
+        //   `open`'s only requirement beyond what is provided to all open functions is that the
+        //   inode's data pointer must point to a `T` that will outlive it, which we know because
+        //   we have a static reference.
+        let ptr = unsafe {
+            bindings::debugfs_create_file_full(
+                name.as_char_ptr(),
+                0o444,
+                parent.as_ptr(),
+                data as *const _ as *mut _,
+                core::ptr::null(),
+                &<T as display_file::DisplayFile>::VTABLE,
+            )
+        };
+
+        // SAFETY: `debugfs_create_file_full` either returns an error code or a legal
+        // dentry pointer, so `Entry::new` is safe to call here.
+        let entry = unsafe { Entry::new(ptr, Some(parent.clone())) };
+
+        File { _entry: entry }
+    }
+
+    #[cfg(not(CONFIG_DEBUG_FS))]
+    fn create_file<T: Display + Sized>(&self, _name: &CStr, _data: &'static T) -> File {
+        File {}
+    }
+
     /// Create a DebugFS subdirectory.
     ///
     /// Subdirectory handles cannot outlive the directory handle they were created from.
@@ -75,6 +115,22 @@ pub fn subdir(&self, name: &CStr) -> Self {
         Dir::create(name, Some(self))
     }
 
+    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
+    /// [`Display::fmt`] on the provided reference.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let dir = Dir::new(c_str!("my_debugfs_dir"));
+    /// dir.display_file(c_str!("foo"), &200);
+    /// // "my_debugfs_dir/foo" now contains the number 200.
+    /// ```
+    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> File {
+        self.create_file(name, data)
+    }
+
     /// Create a new directory in DebugFS at the root.
     ///
     /// # Examples
@@ -88,3 +144,9 @@ pub fn new(name: &CStr) -> Self {
         Dir::create(name, None)
     }
 }
+
+/// Handle to a DebugFS file.
+pub struct File {
+    #[cfg(CONFIG_DEBUG_FS)]
+    _entry: Entry,
+}
diff --git a/rust/kernel/debugfs/display_file.rs b/rust/kernel/debugfs/display_file.rs
new file mode 100644
index 0000000000000000000000000000000000000000..e4b551f7092884ad12e18a32cc243d0d037931a6
--- /dev/null
+++ b/rust/kernel/debugfs/display_file.rs
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2025 Google LLC.
+
+use crate::prelude::*;
+use crate::seq_file::SeqFile;
+use crate::seq_print;
+use core::fmt::Display;
+
+/// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`.
+///
+/// # Safety
+///
+/// * `inode`'s private pointer must point to a value of type `T` which will outlive the `inode`
+///   and will not be mutated during this call.
+/// * `file` must point to a live, not-yet-initialized file object.
+pub(crate) unsafe extern "C" fn display_open<T: Display>(
+    inode: *mut bindings::inode,
+    file: *mut bindings::file,
+) -> c_int {
+    // SAFETY:
+    // * `file` is acceptable by caller precondition.
+    // * `print_act` will be called on a `seq_file` with private data set to the third argument,
+    //   so we meet its safety requirements.
+    // * The `data` pointer passed in the third argument is a valid `T` pointer that outlives
+    //   this call by caller preconditions.
+    unsafe { bindings::single_open(file, Some(display_act::<T>), (*inode).i_private) }
+}
+
+/// Prints private data stashed in a seq_file to that seq file.
+///
+/// # Safety
+///
+/// `seq` must point to a live `seq_file` whose private data is a live pointer to a `T` which is
+/// not being mutated.
+pub(crate) unsafe extern "C" fn display_act<T: Display>(
+    seq: *mut bindings::seq_file,
+    _: *mut c_void,
+) -> c_int {
+    // SAFETY: By caller precondition, this pointer is live, points to a value of type `T`, and
+    // is not being mutated.
+    let data = unsafe { &*((*seq).private as *mut T) };
+    // SAFETY: By caller precondition, `seq_file` points to a live `seq_file`, so we can lift
+    // it.
+    let seq_file = unsafe { SeqFile::from_raw(seq) };
+    seq_print!(seq_file, "{}", data);
+    0
+}
+
+// Work around lack of generic const items.
+pub(crate) trait DisplayFile: Display + Sized {
+    const VTABLE: bindings::file_operations = bindings::file_operations {
+        read: Some(bindings::seq_read),
+        llseek: Some(bindings::seq_lseek),
+        release: Some(bindings::single_release),
+        open: Some(display_open::<Self>),
+        // SAFETY: `file_operations` supports zeroes in all fields.
+        ..unsafe { core::mem::zeroed() }
+    };
+}
+
+impl<T: Display + Sized> DisplayFile for T {}
diff --git a/rust/kernel/debugfs/entry.rs b/rust/kernel/debugfs/entry.rs
index ae0e2c4e1d58e878ebb081a71e4ac0f4a7d99b91..2baaf31c326c3071b92b5bc37552907fa1102246 100644
--- a/rust/kernel/debugfs/entry.rs
+++ b/rust/kernel/debugfs/entry.rs
@@ -38,6 +38,14 @@ pub(crate) unsafe fn new(entry: *mut bindings::dentry, parent: Option<Arc<Entry>
         }
     }
 
+    /// Constructs a placeholder DebugFS [`Entry`].
+    pub(crate) fn empty() -> Self {
+        Self {
+            entry: core::ptr::null_mut(),
+            _parent: None,
+        }
+    }
+
     /// Returns the pointer representation of the DebugFS directory.
     ///
     /// # Guarantees

-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v8 3/6] rust: types: Support &'static and &'static mut ForeignOwnable

[Matthew Maurer]

These types live forever and do not require cleanup, so they can
serve as `ForeignOwnable`.

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 rust/kernel/types.rs | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..0a2b15cd05f91c69ef9c678555b845a23c19b82c 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -142,6 +142,64 @@ unsafe fn borrow<'a>(_: *mut Self::PointedTo) -> Self::Borrowed<'a> {}
     unsafe fn borrow_mut<'a>(_: *mut Self::PointedTo) -> Self::BorrowedMut<'a> {}
 }
 
+// SAFETY: The `into_foreign` function derives its pointer from a reference, so it is correctly
+// aligned.
+unsafe impl<T: 'static> ForeignOwnable for &'static T {
+    type PointedTo = T;
+    type Borrowed<'a> = &'a T;
+    type BorrowedMut<'a> = &'a T;
+
+    fn into_foreign(self) -> *mut Self::PointedTo {
+        core::ptr::from_ref(self).cast_mut()
+    }
+
+    unsafe fn from_foreign(foreign: *mut Self::PointedTo) -> Self {
+        // SAFETY: from_foreign has stricter restrictions than borrow
+        unsafe { Self::borrow(foreign) }
+    }
+
+    unsafe fn borrow<'a>(foreign: *mut Self::PointedTo) -> Self::Borrowed<'a> {
+        // SAFETY: We know the original reference lived forever, so we can convert it back
+        unsafe { &*foreign }
+    }
+
+    unsafe fn borrow_mut<'a>(foreign: *mut Self::PointedTo) -> Self::BorrowedMut<'a> {
+        // SAFETY: borrow_mut has stricter restrictions than borrow
+        unsafe { Self::borrow(foreign) }
+    }
+}
+
+// SAFETY: The `into_foreign` function derives its pointer from a reference, so it is correctly
+// aligned.
+unsafe impl<T: 'static> ForeignOwnable for &'static mut T {
+    type PointedTo = T;
+    type Borrowed<'a> = &'a T;
+    type BorrowedMut<'a> = &'a mut T;
+
+    fn into_foreign(self) -> *mut Self::PointedTo {
+        core::ptr::from_mut(self)
+    }
+
+    unsafe fn from_foreign(foreign: *mut Self::PointedTo) -> Self {
+        // SAFETY: from_foreign has stricter restrictions than `borrow_mut`
+        unsafe { Self::borrow_mut(foreign) }
+    }
+
+    unsafe fn borrow<'a>(foreign: *mut Self::PointedTo) -> Self::Borrowed<'a> {
+        // SAFETY: We know the original reference lived forever, and the requirements on the
+        // function indicate that `from_foreign` and `borrow_mut` will not happen concurrently, so
+        // we can do a shared borrow.
+        unsafe { &*foreign }
+    }
+
+    unsafe fn borrow_mut<'a>(foreign: *mut Self::PointedTo) -> Self::BorrowedMut<'a> {
+        // SAFETY: We know the original reference lived forever, and the requirements on the
+        // function indicate that no other borrows will happen concurrently, so we can do a
+        // unique borrow.
+        unsafe { &mut *foreign }
+    }
+}
+
 /// Runs a cleanup function/closure when dropped.
 ///
 /// The [`ScopeGuard::dismiss`] function prevents the cleanup function from running.

-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH v8 3/6] rust: types: Support &'static and &'static mut ForeignOwnable

[Dirk Behme]

On 28/06/2025 01:18, Matthew Maurer wrote:
> These types live forever and do not require cleanup, so they can
> serve as `ForeignOwnable`.
> 
> Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
>  rust/kernel/types.rs | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..0a2b15cd05f91c69ef9c678555b845a23c19b82c 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -142,6 +142,64 @@ unsafe fn borrow<'a>(_: *mut Self::PointedTo) -> Self::Borrowed<'a> {}
>      unsafe fn borrow_mut<'a>(_: *mut Self::PointedTo) -> Self::BorrowedMut<'a> {}
>  }
>  
> +// SAFETY: The `into_foreign` function derives its pointer from a reference, so it is correctly
> +// aligned.
> +unsafe impl<T: 'static> ForeignOwnable for &'static T {
> +    type PointedTo = T;
> +    type Borrowed<'a> = &'a T;
> +    type BorrowedMut<'a> = &'a T;
> +
...
> +// SAFETY: The `into_foreign` function derives its pointer from a reference, so it is correctly
> +// aligned.
> +unsafe impl<T: 'static> ForeignOwnable for &'static mut T {
> +    type PointedTo = T;
> +    type Borrowed<'a> = &'a T;
> +    type BorrowedMut<'a> = &'a mut T;

Just fyi, depending on what hits mainline first, this patch series or

https://lore.kernel.org/rust-for-linux/20250626200054.243480-5-dakr@kernel.org/

the latter might need something like [1].

Dirk

[1]

diff --git a/rust/kernel/debugfs/display_file.rs
b/rust/kernel/debugfs/display_file.rs
index b38675a90e1b..9cea3bd633dc 100644
--- a/rust/kernel/debugfs/display_file.rs
+++ b/rust/kernel/debugfs/display_file.rs
@@ -102,6 +102,7 @@ pub(crate) struct BorrowedAdapter<'a, D:
ForeignOwnable, F> {
 // SAFETY: We delegate to D's implementation of `ForeignOwnable`, so
`into_foreign` produced aligned
 // pointers.
 unsafe impl<D: ForeignOwnable, F> ForeignOwnable for FormatAdapter<D, F> {
+    type Target = D;
     type PointedTo = D::PointedTo;
     type Borrowed<'a> = BorrowedAdapter<'a, D, F>;
     type BorrowedMut<'a> = Self::Borrowed<'a>;
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index ffbd750e1eda..c2d87e077c8f 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -149,6 +149,7 @@ unsafe fn borrow_mut<'a>(_: *mut Self::PointedTo) ->
Self::BorrowedMut<'a> {}
 // SAFETY: The `into_foreign` function derives its pointer from a
reference, so it is correctly
 // aligned.
 unsafe impl<T: 'static> ForeignOwnable for &'static T {
+    type Target = T;
     type PointedTo = T;
     type Borrowed<'a> = &'a T;
     type BorrowedMut<'a> = &'a T;
@@ -176,6 +177,7 @@ unsafe fn borrow_mut<'a>(foreign: *mut
Self::PointedTo) -> Self::BorrowedMut<'a>
 // SAFETY: The `into_foreign` function derives its pointer from a
reference, so it is correctly
 // aligned.
 unsafe impl<T: 'static> ForeignOwnable for &'static mut T {
+    type Target = T;
     type PointedTo = T;
     type Borrowed<'a> = &'a T;
     type BorrowedMut<'a> = &'a mut T;

Re: [PATCH v8 3/6] rust: types: Support &'static and &'static mut ForeignOwnable

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 01:41:44PM +0200, Dirk Behme wrote:
> Just fyi, depending on what hits mainline first, this patch series or
> 
> https://lore.kernel.org/rust-for-linux/20250626200054.243480-5-dakr@kernel.org/

Thanks for pointing this out, but I dropped the linked patch for now.

- Danilo

[PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Matthew Maurer]

This allows `File`s to be backed by `ForeignOwnable` rather than just
`&'static T`. This means that dynamically allocated objects can be
attached to `File`s without needing to take extra steps to create a
pinned reference that's guaranteed to live long enough.

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 rust/kernel/debugfs.rs              | 99 +++++++++++++++++++++++++++++++------
 rust/kernel/debugfs/display_file.rs | 49 +++++++++++-------
 2 files changed, 115 insertions(+), 33 deletions(-)

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 1f20d85da56fcb89476552feefc9d97fab43cc04..929e55ee5629f6888edf29997b9ed77d274e11c8 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -5,11 +5,11 @@
 //!
 //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
 
-#[cfg(CONFIG_DEBUG_FS)]
-use crate::prelude::GFP_KERNEL;
+use crate::prelude::*;
 use crate::str::CStr;
 #[cfg(CONFIG_DEBUG_FS)]
 use crate::sync::Arc;
+use crate::types::ForeignOwnable;
 use core::fmt::Display;
 
 #[cfg(CONFIG_DEBUG_FS)]
@@ -63,40 +63,52 @@ fn create(_name: &CStr, _parent: Option<&Dir>) -> Self {
     }
 
     #[cfg(CONFIG_DEBUG_FS)]
-    fn create_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> File {
+    fn create_file<D: ForeignOwnable + Send + Sync>(&self, name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        let mut file = File {
+            _entry: Entry::empty(),
+            _foreign: ForeignHolder::new(data),
+        };
+
         let Some(parent) = &self.0 else {
-            return File {
-                _entry: Entry::empty(),
-            };
+            return file;
         };
+
         // SAFETY:
         // * `name` is a NUL-terminated C string, living across the call, by `CStr` invariant.
         // * `parent` is a live `dentry` since we have a reference to it.
         // * `vtable` is all stock `seq_file` implementations except for `open`.
         //   `open`'s only requirement beyond what is provided to all open functions is that the
         //   inode's data pointer must point to a `T` that will outlive it, which we know because
-        //   we have a static reference.
+        //   we have an owning `D` in the `File`, and we tear down the file during `Drop`.
         let ptr = unsafe {
             bindings::debugfs_create_file_full(
                 name.as_char_ptr(),
                 0o444,
                 parent.as_ptr(),
-                data as *const _ as *mut _,
+                file._foreign.data,
                 core::ptr::null(),
-                &<T as display_file::DisplayFile>::VTABLE,
+                &<D as display_file::DisplayFile>::VTABLE,
             )
         };
 
         // SAFETY: `debugfs_create_file_full` either returns an error code or a legal
         // dentry pointer, so `Entry::new` is safe to call here.
-        let entry = unsafe { Entry::new(ptr, Some(parent.clone())) };
+        file._entry = unsafe { Entry::new(ptr, Some(parent.clone())) };
 
-        File { _entry: entry }
+        file
     }
 
     #[cfg(not(CONFIG_DEBUG_FS))]
-    fn create_file<T: Display + Sized>(&self, _name: &CStr, _data: &'static T) -> File {
-        File {}
+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
     }
 
     /// Create a DebugFS subdirectory.
@@ -127,7 +139,21 @@ pub fn subdir(&self, name: &CStr) -> Self {
     /// dir.display_file(c_str!("foo"), &200);
     /// // "my_debugfs_dir/foo" now contains the number 200.
     /// ```
-    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> File {
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// # use kernel::prelude::*;
+    /// let val = KBox::new(300, GFP_KERNEL)?;
+    /// let dir = Dir::new(c_str!("my_debugfs_dir"));
+    /// dir.display_file(c_str!("foo"), val);
+    /// // "my_debugfs_dir/foo" now contains the number 300.
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn display_file<D: ForeignOwnable + Send + Sync>(&self, name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
         self.create_file(name, data)
     }
 
@@ -147,6 +173,51 @@ pub fn new(name: &CStr) -> Self {
 
 /// Handle to a DebugFS file.
 pub struct File {
+    // This order is load-bearing for drops - `_entry` must be dropped before `_foreign`
     #[cfg(CONFIG_DEBUG_FS)]
     _entry: Entry,
+    _foreign: ForeignHolder,
+}
+
+struct ForeignHolder {
+    data: *mut c_void,
+    drop_hook: unsafe fn(*mut c_void),
+}
+
+// SAFETY: We only construct `ForeignHolder` using a pointer from a `ForeignOwnable` which
+// is also `Sync`.
+unsafe impl Sync for ForeignHolder {}
+// SAFETY: We only construct `ForeignHolder` using a pointer from a `ForeignOwnable` which
+// is also `Send`.
+unsafe impl Send for ForeignHolder {}
+
+/// Helper function to drop a `D`-typed foreign ownable from its foreign representation, useful for
+/// cases where you want the type erased.
+/// # Safety
+/// * The foreign pointer passed in must have come from `D`'s `ForeignOwnable::into_foreign`
+/// * There must be no outstanding `ForeignOwnable::borrow{,mut}`
+/// * The pointer must not have been `ForeignOwnable::from_foreign`'d
+unsafe fn drop_helper<D: ForeignOwnable>(foreign: *mut c_void) {
+    // SAFETY: By safetydocs, we meet the requirements for `from_foreign`
+    drop(unsafe { D::from_foreign(foreign as _) })
+}
+
+impl ForeignHolder {
+    fn new<D: ForeignOwnable>(data: D) -> Self {
+        Self {
+            data: data.into_foreign() as _,
+            drop_hook: drop_helper::<D>,
+        }
+    }
+}
+
+impl Drop for ForeignHolder {
+    fn drop(&mut self) {
+        // SAFETY: `drop_hook` corresponds to the original `ForeignOwnable` instance's `drop`.
+        // This is only used in the case of `File`, so the only place borrows occur is through the
+        // DebugFS file owned by `_entry`. Since `_entry` occurs earlier in the struct, it will be
+        // dropped first, so no borrows will be ongoing. We know no `from_foreign` has occurred
+        // because this pointer is not exposed anywhere that is called.
+        unsafe { (self.drop_hook)(self.data) }
+    }
 }
diff --git a/rust/kernel/debugfs/display_file.rs b/rust/kernel/debugfs/display_file.rs
index e4b551f7092884ad12e18a32cc243d0d037931a6..0c2dd756fa866425d1b7771beceaa2fb43bf11e5 100644
--- a/rust/kernel/debugfs/display_file.rs
+++ b/rust/kernel/debugfs/display_file.rs
@@ -4,42 +4,48 @@
 use crate::prelude::*;
 use crate::seq_file::SeqFile;
 use crate::seq_print;
+use crate::types::ForeignOwnable;
 use core::fmt::Display;
 
 /// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`.
 ///
 /// # Safety
 ///
-/// * `inode`'s private pointer must point to a value of type `T` which will outlive the `inode`
-///   and will not be mutated during this call.
+/// * `inode`'s private pointer must be the foreign representation of `D`, and no mutable borrows
+///   are outstanding.
 /// * `file` must point to a live, not-yet-initialized file object.
-pub(crate) unsafe extern "C" fn display_open<T: Display>(
+pub(crate) unsafe extern "C" fn display_open<D: ForeignOwnable + Sync>(
     inode: *mut bindings::inode,
     file: *mut bindings::file,
-) -> c_int {
+) -> c_int
+where
+    for<'a> D::Borrowed<'a>: Display,
+{
     // SAFETY:
     // * `file` is acceptable by caller precondition.
     // * `print_act` will be called on a `seq_file` with private data set to the third argument,
     //   so we meet its safety requirements.
-    // * The `data` pointer passed in the third argument is a valid `T` pointer that outlives
-    //   this call by caller preconditions.
-    unsafe { bindings::single_open(file, Some(display_act::<T>), (*inode).i_private) }
+    // * The `data` pointer passed in the third argument is valid by caller preconditions.
+    unsafe { bindings::single_open(file, Some(display_act::<D>), (*inode).i_private) }
 }
 
 /// Prints private data stashed in a seq_file to that seq file.
 ///
 /// # Safety
 ///
-/// `seq` must point to a live `seq_file` whose private data is a live pointer to a `T` which is
-/// not being mutated.
-pub(crate) unsafe extern "C" fn display_act<T: Display>(
+/// `seq` must point to a live `seq_file` whose private data is the foreign representation of `D`,
+/// and no mutable borrows are outsstanding.
+pub(crate) unsafe extern "C" fn display_act<D: ForeignOwnable + Sync>(
     seq: *mut bindings::seq_file,
     _: *mut c_void,
-) -> c_int {
-    // SAFETY: By caller precondition, this pointer is live, points to a value of type `T`, and
-    // is not being mutated.
-    let data = unsafe { &*((*seq).private as *mut T) };
-    // SAFETY: By caller precondition, `seq_file` points to a live `seq_file`, so we can lift
+) -> c_int
+where
+    for<'a> D::Borrowed<'a>: Display,
+{
+    // SAFETY: By caller precondition, this pointer is live, has the right type, and has no mutable
+    // borrows outstanding.
+    let data = unsafe { D::borrow((*seq).private as _) };
+    // SAFETY: By caller precondition, `seq` points to a live `seq_file`, so we can lift
     // it.
     let seq_file = unsafe { SeqFile::from_raw(seq) };
     seq_print!(seq_file, "{}", data);
@@ -47,15 +53,20 @@
 }
 
 // Work around lack of generic const items.
-pub(crate) trait DisplayFile: Display + Sized {
+pub(crate) trait DisplayFile {
+    const VTABLE: bindings::file_operations;
+}
+
+impl<D: ForeignOwnable + Sync> DisplayFile for D
+where
+    for<'a> D::Borrowed<'a>: Display,
+{
     const VTABLE: bindings::file_operations = bindings::file_operations {
         read: Some(bindings::seq_read),
         llseek: Some(bindings::seq_lseek),
         release: Some(bindings::single_release),
-        open: Some(display_open::<Self>),
+        open: Some(display_open::<D>),
         // SAFETY: `file_operations` supports zeroes in all fields.
         ..unsafe { core::mem::zeroed() }
     };
 }
-
-impl<T: Display + Sized> DisplayFile for T {}

-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On 6/28/25 1:18 AM, Matthew Maurer wrote:
> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> +    where
> +        for<'a> D::Borrowed<'a>: Display,
> +    {
> +        File {
> +            _foreign: ForeignHolder::new(data),
> +        }
>       }

What's the motivation for the ForeignHolder abstraction? Why not just make it
File<D> and store data directly?

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Matthew Maurer]

On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > +    where
> > +        for<'a> D::Borrowed<'a>: Display,
> > +    {
> > +        File {
> > +            _foreign: ForeignHolder::new(data),
> > +        }
> >       }
>
> What's the motivation for the ForeignHolder abstraction? Why not just make it
> File<D> and store data directly?

1. A `File<D>` can't be held in collection data structures as easily
unless all your files contain the *same* backing type.
2. None of the APIs or potential APIs for `File` care about which type
it's wrapping, nor are they supposed to. If nothing you can do with a
`File` is different depending on the backing type, making it
polymorphic is just needlessly confusing.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Matthew Maurer]

On Mon, Jun 30, 2025 at 10:34 AM Matthew Maurer <mmaurer@google.com> wrote:
>
> On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > > +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > > +    where
> > > +        for<'a> D::Borrowed<'a>: Display,
> > > +    {
> > > +        File {
> > > +            _foreign: ForeignHolder::new(data),
> > > +        }
> > >       }
> >
> > What's the motivation for the ForeignHolder abstraction? Why not just make it
> > File<D> and store data directly?
>
> 1. A `File<D>` can't be held in collection data structures as easily
> unless all your files contain the *same* backing type.
> 2. None of the APIs or potential APIs for `File` care about which type
> it's wrapping, nor are they supposed to. If nothing you can do with a
> `File` is different depending on the backing type, making it
> polymorphic is just needlessly confusing.

If it helps, effectively what I'm doing is adding a `dyn*
ForeignOwnable`, but `dyn*` isn't available, so writing this without
`ForeignHolder` would involve an unnecessary `Box` to hide the `dyn`.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On 6/30/25 7:34 PM, Matthew Maurer wrote:
> On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On 6/28/25 1:18 AM, Matthew Maurer wrote:
>>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
>>> +    where
>>> +        for<'a> D::Borrowed<'a>: Display,
>>> +    {
>>> +        File {
>>> +            _foreign: ForeignHolder::new(data),
>>> +        }
>>>        }
>>
>> What's the motivation for the ForeignHolder abstraction? Why not just make it
>> File<D> and store data directly?
> 
> 1. A `File<D>` can't be held in collection data structures as easily
> unless all your files contain the *same* backing type.

That sounds reasonable.

> 2. None of the APIs or potential APIs for `File` care about which type
> it's wrapping, nor are they supposed to. If nothing you can do with a
> `File` is different depending on the backing type, making it
> polymorphic is just needlessly confusing.

What if I want to access file.data() and do something with the data? Then I'd
necessarily need to put my data in an Arc and reference count it to still be
able to access it.

That doesn't seem like a reasonable requirement to be able to access data
exposed via debugfs.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Matthew Maurer]

On Mon, Jun 30, 2025 at 10:39 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >>
> >> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> >>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> >>> +    where
> >>> +        for<'a> D::Borrowed<'a>: Display,
> >>> +    {
> >>> +        File {
> >>> +            _foreign: ForeignHolder::new(data),
> >>> +        }
> >>>        }
> >>
> >> What's the motivation for the ForeignHolder abstraction? Why not just make it
> >> File<D> and store data directly?
> >
> > 1. A `File<D>` can't be held in collection data structures as easily
> > unless all your files contain the *same* backing type.
>
> That sounds reasonable.
>
> > 2. None of the APIs or potential APIs for `File` care about which type
> > it's wrapping, nor are they supposed to. If nothing you can do with a
> > `File` is different depending on the backing type, making it
> > polymorphic is just needlessly confusing.
>
> What if I want to access file.data() and do something with the data? Then I'd
> necessarily need to put my data in an Arc and reference count it to still be
> able to access it.
>
> That doesn't seem like a reasonable requirement to be able to access data
> exposed via debugfs.

`pub fn data(&self) -> D` would go against my understanding of Greg's
request for DebugFS files to not really support anything other than
delete. I was even considering making `D` not be retained in the
disabled debugfs case, but left it in for now for so that the
lifecycles wouldn't change.

If you want a `.data()` function, I can add it in, but I don't think
it'll improve flexibility in most cases. If you want to do something
with the data and it's not in an `Arc` / behind a handle of some kind,
you'll need something providing threadsafe interior mutability in the
data structure. If that's a lock, then I have a hard time believing
that `Arc<Mutex<T>>`(or if it's a global, a `&'static Mutex<T>`, which
is why I added that in the stack) is so much more expensive than
`Box<Mutex<T>>` that it's worth a more complex API. If it's an atomic,
e.g. `Arc<AtomicU8>`, then I can see the benefit to having
`Box<AtomicU8>` over that, but it still seems so slim that I think the
simpler "`File` is just a handle to how long the file stays alive, it
doesn't let you do anything else" API makes sense.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On Mon, Jun 30, 2025 at 10:49:51AM -0700, Matthew Maurer wrote:
> On Mon, Jun 30, 2025 at 10:39 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >>
> > >> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > >>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > >>> +    where
> > >>> +        for<'a> D::Borrowed<'a>: Display,
> > >>> +    {
> > >>> +        File {
> > >>> +            _foreign: ForeignHolder::new(data),
> > >>> +        }
> > >>>        }
> > >>
> > >> What's the motivation for the ForeignHolder abstraction? Why not just make it
> > >> File<D> and store data directly?
> > >
> > > 1. A `File<D>` can't be held in collection data structures as easily
> > > unless all your files contain the *same* backing type.
> >
> > That sounds reasonable.
> >
> > > 2. None of the APIs or potential APIs for `File` care about which type
> > > it's wrapping, nor are they supposed to. If nothing you can do with a
> > > `File` is different depending on the backing type, making it
> > > polymorphic is just needlessly confusing.
> >
> > What if I want to access file.data() and do something with the data? Then I'd
> > necessarily need to put my data in an Arc and reference count it to still be
> > able to access it.
> >
> > That doesn't seem like a reasonable requirement to be able to access data
> > exposed via debugfs.
> 
> `pub fn data(&self) -> D` would go against my understanding of Greg's
> request for DebugFS files to not really support anything other than
> delete. I was even considering making `D` not be retained in the
> disabled debugfs case, but left it in for now for so that the
> lifecycles wouldn't change.

Well, that's because the C side does not have anything else. But the C side has
no type system that deals with ownership:

In C you just stuff a pointer of your private data into debugfs_create_file()
without any implication of ownership. debugfs has a pointer, the driver has a
pointer. The question of the ownership semantics is not answered by the API, but
by the implementation of the driver.

The Rust API is different, and it's even implied by the name of the trait you
expect the data to implement: ForeignOwnable.

The File *owns* the data, either entirely or a reference count of the data.

If the *only* way to access the data the File now owns is by making it reference
counted, it:

  1) Is additional overhead imposed on users.

  2) It has implications on the ownership design of your driver. Once something
     is reference counted, you loose the guarantee the something can't out-live
     some event.

I don't want that people have to stuff their data structures into Arc (i.e.
reference count them), even though that's not necessary. It makes it easy to
make mistakes. Things like:

	let foo = bar.clone();

can easily be missed in reviews, whereas some contributor falsely changing a
KBox to an Arc is much harder to miss.

> If you want a `.data()` function, I can add it in,

I think it could even be an implementation of Deref.

> but I don't think
> it'll improve flexibility in most cases. If you want to do something
> with the data and it's not in an `Arc` / behind a handle of some kind,
> you'll need something providing threadsafe interior mutability in the
> data structure. If that's a lock, then I have a hard time believing
> that `Arc<Mutex<T>>`(or if it's a global, a `&'static Mutex<T>`, which
> is why I added that in the stack) is so much more expensive than
> `Box<Mutex<T>>` that it's worth a more complex API. If it's an atomic,
> e.g. `Arc<AtomicU8>`, then I can see the benefit to having
> `Box<AtomicU8>` over that, but it still seems so slim that I think the
> simpler "`File` is just a handle to how long the file stays alive, it
> doesn't let you do anything else" API makes sense.

I don't really see what is complicated about File<T> -- it's a File and it owns
data of type T that is exposed via debugfs. Seems pretty straight forward to me.

Maybe the performance cost is not a huge argument here, but maintainability in
terms of clarity about ownership and lifetime of an object as explained above
clearly is.

- Danilo

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Greg Kroah-Hartman]

On Mon, Jun 30, 2025 at 08:16:55PM +0200, Danilo Krummrich wrote:
> On Mon, Jun 30, 2025 at 10:49:51AM -0700, Matthew Maurer wrote:
> > On Mon, Jun 30, 2025 at 10:39 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > > > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >>
> > > >> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > > >>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > > >>> +    where
> > > >>> +        for<'a> D::Borrowed<'a>: Display,
> > > >>> +    {
> > > >>> +        File {
> > > >>> +            _foreign: ForeignHolder::new(data),
> > > >>> +        }
> > > >>>        }
> > > >>
> > > >> What's the motivation for the ForeignHolder abstraction? Why not just make it
> > > >> File<D> and store data directly?
> > > >
> > > > 1. A `File<D>` can't be held in collection data structures as easily
> > > > unless all your files contain the *same* backing type.
> > >
> > > That sounds reasonable.
> > >
> > > > 2. None of the APIs or potential APIs for `File` care about which type
> > > > it's wrapping, nor are they supposed to. If nothing you can do with a
> > > > `File` is different depending on the backing type, making it
> > > > polymorphic is just needlessly confusing.
> > >
> > > What if I want to access file.data() and do something with the data? Then I'd
> > > necessarily need to put my data in an Arc and reference count it to still be
> > > able to access it.
> > >
> > > That doesn't seem like a reasonable requirement to be able to access data
> > > exposed via debugfs.
> > 
> > `pub fn data(&self) -> D` would go against my understanding of Greg's
> > request for DebugFS files to not really support anything other than
> > delete. I was even considering making `D` not be retained in the
> > disabled debugfs case, but left it in for now for so that the
> > lifecycles wouldn't change.
> 
> Well, that's because the C side does not have anything else. But the C side has
> no type system that deals with ownership:
> 
> In C you just stuff a pointer of your private data into debugfs_create_file()
> without any implication of ownership. debugfs has a pointer, the driver has a
> pointer. The question of the ownership semantics is not answered by the API, but
> by the implementation of the driver.
> 
> The Rust API is different, and it's even implied by the name of the trait you
> expect the data to implement: ForeignOwnable.
> 
> The File *owns* the data, either entirely or a reference count of the data.
> 
> If the *only* way to access the data the File now owns is by making it reference
> counted, it:
> 
>   1) Is additional overhead imposed on users.
> 
>   2) It has implications on the ownership design of your driver. Once something
>      is reference counted, you loose the guarantee the something can't out-live
>      some event.
> 
> I don't want that people have to stuff their data structures into Arc (i.e.
> reference count them), even though that's not necessary. It makes it easy to
> make mistakes. Things like:
> 
> 	let foo = bar.clone();
> 
> can easily be missed in reviews, whereas some contributor falsely changing a
> KBox to an Arc is much harder to miss.
> 
> > If you want a `.data()` function, I can add it in,
> 
> I think it could even be an implementation of Deref.
> 
> > but I don't think
> > it'll improve flexibility in most cases. If you want to do something
> > with the data and it's not in an `Arc` / behind a handle of some kind,
> > you'll need something providing threadsafe interior mutability in the
> > data structure. If that's a lock, then I have a hard time believing
> > that `Arc<Mutex<T>>`(or if it's a global, a `&'static Mutex<T>`, which
> > is why I added that in the stack) is so much more expensive than
> > `Box<Mutex<T>>` that it's worth a more complex API. If it's an atomic,
> > e.g. `Arc<AtomicU8>`, then I can see the benefit to having
> > `Box<AtomicU8>` over that, but it still seems so slim that I think the
> > simpler "`File` is just a handle to how long the file stays alive, it
> > doesn't let you do anything else" API makes sense.
> 
> I don't really see what is complicated about File<T> -- it's a File and it owns
> data of type T that is exposed via debugfs. Seems pretty straight forward to me.
> 
> Maybe the performance cost is not a huge argument here, but maintainability in
> terms of clarity about ownership and lifetime of an object as explained above
> clearly is.

I'm agreeing here.  As one of the primary users of this api is going to
be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make
an example driver to emulate that file with just a local structure, but
the reference counting and access logic just didn't seem to work out
properly.  Odds are I'm doing something stupid though...

So a file callback IS going to want to have access to the data of type T
that is exposed somehow.

And debugfs is NOT for performance things, but really, how bad could it
be?  :)

thanks,

greg k-h

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 03:58:45PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Jun 30, 2025 at 08:16:55PM +0200, Danilo Krummrich wrote:
> > On Mon, Jun 30, 2025 at 10:49:51AM -0700, Matthew Maurer wrote:
> > > On Mon, Jun 30, 2025 at 10:39 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > >
> > > > On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > > > > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > >>
> > > > >> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > > > >>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > > > >>> +    where
> > > > >>> +        for<'a> D::Borrowed<'a>: Display,
> > > > >>> +    {
> > > > >>> +        File {
> > > > >>> +            _foreign: ForeignHolder::new(data),
> > > > >>> +        }
> > > > >>>        }
> > > > >>
> > > > >> What's the motivation for the ForeignHolder abstraction? Why not just make it
> > > > >> File<D> and store data directly?
> > > > >
> > > > > 1. A `File<D>` can't be held in collection data structures as easily
> > > > > unless all your files contain the *same* backing type.
> > > >
> > > > That sounds reasonable.
> > > >
> > > > > 2. None of the APIs or potential APIs for `File` care about which type
> > > > > it's wrapping, nor are they supposed to. If nothing you can do with a
> > > > > `File` is different depending on the backing type, making it
> > > > > polymorphic is just needlessly confusing.
> > > >
> > > > What if I want to access file.data() and do something with the data? Then I'd
> > > > necessarily need to put my data in an Arc and reference count it to still be
> > > > able to access it.
> > > >
> > > > That doesn't seem like a reasonable requirement to be able to access data
> > > > exposed via debugfs.
> > > 
> > > `pub fn data(&self) -> D` would go against my understanding of Greg's
> > > request for DebugFS files to not really support anything other than
> > > delete. I was even considering making `D` not be retained in the
> > > disabled debugfs case, but left it in for now for so that the
> > > lifecycles wouldn't change.
> > 
> > Well, that's because the C side does not have anything else. But the C side has
> > no type system that deals with ownership:
> > 
> > In C you just stuff a pointer of your private data into debugfs_create_file()
> > without any implication of ownership. debugfs has a pointer, the driver has a
> > pointer. The question of the ownership semantics is not answered by the API, but
> > by the implementation of the driver.
> > 
> > The Rust API is different, and it's even implied by the name of the trait you
> > expect the data to implement: ForeignOwnable.
> > 
> > The File *owns* the data, either entirely or a reference count of the data.
> > 
> > If the *only* way to access the data the File now owns is by making it reference
> > counted, it:
> > 
> >   1) Is additional overhead imposed on users.
> > 
> >   2) It has implications on the ownership design of your driver. Once something
> >      is reference counted, you loose the guarantee the something can't out-live
> >      some event.
> > 
> > I don't want that people have to stuff their data structures into Arc (i.e.
> > reference count them), even though that's not necessary. It makes it easy to
> > make mistakes. Things like:
> > 
> > 	let foo = bar.clone();
> > 
> > can easily be missed in reviews, whereas some contributor falsely changing a
> > KBox to an Arc is much harder to miss.
> > 
> > > If you want a `.data()` function, I can add it in,
> > 
> > I think it could even be an implementation of Deref.
> > 
> > > but I don't think
> > > it'll improve flexibility in most cases. If you want to do something
> > > with the data and it's not in an `Arc` / behind a handle of some kind,
> > > you'll need something providing threadsafe interior mutability in the
> > > data structure. If that's a lock, then I have a hard time believing
> > > that `Arc<Mutex<T>>`(or if it's a global, a `&'static Mutex<T>`, which
> > > is why I added that in the stack) is so much more expensive than
> > > `Box<Mutex<T>>` that it's worth a more complex API. If it's an atomic,
> > > e.g. `Arc<AtomicU8>`, then I can see the benefit to having
> > > `Box<AtomicU8>` over that, but it still seems so slim that I think the
> > > simpler "`File` is just a handle to how long the file stays alive, it
> > > doesn't let you do anything else" API makes sense.
> > 
> > I don't really see what is complicated about File<T> -- it's a File and it owns
> > data of type T that is exposed via debugfs. Seems pretty straight forward to me.
> > 
> > Maybe the performance cost is not a huge argument here, but maintainability in
> > terms of clarity about ownership and lifetime of an object as explained above
> > clearly is.
> 
> I'm agreeing here.  As one of the primary users of this api is going to
> be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make
> an example driver to emulate that file with just a local structure, but
> the reference counting and access logic just didn't seem to work out
> properly.  Odds are I'm doing something stupid though...

I think it technically works, but it imposes semantics on drivers that we
shouldn't do; see the example below.

> So a file callback IS going to want to have access to the data of type T
> that is exposed somehow.

With the current API we would need this:

	struct GPU {
	   fw: Arc<Firmware>,
	   fw_file: debugfs::File,
	}

and then I would initialize it the following way:

	let fw = Arc::new(Firmware::new(), GFP_KERNEL)?;
	let fw_file = dir.create_file("firmware", fw.clone());

	fw.do_something();

This is bad, because now my Firmware instance in GPU needs to be reference
counted, even though it should *never* out-live the GPU instance. This is error
prone.

Instead this should just be:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

and then I would initialize it the following way:

	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
	let file = dir.create_file("firmware", fw);

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

	// Access to fw is prevented by the compiler, since it has been moved
	// into file.

This is much better, since now I have the guarantee that my Firmare instance
can't out-live the GPU instance.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Greg Kroah-Hartman]

On Tue, Jul 01, 2025 at 04:13:28PM +0200, Danilo Krummrich wrote:
> On Tue, Jul 01, 2025 at 03:58:45PM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Jun 30, 2025 at 08:16:55PM +0200, Danilo Krummrich wrote:
> > > On Mon, Jun 30, 2025 at 10:49:51AM -0700, Matthew Maurer wrote:
> > > > On Mon, Jun 30, 2025 at 10:39 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > >
> > > > > On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > > > > > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > >>
> > > > > >> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > > > > >>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > > > > >>> +    where
> > > > > >>> +        for<'a> D::Borrowed<'a>: Display,
> > > > > >>> +    {
> > > > > >>> +        File {
> > > > > >>> +            _foreign: ForeignHolder::new(data),
> > > > > >>> +        }
> > > > > >>>        }
> > > > > >>
> > > > > >> What's the motivation for the ForeignHolder abstraction? Why not just make it
> > > > > >> File<D> and store data directly?
> > > > > >
> > > > > > 1. A `File<D>` can't be held in collection data structures as easily
> > > > > > unless all your files contain the *same* backing type.
> > > > >
> > > > > That sounds reasonable.
> > > > >
> > > > > > 2. None of the APIs or potential APIs for `File` care about which type
> > > > > > it's wrapping, nor are they supposed to. If nothing you can do with a
> > > > > > `File` is different depending on the backing type, making it
> > > > > > polymorphic is just needlessly confusing.
> > > > >
> > > > > What if I want to access file.data() and do something with the data? Then I'd
> > > > > necessarily need to put my data in an Arc and reference count it to still be
> > > > > able to access it.
> > > > >
> > > > > That doesn't seem like a reasonable requirement to be able to access data
> > > > > exposed via debugfs.
> > > > 
> > > > `pub fn data(&self) -> D` would go against my understanding of Greg's
> > > > request for DebugFS files to not really support anything other than
> > > > delete. I was even considering making `D` not be retained in the
> > > > disabled debugfs case, but left it in for now for so that the
> > > > lifecycles wouldn't change.
> > > 
> > > Well, that's because the C side does not have anything else. But the C side has
> > > no type system that deals with ownership:
> > > 
> > > In C you just stuff a pointer of your private data into debugfs_create_file()
> > > without any implication of ownership. debugfs has a pointer, the driver has a
> > > pointer. The question of the ownership semantics is not answered by the API, but
> > > by the implementation of the driver.
> > > 
> > > The Rust API is different, and it's even implied by the name of the trait you
> > > expect the data to implement: ForeignOwnable.
> > > 
> > > The File *owns* the data, either entirely or a reference count of the data.
> > > 
> > > If the *only* way to access the data the File now owns is by making it reference
> > > counted, it:
> > > 
> > >   1) Is additional overhead imposed on users.
> > > 
> > >   2) It has implications on the ownership design of your driver. Once something
> > >      is reference counted, you loose the guarantee the something can't out-live
> > >      some event.
> > > 
> > > I don't want that people have to stuff their data structures into Arc (i.e.
> > > reference count them), even though that's not necessary. It makes it easy to
> > > make mistakes. Things like:
> > > 
> > > 	let foo = bar.clone();
> > > 
> > > can easily be missed in reviews, whereas some contributor falsely changing a
> > > KBox to an Arc is much harder to miss.
> > > 
> > > > If you want a `.data()` function, I can add it in,
> > > 
> > > I think it could even be an implementation of Deref.
> > > 
> > > > but I don't think
> > > > it'll improve flexibility in most cases. If you want to do something
> > > > with the data and it's not in an `Arc` / behind a handle of some kind,
> > > > you'll need something providing threadsafe interior mutability in the
> > > > data structure. If that's a lock, then I have a hard time believing
> > > > that `Arc<Mutex<T>>`(or if it's a global, a `&'static Mutex<T>`, which
> > > > is why I added that in the stack) is so much more expensive than
> > > > `Box<Mutex<T>>` that it's worth a more complex API. If it's an atomic,
> > > > e.g. `Arc<AtomicU8>`, then I can see the benefit to having
> > > > `Box<AtomicU8>` over that, but it still seems so slim that I think the
> > > > simpler "`File` is just a handle to how long the file stays alive, it
> > > > doesn't let you do anything else" API makes sense.
> > > 
> > > I don't really see what is complicated about File<T> -- it's a File and it owns
> > > data of type T that is exposed via debugfs. Seems pretty straight forward to me.
> > > 
> > > Maybe the performance cost is not a huge argument here, but maintainability in
> > > terms of clarity about ownership and lifetime of an object as explained above
> > > clearly is.
> > 
> > I'm agreeing here.  As one of the primary users of this api is going to
> > be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make
> > an example driver to emulate that file with just a local structure, but
> > the reference counting and access logic just didn't seem to work out
> > properly.  Odds are I'm doing something stupid though...
> 
> I think it technically works, but it imposes semantics on drivers that we
> shouldn't do; see the example below.
> 
> > So a file callback IS going to want to have access to the data of type T
> > that is exposed somehow.
> 
> With the current API we would need this:
> 
> 	struct GPU {
> 	   fw: Arc<Firmware>,
> 	   fw_file: debugfs::File,
> 	}
> 
> and then I would initialize it the following way:
> 
> 	let fw = Arc::new(Firmware::new(), GFP_KERNEL)?;
> 	let fw_file = dir.create_file("firmware", fw.clone());
> 
> 	fw.do_something();
> 
> This is bad, because now my Firmware instance in GPU needs to be reference
> counted, even though it should *never* out-live the GPU instance. This is error
> prone.

Agreed, AND you just created a new fw structure that you really didn't
need, wasting memory.

> Instead this should just be:
> 
> 	struct GPU {
> 	   fw: debugfs::File<Firmware>,
> 	}
> 
> and then I would initialize it the following way:
> 
> 	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
> 	let file = dir.create_file("firmware", fw);
> 
> 	// debugfs::File<Firmware> dereferences to Firmware
> 	file.do_something();
> 
> 	// Access to fw is prevented by the compiler, since it has been moved
> 	// into file.
> 
> This is much better, since now I have the guarantee that my Firmare instance
> can't out-live the GPU instance.

That's better, yes, but how would multiple files for the same
"structure" work here?  Like a debugfs-file-per-field of a structure
that we often have?

thanks,

greg k-h

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 04:21:56PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jul 01, 2025 at 04:13:28PM +0200, Danilo Krummrich wrote:
> > On Tue, Jul 01, 2025 at 03:58:45PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Jun 30, 2025 at 08:16:55PM +0200, Danilo Krummrich wrote:
> > > > On Mon, Jun 30, 2025 at 10:49:51AM -0700, Matthew Maurer wrote:
> > > > > On Mon, Jun 30, 2025 at 10:39 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > >
> > > > > > On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > > > > > > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > > >>
> > > > > > >> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > > > > > >>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > > > > > >>> +    where
> > > > > > >>> +        for<'a> D::Borrowed<'a>: Display,
> > > > > > >>> +    {
> > > > > > >>> +        File {
> > > > > > >>> +            _foreign: ForeignHolder::new(data),
> > > > > > >>> +        }
> > > > > > >>>        }
> > > > > > >>
> > > > > > >> What's the motivation for the ForeignHolder abstraction? Why not just make it
> > > > > > >> File<D> and store data directly?
> > > > > > >
> > > > > > > 1. A `File<D>` can't be held in collection data structures as easily
> > > > > > > unless all your files contain the *same* backing type.
> > > > > >
> > > > > > That sounds reasonable.
> > > > > >
> > > > > > > 2. None of the APIs or potential APIs for `File` care about which type
> > > > > > > it's wrapping, nor are they supposed to. If nothing you can do with a
> > > > > > > `File` is different depending on the backing type, making it
> > > > > > > polymorphic is just needlessly confusing.
> > > > > >
> > > > > > What if I want to access file.data() and do something with the data? Then I'd
> > > > > > necessarily need to put my data in an Arc and reference count it to still be
> > > > > > able to access it.
> > > > > >
> > > > > > That doesn't seem like a reasonable requirement to be able to access data
> > > > > > exposed via debugfs.
> > > > > 
> > > > > `pub fn data(&self) -> D` would go against my understanding of Greg's
> > > > > request for DebugFS files to not really support anything other than
> > > > > delete. I was even considering making `D` not be retained in the
> > > > > disabled debugfs case, but left it in for now for so that the
> > > > > lifecycles wouldn't change.
> > > > 
> > > > Well, that's because the C side does not have anything else. But the C side has
> > > > no type system that deals with ownership:
> > > > 
> > > > In C you just stuff a pointer of your private data into debugfs_create_file()
> > > > without any implication of ownership. debugfs has a pointer, the driver has a
> > > > pointer. The question of the ownership semantics is not answered by the API, but
> > > > by the implementation of the driver.
> > > > 
> > > > The Rust API is different, and it's even implied by the name of the trait you
> > > > expect the data to implement: ForeignOwnable.
> > > > 
> > > > The File *owns* the data, either entirely or a reference count of the data.
> > > > 
> > > > If the *only* way to access the data the File now owns is by making it reference
> > > > counted, it:
> > > > 
> > > >   1) Is additional overhead imposed on users.
> > > > 
> > > >   2) It has implications on the ownership design of your driver. Once something
> > > >      is reference counted, you loose the guarantee the something can't out-live
> > > >      some event.
> > > > 
> > > > I don't want that people have to stuff their data structures into Arc (i.e.
> > > > reference count them), even though that's not necessary. It makes it easy to
> > > > make mistakes. Things like:
> > > > 
> > > > 	let foo = bar.clone();
> > > > 
> > > > can easily be missed in reviews, whereas some contributor falsely changing a
> > > > KBox to an Arc is much harder to miss.
> > > > 
> > > > > If you want a `.data()` function, I can add it in,
> > > > 
> > > > I think it could even be an implementation of Deref.
> > > > 
> > > > > but I don't think
> > > > > it'll improve flexibility in most cases. If you want to do something
> > > > > with the data and it's not in an `Arc` / behind a handle of some kind,
> > > > > you'll need something providing threadsafe interior mutability in the
> > > > > data structure. If that's a lock, then I have a hard time believing
> > > > > that `Arc<Mutex<T>>`(or if it's a global, a `&'static Mutex<T>`, which
> > > > > is why I added that in the stack) is so much more expensive than
> > > > > `Box<Mutex<T>>` that it's worth a more complex API. If it's an atomic,
> > > > > e.g. `Arc<AtomicU8>`, then I can see the benefit to having
> > > > > `Box<AtomicU8>` over that, but it still seems so slim that I think the
> > > > > simpler "`File` is just a handle to how long the file stays alive, it
> > > > > doesn't let you do anything else" API makes sense.
> > > > 
> > > > I don't really see what is complicated about File<T> -- it's a File and it owns
> > > > data of type T that is exposed via debugfs. Seems pretty straight forward to me.
> > > > 
> > > > Maybe the performance cost is not a huge argument here, but maintainability in
> > > > terms of clarity about ownership and lifetime of an object as explained above
> > > > clearly is.
> > > 
> > > I'm agreeing here.  As one of the primary users of this api is going to
> > > be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make
> > > an example driver to emulate that file with just a local structure, but
> > > the reference counting and access logic just didn't seem to work out
> > > properly.  Odds are I'm doing something stupid though...
> > 
> > I think it technically works, but it imposes semantics on drivers that we
> > shouldn't do; see the example below.
> > 
> > > So a file callback IS going to want to have access to the data of type T
> > > that is exposed somehow.
> > 
> > With the current API we would need this:
> > 
> > 	struct GPU {
> > 	   fw: Arc<Firmware>,
> > 	   fw_file: debugfs::File,
> > 	}
> > 
> > and then I would initialize it the following way:
> > 
> > 	let fw = Arc::new(Firmware::new(), GFP_KERNEL)?;
> > 	let fw_file = dir.create_file("firmware", fw.clone());
> > 
> > 	fw.do_something();
> > 
> > This is bad, because now my Firmware instance in GPU needs to be reference
> > counted, even though it should *never* out-live the GPU instance. This is error
> > prone.
> 
> Agreed, AND you just created a new fw structure that you really didn't
> need, wasting memory.

In case you refer to fw.clone(), since fw is an Arc<Firmware>, clone() just
creates a new reference count to the same object.

> > Instead this should just be:
> > 
> > 	struct GPU {
> > 	   fw: debugfs::File<Firmware>,
> > 	}
> > 
> > and then I would initialize it the following way:
> > 
> > 	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
> > 	let file = dir.create_file("firmware", fw);
> > 
> > 	// debugfs::File<Firmware> dereferences to Firmware
> > 	file.do_something();
> > 
> > 	// Access to fw is prevented by the compiler, since it has been moved
> > 	// into file.
> > 
> > This is much better, since now I have the guarantee that my Firmare instance
> > can't out-live the GPU instance.
> 
> That's better, yes, but how would multiple files for the same
> "structure" work here?  Like a debugfs-file-per-field of a structure
> that we often have?

That is a very good question and I thought about this as well, because with only
the current API this would require us to have more and more dynamic allocations
if we want to have a more fine grained filesystem representations of structures.

The idea I have for this is to use pin-init, which we do in quite some other
places as well.

I think we can add an additional API like this:

	impl Dir {
	   pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
	      pin_init!(Self {
	         data <- data,
	         ...
	      })
	   }
	}

This allows us to do things like:

	#[pin_data]
	struct Firmware {
	   #[pin]
	   minor: debugfs::File<u32>,
	   #[pin]
	   major: debugfs::File<u32>,
	   #[pin]
	   buffer: debugfs::File<[u8]>,
	}

	impl Firmware {
	   pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
	      pin_init!(Self {
	         minor <- dir.create_file("minor", 1),
	         major <- dir.create_file("major", 2),
	         buffer <- dir.create_file("buffer", buffer),
	      })
	   }
	}

	// This is the only allocation we need.
	let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

With this everything is now in a single allocation and since we're using
pin-init, Dir::create_file() can safely store pointers of the corresponding data
in debugfs_create_file(), since this structure is guaranteed to be pinned in
memory.

Actually, we can also implement *only this*, since with this my previous example
would just become this:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

	let file = dir.create_file("firmware", Firmware::new());
	let file = KBox::pin_init(file, GFP_KERNEL)?;

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

Given that, I think we should change things to use pin-init right away for the
debugfs::File API.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Matthew Maurer]

On Tue, Jul 1, 2025 at 8:10 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue, Jul 01, 2025 at 04:21:56PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jul 01, 2025 at 04:13:28PM +0200, Danilo Krummrich wrote:
> > > On Tue, Jul 01, 2025 at 03:58:45PM +0200, Greg Kroah-Hartman wrote:
> > > > On Mon, Jun 30, 2025 at 08:16:55PM +0200, Danilo Krummrich wrote:
> > > > > On Mon, Jun 30, 2025 at 10:49:51AM -0700, Matthew Maurer wrote:
> > > > > > On Mon, Jun 30, 2025 at 10:39 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > > >
> > > > > > > On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > > > > > > > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > > > >>
> > > > > > > >> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > > > > > > >>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > > > > > > >>> +    where
> > > > > > > >>> +        for<'a> D::Borrowed<'a>: Display,
> > > > > > > >>> +    {
> > > > > > > >>> +        File {
> > > > > > > >>> +            _foreign: ForeignHolder::new(data),
> > > > > > > >>> +        }
> > > > > > > >>>        }
> > > > > > > >>
> > > > > > > >> What's the motivation for the ForeignHolder abstraction? Why not just make it
> > > > > > > >> File<D> and store data directly?
> > > > > > > >
> > > > > > > > 1. A `File<D>` can't be held in collection data structures as easily
> > > > > > > > unless all your files contain the *same* backing type.
> > > > > > >
> > > > > > > That sounds reasonable.
> > > > > > >
> > > > > > > > 2. None of the APIs or potential APIs for `File` care about which type
> > > > > > > > it's wrapping, nor are they supposed to. If nothing you can do with a
> > > > > > > > `File` is different depending on the backing type, making it
> > > > > > > > polymorphic is just needlessly confusing.
> > > > > > >
> > > > > > > What if I want to access file.data() and do something with the data? Then I'd
> > > > > > > necessarily need to put my data in an Arc and reference count it to still be
> > > > > > > able to access it.
> > > > > > >
> > > > > > > That doesn't seem like a reasonable requirement to be able to access data
> > > > > > > exposed via debugfs.
> > > > > >
> > > > > > `pub fn data(&self) -> D` would go against my understanding of Greg's
> > > > > > request for DebugFS files to not really support anything other than
> > > > > > delete. I was even considering making `D` not be retained in the
> > > > > > disabled debugfs case, but left it in for now for so that the
> > > > > > lifecycles wouldn't change.
> > > > >
> > > > > Well, that's because the C side does not have anything else. But the C side has
> > > > > no type system that deals with ownership:
> > > > >
> > > > > In C you just stuff a pointer of your private data into debugfs_create_file()
> > > > > without any implication of ownership. debugfs has a pointer, the driver has a
> > > > > pointer. The question of the ownership semantics is not answered by the API, but
> > > > > by the implementation of the driver.
> > > > >
> > > > > The Rust API is different, and it's even implied by the name of the trait you
> > > > > expect the data to implement: ForeignOwnable.
> > > > >
> > > > > The File *owns* the data, either entirely or a reference count of the data.
> > > > >
> > > > > If the *only* way to access the data the File now owns is by making it reference
> > > > > counted, it:
> > > > >
> > > > >   1) Is additional overhead imposed on users.
> > > > >
> > > > >   2) It has implications on the ownership design of your driver. Once something
> > > > >      is reference counted, you loose the guarantee the something can't out-live
> > > > >      some event.
> > > > >
> > > > > I don't want that people have to stuff their data structures into Arc (i.e.
> > > > > reference count them), even though that's not necessary. It makes it easy to
> > > > > make mistakes. Things like:
> > > > >
> > > > >         let foo = bar.clone();
> > > > >
> > > > > can easily be missed in reviews, whereas some contributor falsely changing a
> > > > > KBox to an Arc is much harder to miss.
> > > > >
> > > > > > If you want a `.data()` function, I can add it in,
> > > > >
> > > > > I think it could even be an implementation of Deref.
> > > > >
> > > > > > but I don't think
> > > > > > it'll improve flexibility in most cases. If you want to do something
> > > > > > with the data and it's not in an `Arc` / behind a handle of some kind,
> > > > > > you'll need something providing threadsafe interior mutability in the
> > > > > > data structure. If that's a lock, then I have a hard time believing
> > > > > > that `Arc<Mutex<T>>`(or if it's a global, a `&'static Mutex<T>`, which
> > > > > > is why I added that in the stack) is so much more expensive than
> > > > > > `Box<Mutex<T>>` that it's worth a more complex API. If it's an atomic,
> > > > > > e.g. `Arc<AtomicU8>`, then I can see the benefit to having
> > > > > > `Box<AtomicU8>` over that, but it still seems so slim that I think the
> > > > > > simpler "`File` is just a handle to how long the file stays alive, it
> > > > > > doesn't let you do anything else" API makes sense.
> > > > >
> > > > > I don't really see what is complicated about File<T> -- it's a File and it owns
> > > > > data of type T that is exposed via debugfs. Seems pretty straight forward to me.
> > > > >
> > > > > Maybe the performance cost is not a huge argument here, but maintainability in
> > > > > terms of clarity about ownership and lifetime of an object as explained above
> > > > > clearly is.

The part that is not straightforward is that the primary purpose of
`T` may not be "to be exposed via DebugFS", and leaves us in the
unusual world where DebugFS (or at least our bindings to it) are
load-bearing in builds that have DebugFS disabled. It can work, but it
definitely seems confusing and is privileging DebugFS holding a
reference to a data structure over other things holding a reference to
it.

> > > >
> > > > I'm agreeing here.  As one of the primary users of this api is going to
> > > > be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make
> > > > an example driver to emulate that file with just a local structure, but
> > > > the reference counting and access logic just didn't seem to work out
> > > > properly.  Odds are I'm doing something stupid though...

I have a version of this that works with my initial driver, and keep
intending to revise it to work on top of the new one, but I keep
getting requests for API changes before I get around to reworking it
;)

The way I intended my `ForeignOwnable` variant of this to work was
that I would have a *single* struct that contained all the scanned
data. For the socinfo module, we know how big this could be, so we
could put this as a `static` in the module, in which case we'd have
`&'static MyInfo`. If we didn't want to do that or didn't know, we'd
produce an `Arc<MyInfo>`. Then, for each file, we'd create a file that
passed in either `my_arc_info.clone()` or `my_info` if we had the
static reference, with the functions that do the printing
distinguishing the behavior of each file.

Doing the file-owns-the-specific-field version *could* work, but means
that the debugfs tree code would be interleaved with the calls to
parsing code, or that we'd need two copies of the struct, an initial
one with all the parsed data, and a follow-up one that has has them
all wrapped in `File`.

> > >
> > > I think it technically works, but it imposes semantics on drivers that we
> > > shouldn't do; see the example below.
> > >
> > > > So a file callback IS going to want to have access to the data of type T
> > > > that is exposed somehow.
> > >
> > > With the current API we would need this:
> > >
> > >     struct GPU {
> > >        fw: Arc<Firmware>,
> > >        fw_file: debugfs::File,
> > >     }
> > >
> > > and then I would initialize it the following way:
> > >
> > >     let fw = Arc::new(Firmware::new(), GFP_KERNEL)?;
> > >     let fw_file = dir.create_file("firmware", fw.clone());
> > >
> > >     fw.do_something();
> > >
> > > This is bad, because now my Firmware instance in GPU needs to be reference
> > > counted, even though it should *never* out-live the GPU instance. This is error
> > > prone.

If `Firmware` outliving the GPU instance causes a bug, not just a
resource leak, I'd argue that you should be passing a narrower handle
to your debugfs file. If it just causes a resource leak, then I'd
argue that if you leak a resource (the debugfs file itself), then
leaking a resource is not unexpected.

For the narrower handle example, consider e.g.
```
struct Firmware {
  info: Arc<FirmwareInfo>,
  my_handle: Handle // Some handle whose drop is load bearing
}
// ...
let fw: Firmware = build_fw()?;
let fw_file = fw.info.clone();
fw.do_something();
```

> >
> > Agreed, AND you just created a new fw structure that you really didn't
> > need, wasting memory.
>
> In case you refer to fw.clone(), since fw is an Arc<Firmware>, clone() just
> creates a new reference count to the same object.
>
> > > Instead this should just be:
> > >
> > >     struct GPU {
> > >        fw: debugfs::File<Firmware>,
> > >     }
> > >
> > > and then I would initialize it the following way:
> > >
> > >     let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
> > >     let file = dir.create_file("firmware", fw);
> > >
> > >     // debugfs::File<Firmware> dereferences to Firmware
> > >     file.do_something();
> > >
> > >     // Access to fw is prevented by the compiler, since it has been moved
> > >     // into file.
> > >
> > > This is much better, since now I have the guarantee that my Firmare instance
> > > can't out-live the GPU instance.
> >
> > That's better, yes, but how would multiple files for the same
> > "structure" work here?  Like a debugfs-file-per-field of a structure
> > that we often have?

This seems odd to me because with this model the DebugFS file itself
becomes deeply involved with the ownership of the devices or other
data. There are a few issues I think this creates:
1. It discourages file-per-field, because you can no longer have the
DebugFS file uniquely own something, which you're now trying to
encourage. This seems likely to result in people having debugfs files
that render out structured data, which I think we generally don't
want?
2. If I have a module and want to add a new DebugFS file to it, the
change will edit the core structures of the drivers rather than just
adding an auxiliary field. This is partially remedied if we make
`File<D>` implement `Deref<Target=D>`. It will still either incur
churn or imprecision around commonly derived traits like equality and
debug. If I have `#[derive(PartialEq)]` on my struct including
`DeviceData`, and then I wrap that into a `File<DeviceData>`, we'll
get stuck with a quandry:
  a. Should files `x == y` if the underlying private data is equal?
This has the benefit of preserving the behavior of the derive, but
means that distinct files may compare equal if they have the same
contents.
  b. Should files `x == y` only be true if the underlying *file* is
the same? This seems logical for *file* equality, but will break
equality on the field or structures including it.
  This will also be an incompatible change for anyone already using an
explicit `.deref()` or an explicit `impl Deref<Target = T>` method to
work with the field, because while deref coercion will automatically
chain, `impl Deref<Target = T>` *cannot* automatically chain.
3. If you create an object specifically for debugging, then it will
*have* to be created and kept around even if DebugFS is disabled,
because we can't tell whether the user is relying on its existence.
(My current implementation keeps it around in that case as well, but
that's not guaranteed or required - we could cause it to just
immediately drop data that someone tried to attach.) This means that
rather than relying on the API to determine whether the information is
needed, the modules will need to check the config if they want to get
rid of it.

>
> That is a very good question and I thought about this as well, because with only
> the current API this would require us to have more and more dynamic allocations
> if we want to have a more fine grained filesystem representations of structures.
>
> The idea I have for this is to use pin-init, which we do in quite some other
> places as well.
>
> I think we can add an additional API like this:
>
>         impl Dir {
>            pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
>               pin_init!(Self {
>                  data <- data,
>                  ...
>               })
>            }
>         }
>
> This allows us to do things like:
>
>         #[pin_data]
>         struct Firmware {
>            #[pin]
>            minor: debugfs::File<u32>,
>            #[pin]
>            major: debugfs::File<u32>,
>            #[pin]
>            buffer: debugfs::File<[u8]>,

Nit: This can't work, because a `&[u8]` is double-width, and we only
get to send one pointer into the file. We'd need to add a requirement
`T: Sized` if we really wanted to have `File` embed the data.

>         }
>
>         impl Firmware {
>            pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
>               pin_init!(Self {
>                  minor <- dir.create_file("minor", 1),
>                  major <- dir.create_file("major", 2),
>                  buffer <- dir.create_file("buffer", buffer),
>               })
>            }
>         }
>
>         // This is the only allocation we need.
>         let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
>
> With this everything is now in a single allocation and since we're using
> pin-init, Dir::create_file() can safely store pointers of the corresponding data
> in debugfs_create_file(), since this structure is guaranteed to be pinned in
> memory.
>
> Actually, we can also implement *only this*, since with this my previous example
> would just become this:

If we implement *only* pinned files, we run into an additional problem
- you can't easily extend a pinned vector. This means that you cannot
have dynamically created devices unless you're willing to put every
new `File` into its own `Box`, because you aren't allowed to move any
of the previously allocated `File`s for a resize.

Where previously you would have had

```
debug_files: Vec<File>
```

you would now have

```
debug_files: Vec<PinBox<File<T>>>
```

If it's easy to use the model you described earlier where the sole
owner of the data is the debugfs file, then this is mostly fine,
because to use the old version, you'd already have it live in a
`ForeignOwnable`, which most likely means its own allocation. However,
if you're already forced into an environment where the objects you're
embedding into a file are reference counted, you now have an extra
allocation required for every `File`. Because the `File` has to be
able to respond to `data()`, we also won't have these `File`s truncate
to zero size in the no debugfs scenario.

This means that if you want a `File` to render an object that is
reference counted, and they're dynamically created and destroyed, you
will now be required to incur an allocation per file, even when
DebugFS is disabled.

>
>         struct GPU {
>            fw: debugfs::File<Firmware>,
>         }
>
>         let file = dir.create_file("firmware", Firmware::new());
>         let file = KBox::pin_init(file, GFP_KERNEL)?;
>
>         // debugfs::File<Firmware> dereferences to Firmware
>         file.do_something();
>
> Given that, I think we should change things to use pin-init right away for the
> debugfs::File API.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 11:11:13AM -0700, Matthew Maurer wrote:
> On Tue, Jul 1, 2025 at 8:10 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Tue, Jul 01, 2025 at 04:21:56PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jul 01, 2025 at 04:13:28PM +0200, Danilo Krummrich wrote:
> > > > On Tue, Jul 01, 2025 at 03:58:45PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Mon, Jun 30, 2025 at 08:16:55PM +0200, Danilo Krummrich wrote:
> > > > > > On Mon, Jun 30, 2025 at 10:49:51AM -0700, Matthew Maurer wrote:
> > > > > > > On Mon, Jun 30, 2025 at 10:39 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On 6/30/25 7:34 PM, Matthew Maurer wrote:
> > > > > > > > > On Mon, Jun 30, 2025 at 10:30 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > > > > > > > >>
> > > > > > > > >> On 6/28/25 1:18 AM, Matthew Maurer wrote:
> > > > > > > > >>> +    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
> > > > > > > > >>> +    where
> > > > > > > > >>> +        for<'a> D::Borrowed<'a>: Display,
> > > > > > > > >>> +    {
> > > > > > > > >>> +        File {
> > > > > > > > >>> +            _foreign: ForeignHolder::new(data),
> > > > > > > > >>> +        }
> > > > > > > > >>>        }
> > > > > > > > >>
> > > > > > > > >> What's the motivation for the ForeignHolder abstraction? Why not just make it
> > > > > > > > >> File<D> and store data directly?
> > > > > > > > >
> > > > > > > > > 1. A `File<D>` can't be held in collection data structures as easily
> > > > > > > > > unless all your files contain the *same* backing type.
> > > > > > > >
> > > > > > > > That sounds reasonable.
> > > > > > > >
> > > > > > > > > 2. None of the APIs or potential APIs for `File` care about which type
> > > > > > > > > it's wrapping, nor are they supposed to. If nothing you can do with a
> > > > > > > > > `File` is different depending on the backing type, making it
> > > > > > > > > polymorphic is just needlessly confusing.
> > > > > > > >
> > > > > > > > What if I want to access file.data() and do something with the data? Then I'd
> > > > > > > > necessarily need to put my data in an Arc and reference count it to still be
> > > > > > > > able to access it.
> > > > > > > >
> > > > > > > > That doesn't seem like a reasonable requirement to be able to access data
> > > > > > > > exposed via debugfs.
> > > > > > >
> > > > > > > `pub fn data(&self) -> D` would go against my understanding of Greg's
> > > > > > > request for DebugFS files to not really support anything other than
> > > > > > > delete. I was even considering making `D` not be retained in the
> > > > > > > disabled debugfs case, but left it in for now for so that the
> > > > > > > lifecycles wouldn't change.
> > > > > >
> > > > > > Well, that's because the C side does not have anything else. But the C side has
> > > > > > no type system that deals with ownership:
> > > > > >
> > > > > > In C you just stuff a pointer of your private data into debugfs_create_file()
> > > > > > without any implication of ownership. debugfs has a pointer, the driver has a
> > > > > > pointer. The question of the ownership semantics is not answered by the API, but
> > > > > > by the implementation of the driver.
> > > > > >
> > > > > > The Rust API is different, and it's even implied by the name of the trait you
> > > > > > expect the data to implement: ForeignOwnable.
> > > > > >
> > > > > > The File *owns* the data, either entirely or a reference count of the data.
> > > > > >
> > > > > > If the *only* way to access the data the File now owns is by making it reference
> > > > > > counted, it:
> > > > > >
> > > > > >   1) Is additional overhead imposed on users.
> > > > > >
> > > > > >   2) It has implications on the ownership design of your driver. Once something
> > > > > >      is reference counted, you loose the guarantee the something can't out-live
> > > > > >      some event.
> > > > > >
> > > > > > I don't want that people have to stuff their data structures into Arc (i.e.
> > > > > > reference count them), even though that's not necessary. It makes it easy to
> > > > > > make mistakes. Things like:
> > > > > >
> > > > > >         let foo = bar.clone();
> > > > > >
> > > > > > can easily be missed in reviews, whereas some contributor falsely changing a
> > > > > > KBox to an Arc is much harder to miss.
> > > > > >
> > > > > > > If you want a `.data()` function, I can add it in,
> > > > > >
> > > > > > I think it could even be an implementation of Deref.
> > > > > >
> > > > > > > but I don't think
> > > > > > > it'll improve flexibility in most cases. If you want to do something
> > > > > > > with the data and it's not in an `Arc` / behind a handle of some kind,
> > > > > > > you'll need something providing threadsafe interior mutability in the
> > > > > > > data structure. If that's a lock, then I have a hard time believing
> > > > > > > that `Arc<Mutex<T>>`(or if it's a global, a `&'static Mutex<T>`, which
> > > > > > > is why I added that in the stack) is so much more expensive than
> > > > > > > `Box<Mutex<T>>` that it's worth a more complex API. If it's an atomic,
> > > > > > > e.g. `Arc<AtomicU8>`, then I can see the benefit to having
> > > > > > > `Box<AtomicU8>` over that, but it still seems so slim that I think the
> > > > > > > simpler "`File` is just a handle to how long the file stays alive, it
> > > > > > > doesn't let you do anything else" API makes sense.
> > > > > >
> > > > > > I don't really see what is complicated about File<T> -- it's a File and it owns
> > > > > > data of type T that is exposed via debugfs. Seems pretty straight forward to me.
> > > > > >
> > > > > > Maybe the performance cost is not a huge argument here, but maintainability in
> > > > > > terms of clarity about ownership and lifetime of an object as explained above
> > > > > > clearly is.
> 
> The part that is not straightforward is that the primary purpose of
> `T` may not be "to be exposed via DebugFS", and leaves us in the
> unusual world where DebugFS (or at least our bindings to it) are
> load-bearing in builds that have DebugFS disabled. It can work, but it
> definitely seems confusing and is privileging DebugFS holding a
> reference to a data structure over other things holding a reference to
> it.

The purpose of File<T> is that with an instance of File<T> you can access the T.
This is totally normal. You give the file ownership of some T and you still want
to be able to access this T the File instance has ownership of.

It's not load-bearing at all in builds that have debugfs disabled; there is
zero overhead. When compiled without debugfs File<T>'s only member is the data
itself. In fact, it's even slighly less, since the T can be a KBox<U> rather
than an Arc<U>.

> > > > >
> > > > > I'm agreeing here.  As one of the primary users of this api is going to
> > > > > be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make
> > > > > an example driver to emulate that file with just a local structure, but
> > > > > the reference counting and access logic just didn't seem to work out
> > > > > properly.  Odds are I'm doing something stupid though...
> 
> I have a version of this that works with my initial driver, and keep
> intending to revise it to work on top of the new one, but I keep
> getting requests for API changes before I get around to reworking it
> ;)
> 
> The way I intended my `ForeignOwnable` variant of this to work was
> that I would have a *single* struct that contained all the scanned
> data. For the socinfo module, we know how big this could be, so we
> could put this as a `static` in the module, in which case we'd have
> `&'static MyInfo`. If we didn't want to do that or didn't know, we'd
> produce an `Arc<MyInfo>`. Then, for each file, we'd create a file that
> passed in either `my_arc_info.clone()` or `my_info` if we had the
> static reference, with the functions that do the printing
> distinguishing the behavior of each file.
> 
> Doing the file-owns-the-specific-field version *could* work, but means
> that the debugfs tree code would be interleaved with the calls to
> parsing code, or that we'd need two copies of the struct, an initial
> one with all the parsed data, and a follow-up one that has has them
> all wrapped in `File`.

Why can't you do the latter right away?

> > > >
> > > > I think it technically works, but it imposes semantics on drivers that we
> > > > shouldn't do; see the example below.
> > > >
> > > > > So a file callback IS going to want to have access to the data of type T
> > > > > that is exposed somehow.
> > > >
> > > > With the current API we would need this:
> > > >
> > > >     struct GPU {
> > > >        fw: Arc<Firmware>,
> > > >        fw_file: debugfs::File,
> > > >     }
> > > >
> > > > and then I would initialize it the following way:
> > > >
> > > >     let fw = Arc::new(Firmware::new(), GFP_KERNEL)?;
> > > >     let fw_file = dir.create_file("firmware", fw.clone());
> > > >
> > > >     fw.do_something();
> > > >
> > > > This is bad, because now my Firmware instance in GPU needs to be reference
> > > > counted, even though it should *never* out-live the GPU instance. This is error
> > > > prone.
> 
> If `Firmware` outliving the GPU instance causes a bug, not just a
> resource leak, I'd argue that you should be passing a narrower handle
> to your debugfs file. If it just causes a resource leak, then I'd
> argue that if you leak a resource (the debugfs file itself), then
> leaking a resource is not unexpected.
> 
> For the narrower handle example, consider e.g.
> ```
> struct Firmware {
>   info: Arc<FirmwareInfo>,
>   my_handle: Handle // Some handle whose drop is load bearing
> }
> // ...
> let fw: Firmware = build_fw()?;
> let fw_file = fw.info.clone();
> fw.do_something();
> ```

Sorry, but that really looks like a bad workaround. Now I have an additional
allocation and a reference counted FirmwareInfo instance for no reason, other
than debugfs imposing a bad API on users. :(

In more complicated cases, this could be lots of additional allocations and
reference counted structures just because of debugfs.

> > >
> > > Agreed, AND you just created a new fw structure that you really didn't
> > > need, wasting memory.
> >
> > In case you refer to fw.clone(), since fw is an Arc<Firmware>, clone() just
> > creates a new reference count to the same object.
> >
> > > > Instead this should just be:
> > > >
> > > >     struct GPU {
> > > >        fw: debugfs::File<Firmware>,
> > > >     }
> > > >
> > > > and then I would initialize it the following way:
> > > >
> > > >     let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
> > > >     let file = dir.create_file("firmware", fw);
> > > >
> > > >     // debugfs::File<Firmware> dereferences to Firmware
> > > >     file.do_something();
> > > >
> > > >     // Access to fw is prevented by the compiler, since it has been moved
> > > >     // into file.
> > > >
> > > > This is much better, since now I have the guarantee that my Firmare instance
> > > > can't out-live the GPU instance.
> > >
> > > That's better, yes, but how would multiple files for the same
> > > "structure" work here?  Like a debugfs-file-per-field of a structure
> > > that we often have?
> 
> This seems odd to me because with this model the DebugFS file itself
> becomes deeply involved with the ownership of the devices or other
> data.

This is already the case with this version on the mailing list. You're passing
*ForeignOwnable* data to the File, which the File instance subsequently owns.

There is zero difference in ownership, the difference is that File<T> allows you
to access the data it owns, whereas just File does not, which makes it necessary
to use an Arc.

> There are a few issues I think this creates:
> 1. It discourages file-per-field, because you can no longer have the
> DebugFS file uniquely own something, which you're now trying to
> encourage. This seems likely to result in people having debugfs files
> that render out structured data, which I think we generally don't
> want?

That's what I proposed the pin-init solution below for.

> 2. If I have a module and want to add a new DebugFS file to it, the
> change will edit the core structures of the drivers rather than just
> adding an auxiliary field. This is partially remedied if we make
> `File<D>` implement `Deref<Target=D>`.

So, you're arguing that changing

	struct Foo {
	   bar: Bar,
	}

to

	struct Foo {
	   bar: Arc<Bar>,
	   bar_file: debugfs::File,
	}

is better than

	struct Foo {
	   bar: debugfs::File<Bar>,
	}

correct? If so, I really have to disagree.

> It will still either incur
> churn or imprecision around commonly derived traits like equality and
> debug. If I have `#[derive(PartialEq)]` on my struct including
> `DeviceData`, and then I wrap that into a `File<DeviceData>`, we'll
> get stuck with a quandry:
>   a. Should files `x == y` if the underlying private data is equal?
> This has the benefit of preserving the behavior of the derive, but
> means that distinct files may compare equal if they have the same
> contents.
>   b. Should files `x == y` only be true if the underlying *file* is
> the same? This seems logical for *file* equality, but will break
> equality on the field or structures including it.

Why would you even compare two File instances? If your T implements PartialEq,
and you want to compare it with some other T just use the
ForeignOwnable::Borrowed you get from dereferencing the File.

>   This will also be an incompatible change for anyone already using an
> explicit `.deref()` or an explicit `impl Deref<Target = T>` method to
> work with the field, because while deref coercion will automatically
> chain, `impl Deref<Target = T>` *cannot* automatically chain.

Seems like a pretty minor concern, but if you want you can still do the Arc
dance in your driver, no?

> 3. If you create an object specifically for debugging, then it will
> *have* to be created and kept around even if DebugFS is disabled,
> because we can't tell whether the user is relying on its existence.
> (My current implementation keeps it around in that case as well, but
> that's not guaranteed or required - we could cause it to just
> immediately drop data that someone tried to attach.) This means that
> rather than relying on the API to determine whether the information is
> needed, the modules will need to check the config if they want to get
> rid of it.

This is only half the truth, since this is *only* the case if you don't need to
access the data you move into the File, since otherwise you have to keep an Arc
around.

> >
> > That is a very good question and I thought about this as well, because with only
> > the current API this would require us to have more and more dynamic allocations
> > if we want to have a more fine grained filesystem representations of structures.
> >
> > The idea I have for this is to use pin-init, which we do in quite some other
> > places as well.
> >
> > I think we can add an additional API like this:
> >
> >         impl Dir {
> >            pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
> >               pin_init!(Self {
> >                  data <- data,
> >                  ...
> >               })
> >            }
> >         }
> >
> > This allows us to do things like:
> >
> >         #[pin_data]
> >         struct Firmware {
> >            #[pin]
> >            minor: debugfs::File<u32>,
> >            #[pin]
> >            major: debugfs::File<u32>,
> >            #[pin]
> >            buffer: debugfs::File<[u8]>,
> 
> Nit: This can't work, because a `&[u8]` is double-width, and we only
> get to send one pointer into the file. We'd need to add a requirement
> `T: Sized` if we really wanted to have `File` embed the data.

Indeed, that was a bad example, let's say Vec<u8> then. :)

> 
> >         }
> >
> >         impl Firmware {
> >            pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
> >               pin_init!(Self {
> >                  minor <- dir.create_file("minor", 1),
> >                  major <- dir.create_file("major", 2),
> >                  buffer <- dir.create_file("buffer", buffer),
> >               })
> >            }
> >         }
> >
> >         // This is the only allocation we need.
> >         let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
> >
> > With this everything is now in a single allocation and since we're using
> > pin-init, Dir::create_file() can safely store pointers of the corresponding data
> > in debugfs_create_file(), since this structure is guaranteed to be pinned in
> > memory.
> >
> > Actually, we can also implement *only this*, since with this my previous example
> > would just become this:
> 
> If we implement *only* pinned files, we run into an additional problem
> - you can't easily extend a pinned vector. This means that you cannot
> have dynamically created devices unless you're willing to put every
> new `File` into its own `Box`, because you aren't allowed to move any
> of the previously allocated `File`s for a resize.
> 
> Where previously you would have had
> 
> ```
> debug_files: Vec<File>
> ```
> 
> you would now have
> 
> ```
> debug_files: Vec<PinBox<File<T>>>
> ```

Stuffing single File instances into a Vec seems like the wrong thing to do.

Instead you may have instances of some data structure that is created
dynamically in your driver that you want to expose through debugfs.

Let's say you have (userspace) clients that can be registered arbitrarily, then
you want a Vec<Client>, which contains the client instances. In order to provide
information about the Client in debugfs you then have the client embed things as
discussed above.

	struct Client {
	   id: File<ClientId>,
	   data: File<ClientData>,
	   ...
	}

I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a
Vec<File> separately. Also, note that with the above, your Client instances
don't need to be reference counted anymore.

I think this addresses the concerns below.

> If it's easy to use the model you described earlier where the sole
> owner of the data is the debugfs file, then this is mostly fine,
> because to use the old version, you'd already have it live in a
> `ForeignOwnable`, which most likely means its own allocation. However,
> if you're already forced into an environment where the objects you're
> embedding into a file are reference counted, you now have an extra
> allocation required for every `File`. Because the `File` has to be
> able to respond to `data()`, we also won't have these `File`s truncate
> to zero size in the no debugfs scenario.
> 
> This means that if you want a `File` to render an object that is
> reference counted, and they're dynamically created and destroyed, you
> will now be required to incur an allocation per file, even when
> DebugFS is disabled.
> 
> >
> >         struct GPU {
> >            fw: debugfs::File<Firmware>,
> >         }
> >
> >         let file = dir.create_file("firmware", Firmware::new());
> >         let file = KBox::pin_init(file, GFP_KERNEL)?;
> >
> >         // debugfs::File<Firmware> dereferences to Firmware
> >         file.do_something();
> >
> > Given that, I think we should change things to use pin-init right away for the
> > debugfs::File API.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Benno Lossin]

On Tue Jul 1, 2025 at 9:21 PM CEST, Danilo Krummrich wrote:
> On Tue, Jul 01, 2025 at 11:11:13AM -0700, Matthew Maurer wrote:
>> On Tue, Jul 1, 2025 at 8:10 AM Danilo Krummrich <dakr@kernel.org> wrote:
>> >         impl Firmware {
>> >            pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
>> >               pin_init!(Self {
>> >                  minor <- dir.create_file("minor", 1),
>> >                  major <- dir.create_file("major", 2),
>> >                  buffer <- dir.create_file("buffer", buffer),
>> >               })
>> >            }
>> >         }
>> >
>> >         // This is the only allocation we need.
>> >         let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
>> >
>> > With this everything is now in a single allocation and since we're using
>> > pin-init, Dir::create_file() can safely store pointers of the corresponding data
>> > in debugfs_create_file(), since this structure is guaranteed to be pinned in
>> > memory.
>> >
>> > Actually, we can also implement *only this*, since with this my previous example
>> > would just become this:
>> 
>> If we implement *only* pinned files, we run into an additional problem
>> - you can't easily extend a pinned vector. This means that you cannot
>> have dynamically created devices unless you're willing to put every
>> new `File` into its own `Box`, because you aren't allowed to move any
>> of the previously allocated `File`s for a resize.
>> 
>> Where previously you would have had
>> 
>> ```
>> debug_files: Vec<File>
>> ```
>> 
>> you would now have
>> 
>> ```
>> debug_files: Vec<PinBox<File<T>>>
>> ```
>
> Stuffing single File instances into a Vec seems like the wrong thing to do.
>
> Instead you may have instances of some data structure that is created
> dynamically in your driver that you want to expose through debugfs.
>
> Let's say you have (userspace) clients that can be registered arbitrarily, then
> you want a Vec<Client>, which contains the client instances. In order to provide
> information about the Client in debugfs you then have the client embed things as
> discussed above.
>
> 	struct Client {
> 	   id: File<ClientId>,
> 	   data: File<ClientData>,
> 	   ...
> 	}
>
> I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a
> Vec<File> separately. Also, note that with the above, your Client instances
> don't need to be reference counted anymore.
>
> I think this addresses the concerns below.

You still have the issue that `Client` now needs to be pinned and the
vector can't be resized. But if you know that it's bounded, then we
could just make `Pin<Vec<T>>` work as expected (not relocating the
underlying allocation by not exposing `push`, only
`push_within_capacity`).

We also could have a `SegmentedVec<T>` that doesn't move elements.
Essentially it is
    
    enum SegmentedVec<T> {
        Cons(Segment<T>, KBox<SegmentedVec<T>>)
        Nul,
    }

    struct Segment<T> {
        elements: [T; 16]
    }

or make the segments variable-sized and grow them accordingly.

---
Cheers,
Benno

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On 7/1/25 9:46 PM, Benno Lossin wrote:
> On Tue Jul 1, 2025 at 9:21 PM CEST, Danilo Krummrich wrote:
>> On Tue, Jul 01, 2025 at 11:11:13AM -0700, Matthew Maurer wrote:
>>> On Tue, Jul 1, 2025 at 8:10 AM Danilo Krummrich <dakr@kernel.org> wrote:
>>>>          impl Firmware {
>>>>             pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
>>>>                pin_init!(Self {
>>>>                   minor <- dir.create_file("minor", 1),
>>>>                   major <- dir.create_file("major", 2),
>>>>                   buffer <- dir.create_file("buffer", buffer),
>>>>                })
>>>>             }
>>>>          }
>>>>
>>>>          // This is the only allocation we need.
>>>>          let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
>>>>
>>>> With this everything is now in a single allocation and since we're using
>>>> pin-init, Dir::create_file() can safely store pointers of the corresponding data
>>>> in debugfs_create_file(), since this structure is guaranteed to be pinned in
>>>> memory.
>>>>
>>>> Actually, we can also implement *only this*, since with this my previous example
>>>> would just become this:
>>>
>>> If we implement *only* pinned files, we run into an additional problem
>>> - you can't easily extend a pinned vector. This means that you cannot
>>> have dynamically created devices unless you're willing to put every
>>> new `File` into its own `Box`, because you aren't allowed to move any
>>> of the previously allocated `File`s for a resize.
>>>
>>> Where previously you would have had
>>>
>>> ```
>>> debug_files: Vec<File>
>>> ```
>>>
>>> you would now have
>>>
>>> ```
>>> debug_files: Vec<PinBox<File<T>>>
>>> ```
>>
>> Stuffing single File instances into a Vec seems like the wrong thing to do.
>>
>> Instead you may have instances of some data structure that is created
>> dynamically in your driver that you want to expose through debugfs.
>>
>> Let's say you have (userspace) clients that can be registered arbitrarily, then
>> you want a Vec<Client>, which contains the client instances. In order to provide
>> information about the Client in debugfs you then have the client embed things as
>> discussed above.
>>
>> 	struct Client {
>> 	   id: File<ClientId>,
>> 	   data: File<ClientData>,
>> 	   ...
>> 	}
>>
>> I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a
>> Vec<File> separately. Also, note that with the above, your Client instances
>> don't need to be reference counted anymore.
>>
>> I think this addresses the concerns below.
> 
> You still have the issue that `Client` now needs to be pinned and the
> vector can't be resized. But if you know that it's bounded, then we
> could just make `Pin<Vec<T>>` work as expected (not relocating the
> underlying allocation by not exposing `push`, only
> `push_within_capacity`).
> 
> We also could have a `SegmentedVec<T>` that doesn't move elements.
> Essentially it is
>      
>      enum SegmentedVec<T> {
>          Cons(Segment<T>, KBox<SegmentedVec<T>>)
>          Nul,
>      }
> 
>      struct Segment<T> {
>          elements: [T; 16]
>      }
> 
> or make the segments variable-sized and grow them accordingly.

That sounds a lot like the perfect application for XArray. :)

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Benno Lossin]

On Tue Jul 1, 2025 at 9:58 PM CEST, Danilo Krummrich wrote:
> On 7/1/25 9:46 PM, Benno Lossin wrote:
>> On Tue Jul 1, 2025 at 9:21 PM CEST, Danilo Krummrich wrote:
>>> On Tue, Jul 01, 2025 at 11:11:13AM -0700, Matthew Maurer wrote:
>>>> On Tue, Jul 1, 2025 at 8:10 AM Danilo Krummrich <dakr@kernel.org> wrote:
>>>>>          impl Firmware {
>>>>>             pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
>>>>>                pin_init!(Self {
>>>>>                   minor <- dir.create_file("minor", 1),
>>>>>                   major <- dir.create_file("major", 2),
>>>>>                   buffer <- dir.create_file("buffer", buffer),
>>>>>                })
>>>>>             }
>>>>>          }
>>>>>
>>>>>          // This is the only allocation we need.
>>>>>          let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
>>>>>
>>>>> With this everything is now in a single allocation and since we're using
>>>>> pin-init, Dir::create_file() can safely store pointers of the corresponding data
>>>>> in debugfs_create_file(), since this structure is guaranteed to be pinned in
>>>>> memory.
>>>>>
>>>>> Actually, we can also implement *only this*, since with this my previous example
>>>>> would just become this:
>>>>
>>>> If we implement *only* pinned files, we run into an additional problem
>>>> - you can't easily extend a pinned vector. This means that you cannot
>>>> have dynamically created devices unless you're willing to put every
>>>> new `File` into its own `Box`, because you aren't allowed to move any
>>>> of the previously allocated `File`s for a resize.
>>>>
>>>> Where previously you would have had
>>>>
>>>> ```
>>>> debug_files: Vec<File>
>>>> ```
>>>>
>>>> you would now have
>>>>
>>>> ```
>>>> debug_files: Vec<PinBox<File<T>>>
>>>> ```
>>>
>>> Stuffing single File instances into a Vec seems like the wrong thing to do.
>>>
>>> Instead you may have instances of some data structure that is created
>>> dynamically in your driver that you want to expose through debugfs.
>>>
>>> Let's say you have (userspace) clients that can be registered arbitrarily, then
>>> you want a Vec<Client>, which contains the client instances. In order to provide
>>> information about the Client in debugfs you then have the client embed things as
>>> discussed above.
>>>
>>> 	struct Client {
>>> 	   id: File<ClientId>,
>>> 	   data: File<ClientData>,
>>> 	   ...
>>> 	}
>>>
>>> I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a
>>> Vec<File> separately. Also, note that with the above, your Client instances
>>> don't need to be reference counted anymore.
>>>
>>> I think this addresses the concerns below.
>> 
>> You still have the issue that `Client` now needs to be pinned and the
>> vector can't be resized. But if you know that it's bounded, then we
>> could just make `Pin<Vec<T>>` work as expected (not relocating the
>> underlying allocation by not exposing `push`, only
>> `push_within_capacity`).
>> 
>> We also could have a `SegmentedVec<T>` that doesn't move elements.
>> Essentially it is
>>      
>>      enum SegmentedVec<T> {
>>          Cons(Segment<T>, KBox<SegmentedVec<T>>)
>>          Nul,
>>      }
>> 
>>      struct Segment<T> {
>>          elements: [T; 16]
>>      }
>> 
>> or make the segments variable-sized and grow them accordingly.
>
> That sounds a lot like the perfect application for XArray. :)

Haha I didn't know this already existed in the kernel :) Yeah then we
should make XArray work for this use-case.

---
Cheers,
Benno

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Benno Lossin]

On Tue Jul 1, 2025 at 10:03 PM CEST, Benno Lossin wrote:
> On Tue Jul 1, 2025 at 9:58 PM CEST, Danilo Krummrich wrote:
>> On 7/1/25 9:46 PM, Benno Lossin wrote:
>>> On Tue Jul 1, 2025 at 9:21 PM CEST, Danilo Krummrich wrote:
>>>> On Tue, Jul 01, 2025 at 11:11:13AM -0700, Matthew Maurer wrote:
>>>>> If we implement *only* pinned files, we run into an additional problem
>>>>> - you can't easily extend a pinned vector. This means that you cannot
>>>>> have dynamically created devices unless you're willing to put every
>>>>> new `File` into its own `Box`, because you aren't allowed to move any
>>>>> of the previously allocated `File`s for a resize.
>>>>>
>>>>> Where previously you would have had
>>>>>
>>>>> ```
>>>>> debug_files: Vec<File>
>>>>> ```
>>>>>
>>>>> you would now have
>>>>>
>>>>> ```
>>>>> debug_files: Vec<PinBox<File<T>>>
>>>>> ```
>>>>
>>>> Stuffing single File instances into a Vec seems like the wrong thing to do.
>>>>
>>>> Instead you may have instances of some data structure that is created
>>>> dynamically in your driver that you want to expose through debugfs.
>>>>
>>>> Let's say you have (userspace) clients that can be registered arbitrarily, then
>>>> you want a Vec<Client>, which contains the client instances. In order to provide
>>>> information about the Client in debugfs you then have the client embed things as
>>>> discussed above.
>>>>
>>>> 	struct Client {
>>>> 	   id: File<ClientId>,
>>>> 	   data: File<ClientData>,
>>>> 	   ...
>>>> 	}
>>>>
>>>> I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a
>>>> Vec<File> separately. Also, note that with the above, your Client instances
>>>> don't need to be reference counted anymore.
>>>>
>>>> I think this addresses the concerns below.
>>> 
>>> You still have the issue that `Client` now needs to be pinned and the
>>> vector can't be resized. But if you know that it's bounded, then we
>>> could just make `Pin<Vec<T>>` work as expected (not relocating the
>>> underlying allocation by not exposing `push`, only
>>> `push_within_capacity`).
>>> 
>>> We also could have a `SegmentedVec<T>` that doesn't move elements.
>>> Essentially it is
>>>      
>>>      enum SegmentedVec<T> {
>>>          Cons(Segment<T>, KBox<SegmentedVec<T>>)
>>>          Nul,
>>>      }
>>> 
>>>      struct Segment<T> {
>>>          elements: [T; 16]
>>>      }
>>> 
>>> or make the segments variable-sized and grow them accordingly.
>>
>> That sounds a lot like the perfect application for XArray. :)
>
> Haha I didn't know this already existed in the kernel :) Yeah then we
> should make XArray work for this use-case.

Ah wait, I meant for `SegmentedVec<T>` to store multiple `T` in the same
allocation, so it would only grow sometimes and amortize the allocations
just like `Vec`. It seems to me that XArray only stores pointers, so you
have to box everything (which we're trying to avoid IIUC).

---
Cheers,
Benno

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 10:09:10PM +0200, Benno Lossin wrote:
> On Tue Jul 1, 2025 at 10:03 PM CEST, Benno Lossin wrote:
> > On Tue Jul 1, 2025 at 9:58 PM CEST, Danilo Krummrich wrote:
> >> On 7/1/25 9:46 PM, Benno Lossin wrote:
> >>> On Tue Jul 1, 2025 at 9:21 PM CEST, Danilo Krummrich wrote:
> >>>> On Tue, Jul 01, 2025 at 11:11:13AM -0700, Matthew Maurer wrote:
> >>>>> If we implement *only* pinned files, we run into an additional problem
> >>>>> - you can't easily extend a pinned vector. This means that you cannot
> >>>>> have dynamically created devices unless you're willing to put every
> >>>>> new `File` into its own `Box`, because you aren't allowed to move any
> >>>>> of the previously allocated `File`s for a resize.
> >>>>>
> >>>>> Where previously you would have had
> >>>>>
> >>>>> ```
> >>>>> debug_files: Vec<File>
> >>>>> ```
> >>>>>
> >>>>> you would now have
> >>>>>
> >>>>> ```
> >>>>> debug_files: Vec<PinBox<File<T>>>
> >>>>> ```
> >>>>
> >>>> Stuffing single File instances into a Vec seems like the wrong thing to do.
> >>>>
> >>>> Instead you may have instances of some data structure that is created
> >>>> dynamically in your driver that you want to expose through debugfs.
> >>>>
> >>>> Let's say you have (userspace) clients that can be registered arbitrarily, then
> >>>> you want a Vec<Client>, which contains the client instances. In order to provide
> >>>> information about the Client in debugfs you then have the client embed things as
> >>>> discussed above.
> >>>>
> >>>> 	struct Client {
> >>>> 	   id: File<ClientId>,
> >>>> 	   data: File<ClientData>,
> >>>> 	   ...
> >>>> 	}
> >>>>
> >>>> I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a
> >>>> Vec<File> separately. Also, note that with the above, your Client instances
> >>>> don't need to be reference counted anymore.
> >>>>
> >>>> I think this addresses the concerns below.
> >>> 
> >>> You still have the issue that `Client` now needs to be pinned and the
> >>> vector can't be resized. But if you know that it's bounded, then we
> >>> could just make `Pin<Vec<T>>` work as expected (not relocating the
> >>> underlying allocation by not exposing `push`, only
> >>> `push_within_capacity`).
> >>> 
> >>> We also could have a `SegmentedVec<T>` that doesn't move elements.
> >>> Essentially it is
> >>>      
> >>>      enum SegmentedVec<T> {
> >>>          Cons(Segment<T>, KBox<SegmentedVec<T>>)
> >>>          Nul,
> >>>      }
> >>> 
> >>>      struct Segment<T> {
> >>>          elements: [T; 16]
> >>>      }
> >>> 
> >>> or make the segments variable-sized and grow them accordingly.
> >>
> >> That sounds a lot like the perfect application for XArray. :)
> >
> > Haha I didn't know this already existed in the kernel :) Yeah then we
> > should make XArray work for this use-case.
> 
> Ah wait, I meant for `SegmentedVec<T>` to store multiple `T` in the same
> allocation, so it would only grow sometimes and amortize the allocations
> just like `Vec`. It seems to me that XArray only stores pointers, so you
> have to box everything (which we're trying to avoid IIUC).

Yes, that sounds good. And if the potential number of Client instances can get
pretty large Vec isn't a good choice anyways. If we really get a ton of clients,
they should be allocated with a kmem_cache and stored in an XArray, maple tree,
etc.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Matthew Maurer]

On Tue, Jul 1, 2025 at 1:16 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue, Jul 01, 2025 at 10:09:10PM +0200, Benno Lossin wrote:
> > On Tue Jul 1, 2025 at 10:03 PM CEST, Benno Lossin wrote:
> > > On Tue Jul 1, 2025 at 9:58 PM CEST, Danilo Krummrich wrote:
> > >> On 7/1/25 9:46 PM, Benno Lossin wrote:
> > >>> On Tue Jul 1, 2025 at 9:21 PM CEST, Danilo Krummrich wrote:
> > >>>> On Tue, Jul 01, 2025 at 11:11:13AM -0700, Matthew Maurer wrote:
> > >>>>> If we implement *only* pinned files, we run into an additional problem
> > >>>>> - you can't easily extend a pinned vector. This means that you cannot
> > >>>>> have dynamically created devices unless you're willing to put every
> > >>>>> new `File` into its own `Box`, because you aren't allowed to move any
> > >>>>> of the previously allocated `File`s for a resize.
> > >>>>>
> > >>>>> Where previously you would have had
> > >>>>>
> > >>>>> ```
> > >>>>> debug_files: Vec<File>
> > >>>>> ```
> > >>>>>
> > >>>>> you would now have
> > >>>>>
> > >>>>> ```
> > >>>>> debug_files: Vec<PinBox<File<T>>>
> > >>>>> ```
> > >>>>
> > >>>> Stuffing single File instances into a Vec seems like the wrong thing to do.
> > >>>>
> > >>>> Instead you may have instances of some data structure that is created
> > >>>> dynamically in your driver that you want to expose through debugfs.
> > >>>>
> > >>>> Let's say you have (userspace) clients that can be registered arbitrarily, then
> > >>>> you want a Vec<Client>, which contains the client instances. In order to provide
> > >>>> information about the Client in debugfs you then have the client embed things as
> > >>>> discussed above.
> > >>>>
> > >>>>  struct Client {
> > >>>>     id: File<ClientId>,
> > >>>>     data: File<ClientData>,
> > >>>>     ...
> > >>>>  }
> > >>>>
> > >>>> I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a
> > >>>> Vec<File> separately. Also, note that with the above, your Client instances
> > >>>> don't need to be reference counted anymore.
> > >>>>
> > >>>> I think this addresses the concerns below.
> > >>>
> > >>> You still have the issue that `Client` now needs to be pinned and the
> > >>> vector can't be resized. But if you know that it's bounded, then we
> > >>> could just make `Pin<Vec<T>>` work as expected (not relocating the
> > >>> underlying allocation by not exposing `push`, only
> > >>> `push_within_capacity`).
> > >>>
> > >>> We also could have a `SegmentedVec<T>` that doesn't move elements.
> > >>> Essentially it is
> > >>>
> > >>>      enum SegmentedVec<T> {
> > >>>          Cons(Segment<T>, KBox<SegmentedVec<T>>)
> > >>>          Nul,
> > >>>      }
> > >>>
> > >>>      struct Segment<T> {
> > >>>          elements: [T; 16]
> > >>>      }
> > >>>
> > >>> or make the segments variable-sized and grow them accordingly.
> > >>
> > >> That sounds a lot like the perfect application for XArray. :)
> > >
> > > Haha I didn't know this already existed in the kernel :) Yeah then we
> > > should make XArray work for this use-case.
> >
> > Ah wait, I meant for `SegmentedVec<T>` to store multiple `T` in the same
> > allocation, so it would only grow sometimes and amortize the allocations
> > just like `Vec`. It seems to me that XArray only stores pointers, so you
> > have to box everything (which we're trying to avoid IIUC).
>
> Yes, that sounds good. And if the potential number of Client instances can get
> pretty large Vec isn't a good choice anyways. If we really get a ton of clients,
> they should be allocated with a kmem_cache and stored in an XArray, maple tree,
> etc.

OK. I've outlined why I disagree, but it sounds like consensus here is that:
1. You want a `File<T>` that implements `Deref` to expose a `&T`.
2. To enable `T` to take on values which are not `ForeignOwnable`, you
want me to make `create_file` return a `PinInit<File<T>>` (We will
also grow a `T: Sized` bound here.)

You are aware that:
1. A `File` can no longer be placed in a normal Rust collection, you
will either need to box it or use a special collection kind.
2. Adding or removing DebugFS support for some data in a driver may
cause more noise (code edits non-local to the debug logic) than
expected.

The one thing I still don't see a consensus on:

What do you want me to do about standard traits for `File`? If we're
intending it to be heavily used throughout structs, we'll effectively
break `#[derive]` if I don't add support. For example, if we had
```
#[derive(Debug, PartialEq)]
struct FooAttrs {
  name: String,
  size: usize,
}
// In my current patch world someone adds `File`, it's by putting
`FooAttrs` into an `Arc` and passing that into `create_file`, no
modifications to the structure are made.
```
before, we have an obvious semantics for this. If someone adds `File`
with the new API, we get
```
#[derive(Debug, PartialEq)]
#[pin_data]
struct FooAttrs {
  #[pin]
  name: File<String>,
  #[pin]
  size: File<usize>,
}
```

This means that for the `#[derive]` to keep working, `File` needs to
implement these traits. Do we want it to:
A. Purely delegate, so making this sort of change keeps the same
behavior of derived traits
B. Do what makes sense for a `File`, so e.g. Debug might print 'File {
data: "name_of_foo" }', and PartialEq might test for equality of the
dentry if not an error

I'm guessing you want A, but I've been through so many API reworks at
this point that I wanted to ask in case there's  disagreement here.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 02:53:52PM -0700, Matthew Maurer wrote:
> On Tue, Jul 1, 2025 at 1:16 PM Danilo Krummrich <dakr@kernel.org> wrote:
> > On Tue, Jul 01, 2025 at 10:09:10PM +0200, Benno Lossin wrote:
> > > On Tue Jul 1, 2025 at 10:03 PM CEST, Benno Lossin wrote:
> > > > On Tue Jul 1, 2025 at 9:58 PM CEST, Danilo Krummrich wrote:
> > > >> On 7/1/25 9:46 PM, Benno Lossin wrote:
> > > >>> On Tue Jul 1, 2025 at 9:21 PM CEST, Danilo Krummrich wrote:
> > > >>>> On Tue, Jul 01, 2025 at 11:11:13AM -0700, Matthew Maurer wrote:
> > > >>>>> If we implement *only* pinned files, we run into an additional problem
> > > >>>>> - you can't easily extend a pinned vector. This means that you cannot
> > > >>>>> have dynamically created devices unless you're willing to put every
> > > >>>>> new `File` into its own `Box`, because you aren't allowed to move any
> > > >>>>> of the previously allocated `File`s for a resize.
> > > >>>>>
> > > >>>>> Where previously you would have had
> > > >>>>>
> > > >>>>> ```
> > > >>>>> debug_files: Vec<File>
> > > >>>>> ```
> > > >>>>>
> > > >>>>> you would now have
> > > >>>>>
> > > >>>>> ```
> > > >>>>> debug_files: Vec<PinBox<File<T>>>
> > > >>>>> ```
> > > >>>>
> > > >>>> Stuffing single File instances into a Vec seems like the wrong thing to do.
> > > >>>>
> > > >>>> Instead you may have instances of some data structure that is created
> > > >>>> dynamically in your driver that you want to expose through debugfs.
> > > >>>>
> > > >>>> Let's say you have (userspace) clients that can be registered arbitrarily, then
> > > >>>> you want a Vec<Client>, which contains the client instances. In order to provide
> > > >>>> information about the Client in debugfs you then have the client embed things as
> > > >>>> discussed above.
> > > >>>>
> > > >>>>  struct Client {
> > > >>>>     id: File<ClientId>,
> > > >>>>     data: File<ClientData>,
> > > >>>>     ...
> > > >>>>  }
> > > >>>>
> > > >>>> I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a
> > > >>>> Vec<File> separately. Also, note that with the above, your Client instances
> > > >>>> don't need to be reference counted anymore.
> > > >>>>
> > > >>>> I think this addresses the concerns below.
> > > >>>
> > > >>> You still have the issue that `Client` now needs to be pinned and the
> > > >>> vector can't be resized. But if you know that it's bounded, then we
> > > >>> could just make `Pin<Vec<T>>` work as expected (not relocating the
> > > >>> underlying allocation by not exposing `push`, only
> > > >>> `push_within_capacity`).
> > > >>>
> > > >>> We also could have a `SegmentedVec<T>` that doesn't move elements.
> > > >>> Essentially it is
> > > >>>
> > > >>>      enum SegmentedVec<T> {
> > > >>>          Cons(Segment<T>, KBox<SegmentedVec<T>>)
> > > >>>          Nul,
> > > >>>      }
> > > >>>
> > > >>>      struct Segment<T> {
> > > >>>          elements: [T; 16]
> > > >>>      }
> > > >>>
> > > >>> or make the segments variable-sized and grow them accordingly.
> > > >>
> > > >> That sounds a lot like the perfect application for XArray. :)
> > > >
> > > > Haha I didn't know this already existed in the kernel :) Yeah then we
> > > > should make XArray work for this use-case.
> > >
> > > Ah wait, I meant for `SegmentedVec<T>` to store multiple `T` in the same
> > > allocation, so it would only grow sometimes and amortize the allocations
> > > just like `Vec`. It seems to me that XArray only stores pointers, so you
> > > have to box everything (which we're trying to avoid IIUC).
> >
> > Yes, that sounds good. And if the potential number of Client instances can get
> > pretty large Vec isn't a good choice anyways. If we really get a ton of clients,
> > they should be allocated with a kmem_cache and stored in an XArray, maple tree,
> > etc.
> 
> OK. I've outlined why I disagree, but it sounds like consensus here is that:
> 1. You want a `File<T>` that implements `Deref` to expose a `&T`.
> 2. To enable `T` to take on values which are not `ForeignOwnable`, you
> want me to make `create_file` return a `PinInit<File<T>>` (We will
> also grow a `T: Sized` bound here.)
> 
> You are aware that:
> 1. A `File` can no longer be placed in a normal Rust collection, you
> will either need to box it or use a special collection kind.

I don't see why we would want to do that. I can see why we would want to keep a
Vec of some "payload" structure, e.g. some Client structure. But even then,
using a simple resizable array is rarely a good choice.

IMHO there is just no point in optimizing for this case.

If there is anything specific you have in mind, it would be good to share what
that is.

In drivers we have per device allocations, e.g. the driver's private data, the
private data of a class device registration, the private data of a work items,
the private data if an IRQ, etc.

And for cases where we create instances of some struct dynamically, we usually
just go with a new allocation for each new instance and store it in lists, tree
structures, XArray, etc.

Especially, if there can be lots of instances of the same structure, you don't
want to re-allocate all the time and copy things around, but use a kmem_cache
instead.

> 2. Adding or removing DebugFS support for some data in a driver may
> cause more noise (code edits non-local to the debug logic) than
> expected.

I don't agree with that -- forcing a reference count for something that doesn't
need to be reference counted is clearly more "noise" that a few trivial
changes, such as:

- foo: Foo,
+ foo: debugfs::File<Foo>,

> The one thing I still don't see a consensus on:
> 
> What do you want me to do about standard traits for `File`? If we're
> intending it to be heavily used throughout structs, we'll effectively
> break `#[derive]` if I don't add support. For example, if we had
> ```
> #[derive(Debug, PartialEq)]
> struct FooAttrs {
>   name: String,
>   size: usize,
> }
> // In my current patch world someone adds `File`, it's by putting
> `FooAttrs` into an `Arc` and passing that into `create_file`, no
> modifications to the structure are made.
> ```
> before, we have an obvious semantics for this. If someone adds `File`
> with the new API, we get
> ```
> #[derive(Debug, PartialEq)]
> #[pin_data]
> struct FooAttrs {
>   #[pin]
>   name: File<String>,
>   #[pin]
>   size: File<usize>,
> }
> ```
> 
> This means that for the `#[derive]` to keep working, `File` needs to
> implement these traits. Do we want it to:
> A. Purely delegate, so making this sort of change keeps the same
> behavior of derived traits
> B. Do what makes sense for a `File`, so e.g. Debug might print 'File {
> data: "name_of_foo" }', and PartialEq might test for equality of the
> dentry if not an error
> 
> I'm guessing you want A, but I've been through so many API reworks at
> this point that I wanted to ask in case there's  disagreement here.

I understand that -- and thanks for your work on this, I really appreciate it!

Regarding the question: Yes, I'm clearly all for A, I don't think there's much
value in B.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Benno Lossin]

On Tue Jul 1, 2025 at 5:10 PM CEST, Danilo Krummrich wrote:
> I think we can add an additional API like this:
>
> 	impl Dir {
> 	   pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
> 	      pin_init!(Self {
> 	         data <- data,
> 	         ...
> 	      })
> 	   }
> 	}
>
> This allows us to do things like:
>
> 	#[pin_data]
> 	struct Firmware {
> 	   #[pin]
> 	   minor: debugfs::File<u32>,
> 	   #[pin]
> 	   major: debugfs::File<u32>,
> 	   #[pin]
> 	   buffer: debugfs::File<[u8]>,
> 	}
>
> 	impl Firmware {
> 	   pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
> 	      pin_init!(Self {
> 	         minor <- dir.create_file("minor", 1),
> 	         major <- dir.create_file("major", 2),
> 	         buffer <- dir.create_file("buffer", buffer),
> 	      })
> 	   }
> 	}
>
> 	// This is the only allocation we need.
> 	let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
>
> With this everything is now in a single allocation and since we're using
> pin-init, Dir::create_file() can safely store pointers of the corresponding data
> in debugfs_create_file(), since this structure is guaranteed to be pinned in
> memory.

Yes! This is a textbook example of how to use pin-init in API design!

> Actually, we can also implement *only this*, since with this my previous example
> would just become this:
>
> 	struct GPU {
> 	   fw: debugfs::File<Firmware>,
> 	}
>
> 	let file = dir.create_file("firmware", Firmware::new());
> 	let file = KBox::pin_init(file, GFP_KERNEL)?;
>
> 	// debugfs::File<Firmware> dereferences to Firmware
> 	file.do_something();
>
> Given that, I think we should change things to use pin-init right away for the
> debugfs::File API.

Yeah I think it's a good idea to do that too.

---
Cheers,
Benno

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Alice Ryhl]

On Tue, Jul 01, 2025 at 05:10:47PM +0200, Danilo Krummrich wrote:
> On Tue, Jul 01, 2025 at 04:21:56PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jul 01, 2025 at 04:13:28PM +0200, Danilo Krummrich wrote:
> > > Instead this should just be:
> > > 
> > > 	struct GPU {
> > > 	   fw: debugfs::File<Firmware>,
> > > 	}
> > > 
> > > and then I would initialize it the following way:
> > > 
> > > 	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
> > > 	let file = dir.create_file("firmware", fw);
> > > 
> > > 	// debugfs::File<Firmware> dereferences to Firmware
> > > 	file.do_something();
> > > 
> > > 	// Access to fw is prevented by the compiler, since it has been moved
> > > 	// into file.
> > > 
> > > This is much better, since now I have the guarantee that my Firmare instance
> > > can't out-live the GPU instance.
> > 
> > That's better, yes, but how would multiple files for the same
> > "structure" work here?  Like a debugfs-file-per-field of a structure
> > that we often have?
> 
> That is a very good question and I thought about this as well, because with only
> the current API this would require us to have more and more dynamic allocations
> if we want to have a more fine grained filesystem representations of structures.
> 
> The idea I have for this is to use pin-init, which we do in quite some other
> places as well.
> 
> I think we can add an additional API like this:
> 
> 	impl Dir {
> 	   pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
> 	      pin_init!(Self {
> 	         data <- data,
> 	         ...
> 	      })
> 	   }
> 	}
> 
> This allows us to do things like:
> 
> 	#[pin_data]
> 	struct Firmware {
> 	   #[pin]
> 	   minor: debugfs::File<u32>,
> 	   #[pin]
> 	   major: debugfs::File<u32>,
> 	   #[pin]
> 	   buffer: debugfs::File<[u8]>,
> 	}
> 
> 	impl Firmware {
> 	   pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
> 	      pin_init!(Self {
> 	         minor <- dir.create_file("minor", 1),
> 	         major <- dir.create_file("major", 2),
> 	         buffer <- dir.create_file("buffer", buffer),
> 	      })
> 	   }
> 	}
> 
> 	// This is the only allocation we need.
> 	let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
> 
> With this everything is now in a single allocation and since we're using
> pin-init, Dir::create_file() can safely store pointers of the corresponding data
> in debugfs_create_file(), since this structure is guaranteed to be pinned in
> memory.
> 
> Actually, we can also implement *only this*, since with this my previous example
> would just become this:
> 
> 	struct GPU {
> 	   fw: debugfs::File<Firmware>,
> 	}
> 
> 	let file = dir.create_file("firmware", Firmware::new());
> 	let file = KBox::pin_init(file, GFP_KERNEL)?;
> 
> 	// debugfs::File<Firmware> dereferences to Firmware
> 	file.do_something();
> 
> Given that, I think we should change things to use pin-init right away for the
> debugfs::File API.

Does this actually work in practice for anything except immutable data?
I mean, let's take Rust Binder as an example and lets say that I want to
expose a directory for each Process object with some of the fields
exposed. Let's just simplify Rust Binder a bit and only include some of
the fields:

#[pin_data]
struct Process {
    task: ARef<Task>,
    #[pin]
    inner: SpinLock<ProcessInner>,
}

pub(crate) struct ProcessInner {
    threads: RBTree<i32, Arc<Thread>>,
    nodes: RBTree<u64, DArc<Node>>,
    requested_thread_count: u32,
    max_threads: u32,
    started_thread_count: u32,
}

Rust Binder already does expose some debugging data through a file
system, though it doesn't do so using debugfs. It exposes a lot of data,
but among them are the pid, the number of threads and nodes, as well as
the values of requested_thread_count, started_thread_count, and
max_threads.

Now, we run into problem number one: pinning is not supported inside
mutexes. But let's say we solved that and we could do this:

#[pin_data]
struct Process {
    task: File<ARef<Task>>, // prints the pid
    #[pin]
    inner: SpinLock<ProcessInner>,
}

pub(crate) struct ProcessInner {
    threads: File<RBTree<i32, Arc<Thread>>>, // prints the count
    nodes: File<RBTree<u64, DArc<Node>>>, // prints the count
    requested_thread_count: File<u32>,
    max_threads: File<u32>,
    started_thread_count: File<u32>,
}

However, this still doesn't work! Debugfs may get triggered at any time
and need to read these fields, and there's no way for it to take the
spinlock with the above design - it doesn't know where the spinlock is.
For the integers I guess we could make them atomic to allow reading them
in parallel with mutation, but that option is not available for the
red/black trees.

What is the intended solution in this case? If the argument is that this
is a rare case, then keep in mind that this is a real-world example of
debugging information that we actually expose today in a real driver.
With Matt's current approach, it's relatively easy - just store a bunch
of File<Arc<Process>> instances somewhere and define each one to take
the mutex and print the relevant value.

Alice

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Benno Lossin]

On Thu Jul 3, 2025 at 12:02 PM CEST, Alice Ryhl wrote:
> On Tue, Jul 01, 2025 at 05:10:47PM +0200, Danilo Krummrich wrote:
>> On Tue, Jul 01, 2025 at 04:21:56PM +0200, Greg Kroah-Hartman wrote:
>> > On Tue, Jul 01, 2025 at 04:13:28PM +0200, Danilo Krummrich wrote:
>> > > Instead this should just be:
>> > > 
>> > > 	struct GPU {
>> > > 	   fw: debugfs::File<Firmware>,
>> > > 	}
>> > > 
>> > > and then I would initialize it the following way:
>> > > 
>> > > 	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
>> > > 	let file = dir.create_file("firmware", fw);
>> > > 
>> > > 	// debugfs::File<Firmware> dereferences to Firmware
>> > > 	file.do_something();
>> > > 
>> > > 	// Access to fw is prevented by the compiler, since it has been moved
>> > > 	// into file.
>> > > 
>> > > This is much better, since now I have the guarantee that my Firmare instance
>> > > can't out-live the GPU instance.
>> > 
>> > That's better, yes, but how would multiple files for the same
>> > "structure" work here?  Like a debugfs-file-per-field of a structure
>> > that we often have?
>> 
>> That is a very good question and I thought about this as well, because with only
>> the current API this would require us to have more and more dynamic allocations
>> if we want to have a more fine grained filesystem representations of structures.
>> 
>> The idea I have for this is to use pin-init, which we do in quite some other
>> places as well.
>> 
>> I think we can add an additional API like this:
>> 
>> 	impl Dir {
>> 	   pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
>> 	      pin_init!(Self {
>> 	         data <- data,
>> 	         ...
>> 	      })
>> 	   }
>> 	}
>> 
>> This allows us to do things like:
>> 
>> 	#[pin_data]
>> 	struct Firmware {
>> 	   #[pin]
>> 	   minor: debugfs::File<u32>,
>> 	   #[pin]
>> 	   major: debugfs::File<u32>,
>> 	   #[pin]
>> 	   buffer: debugfs::File<[u8]>,
>> 	}
>> 
>> 	impl Firmware {
>> 	   pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
>> 	      pin_init!(Self {
>> 	         minor <- dir.create_file("minor", 1),
>> 	         major <- dir.create_file("major", 2),
>> 	         buffer <- dir.create_file("buffer", buffer),
>> 	      })
>> 	   }
>> 	}
>> 
>> 	// This is the only allocation we need.
>> 	let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
>> 
>> With this everything is now in a single allocation and since we're using
>> pin-init, Dir::create_file() can safely store pointers of the corresponding data
>> in debugfs_create_file(), since this structure is guaranteed to be pinned in
>> memory.
>> 
>> Actually, we can also implement *only this*, since with this my previous example
>> would just become this:
>> 
>> 	struct GPU {
>> 	   fw: debugfs::File<Firmware>,
>> 	}
>> 
>> 	let file = dir.create_file("firmware", Firmware::new());
>> 	let file = KBox::pin_init(file, GFP_KERNEL)?;
>> 
>> 	// debugfs::File<Firmware> dereferences to Firmware
>> 	file.do_something();
>> 
>> Given that, I think we should change things to use pin-init right away for the
>> debugfs::File API.
>
> Does this actually work in practice for anything except immutable data?
> I mean, let's take Rust Binder as an example and lets say that I want to
> expose a directory for each Process object with some of the fields
> exposed. Let's just simplify Rust Binder a bit and only include some of
> the fields:
>
> #[pin_data]
> struct Process {
>     task: ARef<Task>,
>     #[pin]
>     inner: SpinLock<ProcessInner>,
> }
>
> pub(crate) struct ProcessInner {
>     threads: RBTree<i32, Arc<Thread>>,
>     nodes: RBTree<u64, DArc<Node>>,
>     requested_thread_count: u32,
>     max_threads: u32,
>     started_thread_count: u32,
> }
>
> Rust Binder already does expose some debugging data through a file
> system, though it doesn't do so using debugfs. It exposes a lot of data,
> but among them are the pid, the number of threads and nodes, as well as
> the values of requested_thread_count, started_thread_count, and
> max_threads.
>
> Now, we run into problem number one: pinning is not supported inside
> mutexes. But let's say we solved that and we could do this:
>
> #[pin_data]
> struct Process {
>     task: File<ARef<Task>>, // prints the pid
>     #[pin]
>     inner: SpinLock<ProcessInner>,
> }
>
> pub(crate) struct ProcessInner {
>     threads: File<RBTree<i32, Arc<Thread>>>, // prints the count
>     nodes: File<RBTree<u64, DArc<Node>>>, // prints the count
>     requested_thread_count: File<u32>,
>     max_threads: File<u32>,
>     started_thread_count: File<u32>,
> }
>
> However, this still doesn't work! Debugfs may get triggered at any time
> and need to read these fields, and there's no way for it to take the
> spinlock with the above design - it doesn't know where the spinlock is.
> For the integers I guess we could make them atomic to allow reading them
> in parallel with mutation, but that option is not available for the
> red/black trees.
>
> What is the intended solution in this case? If the argument is that this
> is a rare case, then keep in mind that this is a real-world example of
> debugging information that we actually expose today in a real driver.
> With Matt's current approach, it's relatively easy - just store a bunch
> of File<Arc<Process>> instances somewhere and define each one to take
> the mutex and print the relevant value.

How would your example look like with the current approach? IIUC, it
also wouldn't work, because the debugfs data can't be mutated?

---
Cheers,
Benno

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Alice Ryhl]

On Thu, Jul 3, 2025 at 12:33 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Thu Jul 3, 2025 at 12:02 PM CEST, Alice Ryhl wrote:
> > On Tue, Jul 01, 2025 at 05:10:47PM +0200, Danilo Krummrich wrote:
> >> On Tue, Jul 01, 2025 at 04:21:56PM +0200, Greg Kroah-Hartman wrote:
> >> > On Tue, Jul 01, 2025 at 04:13:28PM +0200, Danilo Krummrich wrote:
> >> > > Instead this should just be:
> >> > >
> >> > >  struct GPU {
> >> > >     fw: debugfs::File<Firmware>,
> >> > >  }
> >> > >
> >> > > and then I would initialize it the following way:
> >> > >
> >> > >  let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
> >> > >  let file = dir.create_file("firmware", fw);
> >> > >
> >> > >  // debugfs::File<Firmware> dereferences to Firmware
> >> > >  file.do_something();
> >> > >
> >> > >  // Access to fw is prevented by the compiler, since it has been moved
> >> > >  // into file.
> >> > >
> >> > > This is much better, since now I have the guarantee that my Firmare instance
> >> > > can't out-live the GPU instance.
> >> >
> >> > That's better, yes, but how would multiple files for the same
> >> > "structure" work here?  Like a debugfs-file-per-field of a structure
> >> > that we often have?
> >>
> >> That is a very good question and I thought about this as well, because with only
> >> the current API this would require us to have more and more dynamic allocations
> >> if we want to have a more fine grained filesystem representations of structures.
> >>
> >> The idea I have for this is to use pin-init, which we do in quite some other
> >> places as well.
> >>
> >> I think we can add an additional API like this:
> >>
> >>      impl Dir {
> >>         pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
> >>            pin_init!(Self {
> >>               data <- data,
> >>               ...
> >>            })
> >>         }
> >>      }
> >>
> >> This allows us to do things like:
> >>
> >>      #[pin_data]
> >>      struct Firmware {
> >>         #[pin]
> >>         minor: debugfs::File<u32>,
> >>         #[pin]
> >>         major: debugfs::File<u32>,
> >>         #[pin]
> >>         buffer: debugfs::File<[u8]>,
> >>      }
> >>
> >>      impl Firmware {
> >>         pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
> >>            pin_init!(Self {
> >>               minor <- dir.create_file("minor", 1),
> >>               major <- dir.create_file("major", 2),
> >>               buffer <- dir.create_file("buffer", buffer),
> >>            })
> >>         }
> >>      }
> >>
> >>      // This is the only allocation we need.
> >>      let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
> >>
> >> With this everything is now in a single allocation and since we're using
> >> pin-init, Dir::create_file() can safely store pointers of the corresponding data
> >> in debugfs_create_file(), since this structure is guaranteed to be pinned in
> >> memory.
> >>
> >> Actually, we can also implement *only this*, since with this my previous example
> >> would just become this:
> >>
> >>      struct GPU {
> >>         fw: debugfs::File<Firmware>,
> >>      }
> >>
> >>      let file = dir.create_file("firmware", Firmware::new());
> >>      let file = KBox::pin_init(file, GFP_KERNEL)?;
> >>
> >>      // debugfs::File<Firmware> dereferences to Firmware
> >>      file.do_something();
> >>
> >> Given that, I think we should change things to use pin-init right away for the
> >> debugfs::File API.
> >
> > Does this actually work in practice for anything except immutable data?
> > I mean, let's take Rust Binder as an example and lets say that I want to
> > expose a directory for each Process object with some of the fields
> > exposed. Let's just simplify Rust Binder a bit and only include some of
> > the fields:
> >
> > #[pin_data]
> > struct Process {
> >     task: ARef<Task>,
> >     #[pin]
> >     inner: SpinLock<ProcessInner>,
> > }
> >
> > pub(crate) struct ProcessInner {
> >     threads: RBTree<i32, Arc<Thread>>,
> >     nodes: RBTree<u64, DArc<Node>>,
> >     requested_thread_count: u32,
> >     max_threads: u32,
> >     started_thread_count: u32,
> > }
> >
> > Rust Binder already does expose some debugging data through a file
> > system, though it doesn't do so using debugfs. It exposes a lot of data,
> > but among them are the pid, the number of threads and nodes, as well as
> > the values of requested_thread_count, started_thread_count, and
> > max_threads.
> >
> > Now, we run into problem number one: pinning is not supported inside
> > mutexes. But let's say we solved that and we could do this:
> >
> > #[pin_data]
> > struct Process {
> >     task: File<ARef<Task>>, // prints the pid
> >     #[pin]
> >     inner: SpinLock<ProcessInner>,
> > }
> >
> > pub(crate) struct ProcessInner {
> >     threads: File<RBTree<i32, Arc<Thread>>>, // prints the count
> >     nodes: File<RBTree<u64, DArc<Node>>>, // prints the count
> >     requested_thread_count: File<u32>,
> >     max_threads: File<u32>,
> >     started_thread_count: File<u32>,
> > }
> >
> > However, this still doesn't work! Debugfs may get triggered at any time
> > and need to read these fields, and there's no way for it to take the
> > spinlock with the above design - it doesn't know where the spinlock is.
> > For the integers I guess we could make them atomic to allow reading them
> > in parallel with mutation, but that option is not available for the
> > red/black trees.
> >
> > What is the intended solution in this case? If the argument is that this
> > is a rare case, then keep in mind that this is a real-world example of
> > debugging information that we actually expose today in a real driver.
> > With Matt's current approach, it's relatively easy - just store a bunch
> > of File<Arc<Process>> instances somewhere and define each one to take
> > the mutex and print the relevant value.
>
> How would your example look like with the current approach? IIUC, it
> also wouldn't work, because the debugfs data can't be mutated?

I would store a bunch of `File<Arc<Process>>` instances somewhere.
Each one has a closure that takes the spinlock and prints the
appropriate value.

Alice

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Greg Kroah-Hartman]

On Thu, Jul 03, 2025 at 12:54:18PM +0200, Alice Ryhl wrote:
> On Thu, Jul 3, 2025 at 12:33 PM Benno Lossin <lossin@kernel.org> wrote:
> >
> > On Thu Jul 3, 2025 at 12:02 PM CEST, Alice Ryhl wrote:
> > > On Tue, Jul 01, 2025 at 05:10:47PM +0200, Danilo Krummrich wrote:
> > >> On Tue, Jul 01, 2025 at 04:21:56PM +0200, Greg Kroah-Hartman wrote:
> > >> > On Tue, Jul 01, 2025 at 04:13:28PM +0200, Danilo Krummrich wrote:
> > >> > > Instead this should just be:
> > >> > >
> > >> > >  struct GPU {
> > >> > >     fw: debugfs::File<Firmware>,
> > >> > >  }
> > >> > >
> > >> > > and then I would initialize it the following way:
> > >> > >
> > >> > >  let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
> > >> > >  let file = dir.create_file("firmware", fw);
> > >> > >
> > >> > >  // debugfs::File<Firmware> dereferences to Firmware
> > >> > >  file.do_something();
> > >> > >
> > >> > >  // Access to fw is prevented by the compiler, since it has been moved
> > >> > >  // into file.
> > >> > >
> > >> > > This is much better, since now I have the guarantee that my Firmare instance
> > >> > > can't out-live the GPU instance.
> > >> >
> > >> > That's better, yes, but how would multiple files for the same
> > >> > "structure" work here?  Like a debugfs-file-per-field of a structure
> > >> > that we often have?
> > >>
> > >> That is a very good question and I thought about this as well, because with only
> > >> the current API this would require us to have more and more dynamic allocations
> > >> if we want to have a more fine grained filesystem representations of structures.
> > >>
> > >> The idea I have for this is to use pin-init, which we do in quite some other
> > >> places as well.
> > >>
> > >> I think we can add an additional API like this:
> > >>
> > >>      impl Dir {
> > >>         pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
> > >>            pin_init!(Self {
> > >>               data <- data,
> > >>               ...
> > >>            })
> > >>         }
> > >>      }
> > >>
> > >> This allows us to do things like:
> > >>
> > >>      #[pin_data]
> > >>      struct Firmware {
> > >>         #[pin]
> > >>         minor: debugfs::File<u32>,
> > >>         #[pin]
> > >>         major: debugfs::File<u32>,
> > >>         #[pin]
> > >>         buffer: debugfs::File<[u8]>,
> > >>      }
> > >>
> > >>      impl Firmware {
> > >>         pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
> > >>            pin_init!(Self {
> > >>               minor <- dir.create_file("minor", 1),
> > >>               major <- dir.create_file("major", 2),
> > >>               buffer <- dir.create_file("buffer", buffer),
> > >>            })
> > >>         }
> > >>      }
> > >>
> > >>      // This is the only allocation we need.
> > >>      let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
> > >>
> > >> With this everything is now in a single allocation and since we're using
> > >> pin-init, Dir::create_file() can safely store pointers of the corresponding data
> > >> in debugfs_create_file(), since this structure is guaranteed to be pinned in
> > >> memory.
> > >>
> > >> Actually, we can also implement *only this*, since with this my previous example
> > >> would just become this:
> > >>
> > >>      struct GPU {
> > >>         fw: debugfs::File<Firmware>,
> > >>      }
> > >>
> > >>      let file = dir.create_file("firmware", Firmware::new());
> > >>      let file = KBox::pin_init(file, GFP_KERNEL)?;
> > >>
> > >>      // debugfs::File<Firmware> dereferences to Firmware
> > >>      file.do_something();
> > >>
> > >> Given that, I think we should change things to use pin-init right away for the
> > >> debugfs::File API.
> > >
> > > Does this actually work in practice for anything except immutable data?
> > > I mean, let's take Rust Binder as an example and lets say that I want to
> > > expose a directory for each Process object with some of the fields
> > > exposed. Let's just simplify Rust Binder a bit and only include some of
> > > the fields:
> > >
> > > #[pin_data]
> > > struct Process {
> > >     task: ARef<Task>,
> > >     #[pin]
> > >     inner: SpinLock<ProcessInner>,
> > > }
> > >
> > > pub(crate) struct ProcessInner {
> > >     threads: RBTree<i32, Arc<Thread>>,
> > >     nodes: RBTree<u64, DArc<Node>>,
> > >     requested_thread_count: u32,
> > >     max_threads: u32,
> > >     started_thread_count: u32,
> > > }
> > >
> > > Rust Binder already does expose some debugging data through a file
> > > system, though it doesn't do so using debugfs. It exposes a lot of data,
> > > but among them are the pid, the number of threads and nodes, as well as
> > > the values of requested_thread_count, started_thread_count, and
> > > max_threads.
> > >
> > > Now, we run into problem number one: pinning is not supported inside
> > > mutexes. But let's say we solved that and we could do this:
> > >
> > > #[pin_data]
> > > struct Process {
> > >     task: File<ARef<Task>>, // prints the pid
> > >     #[pin]
> > >     inner: SpinLock<ProcessInner>,
> > > }
> > >
> > > pub(crate) struct ProcessInner {
> > >     threads: File<RBTree<i32, Arc<Thread>>>, // prints the count
> > >     nodes: File<RBTree<u64, DArc<Node>>>, // prints the count
> > >     requested_thread_count: File<u32>,
> > >     max_threads: File<u32>,
> > >     started_thread_count: File<u32>,
> > > }
> > >
> > > However, this still doesn't work! Debugfs may get triggered at any time
> > > and need to read these fields, and there's no way for it to take the
> > > spinlock with the above design - it doesn't know where the spinlock is.
> > > For the integers I guess we could make them atomic to allow reading them
> > > in parallel with mutation, but that option is not available for the
> > > red/black trees.
> > >
> > > What is the intended solution in this case? If the argument is that this
> > > is a rare case, then keep in mind that this is a real-world example of
> > > debugging information that we actually expose today in a real driver.
> > > With Matt's current approach, it's relatively easy - just store a bunch
> > > of File<Arc<Process>> instances somewhere and define each one to take
> > > the mutex and print the relevant value.
> >
> > How would your example look like with the current approach? IIUC, it
> > also wouldn't work, because the debugfs data can't be mutated?
> 
> I would store a bunch of `File<Arc<Process>>` instances somewhere.
> Each one has a closure that takes the spinlock and prints the
> appropriate value.

Ok, I think we need to see some "real" examples here of the api in use
before figuring it out further as I'm totally confused :)

Yes, we need to be able to have a debugfs file callback handle a mutable
structure in order to lock things correctly.  We also need to have it be
mutable so that it can MODIFY the value (everyone seems to forget that
debugfs allows that...)

So how about a platform driver that exposes values read from a platform
device (i.e. a soc info driver), that also includes a
local-to-the-device data structure that can be locked and modified?
That should cover all the use cases that I can think of at the moment.

thanks,

greg k-h

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On Thu, Jul 03, 2025 at 01:41:53PM +0200, Greg Kroah-Hartman wrote:
> Yes, we need to be able to have a debugfs file callback handle a mutable
> structure in order to lock things correctly.  We also need to have it be
> mutable so that it can MODIFY the value (everyone seems to forget that
> debugfs allows that...)

Well, that's possible with both approaches. Data behind a lock becomes mutable
once you grabbed the lock. That's the same in both cases.

The difference is that with the pin-init approach I propose you can't have what
Alice sketched up. And I think it's even desirable that you can't do it.

Let's me explain the difference on a simplified example, based on Alice'
example.

ForeignOwnable API
------------------

	#[pin_data]
	struct Process {
	    task: ARef<Task>,
	    #[pin]
	    inner: SpinLock<ProcessInner>,
	}
	
	pub(crate) struct ProcessInner {
	    threads: RBTree<i32, Arc<Thread>>,
	    max_threads: u32,
	}

Here we have to create an Arc<Process> (let's call it process) and create files
from it.

	let file_threads = dir.create_file("threads", process);
	let file_max_threads = dir.create_file("max_threads", process);

In the file system callback of both of these, we now have an Arc<Process>, hence
we can access:

	let guard = process.inner.lock();

	read_or_write(guard.max_threads);

and in the other file:

	let guard = process.inner.lock();

	read_or_write(guard.max_threads);

Pin-Init API
------------

	#[pin_data]
	struct Process {
	    task: ARef<Task>,
	    #[pin]
	    inner: File<SpinLock<ProcessInner>>,
	}
	
	pub(crate) struct ProcessInner {
	    threads: RBTree<i32, Arc<Thread>>,
	    max_threads: u32,
	}

Here Process does not need to be within an Arc and no separate file instances
need to be kept around, that happens already within the constructor of Process:

	pin_init!(Process {
	   inner <- dir.create_file("process_inner", ...),
	   [...]
	})

The file itself has a reference to SpinLock<ProcessInner>, hence we can access:

	let guard = inner.lock();

	read_or_write(guard.threads)
	read_or_write(guard.max_threads)

The difference is that with the ForeignOwnable API it was possible to have
separate files for threads and max_threads.

While with the pin-init one we either have to have a single file exposing
ProcessInner (which is what I did above) or protect threads and max_threads
with separate locks (of course max_threads could also just be an atomic).

(If you like I can sketch up this case as well.)

At a first glance this seems like an undesirable limitation, but I argue that
this is a good thing.

The reason I think so is what I also explained in [1], but let me adjust it a
bit for this reply:

threads and max_threads being protected by the same lock means that they are in
a certain relationship to each other. Meaning that they only really make sense
looking at them atomically.

So I argue it does not make sense to expose those values to userspace through
separate files.

For instance:

	$ cat max_threads && cat threads
	$ 5
	$ 10

This way you may read 5 max_threads, but 10 actual threads, because things may
have changed in between the two cat commands.

However, if instead, they are exposed through a single file, we get an atomic
view of them, such that the semantic relationship between them is preserved.

For instance:

	$ cat process_info
	$ threads: 2
	$ max_threads: 10

So, what I'm trying to say is, I think it's a good thing if fields that are
protected by the same lock can't be exposed through separate files, because it
means that the fields only make sense in the context of each other.

Or saying it the other way around, if it makes sense to expose fields through
separate files, it means they're unrelated to each other and hence should be
protected with separate locks, rather than a common one.

IMHO it's even a good thing beyond the scope of debugfs, because it forces
developers to really think about organizing structures properly, e.g. in a way
that only fields that really belong behind a certain lock are placed behind this
lock.

> So how about a platform driver that exposes values read from a platform
> device (i.e. a soc info driver), that also includes a
> local-to-the-device data structure that can be locked and modified?
> That should cover all the use cases that I can think of at the moment.

Yes, I also really like to have that.

But, again, both approaches can do this. It's just that I really discourage the
one that forces us to have an Arc instance on structures exposed through
debugfs, since this messes with the driver's lifetime and ownership
architecture in a bad way.

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Greg Kroah-Hartman]

On Thu, Jul 03, 2025 at 02:29:31PM +0200, Danilo Krummrich wrote:
> On Thu, Jul 03, 2025 at 01:41:53PM +0200, Greg Kroah-Hartman wrote:
> > Yes, we need to be able to have a debugfs file callback handle a mutable
> > structure in order to lock things correctly.  We also need to have it be
> > mutable so that it can MODIFY the value (everyone seems to forget that
> > debugfs allows that...)
> 
> Well, that's possible with both approaches. Data behind a lock becomes mutable
> once you grabbed the lock. That's the same in both cases.
> 
> The difference is that with the pin-init approach I propose you can't have what
> Alice sketched up. And I think it's even desirable that you can't do it.
> 
> Let's me explain the difference on a simplified example, based on Alice'
> example.
> 
> ForeignOwnable API
> ------------------
> 
> 	#[pin_data]
> 	struct Process {
> 	    task: ARef<Task>,
> 	    #[pin]
> 	    inner: SpinLock<ProcessInner>,
> 	}
> 	
> 	pub(crate) struct ProcessInner {
> 	    threads: RBTree<i32, Arc<Thread>>,
> 	    max_threads: u32,
> 	}
> 
> Here we have to create an Arc<Process> (let's call it process) and create files
> from it.
> 
> 	let file_threads = dir.create_file("threads", process);
> 	let file_max_threads = dir.create_file("max_threads", process);
> 
> In the file system callback of both of these, we now have an Arc<Process>, hence
> we can access:
> 
> 	let guard = process.inner.lock();
> 
> 	read_or_write(guard.max_threads);
> 
> and in the other file:
> 
> 	let guard = process.inner.lock();
> 
> 	read_or_write(guard.max_threads);
> 
> Pin-Init API
> ------------
> 
> 	#[pin_data]
> 	struct Process {
> 	    task: ARef<Task>,
> 	    #[pin]
> 	    inner: File<SpinLock<ProcessInner>>,
> 	}
> 	
> 	pub(crate) struct ProcessInner {
> 	    threads: RBTree<i32, Arc<Thread>>,
> 	    max_threads: u32,
> 	}
> 
> Here Process does not need to be within an Arc and no separate file instances
> need to be kept around, that happens already within the constructor of Process:
> 
> 	pin_init!(Process {
> 	   inner <- dir.create_file("process_inner", ...),
> 	   [...]
> 	})
> 
> The file itself has a reference to SpinLock<ProcessInner>, hence we can access:
> 
> 	let guard = inner.lock();
> 
> 	read_or_write(guard.threads)
> 	read_or_write(guard.max_threads)
> 
> The difference is that with the ForeignOwnable API it was possible to have
> separate files for threads and max_threads.
> 
> While with the pin-init one we either have to have a single file exposing
> ProcessInner (which is what I did above) or protect threads and max_threads
> with separate locks (of course max_threads could also just be an atomic).
> 
> (If you like I can sketch up this case as well.)
> 
> At a first glance this seems like an undesirable limitation, but I argue that
> this is a good thing.
> 
> The reason I think so is what I also explained in [1], but let me adjust it a
> bit for this reply:
> 
> threads and max_threads being protected by the same lock means that they are in
> a certain relationship to each other. Meaning that they only really make sense
> looking at them atomically.
> 
> So I argue it does not make sense to expose those values to userspace through
> separate files.
> 
> For instance:
> 
> 	$ cat max_threads && cat threads
> 	$ 5
> 	$ 10
> 
> This way you may read 5 max_threads, but 10 actual threads, because things may
> have changed in between the two cat commands.
> 
> However, if instead, they are exposed through a single file, we get an atomic
> view of them, such that the semantic relationship between them is preserved.
> 
> For instance:
> 
> 	$ cat process_info
> 	$ threads: 2
> 	$ max_threads: 10

I think you mean to write:
 	$ cat process_info
 	threads: 2
 	max_threads: 10

right?

> So, what I'm trying to say is, I think it's a good thing if fields that are
> protected by the same lock can't be exposed through separate files, because it
> means that the fields only make sense in the context of each other.
> 
> Or saying it the other way around, if it makes sense to expose fields through
> separate files, it means they're unrelated to each other and hence should be
> protected with separate locks, rather than a common one.
> 
> IMHO it's even a good thing beyond the scope of debugfs, because it forces
> developers to really think about organizing structures properly, e.g. in a way
> that only fields that really belong behind a certain lock are placed behind this
> lock.
> 
> > So how about a platform driver that exposes values read from a platform
> > device (i.e. a soc info driver), that also includes a
> > local-to-the-device data structure that can be locked and modified?
> > That should cover all the use cases that I can think of at the moment.
> 
> Yes, I also really like to have that.
> 
> But, again, both approaches can do this. It's just that I really discourage the
> one that forces us to have an Arc instance on structures exposed through
> debugfs, since this messes with the driver's lifetime and ownership
> architecture in a bad way.
> 

Thanks for the long descriptions, that's great to help out here.  I'll
wait for the next patch series with a real example to show my ignorance
of rust some more :)

thanks,

greg k-h

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On Thu, Jul 03, 2025 at 02:50:48PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 03, 2025 at 02:29:31PM +0200, Danilo Krummrich wrote:
> > On Thu, Jul 03, 2025 at 01:41:53PM +0200, Greg Kroah-Hartman wrote:
> > > Yes, we need to be able to have a debugfs file callback handle a mutable
> > > structure in order to lock things correctly.  We also need to have it be
> > > mutable so that it can MODIFY the value (everyone seems to forget that
> > > debugfs allows that...)
> > 
> > Well, that's possible with both approaches. Data behind a lock becomes mutable
> > once you grabbed the lock. That's the same in both cases.
> > 
> > The difference is that with the pin-init approach I propose you can't have what
> > Alice sketched up. And I think it's even desirable that you can't do it.
> > 
> > Let's me explain the difference on a simplified example, based on Alice'
> > example.
> > 
> > ForeignOwnable API
> > ------------------
> > 
> > 	#[pin_data]
> > 	struct Process {
> > 	    task: ARef<Task>,
> > 	    #[pin]
> > 	    inner: SpinLock<ProcessInner>,
> > 	}
> > 	
> > 	pub(crate) struct ProcessInner {
> > 	    threads: RBTree<i32, Arc<Thread>>,
> > 	    max_threads: u32,
> > 	}
> > 
> > Here we have to create an Arc<Process> (let's call it process) and create files
> > from it.
> > 
> > 	let file_threads = dir.create_file("threads", process);
> > 	let file_max_threads = dir.create_file("max_threads", process);
> > 
> > In the file system callback of both of these, we now have an Arc<Process>, hence
> > we can access:
> > 
> > 	let guard = process.inner.lock();
> > 
> > 	read_or_write(guard.max_threads);
> > 
> > and in the other file:
> > 
> > 	let guard = process.inner.lock();
> > 
> > 	read_or_write(guard.max_threads);
> > 
> > Pin-Init API
> > ------------
> > 
> > 	#[pin_data]
> > 	struct Process {
> > 	    task: ARef<Task>,
> > 	    #[pin]
> > 	    inner: File<SpinLock<ProcessInner>>,
> > 	}
> > 	
> > 	pub(crate) struct ProcessInner {
> > 	    threads: RBTree<i32, Arc<Thread>>,
> > 	    max_threads: u32,
> > 	}
> > 
> > Here Process does not need to be within an Arc and no separate file instances
> > need to be kept around, that happens already within the constructor of Process:
> > 
> > 	pin_init!(Process {
> > 	   inner <- dir.create_file("process_inner", ...),
> > 	   [...]
> > 	})
> > 
> > The file itself has a reference to SpinLock<ProcessInner>, hence we can access:
> > 
> > 	let guard = inner.lock();
> > 
> > 	read_or_write(guard.threads)
> > 	read_or_write(guard.max_threads)
> > 
> > The difference is that with the ForeignOwnable API it was possible to have
> > separate files for threads and max_threads.
> > 
> > While with the pin-init one we either have to have a single file exposing
> > ProcessInner (which is what I did above) or protect threads and max_threads
> > with separate locks (of course max_threads could also just be an atomic).
> > 
> > (If you like I can sketch up this case as well.)
> > 
> > At a first glance this seems like an undesirable limitation, but I argue that
> > this is a good thing.
> > 
> > The reason I think so is what I also explained in [1], but let me adjust it a
> > bit for this reply:
> > 
> > threads and max_threads being protected by the same lock means that they are in
> > a certain relationship to each other. Meaning that they only really make sense
> > looking at them atomically.
> > 
> > So I argue it does not make sense to expose those values to userspace through
> > separate files.
> > 
> > For instance:
> > 
> > 	$ cat max_threads && cat threads
> > 	$ 5
> > 	$ 10
> > 
> > This way you may read 5 max_threads, but 10 actual threads, because things may
> > have changed in between the two cat commands.
> > 
> > However, if instead, they are exposed through a single file, we get an atomic
> > view of them, such that the semantic relationship between them is preserved.
> > 
> > For instance:
> > 
> > 	$ cat process_info
> > 	$ threads: 2
> > 	$ max_threads: 10
> 
> I think you mean to write:
>  	$ cat process_info
>  	threads: 2
>  	max_threads: 10
> 
> right?

Yes, indeed. :)

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Benno Lossin]

On Thu Jul 3, 2025 at 2:29 PM CEST, Danilo Krummrich wrote:
> So, what I'm trying to say is, I think it's a good thing if fields that are
> protected by the same lock can't be exposed through separate files, because it
> means that the fields only make sense in the context of each other.

I think that even the pin-init API can have multiple files for different
elements of the locked structure, you just need to nest `File`:

    #[pin_data]
    struct Process {
        task: ARef<Task>,
        #[pin]
        inner: File<File<SpinLock<ProcessInner>>>,
    }
    
    pub(crate) struct ProcessInner {
        threads: RBTree<i32, Arc<Thread>>,
        max_threads: u32,
    }

Now you can do:

    pin_init!(Process {
        inner <- dir.create_file(
            "threads",
            dir.create_file("max_threads", new_spinlock!(...)),
        ),
        // ...
    })

But I'd say this will at least raise eyebrows for the reviewers, which
is good, since it catches the footgun.

---
Cheers,
Benno

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On Thu, Jul 03, 2025 at 03:34:23PM +0200, Benno Lossin wrote:
> On Thu Jul 3, 2025 at 2:29 PM CEST, Danilo Krummrich wrote:
> > So, what I'm trying to say is, I think it's a good thing if fields that are
> > protected by the same lock can't be exposed through separate files, because it
> > means that the fields only make sense in the context of each other.
> 
> I think that even the pin-init API can have multiple files for different
> elements of the locked structure, you just need to nest `File`:
> 
>     #[pin_data]
>     struct Process {
>         task: ARef<Task>,
>         #[pin]
>         inner: File<File<SpinLock<ProcessInner>>>,
>     }
>     
>     pub(crate) struct ProcessInner {
>         threads: RBTree<i32, Arc<Thread>>,
>         max_threads: u32,
>     }
> 
> Now you can do:
> 
>     pin_init!(Process {
>         inner <- dir.create_file(
>             "threads",
>             dir.create_file("max_threads", new_spinlock!(...)),
>         ),
>         // ...
>     })
> 
> But I'd say this will at least raise eyebrows for the reviewers, which
> is good, since it catches the footgun.

Heh! I didn't even think of this trick. I agree, this should at least raise some
eyebrows. :)

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Benno Lossin]

On Thu Jul 3, 2025 at 2:29 PM CEST, Danilo Krummrich wrote:
> On Thu, Jul 03, 2025 at 01:41:53PM +0200, Greg Kroah-Hartman wrote:
>> Yes, we need to be able to have a debugfs file callback handle a mutable
>> structure in order to lock things correctly.  We also need to have it be
>> mutable so that it can MODIFY the value (everyone seems to forget that
>> debugfs allows that...)
>
> Well, that's possible with both approaches. Data behind a lock becomes mutable
> once you grabbed the lock. That's the same in both cases.
>
> The difference is that with the pin-init approach I propose you can't have what
> Alice sketched up. And I think it's even desirable that you can't do it.
>
> Let's me explain the difference on a simplified example, based on Alice'
> example.
>
> ForeignOwnable API
> ------------------
>
> 	#[pin_data]
> 	struct Process {
> 	    task: ARef<Task>,
> 	    #[pin]
> 	    inner: SpinLock<ProcessInner>,
> 	}
> 	
> 	pub(crate) struct ProcessInner {
> 	    threads: RBTree<i32, Arc<Thread>>,
> 	    max_threads: u32,
> 	}
>
> Here we have to create an Arc<Process> (let's call it process) and create files
> from it.
>
> 	let file_threads = dir.create_file("threads", process);
> 	let file_max_threads = dir.create_file("max_threads", process);
>
> In the file system callback of both of these, we now have an Arc<Process>, hence
> we can access:
>
> 	let guard = process.inner.lock();
>
> 	read_or_write(guard.max_threads);
>
> and in the other file:
>
> 	let guard = process.inner.lock();
>
> 	read_or_write(guard.max_threads);

Where do you actually specify this callback? At the moment, the API asks
for `Display` and thus it can only read values?

---
Cheers,
Benno

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Alice Ryhl]

On Thu, Jul 3, 2025 at 3:35 PM Benno Lossin <lossin@kernel.org> wrote:
> Where do you actually specify this callback? At the moment, the API asks
> for `Display` and thus it can only read values?

A later patch in this series adds closure support.

Alice

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Benno Lossin]

On Thu Jul 3, 2025 at 1:41 PM CEST, Greg Kroah-Hartman wrote:
> On Thu, Jul 03, 2025 at 12:54:18PM +0200, Alice Ryhl wrote:
>> On Thu, Jul 3, 2025 at 12:33 PM Benno Lossin <lossin@kernel.org> wrote:
>> > How would your example look like with the current approach? IIUC, it
>> > also wouldn't work, because the debugfs data can't be mutated?
>> 
>> I would store a bunch of `File<Arc<Process>>` instances somewhere.
>> Each one has a closure that takes the spinlock and prints the
>> appropriate value.

But you could also do that with the pin-init design?

> Ok, I think we need to see some "real" examples here of the api in use
> before figuring it out further as I'm totally confused :)

Agreed :)

> Yes, we need to be able to have a debugfs file callback handle a mutable
> structure in order to lock things correctly.

To me this seems orthogonal to storing the value in-place or in a
`ForeignOwnable`.

> We also need to have it be mutable so that it can MODIFY the value
> (everyone seems to forget that debugfs allows that...)

Well that changes things a lot IMO... How does the C side usually handle
synchronization here? Does the driver decide that the structure exposed
to debugfs is locked with eg a spinlock and then in the debugfs callback
they just lock the same one?

---
Cheers,
Benno

> So how about a platform driver that exposes values read from a platform
> device (i.e. a soc info driver), that also includes a
> local-to-the-device data structure that can be locked and modified?
> That should cover all the use cases that I can think of at the moment.
>
> thanks,
>
> greg k-h

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Greg Kroah-Hartman]

On Thu, Jul 03, 2025 at 02:34:13PM +0200, Benno Lossin wrote:
> On Thu Jul 3, 2025 at 1:41 PM CEST, Greg Kroah-Hartman wrote:
> > On Thu, Jul 03, 2025 at 12:54:18PM +0200, Alice Ryhl wrote:
> >> On Thu, Jul 3, 2025 at 12:33 PM Benno Lossin <lossin@kernel.org> wrote:
> >> > How would your example look like with the current approach? IIUC, it
> >> > also wouldn't work, because the debugfs data can't be mutated?
> >> 
> >> I would store a bunch of `File<Arc<Process>>` instances somewhere.
> >> Each one has a closure that takes the spinlock and prints the
> >> appropriate value.
> 
> But you could also do that with the pin-init design?
> 
> > Ok, I think we need to see some "real" examples here of the api in use
> > before figuring it out further as I'm totally confused :)
> 
> Agreed :)
> 
> > Yes, we need to be able to have a debugfs file callback handle a mutable
> > structure in order to lock things correctly.
> 
> To me this seems orthogonal to storing the value in-place or in a
> `ForeignOwnable`.
> 
> > We also need to have it be mutable so that it can MODIFY the value
> > (everyone seems to forget that debugfs allows that...)
> 
> Well that changes things a lot IMO... How does the C side usually handle
> synchronization here? Does the driver decide that the structure exposed
> to debugfs is locked with eg a spinlock and then in the debugfs callback
> they just lock the same one?

It does whatever it feels like, so both are valid uses.

Sometimes a lock is involved, sometimes not.  Remember this is debugfs,
the only rule for it is "there are no rules except it requires root
permissions to access".

thanks,

greg k-h

Re: [PATCH v8 4/6] rust: debugfs: Support arbitrary owned backing for File

[Danilo Krummrich]

On Thu, Jul 03, 2025 at 10:02:58AM +0000, Alice Ryhl wrote:
> On Tue, Jul 01, 2025 at 05:10:47PM +0200, Danilo Krummrich wrote:
> > On Tue, Jul 01, 2025 at 04:21:56PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jul 01, 2025 at 04:13:28PM +0200, Danilo Krummrich wrote:
> > > > Instead this should just be:
> > > > 
> > > > 	struct GPU {
> > > > 	   fw: debugfs::File<Firmware>,
> > > > 	}
> > > > 
> > > > and then I would initialize it the following way:
> > > > 
> > > > 	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
> > > > 	let file = dir.create_file("firmware", fw);
> > > > 
> > > > 	// debugfs::File<Firmware> dereferences to Firmware
> > > > 	file.do_something();
> > > > 
> > > > 	// Access to fw is prevented by the compiler, since it has been moved
> > > > 	// into file.
> > > > 
> > > > This is much better, since now I have the guarantee that my Firmare instance
> > > > can't out-live the GPU instance.
> > > 
> > > That's better, yes, but how would multiple files for the same
> > > "structure" work here?  Like a debugfs-file-per-field of a structure
> > > that we often have?
> > 
> > That is a very good question and I thought about this as well, because with only
> > the current API this would require us to have more and more dynamic allocations
> > if we want to have a more fine grained filesystem representations of structures.
> > 
> > The idea I have for this is to use pin-init, which we do in quite some other
> > places as well.
> > 
> > I think we can add an additional API like this:
> > 
> > 	impl Dir {
> > 	   pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
> > 	      pin_init!(Self {
> > 	         data <- data,
> > 	         ...
> > 	      })
> > 	   }
> > 	}
> > 
> > This allows us to do things like:
> > 
> > 	#[pin_data]
> > 	struct Firmware {
> > 	   #[pin]
> > 	   minor: debugfs::File<u32>,
> > 	   #[pin]
> > 	   major: debugfs::File<u32>,
> > 	   #[pin]
> > 	   buffer: debugfs::File<[u8]>,
> > 	}
> > 
> > 	impl Firmware {
> > 	   pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
> > 	      pin_init!(Self {
> > 	         minor <- dir.create_file("minor", 1),
> > 	         major <- dir.create_file("major", 2),
> > 	         buffer <- dir.create_file("buffer", buffer),
> > 	      })
> > 	   }
> > 	}
> > 
> > 	// This is the only allocation we need.
> > 	let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;
> > 
> > With this everything is now in a single allocation and since we're using
> > pin-init, Dir::create_file() can safely store pointers of the corresponding data
> > in debugfs_create_file(), since this structure is guaranteed to be pinned in
> > memory.
> > 
> > Actually, we can also implement *only this*, since with this my previous example
> > would just become this:
> > 
> > 	struct GPU {
> > 	   fw: debugfs::File<Firmware>,
> > 	}
> > 
> > 	let file = dir.create_file("firmware", Firmware::new());
> > 	let file = KBox::pin_init(file, GFP_KERNEL)?;
> > 
> > 	// debugfs::File<Firmware> dereferences to Firmware
> > 	file.do_something();
> > 
> > Given that, I think we should change things to use pin-init right away for the
> > debugfs::File API.
> 
> Does this actually work in practice for anything except immutable data?
> I mean, let's take Rust Binder as an example and lets say that I want to
> expose a directory for each Process object with some of the fields
> exposed. Let's just simplify Rust Binder a bit and only include some of
> the fields:
> 
> #[pin_data]
> struct Process {
>     task: ARef<Task>,
>     #[pin]
>     inner: SpinLock<ProcessInner>,
> }
> 
> pub(crate) struct ProcessInner {
>     threads: RBTree<i32, Arc<Thread>>,
>     nodes: RBTree<u64, DArc<Node>>,
>     requested_thread_count: u32,
>     max_threads: u32,
>     started_thread_count: u32,
> }
> 
> Rust Binder already does expose some debugging data through a file
> system, though it doesn't do so using debugfs. It exposes a lot of data,
> but among them are the pid, the number of threads and nodes, as well as
> the values of requested_thread_count, started_thread_count, and
> max_threads.
> 
> Now, we run into problem number one: pinning is not supported inside
> mutexes. But let's say we solved that and we could do this:
> 
> #[pin_data]
> struct Process {
>     task: File<ARef<Task>>, // prints the pid
>     #[pin]
>     inner: SpinLock<ProcessInner>,
> }
> 
> pub(crate) struct ProcessInner {
>     threads: File<RBTree<i32, Arc<Thread>>>, // prints the count
>     nodes: File<RBTree<u64, DArc<Node>>>, // prints the count
>     requested_thread_count: File<u32>,
>     max_threads: File<u32>,
>     started_thread_count: File<u32>,
> }
> 
> However, this still doesn't work! Debugfs may get triggered at any time
> and need to read these fields, and there's no way for it to take the
> spinlock with the above design - it doesn't know where the spinlock is.
> For the integers I guess we could make them atomic to allow reading them
> in parallel with mutation, but that option is not available for the
> red/black trees.
> 
> What is the intended solution in this case? If the argument is that this
> is a rare case, then keep in mind that this is a real-world example of
> debugging information that we actually expose today in a real driver.

I'd like to question the decision of having a separate File for each member of
ProcessInner in this case a bit:

Given the decision to protect all members of ProcessInner with the same lock, I
assume that they are in a certain relationship to each other, such that it is
either necessary or (at least) makes the most sense to keep them synchronized
with each other.

In other words, you probably don't want that, for instance, changes on
requested_thread_count can race with threads, or max_threads, etc.

So the question I want to raise is, given that, does it make sense to expose
those values to userspace unsychronized to each other, i.e. through separate
files?

For instance:

	$ cat max_threads && cat threads
	$ 5
	$ 10

This way you may read 5 max_threads, but 10 actual threads, because things have
changed in between the two cat commands.

Or is it more that you want to expose them all at once, such that reading
those values from debugfs you can be sure that they were *all* captured under
the same lock at the same time and are in a valid relationship to each other?

For instance:

	$ cat process_info
	$ threads: 2
	$ nodes: 2
	$ requested_thread_count: 5
	$ max_threads: 10
	$ started_thread_count: 1

(I don't know if the numbers make sense semantically, but you get the idea.)

So, what I'm trying to say is, I think that usually we want to expose data
protected with the same lock as a whole and otherwise protect data that is
independant from other data with separate locks.

- Danilo

[PATCH v8 5/6] rust: debugfs: Support format hooks

[Matthew Maurer]

Rather than always using Display, allow hooking arbitrary functions to
arbitrary files. Display technically has the expressiveness to do this,
but requires a new type be declared for every different way to render
things, which can be very clumsy.

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 rust/kernel/debugfs.rs              | 45 ++++++++++++++++++++
 rust/kernel/debugfs/display_file.rs | 85 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 129 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 929e55ee5629f6888edf29997b9ed77d274e11c8..d74b599e8534536b10502e6db8c6f3197f7ab4a5 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -10,7 +10,9 @@
 #[cfg(CONFIG_DEBUG_FS)]
 use crate::sync::Arc;
 use crate::types::ForeignOwnable;
+use core::fmt;
 use core::fmt::Display;
+use core::ops::Deref;
 
 #[cfg(CONFIG_DEBUG_FS)]
 mod display_file;
@@ -157,6 +159,49 @@ pub fn display_file<D: ForeignOwnable + Send + Sync>(&self, name: &CStr, data: D
         self.create_file(name, data)
     }
 
+    /// Create a file in a DebugFS directory with the provided name, and contents from invoking `f`
+    /// on the provided reference.
+    ///
+    /// `f` must be a function item or a non-capturing closure, or this will fail to compile.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use core::sync::atomic::{AtomicU32, Ordering};
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let dir = Dir::new(c_str!("foo"));
+    /// static MY_ATOMIC: AtomicU32 = AtomicU32::new(3);
+    /// let file = dir.fmt_file(c_str!("bar"), &MY_ATOMIC, &|val, f| {
+    ///   let out = val.load(Ordering::Relaxed);
+    ///   writeln!(f, "{out:#010x}")
+    /// });
+    /// MY_ATOMIC.store(10, Ordering::Relaxed);
+    /// ```
+    pub fn fmt_file<
+        D: ForeignOwnable + Send + Sync,
+        T,
+        F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result + Send + Sync,
+    >(
+        &self,
+        name: &CStr,
+        data: D,
+        f: &'static F,
+    ) -> File
+    where
+        for<'a> D::Borrowed<'a>: Deref<Target = T>,
+    {
+        #[cfg(CONFIG_DEBUG_FS)]
+        let data_adapted = display_file::FormatAdapter::new(data, f);
+        #[cfg(not(CONFIG_DEBUG_FS))]
+        let data_adapted = {
+            // Mark used
+            let (_, _) = (data, f);
+            &0
+        };
+        self.display_file(name, data_adapted)
+    }
+
     /// Create a new directory in DebugFS at the root.
     ///
     /// # Examples
diff --git a/rust/kernel/debugfs/display_file.rs b/rust/kernel/debugfs/display_file.rs
index 0c2dd756fa866425d1b7771beceaa2fb43bf11e5..b38675a90e1b2e359fb54afd91062b26d1c32ba2 100644
--- a/rust/kernel/debugfs/display_file.rs
+++ b/rust/kernel/debugfs/display_file.rs
@@ -5,7 +5,9 @@
 use crate::seq_file::SeqFile;
 use crate::seq_print;
 use crate::types::ForeignOwnable;
-use core::fmt::Display;
+use core::fmt::{Display, Formatter, Result};
+use core::marker::PhantomData;
+use core::ops::Deref;
 
 /// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`.
 ///
@@ -70,3 +72,84 @@ impl<D: ForeignOwnable + Sync> DisplayFile for D
         ..unsafe { core::mem::zeroed() }
     };
 }
+
+/// Adapter to implement `Display` via a callback with the same representation as `T`.
+///
+/// # Invariants
+///
+/// If an instance for `FormatAdapter<_, F>` is constructed, `F` is inhabited.
+#[repr(transparent)]
+pub(crate) struct FormatAdapter<D, F> {
+    inner: D,
+    _formatter: PhantomData<F>,
+}
+
+impl<D, F> FormatAdapter<D, F> {
+    pub(crate) fn new(inner: D, _f: &'static F) -> Self {
+        // INVARIANT: We were passed a reference to F, so it is inhabited.
+        FormatAdapter {
+            inner,
+            _formatter: PhantomData,
+        }
+    }
+}
+
+pub(crate) struct BorrowedAdapter<'a, D: ForeignOwnable, F> {
+    borrowed: D::Borrowed<'a>,
+    _formatter: PhantomData<F>,
+}
+
+// SAFETY: We delegate to D's implementation of `ForeignOwnable`, so `into_foreign` produced aligned
+// pointers.
+unsafe impl<D: ForeignOwnable, F> ForeignOwnable for FormatAdapter<D, F> {
+    type PointedTo = D::PointedTo;
+    type Borrowed<'a> = BorrowedAdapter<'a, D, F>;
+    type BorrowedMut<'a> = Self::Borrowed<'a>;
+    fn into_foreign(self) -> *mut Self::PointedTo {
+        self.inner.into_foreign()
+    }
+    unsafe fn from_foreign(foreign: *mut Self::PointedTo) -> Self {
+        Self {
+            // SAFETY: `into_foreign` is delegated, so a delegated `from_foreign` is safe.
+            inner: unsafe { D::from_foreign(foreign) },
+            _formatter: PhantomData,
+        }
+    }
+    unsafe fn borrow<'a>(foreign: *mut Self::PointedTo) -> Self::Borrowed<'a> {
+        BorrowedAdapter {
+            // SAFETY: `into_foreign` is delegated, so a delegated `borrow` is safe.
+            borrowed: unsafe { D::borrow(foreign) },
+            _formatter: PhantomData,
+        }
+    }
+    unsafe fn borrow_mut<'a>(foreign: *mut Self::PointedTo) -> Self::BorrowedMut<'a> {
+        // SAFETY: `borrow_mut` has stricter requirements than `borrow`
+        unsafe { Self::borrow(foreign) }
+    }
+}
+
+impl<'a, D: ForeignOwnable<Borrowed<'a>: Deref<Target = T>>, T, F> Display
+    for BorrowedAdapter<'a, D, F>
+where
+    F: Fn(&T, &mut Formatter<'_>) -> Result + 'static,
+{
+    fn fmt(&self, fmt: &mut Formatter<'_>) -> Result {
+        // SAFETY: FormatAdapter<_, F> can only be constructed if F is inhabited
+        let f: &F = unsafe { materialize_zst_fmt() };
+        f(&self.borrowed, fmt)
+    }
+}
+
+/// For types with a unique value, produce a static reference to it.
+///
+/// # Safety
+///
+/// The caller asserts that F is inhabited
+unsafe fn materialize_zst_fmt<F>() -> &'static F {
+    const { assert!(core::mem::size_of::<F>() == 0) };
+    let zst_dangle: core::ptr::NonNull<F> = core::ptr::NonNull::dangling();
+    // SAFETY: While the pointer is dangling, it is a dangling pointer to a ZST, based on the
+    // assertion above. The type is also inhabited, by the caller's assertion. This means
+    // we can materialize it.
+    unsafe { zst_dangle.as_ref() }
+}

-- 
2.50.0.727.gbf7dc18ff4-goog

[PATCH v8 6/6] rust: samples: Add debugfs sample

[Matthew Maurer]

Provides an example of using the Rust DebugFS bindings.

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 MAINTAINERS                  |  1 +
 samples/rust/Kconfig         | 11 +++++++
 samples/rust/Makefile        |  1 +
 samples/rust/rust_debugfs.rs | 76 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 551e3a6a16d9051a2d58a77614c458287da2bdcc..f55c99cb2907655d109cb1fa248cdec803b5ce9a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7374,6 +7374,7 @@ F:	rust/kernel/devres.rs
 F:	rust/kernel/driver.rs
 F:	rust/kernel/faux.rs
 F:	rust/kernel/platform.rs
+F:	samples/rust/rust_debugfs.rs
 F:	samples/rust/rust_driver_platform.rs
 F:	samples/rust/rust_driver_faux.rs
 
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 7f7371a004ee0a8f67dca99c836596709a70c4fa..01101db41ae31b08a86d048cdd27da8ef9bb23a2 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -62,6 +62,17 @@ config SAMPLE_RUST_DMA
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_DEBUGFS
+	tristate "DebugFS Test Module"
+	depends on DEBUG_FS
+	help
+	  This option builds the Rust DebugFS Test module sample.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_debugfs.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_DRIVER_PCI
 	tristate "PCI Driver"
 	depends on PCI
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index bd2faad63b4f3befe7d1ed5139fe25c7a8b6d7f6..61276222a99f8cc6d7f84c26d0533b30815ebadd 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -4,6 +4,7 @@ ccflags-y += -I$(src)				# needed for trace events
 obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
 obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
+obj-$(CONFIG_SAMPLE_RUST_DEBUGFS)		+= rust_debugfs.o
 obj-$(CONFIG_SAMPLE_RUST_DMA)			+= rust_dma.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
diff --git a/samples/rust/rust_debugfs.rs b/samples/rust/rust_debugfs.rs
new file mode 100644
index 0000000000000000000000000000000000000000..6af9177c711a07764f0323e03a72d115287f1205
--- /dev/null
+++ b/samples/rust/rust_debugfs.rs
@@ -0,0 +1,76 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Google LLC.
+
+//! Sample DebugFS exporting module
+
+use core::sync::atomic::{AtomicU32, Ordering};
+use kernel::c_str;
+use kernel::debugfs::{Dir, File};
+use kernel::prelude::*;
+use kernel::sync::{new_mutex, Arc};
+
+module! {
+    type: RustDebugFs,
+    name: "rust_debugfs",
+    authors: ["Matthew Maurer"],
+    description: "Rust DebugFS usage sample",
+    license: "GPL",
+}
+
+struct RustDebugFs {
+    // As we only hold these for drop effect (to remove the directory/files) we have a leading
+    // underscore to indicate to the compiler that we don't expect to use this field directly.
+    _debugfs: Dir,
+    _subdir: Dir,
+    _file: File,
+    _file_2: File,
+}
+
+static EXAMPLE: AtomicU32 = AtomicU32::new(8);
+
+impl kernel::Module for RustDebugFs {
+    fn init(_this: &'static ThisModule) -> Result<Self> {
+        // Create a debugfs directory in the root of the filesystem called "sample_debugfs".
+        let debugfs = Dir::new(c_str!("sample_debugfs"));
+
+        // Create a subdirectory, so "sample_debugfs/subdir" now exists.
+        let sub = debugfs.subdir(c_str!("subdir"));
+
+        // Create a single file in the subdirectory called "example" that will read from the
+        // `EXAMPLE` atomic variable.
+        // We `forget` the result to avoid deleting the file at the end of the scope.
+        let file = sub.fmt_file(c_str!("example"), &EXAMPLE, &|example, f| {
+            writeln!(f, "Reading atomic: {}", example.load(Ordering::Relaxed))
+        });
+        // Now, "sample_debugfs/subdir/example" will print "Reading atomic: 8\n" when read.
+
+        // Change the value in the variable displayed by the file. This is intended to demonstrate
+        // that the module can continue to change the value used by the file.
+        EXAMPLE.store(10, Ordering::Relaxed);
+        // Now, "sample_debugfs/subdir/example" will print "Reading atomic: 10\n" when read.
+
+        // In addition to globals, we can also attach any kind of owned data. Most commonly, this
+        // will look like an `Arc<MyObject>` as those can be shared with the rest of the module.
+        let my_arc = Arc::pin_init(new_mutex!(10), GFP_KERNEL)?;
+        // An `Arc<Mutex<usize>>` doesn't implement display, so let's give explicit instructions on
+        // how to print it
+        let file_2 = sub.fmt_file(c_str!("arc_backed"), my_arc.clone(), &|val, f| {
+            writeln!(f, "locked value: {:#010x}", *val.lock())
+        });
+
+        // Since it's an `Arc` and we cloned it, we continue to have access to `my_arc`. If this
+        // were real, we'd probably stash it in our module struct and do something with it when
+        // handling real calls.
+        *my_arc.lock() = 99;
+
+        // Save the handles we want to preserve to our module object. They will be automatically
+        // cleaned up when our module is unloaded.
+        Ok(Self {
+            _debugfs: debugfs,
+            _subdir: sub,
+            _file: file,
+            _file_2: file_2,
+        })
+    }
+}

-- 
2.50.0.727.gbf7dc18ff4-goog

Re: [PATCH v8 6/6] rust: samples: Add debugfs sample

[Greg Kroah-Hartman]

On Fri, Jun 27, 2025 at 11:18:29PM +0000, Matthew Maurer wrote:
> +        // An `Arc<Mutex<usize>>` doesn't implement display, so let's give explicit instructions on
> +        // how to print it
> +        let file_2 = sub.fmt_file(c_str!("arc_backed"), my_arc.clone(), &|val, f| {
> +            writeln!(f, "locked value: {:#010x}", *val.lock())
> +        });

While cute, is this really going to be the way to describe all "custom"
debugfs function callbacks?  No other way to point to a function itself
instead?  Look at "fun" debugfs functions like qh_lines() in
drivers/usb/host/ehci-dbg.c that is dumping tons of data out.  Putting
that inline here is going to be a bit ackward :)

So can you show an example of a "traditional" debugfs file output with
multiple lines that is dealing with a dynamically allocated device that
is associated with the module (not the static example you showed here),
as that's going to be the real way this is used, not with static
variables.

thanks,

greg k-h

Re: [PATCH v8 6/6] rust: samples: Add debugfs sample

[Matthew Maurer]

On Tue, Jul 1, 2025 at 7:03 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Jun 27, 2025 at 11:18:29PM +0000, Matthew Maurer wrote:
> > +        // An `Arc<Mutex<usize>>` doesn't implement display, so let's give explicit instructions on
> > +        // how to print it
> > +        let file_2 = sub.fmt_file(c_str!("arc_backed"), my_arc.clone(), &|val, f| {
> > +            writeln!(f, "locked value: {:#010x}", *val.lock())
> > +        });
>
> While cute, is this really going to be the way to describe all "custom"
> debugfs function callbacks?  No other way to point to a function itself
> instead?  Look at "fun" debugfs functions like qh_lines() in
> drivers/usb/host/ehci-dbg.c that is dumping tons of data out.  Putting
> that inline here is going to be a bit ackward :)

Good news, function pointers are legal to pass in here as well
already, I can add that usage to make it clear.

>
> So can you show an example of a "traditional" debugfs file output with
> multiple lines that is dealing with a dynamically allocated device that
> is associated with the module (not the static example you showed here),
> as that's going to be the real way this is used, not with static
> variables.

Sure, do we want to:
* Finish creating the driver struct early in `init`, then call dynamic
`.create(&str)` or `.destroy(&str)` `.modify(&str)` type things on it
in `init` to show how it would work
* Actually wire up an input source to drive create/destroy/modify
dynamically (e.g. I could implement a miscdevice) - if you want this
one, do you have a preference on where I get my input signal from?

>
> thanks,
>
> greg k-h

Re: [PATCH v8 6/6] rust: samples: Add debugfs sample

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 10:24:04AM -0700, Matthew Maurer wrote:
> On Tue, Jul 1, 2025 at 7:03 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Jun 27, 2025 at 11:18:29PM +0000, Matthew Maurer wrote:
> > > +        // An `Arc<Mutex<usize>>` doesn't implement display, so let's give explicit instructions on
> > > +        // how to print it
> > > +        let file_2 = sub.fmt_file(c_str!("arc_backed"), my_arc.clone(), &|val, f| {
> > > +            writeln!(f, "locked value: {:#010x}", *val.lock())
> > > +        });
> >
> > While cute, is this really going to be the way to describe all "custom"
> > debugfs function callbacks?  No other way to point to a function itself
> > instead?  Look at "fun" debugfs functions like qh_lines() in
> > drivers/usb/host/ehci-dbg.c that is dumping tons of data out.  Putting
> > that inline here is going to be a bit ackward :)
> 
> Good news, function pointers are legal to pass in here as well
> already, I can add that usage to make it clear.
> 
> >
> > So can you show an example of a "traditional" debugfs file output with
> > multiple lines that is dealing with a dynamically allocated device that
> > is associated with the module (not the static example you showed here),
> > as that's going to be the real way this is used, not with static
> > variables.
> 
> Sure, do we want to:
> * Finish creating the driver struct early in `init`, then call dynamic
> `.create(&str)` or `.destroy(&str)` `.modify(&str)` type things on it
> in `init` to show how it would work
> * Actually wire up an input source to drive create/destroy/modify
> dynamically (e.g. I could implement a miscdevice) - if you want this
> one, do you have a preference on where I get my input signal from?

I think the idea was to show how it works in a real driver context, e.g. a
platform driver, just like what samples/rust/rust_driver_platform.rs does. Not a
miscdevice registered from a module, which is a rather rare use-case.

If you rebase on the latest driver-core-next, you can write a platform driver
with an ACPI ID table, which can easily probed by passing
`-acpitable file=ssdt.aml` to qemu, i.e. no need to mess with OF.

Re: [PATCH v8 6/6] rust: samples: Add debugfs sample

[Matthew Maurer]

On Tue, Jul 1, 2025 at 10:34 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Tue, Jul 01, 2025 at 10:24:04AM -0700, Matthew Maurer wrote:
> > On Tue, Jul 1, 2025 at 7:03 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Fri, Jun 27, 2025 at 11:18:29PM +0000, Matthew Maurer wrote:
> > > > +        // An `Arc<Mutex<usize>>` doesn't implement display, so let's give explicit instructions on
> > > > +        // how to print it
> > > > +        let file_2 = sub.fmt_file(c_str!("arc_backed"), my_arc.clone(), &|val, f| {
> > > > +            writeln!(f, "locked value: {:#010x}", *val.lock())
> > > > +        });
> > >
> > > While cute, is this really going to be the way to describe all "custom"
> > > debugfs function callbacks?  No other way to point to a function itself
> > > instead?  Look at "fun" debugfs functions like qh_lines() in
> > > drivers/usb/host/ehci-dbg.c that is dumping tons of data out.  Putting
> > > that inline here is going to be a bit ackward :)
> >
> > Good news, function pointers are legal to pass in here as well
> > already, I can add that usage to make it clear.
> >
> > >
> > > So can you show an example of a "traditional" debugfs file output with
> > > multiple lines that is dealing with a dynamically allocated device that
> > > is associated with the module (not the static example you showed here),
> > > as that's going to be the real way this is used, not with static
> > > variables.
> >
> > Sure, do we want to:
> > * Finish creating the driver struct early in `init`, then call dynamic
> > `.create(&str)` or `.destroy(&str)` `.modify(&str)` type things on it
> > in `init` to show how it would work
> > * Actually wire up an input source to drive create/destroy/modify
> > dynamically (e.g. I could implement a miscdevice) - if you want this
> > one, do you have a preference on where I get my input signal from?
>
> I think the idea was to show how it works in a real driver context, e.g. a
> platform driver, just like what samples/rust/rust_driver_platform.rs does. Not a
> miscdevice registered from a module, which is a rather rare use-case.
>
> If you rebase on the latest driver-core-next, you can write a platform driver
> with an ACPI ID table, which can easily probed by passing
> `-acpitable file=ssdt.aml` to qemu, i.e. no need to mess with OF.

I'm confused as to how registering as a platform driver would result
in an input source that would let me trigger the creation/destruction
of DebugFS files. I need some kind of input stream to do that. Is
there some input stream that's available to a platform driver that I'm
missing, or are you suggesting that the input stream would effectively
be the probe's `id_info` field? If I did that, wouldn't I still have a
static arrangement of DebugFS files in my driver struct?

I could have misunderstood, but I don't think that's what Greg is
asking for - I think he wants to see how at a data structure level, I
can handle creating and destroying DebugFS files that correspond to
some kind of object being created and destroyed, rather than just
having a static list of slots in my driver struct for keeping them
alive.

Re: [PATCH v8 6/6] rust: samples: Add debugfs sample

[Danilo Krummrich]

On Tue, Jul 01, 2025 at 11:32:42AM -0700, Matthew Maurer wrote:
> On Tue, Jul 1, 2025 at 10:34 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Tue, Jul 01, 2025 at 10:24:04AM -0700, Matthew Maurer wrote:
> > > On Tue, Jul 1, 2025 at 7:03 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Fri, Jun 27, 2025 at 11:18:29PM +0000, Matthew Maurer wrote:
> > > > > +        // An `Arc<Mutex<usize>>` doesn't implement display, so let's give explicit instructions on
> > > > > +        // how to print it
> > > > > +        let file_2 = sub.fmt_file(c_str!("arc_backed"), my_arc.clone(), &|val, f| {
> > > > > +            writeln!(f, "locked value: {:#010x}", *val.lock())
> > > > > +        });
> > > >
> > > > While cute, is this really going to be the way to describe all "custom"
> > > > debugfs function callbacks?  No other way to point to a function itself
> > > > instead?  Look at "fun" debugfs functions like qh_lines() in
> > > > drivers/usb/host/ehci-dbg.c that is dumping tons of data out.  Putting
> > > > that inline here is going to be a bit ackward :)
> > >
> > > Good news, function pointers are legal to pass in here as well
> > > already, I can add that usage to make it clear.
> > >
> > > >
> > > > So can you show an example of a "traditional" debugfs file output with
> > > > multiple lines that is dealing with a dynamically allocated device that
> > > > is associated with the module (not the static example you showed here),
> > > > as that's going to be the real way this is used, not with static
> > > > variables.
> > >
> > > Sure, do we want to:
> > > * Finish creating the driver struct early in `init`, then call dynamic
> > > `.create(&str)` or `.destroy(&str)` `.modify(&str)` type things on it
> > > in `init` to show how it would work
> > > * Actually wire up an input source to drive create/destroy/modify
> > > dynamically (e.g. I could implement a miscdevice) - if you want this
> > > one, do you have a preference on where I get my input signal from?
> >
> > I think the idea was to show how it works in a real driver context, e.g. a
> > platform driver, just like what samples/rust/rust_driver_platform.rs does. Not a
> > miscdevice registered from a module, which is a rather rare use-case.
> >
> > If you rebase on the latest driver-core-next, you can write a platform driver
> > with an ACPI ID table, which can easily probed by passing
> > `-acpitable file=ssdt.aml` to qemu, i.e. no need to mess with OF.
> 
> I'm confused as to how registering as a platform driver would result
> in an input source that would let me trigger the creation/destruction
> of DebugFS files. I need some kind of input stream to do that. Is
> there some input stream that's available to a platform driver that I'm
> missing, or are you suggesting that the input stream would effectively
> be the probe's `id_info` field? If I did that, wouldn't I still have a
> static arrangement of DebugFS files in my driver struct?

If it's about having some dynamic input stream, creating an example with a
platform driver clearly doesn't help by itself.

But that wasn't my understanding. My understanding was that the request is to
show it in a driver context, where you won't get away with statics. :)

But that's maybe because *I* would like to focus more on this case, because it's
the common one.

> I could have misunderstood, but I don't think that's what Greg is
> asking for - I think he wants to see how at a data structure level, I
> can handle creating and destroying DebugFS files that correspond to
> some kind of object being created and destroyed, rather than just
> having a static list of slots in my driver struct for keeping them
> alive.

If that's the request, you could simply create a function that returns a
Vec with some random entries and then you just iterate over it in
Driver::probe() or Module::init()?

I don't know if that case is too interesting, since conceptually it doesn't make
a huge difference to the case where you have a single instance, i.e.

Current
-------

	struct Bar {
	   a: A,
	   b: B,
	}

	// Single.
	struct Foo {
	   bar: Arc<Bar>,
	   bar_file: File,
	}

	// Multiple.
	struct Foo {
	  bars: Vec<Arc<Bar>>
	  bar_files: Vec<File>, // contains a File for each field for every Bar
	}

Proposed (pin-init)
-------------------

	struct Bar {
	   a: File<A>,
	   b: File<B>,
	}

	// Single.
	struct Foo {
	   bar: Bar,
	}

	// Multiple.
	struct Foo {
	   bar: Vec<Bar>
	}

Re: [PATCH v8 0/6] rust: DebugFS Bindings

[Alice Ryhl]

On Fri, Jun 27, 2025 at 11:18:23PM +0000, Matthew Maurer wrote:
> This series provides safe DebugFS bindings for Rust, with a sample
> module using them.
> 
> Signed-off-by: Matthew Maurer <mmaurer@google.com>

Looks good to me!

Reviewed-by: Alice Ryhl <aliceryhl@google.com>