rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] rust: DebugFS Bindings
@ 2025-04-30 23:31 Matthew Maurer
  2025-04-30 23:31 ` [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation Matthew Maurer
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Matthew Maurer @ 2025-04-30 23:31 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Sami Tolvanen, Timur Tabi
  Cc: linux-kernel, rust-for-linux, Matthew Maurer

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

This series currently only supports referencing `'static` lifetime
objects, because we need to know the objects outlive the DebugFS files.
A subsequent series may be able to relax this.

A few points that might be worth discussing on the API here:

1. The handles returned by directory and file creation currently remove
   themselves when they go out of scope. This behavior can be suppressed
   via `.keep()`. We could flip it to the other sense, and require that
   a returned handle be *explicitly* upconverted to get removed on drop.
   If we did this, we could consider making a different default for
   `subdir` vs `Dir::new`. The downside of doing this would be more
   complex types in the interface.
2. File creation currently returns a handle to preserve expressiveness
   (the ability to delete a file without deleting the directory it's
   in). I see (e.g. in ceph) explicit deletion of files, but this
   appears to be legacy code from before `recursive_remove` as opposed
   to wanting manual deletion. We could choose not to provide a handle
   here.

Signed-off-by: Matthew Maurer <mmaurer@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 (4):
      rust: debugfs: Bind DebugFS directory creation
      rust: debugfs: Bind file creation for long-lived Display
      rust: debugfs: Support format hooks
      rust: samples: Add debugfs sample

 MAINTAINERS                     |   2 +
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/debugfs.rs          | 345 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 samples/rust/Kconfig            |  11 ++
 samples/rust/Makefile           |   1 +
 samples/rust/rust_debugfs.rs    |  37 +++++
 7 files changed, 398 insertions(+)
---
base-commit: b6a7783d306baf3150ac54cd5124f6e85dd375b0
change-id: 20250428-debugfs-rust-3cd5c97eb7d1

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation
  2025-04-30 23:31 [PATCH v2 0/4] rust: DebugFS Bindings Matthew Maurer
@ 2025-04-30 23:31 ` Matthew Maurer
  2025-05-01 10:00   ` Miguel Ojeda
  2025-05-01 10:16   ` Danilo Krummrich
  2025-04-30 23:31 ` [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display Matthew Maurer
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 15+ messages in thread
From: Matthew Maurer @ 2025-04-30 23:31 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Sami Tolvanen, Timur Tabi
  Cc: linux-kernel, rust-for-linux, Matthew Maurer

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

By default, when a directory handle leaves scope, it will be cleaned up.
This can be suppressed by calling `.keep()` on it.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/debugfs.rs          | 135 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 4 files changed, 138 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 906881b6c5cb6ff743e13b251873b89138c69a1c..a3b835e427b083a4ddd690d9e7739851f0af47ae 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7271,6 +7271,7 @@ 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/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 8a2add69e5d66d1c2ebed9d2c950380e61c48842..787f928467faabd02a7f3cf041378fac856c4f89 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -13,6 +13,7 @@
 #include <linux/blkdev.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..b533ab21aaa775d4e3f33caf89e2d67ef85592f8
--- /dev/null
+++ b/rust/kernel/debugfs.rs
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Google LLC.
+
+//! DebugFS Abstraction
+//!
+//! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
+
+use crate::str::CStr;
+
+/// Handle to a DebugFS directory.
+// INVARIANT: The wrapped pointer will always be NULL, an error, or an owned DebugFS `dentry`
+pub struct Dir(#[cfg(CONFIG_DEBUG_FS)] *mut bindings::dentry);
+
+// SAFETY: Dir is just a `dentry` under the hood, which the API promises can be transferred
+// between threads.
+unsafe impl Send for Dir {}
+
+// 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 Dir {}
+
+impl Dir {
+    /// Create a new directory in DebugFS at the root.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// {
+    ///    let parent = Dir::new(c_str!("parent"));
+    ///    // parent exists in DebugFS here.
+    /// }
+    /// // It does not exist here.
+    /// ```
+    pub fn new(name: &CStr) -> Self {
+        Self::create(name, None)
+    }
+
+    /// Create a DebugFS subdirectory.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// {
+    ///    let parent = Dir::new(c_str!("parent"));
+    ///    // parent exists in DebugFS here.
+    ///    let child = parent.subdir(c_str!("child"));
+    ///    // parent/child exists in DebugFS here.
+    /// }
+    /// // Neither exist here.
+    /// ```
+    pub fn subdir(&self, name: &CStr) -> Self {
+        Self::create(name, Some(self))
+    }
+
+    /// 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<&Self>) -> Self {
+        let parent_ptr = match parent {
+            Some(parent) => parent.as_ptr(),
+            None => core::ptr::null_mut(),
+        };
+        // SAFETY:
+        // * name argument points to a null terminated string that lives across the call, by
+        //   invariants of `&CStr`
+        // * If parent is None, parent accepts null pointers to mean create at root
+        // * If parent is Some, parent accepts live dentry debugfs pointers
+        // * `debugfs_create_dir` either returns an error code or a legal `dentry` pointer,
+        //   so we can call `Self::from_ptr`
+        unsafe { Self::from_ptr(bindings::debugfs_create_dir(name.as_char_ptr(), parent_ptr)) }
+    }
+
+    #[cfg(not(CONFIG_DEBUG_FS))]
+    fn create(_name: &CStr, _parent: Option<&Self>) -> Self {
+        Self()
+    }
+
+    /// Constructs a new DebugFS [`Dir`] 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.
+    #[cfg(CONFIG_DEBUG_FS)]
+    unsafe fn from_ptr(ptr: *mut bindings::dentry) -> Self {
+        Self(ptr)
+    }
+
+    /// Returns the pointer representation of the DebugFS directory.
+    ///
+    /// # Invariant
+    ///
+    /// The value returned from this function will always be an error code, NUL, or a live DebugFS
+    /// directory.
+    // If this function is ever needed with `not(CONFIG_DEBUG_FS)`, hardcode it to return `ENODEV`.
+    #[cfg(CONFIG_DEBUG_FS)]
+    fn as_ptr(&self) -> *mut bindings::dentry {
+        self.0
+    }
+
+    /// Allow the handle to go out of scope without removing the directory.
+    ///
+    /// Equivalent to `core::mem::forget`, but with a more semantically meaningful name.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// {
+    ///    let parent = Dir::new(c_str!("parent"));
+    ///    parent.subdir(c_str!("child")).keep();
+    ///    // We have no handle to the child, but it will not be deleted directly.
+    /// }
+    /// // When parent is deleted, it will still be deleted.
+    /// ```
+    pub fn keep(self) {
+        core::mem::forget(self)
+    }
+}
+
+impl Drop for Dir {
+    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.
+        #[cfg(CONFIG_DEBUG_FS)]
+        unsafe {
+            bindings::debugfs_remove(self.as_ptr())
+        }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c3762e80b314316b4b0cee3bfd9442f8f0510b91..86f6055b828d5f711578293d8916a517f2436977 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -45,6 +45,7 @@
 #[doc(hidden)]
 pub mod build_assert;
 pub mod cred;
+pub mod debugfs;
 pub mod device;
 pub mod device_id;
 pub mod devres;

-- 
2.49.0.906.g1f30a19c02-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display
  2025-04-30 23:31 [PATCH v2 0/4] rust: DebugFS Bindings Matthew Maurer
  2025-04-30 23:31 ` [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation Matthew Maurer
@ 2025-04-30 23:31 ` Matthew Maurer
  2025-05-01 10:37   ` Danilo Krummrich
  2025-04-30 23:31 ` [PATCH v2 3/4] rust: debugfs: Support format hooks Matthew Maurer
  2025-04-30 23:31 ` [PATCH v2 4/4] rust: samples: Add debugfs sample Matthew Maurer
  3 siblings, 1 reply; 15+ messages in thread
From: Matthew Maurer @ 2025-04-30 23:31 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Sami Tolvanen, Timur Tabi
  Cc: linux-kernel, rust-for-linux, Matthew Maurer

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

The reference must live forever because we do not have a maximum
lifetime for the file we are creating.

The `Display` implementation is used because `seq_printf` needs to route
through `%pA`, which in turn routes through Arguments. A more generic
API is provided later in the series, implemented in terms of this one.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 rust/kernel/debugfs.rs | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index b533ab21aaa775d4e3f33caf89e2d67ef85592f8..87de94da3b27c2a399bb377afd47280f65208d41 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -7,6 +7,7 @@
 //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
 
 use crate::str::CStr;
+use core::fmt::Display;
 
 /// Handle to a DebugFS directory.
 // INVARIANT: The wrapped pointer will always be NULL, an error, or an owned DebugFS `dentry`
@@ -121,6 +122,47 @@ fn as_ptr(&self) -> *mut bindings::dentry {
     pub fn keep(self) {
         core::mem::forget(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).keep();
+    /// // "my_debugfs_dir/foo" now contains the number 200.
+    /// ```
+    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> Self {
+        // 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.
+        // * debugfs_create_file_full either returns an error code or a legal dentry pointer, so
+        //   `Self::from_ptr` is safe to call here.
+        #[cfg(CONFIG_DEBUG_FS)]
+        unsafe {
+            Self::from_ptr(bindings::debugfs_create_file_full(
+                name.as_char_ptr(),
+                0o444,
+                self.as_ptr(),
+                data as *const _ as *mut _,
+                core::ptr::null(),
+                &<T as DisplayFile>::VTABLE,
+            ))
+        }
+        #[cfg(not(CONFIG_DEBUG_FS))]
+        {
+            // Mark parameters used
+            let (_, _) = (name, data);
+            Self()
+        }
+    }
 }
 
 impl Drop for Dir {
@@ -133,3 +175,63 @@ fn drop(&mut self) {
         }
     }
 }
+
+#[cfg(CONFIG_DEBUG_FS)]
+mod helpers {
+    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,
+    ) -> i32 {
+        // 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 core::ffi::c_void,
+    ) -> i32 {
+        // 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> as _),
+            // SAFETY: file_operations supports zeroes in all fields
+            ..unsafe { core::mem::zeroed() }
+        };
+    }
+
+    impl<T: Display + Sized> DisplayFile for T {}
+}
+
+#[cfg(CONFIG_DEBUG_FS)]
+use helpers::*;

-- 
2.49.0.906.g1f30a19c02-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 3/4] rust: debugfs: Support format hooks
  2025-04-30 23:31 [PATCH v2 0/4] rust: DebugFS Bindings Matthew Maurer
  2025-04-30 23:31 ` [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation Matthew Maurer
  2025-04-30 23:31 ` [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display Matthew Maurer
@ 2025-04-30 23:31 ` Matthew Maurer
  2025-05-01 10:00   ` Miguel Ojeda
  2025-04-30 23:31 ` [PATCH v2 4/4] rust: samples: Add debugfs sample Matthew Maurer
  3 siblings, 1 reply; 15+ messages in thread
From: Matthew Maurer @ 2025-04-30 23:31 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Sami Tolvanen, Timur Tabi
  Cc: linux-kernel, rust-for-linux, 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.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 rust/kernel/debugfs.rs | 110 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 87de94da3b27c2a399bb377afd47280f65208d41..2935c7ffbfaf460fff5b5f1ffc768f803c2da345 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -7,6 +7,7 @@
 //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
 
 use crate::str::CStr;
+use core::fmt;
 use core::fmt::Display;
 
 /// Handle to a DebugFS directory.
@@ -136,6 +137,47 @@ pub fn keep(self) {
     /// // "my_debugfs_dir/foo" now contains the number 200.
     /// ```
     pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> Self {
+        // SAFETY: As `data` lives for the static lifetime, it outlives the file.
+        unsafe { self.display_file_raw(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<T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
+        &self,
+        name: &CStr,
+        data: &'static T,
+        f: &'static F,
+    ) -> Self {
+        // SAFETY: As `data` lives for the static lifetime, it outlives the file
+        unsafe { self.fmt_file_raw(name, data, f) }
+    }
+
+    /// Creates a DebugFS file backed by the display implementation of the provided pointer.
+    ///
+    /// # Safety
+    /// The pointee of `data` must outlive the accessibility of the `Dir` returned by this function.
+    /// This means that before `data` may become invalid, either:
+    /// * The refcount must go to zero
+    /// * The file must be rendered inaccessible, e.g. via `debugfs_remove`
+    unsafe fn display_file_raw<T: Display + Sized>(&self, name: &CStr, data: *const T) -> Self {
         // 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
@@ -163,6 +205,32 @@ pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) ->
             Self()
         }
     }
+
+    /// Create a file in a DebugFS directory with the provided name, and contents from invoking the
+    /// fomatter on the attached data.
+    ///
+    /// The attached function must be a ZST, and will cause a compilation error if it is not.
+    ///
+    /// # Safety
+    ///
+    /// `data` must outlive the resulting file's accessibility
+    unsafe fn fmt_file_raw<T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
+        &self,
+        name: &CStr,
+        data: &T,
+        f: &'static F,
+    ) -> Self {
+        #[cfg(CONFIG_DEBUG_FS)]
+        let data_adapted = FormatAdapter::new(data, f);
+        #[cfg(not(CONFIG_DEBUG_FS))]
+        let data_adapted = {
+            // Mark used
+            let (_, _) = (data, f);
+            &0
+        };
+        // SAFETY: data outlives the file's accessibility, so data_adapted does too
+        unsafe { self.display_file_raw(name, data_adapted) }
+    }
 }
 
 impl Drop for Dir {
@@ -180,7 +248,9 @@ fn drop(&mut self) {
 mod helpers {
     use crate::seq_file::SeqFile;
     use crate::seq_print;
-    use core::fmt::Display;
+    use core::fmt;
+    use core::fmt::{Display, Formatter};
+    use core::marker::PhantomData;
 
     /// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`
     ///
@@ -231,6 +301,44 @@ pub(crate) trait DisplayFile: Display + Sized {
     }
 
     impl<T: Display + Sized> DisplayFile for T {}
+
+    // INVARIANT: F is inhabited
+    #[repr(transparent)]
+    pub(crate) struct FormatAdapter<T, F> {
+        inner: T,
+        _formatter: PhantomData<F>,
+    }
+
+    impl<T, F> FormatAdapter<T, F> {
+        pub(crate) fn new<'a>(inner: &'a T, _f: &'static F) -> &'a Self {
+            // SAFETY: FormatAdapater is a repr(transparent) wrapper around T, so
+            // casting a reference is legal
+            // INVARIANT: We were passed a reference to F, so it is inhabited.
+            unsafe { core::mem::transmute(inner) }
+        }
+    }
+
+    impl<T, F> Display for FormatAdapter<T, F>
+    where
+        F: Fn(&T, &mut Formatter<'_>) -> fmt::Result + 'static,
+    {
+        fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
+            // SAFETY: FormatAdapter<_, F> can only be constructed if F is inhabited
+            let f: &F = unsafe { materialize_zst_fmt() };
+            f(&self.inner, fmt)
+        }
+    }
+
+    /// # 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() }
+    }
 }
 
 #[cfg(CONFIG_DEBUG_FS)]

-- 
2.49.0.906.g1f30a19c02-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2 4/4] rust: samples: Add debugfs sample
  2025-04-30 23:31 [PATCH v2 0/4] rust: DebugFS Bindings Matthew Maurer
                   ` (2 preceding siblings ...)
  2025-04-30 23:31 ` [PATCH v2 3/4] rust: debugfs: Support format hooks Matthew Maurer
@ 2025-04-30 23:31 ` Matthew Maurer
  2025-05-01  7:40   ` Greg Kroah-Hartman
  3 siblings, 1 reply; 15+ messages in thread
From: Matthew Maurer @ 2025-04-30 23:31 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Sami Tolvanen, Timur Tabi
  Cc: linux-kernel, rust-for-linux, Matthew Maurer

Provides an example of using the Rust DebugFS bindings.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 MAINTAINERS                  |  1 +
 samples/rust/Kconfig         | 11 +++++++++++
 samples/rust/Makefile        |  1 +
 samples/rust/rust_debugfs.rs | 37 +++++++++++++++++++++++++++++++++++++
 4 files changed, 50 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a3b835e427b083a4ddd690d9e7739851f0af47ae..426bcdac025134e20911de8e2cf5c9efb0591814 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7278,6 +7278,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 43cb72d72631bb2d6e06185e1d88778edff6ee13..6c42ed73f842cda26256039e6917bb443738d3f1 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -51,6 +51,17 @@ config SAMPLE_RUST_DMA
 
 	  If unsure, say N.
 
+config SAMPLE_RUST_DEBUGFS
+	tristate "DebugFS Test Driver"
+	depends on DEBUG_FS
+	help
+	  This option builds the Rust DebugFS Test driver 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 6a466afd2a21eba84a3b7b2be29f25dce44e9053..b1fc4677ed53fcf7d0f5a3dbf322f65851bc1784 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..ce3e1a997bf314f63026446f5a6d2a00579c602a
--- /dev/null
+++ b/samples/rust/rust_debugfs.rs
@@ -0,0 +1,37 @@
+// 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;
+use kernel::prelude::*;
+
+module! {
+    type: RustDebugFs,
+    name: "rust_debugfs",
+    authors: ["Matthew Maurer"],
+    description: "Rust DebugFS usage sample",
+    license: "GPL",
+}
+
+struct RustDebugFs {
+    _debugfs: Dir,
+}
+
+static EXAMPLE: AtomicU32 = AtomicU32::new(8);
+
+impl kernel::Module for RustDebugFs {
+    fn init(_this: &'static ThisModule) -> Result<Self> {
+        let debugfs = Dir::new(c_str!("sample_debugfs"));
+        debugfs
+            .fmt_file(c_str!("example"), &EXAMPLE, &|example, f| {
+                writeln!(f, "Reading atomic: {}", example.load(Ordering::Relaxed))
+            })
+            .keep();
+        EXAMPLE.store(10, Ordering::Relaxed);
+        Ok(Self { _debugfs: debugfs })
+    }
+}

-- 
2.49.0.906.g1f30a19c02-goog


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 4/4] rust: samples: Add debugfs sample
  2025-04-30 23:31 ` [PATCH v2 4/4] rust: samples: Add debugfs sample Matthew Maurer
@ 2025-05-01  7:40   ` Greg Kroah-Hartman
  2025-05-01 16:44     ` Timur Tabi
  0 siblings, 1 reply; 15+ messages in thread
From: Greg Kroah-Hartman @ 2025-05-01  7:40 UTC (permalink / raw)
  To: Matthew Maurer
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Rafael J. Wysocki, Sami Tolvanen,
	Timur Tabi, linux-kernel, rust-for-linux

On Wed, Apr 30, 2025 at 11:31:59PM +0000, Matthew Maurer wrote:
> Provides an example of using the Rust DebugFS bindings.
> 
> Signed-off-by: Matthew Maurer <mmaurer@google.com>

Much nicer, many thanks for this!

Some minor comments on the sample code here.  As someone coming from C
with limited Rust experience, I think I do understand it, but I think it
could use a bunch of comments to make it more "obvious" what is
happening, see below.

> +static EXAMPLE: AtomicU32 = AtomicU32::new(8);

Wait, why is this set to 8 and then you automatically set it to 10 after
you create the file?  No one is ever going to see 8 as a valid value
unless they really race to read the file, right?

> +impl kernel::Module for RustDebugFs {
> +    fn init(_this: &'static ThisModule) -> Result<Self> {
> +        let debugfs = Dir::new(c_str!("sample_debugfs"));
> +        debugfs
> +            .fmt_file(c_str!("example"), &EXAMPLE, &|example, f| {
> +                writeln!(f, "Reading atomic: {}", example.load(Ordering::Relaxed))
> +            })
> +            .keep();
> +        EXAMPLE.store(10, Ordering::Relaxed);
> +        Ok(Self { _debugfs: debugfs })
> +    }
> +}


How about this rewrite with comments added to help make things more
obvious:

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 single file in the directory called "example" that
        // allows to read from the EXAMPLE atomic variable, and make
        // sure it lives past the scope of this function by calling
        // .keep() on it.
        debugfs
            .fmt_file(c_str!("example"), &EXAMPLE, &|example, f| {
                writeln!(f, "Reading atomic: {}", example.load(Ordering::Relaxed))
            })
            .keep();

        // Change the value of EXAMPLE to be 10 so that will be the
        // value read from the file.  Note, the original value 8 will be
        // read if the file is read right before this is called.
        EXAMPLE.store(10, Ordering::Relaxed);

        // Create our module object and save off the pointer to the
        // debugfs directory we created.  It will be automatically
        // removed when the module is unloaded by virtue of the
        // reference count to the structure being dropped at that point
        // in time.
        Ok(Self { _debugfs: debugfs })
    }
}

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 3/4] rust: debugfs: Support format hooks
  2025-04-30 23:31 ` [PATCH v2 3/4] rust: debugfs: Support format hooks Matthew Maurer
@ 2025-05-01 10:00   ` Miguel Ojeda
  0 siblings, 0 replies; 15+ messages in thread
From: Miguel Ojeda @ 2025-05-01 10:00 UTC (permalink / raw)
  To: Matthew Maurer
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Sami Tolvanen, Timur Tabi, linux-kernel,
	rust-for-linux

On Thu, May 1, 2025 at 1:32 AM Matthew Maurer <mmaurer@google.com> wrote:
>
> +
> +    // INVARIANT: F is inhabited
> +    #[repr(transparent)]

This and a few other bits didn't change?

i.e. https://lore.kernel.org/rust-for-linux/CANiq72=vM9Zr-q=BWvE258=9BV1Q4S_9sYo+gfCnaUUj_E09ow@mail.gmail.com/

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation
  2025-04-30 23:31 ` [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation Matthew Maurer
@ 2025-05-01 10:00   ` Miguel Ojeda
  2025-05-01 10:16   ` Danilo Krummrich
  1 sibling, 0 replies; 15+ messages in thread
From: Miguel Ojeda @ 2025-05-01 10:00 UTC (permalink / raw)
  To: Matthew Maurer
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman,
	Rafael J. Wysocki, Sami Tolvanen, Timur Tabi, linux-kernel,
	rust-for-linux

On Thu, May 1, 2025 at 1:32 AM Matthew Maurer <mmaurer@google.com> wrote:
>
> +// INVARIANT: The wrapped pointer will always be NULL, an error, or an owned DebugFS `dentry`

We do it the other way around, i.e. `# Invariants` section for the
type, and `// INVARIANT: ...` comments for the places that ensure the
type invariant.

+    ///    // parent exists in DebugFS here.

Parent ...

> +        // * If parent is None, parent accepts null pointers to mean create at root
> +        // * If parent is Some, parent accepts live dentry debugfs pointers

`None`, `Some` (also please end sentences with periods)

> +    /// Returns the pointer representation of the DebugFS directory.
> +    ///
> +    /// # Invariant
> +    ///
> +    /// The value returned from this function will always be an error code, NUL, or a live DebugFS

Hmm... so far we use `# Invariants` only for types. Perhaps you could
say something like "Thanks to the type invariant, the return value
will always be ..."

`NULL`?

Thanks!

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation
  2025-04-30 23:31 ` [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation Matthew Maurer
  2025-05-01 10:00   ` Miguel Ojeda
@ 2025-05-01 10:16   ` Danilo Krummrich
  2025-05-01 16:02     ` Matthew Maurer
  1 sibling, 1 reply; 15+ messages in thread
From: Danilo Krummrich @ 2025-05-01 10:16 UTC (permalink / raw)
  To: Matthew Maurer
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Sami Tolvanen, Timur Tabi, linux-kernel, rust-for-linux

On Wed, Apr 30, 2025 at 11:31:56PM +0000, Matthew Maurer wrote:
> 
> +    /// Create a DebugFS subdirectory.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// {
> +    ///    let parent = Dir::new(c_str!("parent"));
> +    ///    // parent exists in DebugFS here.
> +    ///    let child = parent.subdir(c_str!("child"));
> +    ///    // parent/child exists in DebugFS here.
> +    /// }
> +    /// // Neither exist here.
> +    /// ```
> +    pub fn subdir(&self, name: &CStr) -> Self {
> +        Self::create(name, Some(self))
> +    }

I think this should return a new type (SubDir), which is a transparent wrapper
of Dir and dereferences to Dir.

Subsequently, we can remove Dir::keep() implement SubDir::keep() instead. This
ensures that we can never call keep() on the root directory, which would always
be a bug.

As an alternative to the Deref impl, you can also implement
`From<SubDir> for Dir`, such that a SubDir can either be "kept" or converted to
a Dir. Probably, that's even better.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display
  2025-04-30 23:31 ` [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display Matthew Maurer
@ 2025-05-01 10:37   ` Danilo Krummrich
  2025-05-01 16:09     ` Matthew Maurer
  0 siblings, 1 reply; 15+ messages in thread
From: Danilo Krummrich @ 2025-05-01 10:37 UTC (permalink / raw)
  To: Matthew Maurer
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Sami Tolvanen, Timur Tabi, linux-kernel, rust-for-linux

On Wed, Apr 30, 2025 at 11:31:57PM +0000, Matthew Maurer wrote:
> Allows creation of files for references that live forever and lack
> metadata through the `Display` implementation.
> 
> The reference must live forever because we do not have a maximum
> lifetime for the file we are creating.
> 
> The `Display` implementation is used because `seq_printf` needs to route
> through `%pA`, which in turn routes through Arguments. A more generic
> API is provided later in the series, implemented in terms of this one.
> 
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
>  rust/kernel/debugfs.rs | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 102 insertions(+)
> 
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index b533ab21aaa775d4e3f33caf89e2d67ef85592f8..87de94da3b27c2a399bb377afd47280f65208d41 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -7,6 +7,7 @@
>  //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
>  
>  use crate::str::CStr;
> +use core::fmt::Display;
>  
>  /// Handle to a DebugFS directory.
>  // INVARIANT: The wrapped pointer will always be NULL, an error, or an owned DebugFS `dentry`
> @@ -121,6 +122,47 @@ fn as_ptr(&self) -> *mut bindings::dentry {
>      pub fn keep(self) {
>          core::mem::forget(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).keep();
> +    /// // "my_debugfs_dir/foo" now contains the number 200.
> +    /// ```
> +    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> Self {
> +        // 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.
> +        // * debugfs_create_file_full either returns an error code or a legal dentry pointer, so
> +        //   `Self::from_ptr` is safe to call here.
> +        #[cfg(CONFIG_DEBUG_FS)]
> +        unsafe {
> +            Self::from_ptr(bindings::debugfs_create_file_full(
> +                name.as_char_ptr(),
> +                0o444,
> +                self.as_ptr(),
> +                data as *const _ as *mut _,
> +                core::ptr::null(),
> +                &<T as DisplayFile>::VTABLE,
> +            ))
> +        }
> +        #[cfg(not(CONFIG_DEBUG_FS))]
> +        {
> +            // Mark parameters used
> +            let (_, _) = (name, data);
> +            Self()
> +        }
> +    }

Analogous to SubDir, this should be a new type, such that we can't leak the root
directory. Also, methods like subdir() don't really make sense for a file, no?

Besides that, don't we also need a separate type for a file to be able to attach
non-static data anyways? I.e. something like:

	#[cfg(CONFIG_DEBUG_FS)]
	struct File<T> {
	   dentry: *mut bindings::dentry,
	   data: T,
	}

	#[cfg(not(CONFIG_DEBUG_FS))]
	struct File<T> {
	   _p: PhantomData<T>,
	}

I'm not exactly sure how v1 did this; I haven't had time to look at v1 before v2
was posted. I seems like v1 relied on a separate structure storing the data,
which also held a reference to the corresponding dentry or something along those
lines?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation
  2025-05-01 10:16   ` Danilo Krummrich
@ 2025-05-01 16:02     ` Matthew Maurer
  2025-05-01 16:35       ` Danilo Krummrich
  0 siblings, 1 reply; 15+ messages in thread
From: Matthew Maurer @ 2025-05-01 16:02 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Sami Tolvanen, Timur Tabi, linux-kernel, rust-for-linux

On Thu, May 1, 2025 at 3:16 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Apr 30, 2025 at 11:31:56PM +0000, Matthew Maurer wrote:
> >
> > +    /// Create a DebugFS subdirectory.
> > +    ///
> > +    /// # Examples
> > +    ///
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// {
> > +    ///    let parent = Dir::new(c_str!("parent"));
> > +    ///    // parent exists in DebugFS here.
> > +    ///    let child = parent.subdir(c_str!("child"));
> > +    ///    // parent/child exists in DebugFS here.
> > +    /// }
> > +    /// // Neither exist here.
> > +    /// ```
> > +    pub fn subdir(&self, name: &CStr) -> Self {
> > +        Self::create(name, Some(self))
> > +    }
>
> I think this should return a new type (SubDir), which is a transparent wrapper
> of Dir and dereferences to Dir.
>
> Subsequently, we can remove Dir::keep() implement SubDir::keep() instead. This
> ensures that we can never call keep() on the root directory, which would always
> be a bug.
1. If the code in question is builtin rather than a module, discarding
this without tearing it down may not be a bug.
2. Users could always write `core::mem::forget()`, so this will always
be reachable (even if we decide to remove `::keep` to make it harder
to choose).
>
> As an alternative to the Deref impl, you can also implement
> `From<SubDir> for Dir`, such that a SubDir can either be "kept" or converted to
> a Dir. Probably, that's even better.

Yes, this was the "extra type complexity" I referenced in the cover
letter that I was considering doing. I think that probably what I'll
do for v3 is to have both the `Deref` *and* `From` implementation, so
that `SubDir` still automatically gets all of `Dir`s stuff, since your
later `File` comment convinces me we can't just have everything be
`Dir`.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display
  2025-05-01 10:37   ` Danilo Krummrich
@ 2025-05-01 16:09     ` Matthew Maurer
  2025-05-01 17:32       ` Danilo Krummrich
  0 siblings, 1 reply; 15+ messages in thread
From: Matthew Maurer @ 2025-05-01 16:09 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Sami Tolvanen, Timur Tabi, linux-kernel, rust-for-linux

On Thu, May 1, 2025 at 3:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Wed, Apr 30, 2025 at 11:31:57PM +0000, Matthew Maurer wrote:
> > Allows creation of files for references that live forever and lack
> > metadata through the `Display` implementation.
> >
> > The reference must live forever because we do not have a maximum
> > lifetime for the file we are creating.
> >
> > The `Display` implementation is used because `seq_printf` needs to route
> > through `%pA`, which in turn routes through Arguments. A more generic
> > API is provided later in the series, implemented in terms of this one.
> >
> > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> > ---
> >  rust/kernel/debugfs.rs | 102 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 102 insertions(+)
> >
> > diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> > index b533ab21aaa775d4e3f33caf89e2d67ef85592f8..87de94da3b27c2a399bb377afd47280f65208d41 100644
> > --- a/rust/kernel/debugfs.rs
> > +++ b/rust/kernel/debugfs.rs
> > @@ -7,6 +7,7 @@
> >  //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
> >
> >  use crate::str::CStr;
> > +use core::fmt::Display;
> >
> >  /// Handle to a DebugFS directory.
> >  // INVARIANT: The wrapped pointer will always be NULL, an error, or an owned DebugFS `dentry`
> > @@ -121,6 +122,47 @@ fn as_ptr(&self) -> *mut bindings::dentry {
> >      pub fn keep(self) {
> >          core::mem::forget(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).keep();
> > +    /// // "my_debugfs_dir/foo" now contains the number 200.
> > +    /// ```
> > +    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'static T) -> Self {
> > +        // 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.
> > +        // * debugfs_create_file_full either returns an error code or a legal dentry pointer, so
> > +        //   `Self::from_ptr` is safe to call here.
> > +        #[cfg(CONFIG_DEBUG_FS)]
> > +        unsafe {
> > +            Self::from_ptr(bindings::debugfs_create_file_full(
> > +                name.as_char_ptr(),
> > +                0o444,
> > +                self.as_ptr(),
> > +                data as *const _ as *mut _,
> > +                core::ptr::null(),
> > +                &<T as DisplayFile>::VTABLE,
> > +            ))
> > +        }
> > +        #[cfg(not(CONFIG_DEBUG_FS))]
> > +        {
> > +            // Mark parameters used
> > +            let (_, _) = (name, data);
> > +            Self()
> > +        }
> > +    }
>
> Analogous to SubDir, this should be a new type, such that we can't leak the root
> directory. Also, methods like subdir() don't really make sense for a file, no?
I agree, v3 will have a `File` type which:
1. Doesn't auto-discard the file, but provides a method to discard it.
2. Doesn't deref to `Dir`, so you can't call subdir.
>
> Besides that, don't we also need a separate type for a file to be able to attach
> non-static data anyways? I.e. something like:
>
>         #[cfg(CONFIG_DEBUG_FS)]
>         struct File<T> {
>            dentry: *mut bindings::dentry,
>            data: T,
>         }
>
>         #[cfg(not(CONFIG_DEBUG_FS))]
>         struct File<T> {
>            _p: PhantomData<T>,
>         }
>
> I'm not exactly sure how v1 did this; I haven't had time to look at v1 before v2
> was posted. I seems like v1 relied on a separate structure storing the data,
> which also held a reference to the corresponding dentry or something along those
> lines?
In v1, this was done via
```
#[pin_data]
struct Values<T> {
  dir: /* ignore this type */,
  #[pin]
  backing: T
}
```
Then, there was an interface that let the user provide a building
function which had to have a fully polymorphic lifetime, which would
be passed a backing reference that it was allowed to attach to
subdirectory files. Since the dir would be cleaned up before the
backing went away, we could know that it successfully outlived it.
It'll probably look a little different when I send the follow-up
series on top of this one.

Attaching to the root directory rather than each individual file made
sense to me because this meant that if you had
```
struct Foo {
  prop_a: u32,
  prop_b: u32
}
```
it would not be as tricky to attach `prop_a` to one file and `prop_b`
to another, because the directory would own `Foo`. This'll probably be
clearer when I send up a dependent series on top of v3 later today.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation
  2025-05-01 16:02     ` Matthew Maurer
@ 2025-05-01 16:35       ` Danilo Krummrich
  0 siblings, 0 replies; 15+ messages in thread
From: Danilo Krummrich @ 2025-05-01 16:35 UTC (permalink / raw)
  To: Matthew Maurer
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Sami Tolvanen, Timur Tabi, linux-kernel, rust-for-linux

On Thu, May 01, 2025 at 09:02:58AM -0700, Matthew Maurer wrote:
> On Thu, May 1, 2025 at 3:16 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Wed, Apr 30, 2025 at 11:31:56PM +0000, Matthew Maurer wrote:
> > >
> > > +    /// Create a DebugFS subdirectory.
> > > +    ///
> > > +    /// # Examples
> > > +    ///
> > > +    /// ```
> > > +    /// # use kernel::c_str;
> > > +    /// # use kernel::debugfs::Dir;
> > > +    /// {
> > > +    ///    let parent = Dir::new(c_str!("parent"));
> > > +    ///    // parent exists in DebugFS here.
> > > +    ///    let child = parent.subdir(c_str!("child"));
> > > +    ///    // parent/child exists in DebugFS here.
> > > +    /// }
> > > +    /// // Neither exist here.
> > > +    /// ```
> > > +    pub fn subdir(&self, name: &CStr) -> Self {
> > > +        Self::create(name, Some(self))
> > > +    }
> >
> > I think this should return a new type (SubDir), which is a transparent wrapper
> > of Dir and dereferences to Dir.
> >
> > Subsequently, we can remove Dir::keep() implement SubDir::keep() instead. This
> > ensures that we can never call keep() on the root directory, which would always
> > be a bug.
> 1. If the code in question is builtin rather than a module, discarding
> this without tearing it down may not be a bug.

True, if builtin *and* never intended to remove, it's indeed not a bug, but
arguably not very useful either.

> 2. Users could always write `core::mem::forget()`, so this will always
> be reachable (even if we decide to remove `::keep` to make it harder
> to choose).

Yet I wouldn't encourage users to create bugs by offering them a convenient way
to do so, without any benefit. :)

> > As an alternative to the Deref impl, you can also implement
> > `From<SubDir> for Dir`, such that a SubDir can either be "kept" or converted to
> > a Dir. Probably, that's even better.
> 
> Yes, this was the "extra type complexity" I referenced in the cover
> letter that I was considering doing. I think that probably what I'll
> do for v3 is to have both the `Deref` *and* `From` implementation, so
> that `SubDir` still automatically gets all of `Dir`s stuff, since your
> later `File` comment convinces me we can't just have everything be
> `Dir`.

Sounds good to me, thanks!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 4/4] rust: samples: Add debugfs sample
  2025-05-01  7:40   ` Greg Kroah-Hartman
@ 2025-05-01 16:44     ` Timur Tabi
  0 siblings, 0 replies; 15+ messages in thread
From: Timur Tabi @ 2025-05-01 16:44 UTC (permalink / raw)
  To: gregkh@linuxfoundation.org, mmaurer@google.com
  Cc: tmgross@umich.edu, benno.lossin@proton.me, rafael@kernel.org,
	gary@garyguo.net, a.hindborg@kernel.org,
	rust-for-linux@vger.kernel.org, bjorn3_gh@protonmail.com,
	boqun.feng@gmail.com, dakr@kernel.org, alex.gaynor@gmail.com,
	aliceryhl@google.com, ojeda@kernel.org,
	linux-kernel@vger.kernel.org, samitolvanen@google.com

On Thu, 2025-05-01 at 09:40 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 30, 2025 at 11:31:59PM +0000, Matthew Maurer wrote:
> > Provides an example of using the Rust DebugFS bindings.
> > 
> > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> 
> Much nicer, many thanks for this!
> 
> Some minor comments on the sample code here.  As someone coming from C
> with limited Rust experience, I think I do understand it, but I think it
> could use a bunch of comments to make it more "obvious" what is
> happening, see below.

Agree 100%.  I'm using this patch set to learn more about R4L.

> > +static EXAMPLE: AtomicU32 = AtomicU32::new(8);
> 
> Wait, why is this set to 8 and then you automatically set it to 10 after
> you create the file?  No one is ever going to see 8 as a valid value
> unless they really race to read the file, right?

Well, it does show that the driver still has the ability to write to EXAMPLE even after the debugfs
entry is created.  That demonstrates that the driver retains ownership, which is a useful test for
any debugfs implementation.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display
  2025-05-01 16:09     ` Matthew Maurer
@ 2025-05-01 17:32       ` Danilo Krummrich
  0 siblings, 0 replies; 15+ messages in thread
From: Danilo Krummrich @ 2025-05-01 17:32 UTC (permalink / raw)
  To: Matthew Maurer
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Greg Kroah-Hartman, Rafael J. Wysocki,
	Sami Tolvanen, Timur Tabi, linux-kernel, rust-for-linux

On Thu, May 01, 2025 at 09:09:27AM -0700, Matthew Maurer wrote:
> On Thu, May 1, 2025 at 3:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > On Wed, Apr 30, 2025 at 11:31:57PM +0000, Matthew Maurer wrote:
> > Besides that, don't we also need a separate type for a file to be able to attach
> > non-static data anyways? I.e. something like:
> >
> >         #[cfg(CONFIG_DEBUG_FS)]
> >         struct File<T> {
> >            dentry: *mut bindings::dentry,
> >            data: T,
> >         }
> >
> >         #[cfg(not(CONFIG_DEBUG_FS))]
> >         struct File<T> {
> >            _p: PhantomData<T>,
> >         }
> >
> > I'm not exactly sure how v1 did this; I haven't had time to look at v1 before v2
> > was posted. I seems like v1 relied on a separate structure storing the data,
> > which also held a reference to the corresponding dentry or something along those
> > lines?
> In v1, this was done via
> ```
> #[pin_data]
> struct Values<T> {
>   dir: /* ignore this type */,
>   #[pin]
>   backing: T
> }
> ```
> Then, there was an interface that let the user provide a building
> function which had to have a fully polymorphic lifetime, which would
> be passed a backing reference that it was allowed to attach to
> subdirectory files. Since the dir would be cleaned up before the
> backing went away, we could know that it successfully outlived it.
> It'll probably look a little different when I send the follow-up
> series on top of this one.
> 
> Attaching to the root directory rather than each individual file made
> sense to me because this meant that if you had
> ```
> struct Foo {
>   prop_a: u32,
>   prop_b: u32
> }
> ```
> it would not be as tricky to attach `prop_a` to one file and `prop_b`
> to another, because the directory would own `Foo`.

Thanks for the explanation! I need to think a bit more about this approach.

> This'll probably be
> clearer when I send up a dependent series on top of v3 later today.

At least from my side, no need to rush. :) I'll have quite limited bandwidth for
the next ~10 days anyways.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2025-05-01 17:32 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-30 23:31 [PATCH v2 0/4] rust: DebugFS Bindings Matthew Maurer
2025-04-30 23:31 ` [PATCH v2 1/4] rust: debugfs: Bind DebugFS directory creation Matthew Maurer
2025-05-01 10:00   ` Miguel Ojeda
2025-05-01 10:16   ` Danilo Krummrich
2025-05-01 16:02     ` Matthew Maurer
2025-05-01 16:35       ` Danilo Krummrich
2025-04-30 23:31 ` [PATCH v2 2/4] rust: debugfs: Bind file creation for long-lived Display Matthew Maurer
2025-05-01 10:37   ` Danilo Krummrich
2025-05-01 16:09     ` Matthew Maurer
2025-05-01 17:32       ` Danilo Krummrich
2025-04-30 23:31 ` [PATCH v2 3/4] rust: debugfs: Support format hooks Matthew Maurer
2025-05-01 10:00   ` Miguel Ojeda
2025-04-30 23:31 ` [PATCH v2 4/4] rust: samples: Add debugfs sample Matthew Maurer
2025-05-01  7:40   ` Greg Kroah-Hartman
2025-05-01 16:44     ` Timur Tabi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).