[PATCH 0/8] rust: DebugFS Bindings

[PATCH 0/8] rust: DebugFS Bindings

[Matthew Maurer]

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

The primary intended way to use these bindings is to attach a data
object to a DebugFS directory handle, and use the contents of that
attached object to serve what's inside the directory. Through a
combination of pinning and equivariance, we can ensure that the pointers
in the attached object will remain valid until after the DebugFS
directory is made inaccessible.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
Matthew Maurer (8):
      rust: debugfs: Bind DebugFS directory creation
      rust: debugfs: Bind file creation for long-lived Display
      rust: debugfs: Add scoped builder interface
      rust: debugfs: Allow subdir creation in builder interface
      rust: debugfs: Support format hooks
      rust: debugfs: Implement display_file in terms of fmt_file
      rust: debugfs: Helper macro for common case implementations
      rust: samples: Add debugfs sample

 MAINTAINERS                     |   2 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/dcache.c           |  12 +
 rust/helpers/helpers.c          |   1 +
 rust/kernel/debugfs.rs          | 514 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 samples/rust/Kconfig            |  11 +
 samples/rust/Makefile           |   1 +
 samples/rust/rust_debugfs.rs    | 120 ++++++++++
 9 files changed, 663 insertions(+)
---
base-commit: b6a7783d306baf3150ac54cd5124f6e85dd375b0
change-id: 20250428-debugfs-rust-3cd5c97eb7d1

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

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

[Matthew Maurer]

The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove`
is delayed until the more full-featured API, because we need to avoid
the user having an reference to a dir that is recursively removed.

Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/dcache.c           |  12 +++++
 rust/helpers/helpers.c          |   1 +
 rust/kernel/debugfs.rs          | 100 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 6 files changed, 116 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/helpers/dcache.c b/rust/helpers/dcache.c
new file mode 100644
index 0000000000000000000000000000000000000000..2396cdaa89a95a2be69fd84ec205e0f5f1b63f0c
--- /dev/null
+++ b/rust/helpers/dcache.c
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright (C) 2025 Google LLC.
+ */
+
+#include <linux/dcache.h>
+
+struct dentry *rust_helper_dget(struct dentry *d)
+{
+	return dget(d);
+}
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index f34320e6d1f2fb56cc151ee2ffe5d331713fd36a..95f486c1175191483297b7140b99f1aa364c081c 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -15,6 +15,7 @@
 #include "cpumask.c"
 #include "cred.c"
 #include "device.c"
+#include "dcache.c"
 #include "dma.c"
 #include "err.c"
 #include "fs.c"
diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
new file mode 100644
index 0000000000000000000000000000000000000000..4d06cce7099607f95b684bad329f791a815d3e86
--- /dev/null
+++ b/rust/kernel/debugfs.rs
@@ -0,0 +1,100 @@
+// 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::error::{from_err_ptr, Result};
+use crate::str::CStr;
+use crate::types::{ARef, AlwaysRefCounted, Opaque};
+use core::ptr::NonNull;
+
+/// Handle to a DebugFS directory.
+pub struct Dir {
+    inner: Opaque<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 {}
+
+// SAFETY: Dir is actually `dentry`, and dget/dput are the reference counting functions
+// for it.
+unsafe impl AlwaysRefCounted for Dir {
+    #[inline]
+    fn inc_ref(&self) {
+        // SAFETY: Since we have a reference to the directory,
+        // it's live, so it's safe to call dget on it.
+        unsafe {
+            kernel::bindings::dget(self.as_ptr());
+        }
+    }
+    #[inline]
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: By the caller precondition on the trait, we know that the caller has a reference
+        // count to the object.
+        unsafe {
+            kernel::bindings::dput(obj.cast().as_ptr());
+        }
+    }
+}
+
+impl Dir {
+    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// {
+    ///     let dir = Dir::new(c_str!("my_debug_dir"), None)?;
+    ///     // The directory will exist in DebugFS here.
+    /// }
+    /// // The directory will no longer exist in DebugFS here.
+    /// # Ok::<(), Error>(())
+    /// ```
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let parent = Dir::new(c_str!("parent"), None)?;
+    /// let child = Dir::new(c_str!("child"), Some(&parent))?;
+    /// // parent/child exists in DebugFS here.
+    /// drop(parent);
+    /// // The child dentry is still valid here, but DebugFS will have neither directory.
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn new(name: &CStr, parent: Option<&Self>) -> Result<ARef<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 `NonNull::new_unchecked`.
+        let dir = unsafe {
+            NonNull::new_unchecked(from_err_ptr(kernel::bindings::debugfs_create_dir(
+                name.as_char_ptr(),
+                parent_ptr,
+            ))?)
+        };
+        // SAFETY: Dir is a transparent wrapper for an Opaque<dentry>, and we received a live
+        // owning dentry from `debugfs_create_dir`, so we can wrap it in an ARef
+        Ok(unsafe { ARef::from_raw(dir.cast()) })
+    }
+
+    fn as_ptr(&self) -> *mut bindings::dentry {
+        self.inner.get()
+    }
+}
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.901.g37484f566f-goog

Re: [PATCH 1/8] rust: debugfs: Bind DebugFS directory creation

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote:
> The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove`
> is delayed until the more full-featured API, because we need to avoid
> the user having an reference to a dir that is recursively removed.
> 
> Signed-off-by: Matthew Maurer <mmaurer@google.com>

First off, many thanks for doing this.  I like this in general, but I
have loads of specific questions/comments.  Don't take that as a
criticism of this feature, I really want these bindings to be in the
tree and work hopefully better/cleaner than the userspace ones do.

First off, the main "rule" of debugfs is that you should NEVER care
about the return value of any debugfs function.  So much so that the C
side hides errors almost entirely where possible.  I'd like to see this
carried through to the Rust side as well, but I think you didn't do that
for various "traditional" reasons.

Please try to unwind all of that in your next version, I'll point this
out below in one place:


> ---
>  MAINTAINERS                     |   1 +
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/dcache.c           |  12 +++++
>  rust/helpers/helpers.c          |   1 +
>  rust/kernel/debugfs.rs          | 100 ++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   1 +
>  6 files changed, 116 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/helpers/dcache.c b/rust/helpers/dcache.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..2396cdaa89a95a2be69fd84ec205e0f5f1b63f0c
> --- /dev/null
> +++ b/rust/helpers/dcache.c
> @@ -0,0 +1,12 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Copyright (C) 2025 Google LLC.
> + */
> +
> +#include <linux/dcache.h>
> +
> +struct dentry *rust_helper_dget(struct dentry *d)
> +{
> +	return dget(d);
> +}
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index f34320e6d1f2fb56cc151ee2ffe5d331713fd36a..95f486c1175191483297b7140b99f1aa364c081c 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -15,6 +15,7 @@
>  #include "cpumask.c"
>  #include "cred.c"
>  #include "device.c"
> +#include "dcache.c"
>  #include "dma.c"
>  #include "err.c"
>  #include "fs.c"
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4d06cce7099607f95b684bad329f791a815d3e86
> --- /dev/null
> +++ b/rust/kernel/debugfs.rs
> @@ -0,0 +1,100 @@
> +// 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::error::{from_err_ptr, Result};
> +use crate::str::CStr;
> +use crate::types::{ARef, AlwaysRefCounted, Opaque};
> +use core::ptr::NonNull;
> +
> +/// Handle to a DebugFS directory.
> +pub struct Dir {
> +    inner: Opaque<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 {}
> +
> +// SAFETY: Dir is actually `dentry`, and dget/dput are the reference counting functions
> +// for it.
> +unsafe impl AlwaysRefCounted for Dir {
> +    #[inline]
> +    fn inc_ref(&self) {
> +        // SAFETY: Since we have a reference to the directory,
> +        // it's live, so it's safe to call dget on it.
> +        unsafe {
> +            kernel::bindings::dget(self.as_ptr());
> +        }
> +    }
> +    #[inline]
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: By the caller precondition on the trait, we know that the caller has a reference
> +        // count to the object.
> +        unsafe {
> +            kernel::bindings::dput(obj.cast().as_ptr());
> +        }
> +    }
> +}
> +
> +impl Dir {
> +    /// Create a new directory in DebugFS. If `parent` is [`None`], it will be created at the root.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// {
> +    ///     let dir = Dir::new(c_str!("my_debug_dir"), None)?;
> +    ///     // The directory will exist in DebugFS here.
> +    /// }
> +    /// // The directory will no longer exist in DebugFS here.
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let parent = Dir::new(c_str!("parent"), None)?;

Why are you checking this?  You shouldn't, it should never return an
error.  What it returns you can ALWAYS pass back into any call that
expects to get a "debugfs::Dir" object.  You should never even be able
to know if it succeeded or not as your code should never do anything
different depending on the result.

> +    /// let child = Dir::new(c_str!("child"), Some(&parent))?;
> +    /// // parent/child exists in DebugFS here.
> +    /// drop(parent);

Wait, what?  Why would an explicit drop(parent) be required here?  That
feels wrong, and way too complex.  Why can't you rely on the child
creation to properly manage this if needed (hint, it shouldn't be.)

> +    /// // The child dentry is still valid here, but DebugFS will have neither directory.
> +    /// # Ok::<(), Error>(())

Again, no error checking please.

> +    /// ```
> +    pub fn new(name: &CStr, parent: Option<&Self>) -> Result<ARef<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 `NonNull::new_unchecked`.
> +        let dir = unsafe {
> +            NonNull::new_unchecked(from_err_ptr(kernel::bindings::debugfs_create_dir(
> +                name.as_char_ptr(),
> +                parent_ptr,
> +            ))?)
> +        };
> +        // SAFETY: Dir is a transparent wrapper for an Opaque<dentry>, and we received a live
> +        // owning dentry from `debugfs_create_dir`, so we can wrap it in an ARef
> +        Ok(unsafe { ARef::from_raw(dir.cast()) })

Why do you want this wrapped?  And I think you "wrapped" it wrong here.
When the object is "gone" it should have called
debugfs_remove_recursive(), not dput(), in order to properly drop
everything.  Where is that call happening?

The debugfs core already handles the reference counting of this object,
please don't add some extra reference count calls with dput/get, that's
just going to be a mess.

You should NEVER be encoding the fact that the return value of a
debugfs_*() call is a dentry.  Just treat that as an opaque pointer that
just happens to have a common name that might refer to something else.
That pointer can be passed back into other debugfs functions, and THAT
IS IT.  No checking for it, no passing it to any vfs functions, or
anything else.

So the dput() stuff here should not be present at all (hint, no C code
that used debugfs ever calls that, so the rust bindings shouldn't
either.)

thanks,

greg k-h

Re: [PATCH 1/8] rust: debugfs: Bind DebugFS directory creation

[Matthew Maurer]

On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote:
> > The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove`
> > is delayed until the more full-featured API, because we need to avoid
> > the user having an reference to a dir that is recursively removed.
> >
> > Signed-off-by: Matthew Maurer <mmaurer@google.com>
>
> First off, many thanks for doing this.  I like this in general, but I
> have loads of specific questions/comments.  Don't take that as a
> criticism of this feature, I really want these bindings to be in the
> tree and work hopefully better/cleaner than the userspace ones do.
>
> First off, the main "rule" of debugfs is that you should NEVER care
> about the return value of any debugfs function.  So much so that the C
> side hides errors almost entirely where possible.  I'd like to see this
> carried through to the Rust side as well, but I think you didn't do that
> for various "traditional" reasons.

Sure, I mostly had to do error checking because I was using an
`ARef<Dir>` to represent a directory, which meant that the underlying
directory needed to be a valid pointer. Given that you've said that
the returned `dentry *` should never be used as an actual `dentry`,
only as an abstract handle to a DebugFS object, that requirement goes
away, and I can remove the error handling code and always successfully
return a `Dir`, even in cases where an error has occurred.

>
> Wait, what?  Why would an explicit drop(parent) be required here?  That
> feels wrong, and way too complex.  Why can't you rely on the child
> creation to properly manage this if needed (hint, it shouldn't be.)

The explicit `drop` is not required for normal usage, it was intended
to be illustrative - I was trying to explain what the semantics would
be if the parent `dentry` was released before the child was. As
before, now that the child will not be an `ARef<Dir>`, and so not
assumed to be a valid `dentry` pointer, this example won't be relevant
anymore.

>
> Why do you want this wrapped?  And I think you "wrapped" it wrong here.
> When the object is "gone" it should have called
> debugfs_remove_recursive(), not dput(), in order to properly drop
> everything.  Where is that call happening?
>
> The debugfs core already handles the reference counting of this object,
> please don't add some extra reference count calls with dput/get, that's
> just going to be a mess.
>
> You should NEVER be encoding the fact that the return value of a
> debugfs_*() call is a dentry.  Just treat that as an opaque pointer that
> just happens to have a common name that might refer to something else.
> That pointer can be passed back into other debugfs functions, and THAT
> IS IT.  No checking for it, no passing it to any vfs functions, or
> anything else.
>
> So the dput() stuff here should not be present at all (hint, no C code
> that used debugfs ever calls that, so the rust bindings shouldn't
> either.)
>

I incorrectly assumed that if a `dentry *` was being returned, then at
some point it would be used as a `dentry *` rather than as a
debugfs-only handle. I bound it into an `ARef` to avoid backing myself
into a corner later if someone needed to use the `dentry *` for
something, but if that's against the API contract of DebugFS, I can
avoid it entirely.

Re: [PATCH 1/8] rust: debugfs: Bind DebugFS directory creation

[Greg Kroah-Hartman]

On Wed, Apr 30, 2025 at 08:10:44AM -0700, Matthew Maurer wrote:
> On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote:
> > > The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove`
> > > is delayed until the more full-featured API, because we need to avoid
> > > the user having an reference to a dir that is recursively removed.
> > >
> > > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> >
> > First off, many thanks for doing this.  I like this in general, but I
> > have loads of specific questions/comments.  Don't take that as a
> > criticism of this feature, I really want these bindings to be in the
> > tree and work hopefully better/cleaner than the userspace ones do.
> >
> > First off, the main "rule" of debugfs is that you should NEVER care
> > about the return value of any debugfs function.  So much so that the C
> > side hides errors almost entirely where possible.  I'd like to see this
> > carried through to the Rust side as well, but I think you didn't do that
> > for various "traditional" reasons.
> 
> Sure, I mostly had to do error checking because I was using an
> `ARef<Dir>` to represent a directory, which meant that the underlying
> directory needed to be a valid pointer. Given that you've said that
> the returned `dentry *` should never be used as an actual `dentry`,
> only as an abstract handle to a DebugFS object, that requirement goes
> away, and I can remove the error handling code and always successfully
> return a `Dir`, even in cases where an error has occurred.

Great!

Except when debugfs is not enabled, then what are you going to return?
The same structure, or an error?

I'd vote for the same pointer to the structure, just to make it more
obvious that this is a totally opaque thing and that no caller should
ever know or care if debugfs is working or even present in the system.

Note that some drivers will want to save a bit of space if debugfs is
not enabled in the build, so be prepared to make the binding work
somehow that way too.  Can you have an "empty" object that takes no
memory?  Or is this too overthinking things?

> > Wait, what?  Why would an explicit drop(parent) be required here?  That
> > feels wrong, and way too complex.  Why can't you rely on the child
> > creation to properly manage this if needed (hint, it shouldn't be.)
> 
> The explicit `drop` is not required for normal usage, it was intended
> to be illustrative - I was trying to explain what the semantics would
> be if the parent `dentry` was released before the child was. As
> before, now that the child will not be an `ARef<Dir>`, and so not
> assumed to be a valid `dentry` pointer, this example won't be relevant
> anymore.

Great!

thanks, hopefully this should make things simpler.

greg k-h

Re: [PATCH 1/8] rust: debugfs: Bind DebugFS directory creation

[Matthew Maurer]

On Wed, Apr 30, 2025 at 8:23 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Apr 30, 2025 at 08:10:44AM -0700, Matthew Maurer wrote:
> > On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote:
> > > > The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove`
> > > > is delayed until the more full-featured API, because we need to avoid
> > > > the user having an reference to a dir that is recursively removed.
> > > >
> > > > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> > >
> > > First off, many thanks for doing this.  I like this in general, but I
> > > have loads of specific questions/comments.  Don't take that as a
> > > criticism of this feature, I really want these bindings to be in the
> > > tree and work hopefully better/cleaner than the userspace ones do.
> > >
> > > First off, the main "rule" of debugfs is that you should NEVER care
> > > about the return value of any debugfs function.  So much so that the C
> > > side hides errors almost entirely where possible.  I'd like to see this
> > > carried through to the Rust side as well, but I think you didn't do that
> > > for various "traditional" reasons.
> >
> > Sure, I mostly had to do error checking because I was using an
> > `ARef<Dir>` to represent a directory, which meant that the underlying
> > directory needed to be a valid pointer. Given that you've said that
> > the returned `dentry *` should never be used as an actual `dentry`,
> > only as an abstract handle to a DebugFS object, that requirement goes
> > away, and I can remove the error handling code and always successfully
> > return a `Dir`, even in cases where an error has occurred.
>
> Great!
>
> Except when debugfs is not enabled, then what are you going to return?
> The same structure, or an error?
>
> I'd vote for the same pointer to the structure, just to make it more
> obvious that this is a totally opaque thing and that no caller should
> ever know or care if debugfs is working or even present in the system.
>
> Note that some drivers will want to save a bit of space if debugfs is
> not enabled in the build, so be prepared to make the binding work
> somehow that way too.  Can you have an "empty" object that takes no
> memory?  Or is this too overthinking things?

Based on what you've expressed, I think what makes sense is:

* Initial patch will always return the same `Dir`, just sometimes it
will be a wrapper around a pointer that is an error code, and
sometimes it will be a useful `dentry` pointer. This will match the
current behavior of C code to my understanding.
* Follow-up (probably still in this series) will check
`CONFIG_DEBUG_FS`, and if it's off, will just make `Dir` into a ZST,
and just discard the pointer. This would be an improvement upon the C
interface, because drivers would get the shrinkage without needing to
add conditionals on `CONFIG_DEBUG_FS` in their own driver.

>
> > > Wait, what?  Why would an explicit drop(parent) be required here?  That
> > > feels wrong, and way too complex.  Why can't you rely on the child
> > > creation to properly manage this if needed (hint, it shouldn't be.)
> >
> > The explicit `drop` is not required for normal usage, it was intended
> > to be illustrative - I was trying to explain what the semantics would
> > be if the parent `dentry` was released before the child was. As
> > before, now that the child will not be an `ARef<Dir>`, and so not
> > assumed to be a valid `dentry` pointer, this example won't be relevant
> > anymore.
>
> Great!
>
> thanks, hopefully this should make things simpler.
>
> greg k-h

Re: [PATCH 1/8] rust: debugfs: Bind DebugFS directory creation

[Greg Kroah-Hartman]

On Wed, Apr 30, 2025 at 08:31:29AM -0700, Matthew Maurer wrote:
> On Wed, Apr 30, 2025 at 8:23 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Apr 30, 2025 at 08:10:44AM -0700, Matthew Maurer wrote:
> > > On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Tue, Apr 29, 2025 at 11:15:55PM +0000, Matthew Maurer wrote:
> > > > > The basic API relies on `dput` to prevent leaks. Use of `debugfs_remove`
> > > > > is delayed until the more full-featured API, because we need to avoid
> > > > > the user having an reference to a dir that is recursively removed.
> > > > >
> > > > > Signed-off-by: Matthew Maurer <mmaurer@google.com>
> > > >
> > > > First off, many thanks for doing this.  I like this in general, but I
> > > > have loads of specific questions/comments.  Don't take that as a
> > > > criticism of this feature, I really want these bindings to be in the
> > > > tree and work hopefully better/cleaner than the userspace ones do.
> > > >
> > > > First off, the main "rule" of debugfs is that you should NEVER care
> > > > about the return value of any debugfs function.  So much so that the C
> > > > side hides errors almost entirely where possible.  I'd like to see this
> > > > carried through to the Rust side as well, but I think you didn't do that
> > > > for various "traditional" reasons.
> > >
> > > Sure, I mostly had to do error checking because I was using an
> > > `ARef<Dir>` to represent a directory, which meant that the underlying
> > > directory needed to be a valid pointer. Given that you've said that
> > > the returned `dentry *` should never be used as an actual `dentry`,
> > > only as an abstract handle to a DebugFS object, that requirement goes
> > > away, and I can remove the error handling code and always successfully
> > > return a `Dir`, even in cases where an error has occurred.
> >
> > Great!
> >
> > Except when debugfs is not enabled, then what are you going to return?
> > The same structure, or an error?
> >
> > I'd vote for the same pointer to the structure, just to make it more
> > obvious that this is a totally opaque thing and that no caller should
> > ever know or care if debugfs is working or even present in the system.
> >
> > Note that some drivers will want to save a bit of space if debugfs is
> > not enabled in the build, so be prepared to make the binding work
> > somehow that way too.  Can you have an "empty" object that takes no
> > memory?  Or is this too overthinking things?
> 
> Based on what you've expressed, I think what makes sense is:
> 
> * Initial patch will always return the same `Dir`, just sometimes it
> will be a wrapper around a pointer that is an error code, and
> sometimes it will be a useful `dentry` pointer. This will match the
> current behavior of C code to my understanding.

Great.

> * Follow-up (probably still in this series) will check
> `CONFIG_DEBUG_FS`, and if it's off, will just make `Dir` into a ZST,
> and just discard the pointer. This would be an improvement upon the C
> interface, because drivers would get the shrinkage without needing to
> add conditionals on `CONFIG_DEBUG_FS` in their own driver.

You're going to have to check CONFIG_DEBUG_FS anyway for this series,
otherwise things aren't going to build properly, so this sounds like a
great way to handle that.

thanks,

greg k-h

[PATCH 2/8] 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 reference must live forever because the corresponding file is
reference counted, so there's no way to say the lifetime outlives it
otherwise. This restriction will be relaxed later in the series through
use of `debugfs_remove`.

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.

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

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 4d06cce7099607f95b684bad329f791a815d3e86..b20df5fce692b3047c804f7ad5af90fc4248979b 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -7,8 +7,11 @@
 //! C header: [`include/linux/debugfs.h`](srctree/include/linux/debugfs.h)
 
 use crate::error::{from_err_ptr, Result};
+use crate::seq_file::SeqFile;
+use crate::seq_print;
 use crate::str::CStr;
 use crate::types::{ARef, AlwaysRefCounted, Opaque};
+use core::fmt::Display;
 use core::ptr::NonNull;
 
 /// Handle to a DebugFS directory.
@@ -97,4 +100,95 @@ pub fn new(name: &CStr, parent: Option<&Self>) -> Result<ARef<Self>> {
     fn as_ptr(&self) -> *mut bindings::dentry {
         self.inner.get()
     }
+
+    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
+    /// [`Display::fmt`] on the provided reference.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let dir = Dir::new(c_str!("my_debugfs_dir"), None)?;
+    /// let file = dir.display_file(c_str!("foo"), &200)?;
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn display_file<T: Display + Sized>(
+        &self,
+        name: &CStr,
+        data: &'static T,
+    ) -> Result<ARef<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
+        //   `NonNull::new_unchecked` is safe here.
+        let ptr = unsafe {
+            NonNull::new_unchecked(from_err_ptr(kernel::bindings::debugfs_create_file_full(
+                name.as_char_ptr(),
+                0444,
+                self.as_ptr(),
+                data as *const _ as *mut _,
+                core::ptr::null(),
+                &<T as DisplayFile>::VTABLE,
+            ))?)
+        };
+        // SAFETY: Dir is a transparent wrapper for an Opaque<dentry>, and we received a live
+        // owning dentry from debugfs_create_dir, so we can wrap it in an ARef
+        Ok(unsafe { ARef::from_raw(ptr.cast()) })
+    }
+}
+
+/// 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
+unsafe extern "C" fn display_open<T: Display>(
+    inode: *mut kernel::bindings::inode,
+    file: *mut kernel::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 { kernel::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.
+unsafe extern "C" fn display_act<T: Display>(
+    seq: *mut kernel::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
+trait DisplayFile: Display + Sized {
+    const VTABLE: kernel::bindings::file_operations = kernel::bindings::file_operations {
+        read: Some(kernel::bindings::seq_read),
+        llseek: Some(kernel::bindings::seq_lseek),
+        release: Some(kernel::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 {}

-- 
2.49.0.901.g37484f566f-goog

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

[Miguel Ojeda]

Hi Matthew,

Since I was playing around with this series, I noticed Clippy
complained about a few things (please see `make ..... CLIPPY=1`):

On Wed, Apr 30, 2025 at 1:16 AM Matthew Maurer <mmaurer@google.com> wrote:
>
> +                0444,

This is decimal rather than octal.

> +// # Safety
> +// * seq must point to a live seq_file whose private data is a live pointer to a T which is not
> +//   being mutated.

This should be `///` (also, since I am here, please use Markdown etc.).

There are a couple other classes of lints that Clippy mentions that
should be cleaned up or allowed elsewhere.

Thanks!

Cheers,
Miguel

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

[Matthew Maurer]

On Tue, Apr 29, 2025 at 8:27 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Hi Matthew,
>
> Since I was playing around with this series, I noticed Clippy
> complained about a few things (please see `make ..... CLIPPY=1`):
>
> On Wed, Apr 30, 2025 at 1:16 AM Matthew Maurer <mmaurer@google.com> wrote:
> >
> > +                0444,
>
> This is decimal rather than octal.

Oops, will-fix.

>
> > +// # Safety
> > +// * seq must point to a live seq_file whose private data is a live pointer to a T which is not
> > +//   being mutated.
>
> This should be `///` (also, since I am here, please use Markdown etc.).

I thought I used markdown everywhere, I'll do an extra pass before I
upload the next version.

>
> There are a couple other classes of lints that Clippy mentions that
> should be cleaned up or allowed elsewhere.

It looks like when I transitioned over from running on a physical
board to running on an emulator I typo'd my clippy-check script, sorry
about that. I've got the errors confirmed coming out now, so I'll be
clean there next time.

>
> Thanks!
>
> Cheers,
> Miguel

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

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 11:15:56PM +0000, Matthew Maurer wrote:
> Allows creation of files for references that live forever and lack
> metadata through the `Display` implementation.

What do you mean "live forever"?  There's always a structure somewhere
that holds it, when it goes away the file/directory should also go away.

> The reference must live forever because the corresponding file is
> reference counted, so there's no way to say the lifetime outlives it
> otherwise. This restriction will be relaxed later in the series through
> use of `debugfs_remove`.

Why not use that from the beginning?

> 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.

Building stuff up to review only to remove it later makes it harder to
review :)

thanks,

greg k-h

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

[Matthew Maurer]

On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 29, 2025 at 11:15:56PM +0000, Matthew Maurer wrote:
> > Allows creation of files for references that live forever and lack
> > metadata through the `Display` implementation.
>
> What do you mean "live forever"?  There's always a structure somewhere
> that holds it, when it goes away the file/directory should also go away.

That's the builder implementation I mentioned later - it lets you
connect the directory to a resource. This is an initial simpler
version to keep the patches small which lets you export e.g. the
contents of global variables safely.

>
> > The reference must live forever because the corresponding file is
> > reference counted, so there's no way to say the lifetime outlives it
> > otherwise. This restriction will be relaxed later in the series through
> > use of `debugfs_remove`.
>
> Why not use that from the beginning?

Because that requires the builder API + pinning, which I thought would
be a more complex conversation, and wanted to keep the "just register
a file" patch separate from the "here's how you ensure that data lives
at least as long as the debugfs directory" patch.

>
> > 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.
>
> Building stuff up to review only to remove it later makes it harder to
> review :)

It doesn't really get removed - `display_file_raw` is still the
underlying implementation for the more generic API, and this API stays
around as a convenience API.

>
> thanks,
>
> greg k-h

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

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 11:15:56PM +0000, Matthew Maurer wrote:
> +    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
> +    /// [`Display::fmt`] on the provided reference.
> +    ///
> +    /// # Example
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::Dir;
> +    /// let dir = Dir::new(c_str!("my_debugfs_dir"), None)?;

Again, can never fail, so don't check it.

> +    /// let file = dir.display_file(c_str!("foo"), &200)?;

Again, can never fail, so don't check it.

And "200" is wrong, did you test these?


> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    pub fn display_file<T: Display + Sized>(
> +        &self,
> +        name: &CStr,
> +        data: &'static T,
> +    ) -> Result<ARef<Self>> {

Again, will always succeed, you don't want any checking of Result<>

thanks,

greg k-h

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

[Matthew Maurer]

On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Apr 29, 2025 at 11:15:56PM +0000, Matthew Maurer wrote:
> > +    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
> > +    /// [`Display::fmt`] on the provided reference.
> > +    ///
> > +    /// # Example
> > +    ///
> > +    /// ```
> > +    /// # use kernel::c_str;
> > +    /// # use kernel::debugfs::Dir;
> > +    /// let dir = Dir::new(c_str!("my_debugfs_dir"), None)?;
>
> Again, can never fail, so don't check it.
>
> > +    /// let file = dir.display_file(c_str!("foo"), &200)?;
>
> Again, can never fail, so don't check it.
>
> And "200" is wrong, did you test these?

How is 200 wrong? This displays the number "200" in a file called
"foo" in the directory "my_debugfs_dir"?

>
>
> > +    /// # Ok::<(), Error>(())
> > +    /// ```
> > +    pub fn display_file<T: Display + Sized>(
> > +        &self,
> > +        name: &CStr,
> > +        data: &'static T,
> > +    ) -> Result<ARef<Self>> {
>
> Again, will always succeed, you don't want any checking of Result<>
>
> thanks,
>
> greg k-h

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

[Greg Kroah-Hartman]

On Wed, Apr 30, 2025 at 08:12:09AM -0700, Matthew Maurer wrote:
> On Wed, Apr 30, 2025 at 5:06 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Apr 29, 2025 at 11:15:56PM +0000, Matthew Maurer wrote:
> > > +    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
> > > +    /// [`Display::fmt`] on the provided reference.
> > > +    ///
> > > +    /// # Example
> > > +    ///
> > > +    /// ```
> > > +    /// # use kernel::c_str;
> > > +    /// # use kernel::debugfs::Dir;
> > > +    /// let dir = Dir::new(c_str!("my_debugfs_dir"), None)?;
> >
> > Again, can never fail, so don't check it.
> >
> > > +    /// let file = dir.display_file(c_str!("foo"), &200)?;
> >
> > Again, can never fail, so don't check it.
> >
> > And "200" is wrong, did you test these?
> 
> How is 200 wrong? This displays the number "200" in a file called
> "foo" in the directory "my_debugfs_dir"?

Oops, sorry, I thought that was the "mode" of the file being created :)

greg k-h

[PATCH 3/8] rust: debugfs: Add scoped builder interface

[Matthew Maurer]

This adds an interface which allows access to references which may not
live indefinitely by forcing them to live at least as long as the
DebugFS directory itself.

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

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index b20df5fce692b3047c804f7ad5af90fc4248979b..f6240fd056f8598d5ef06bdaf61c5c33eab5b734 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -12,7 +12,12 @@
 use crate::str::CStr;
 use crate::types::{ARef, AlwaysRefCounted, Opaque};
 use core::fmt::Display;
+use core::marker::{PhantomData, PhantomPinned};
+use core::mem::ManuallyDrop;
+use core::ops::Deref;
+use core::pin::Pin;
 use core::ptr::NonNull;
+use pin_init::{pin_data, pinned_drop, PinInit};
 
 /// Handle to a DebugFS directory.
 pub struct Dir {
@@ -117,6 +122,22 @@ pub fn display_file<T: Display + Sized>(
         &self,
         name: &CStr,
         data: &'static T,
+    ) -> Result<ARef<Self>> {
+        // SAFETY: As `data` lives for the static lifetime, it outlives the file.
+        unsafe { self.display_file_raw(name, data) }
+    }
+
+    /// 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,
     ) -> Result<ARef<Self>> {
         // SAFETY:
         // * `name` is a NUL-terminated C string, living across the call, by CStr invariant
@@ -192,3 +213,145 @@ trait DisplayFile: Display + Sized {
 }
 
 impl<T: Display + Sized> DisplayFile for T {}
+
+#[pin_data(PinnedDrop)]
+/// A DebugFS directory combined with a backing store for data to implement it
+pub struct Values<T> {
+    #[pin]
+    backing: T,
+    // Calling `debugfs_remove`, as we will do in our `Drop` impl, will consume the refcount we
+    // hold after cleaning up all the child directories.
+    dir: ManuallyDrop<ARef<Dir>>,
+    // Since the files present under our directory may point into backing, we are !Unpin
+    #[pin]
+    _pin: PhantomPinned,
+}
+
+impl<T> Deref for Values<T> {
+    type Target = T;
+    fn deref(&self) -> &T {
+        &self.backing
+    }
+}
+
+impl<T> Values<T> {
+    /// Attach backing data to a DebugFS directory. When the resulting object is destroyed, the
+    /// DebugFS directory will be recursively removed as well.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::{Dir, Values};
+    /// let dir = Dir::new(c_str!("foo"), None)?;
+    /// let _foo = KBox::pin_init(Values::attach(0, dir), GFP_KERNEL)?;
+    /// // foo can now be used with `Values::build` to allow access to the attached value when
+    /// // printing
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn attach(backing: impl PinInit<T>, dir: ARef<Dir>) -> impl PinInit<Self> {
+        pin_init::pin_init! { Self {
+            backing <- backing,
+            dir: ManuallyDrop::new(dir),
+            _pin: PhantomPinned,
+        } }
+    }
+
+    /// Runs a closure which has access to the backing data and a builder that will allow you to
+    /// build a DebugFS structure off the backing data using its methods.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::{Dir, Values};
+    /// let dir = Dir::new(c_str!("foo"), None)?;
+    /// let foo = KBox::pin_init(Values::attach(0, dir), GFP_KERNEL)?;
+    /// foo.as_ref().build(|_value, _builder| {
+    ///   // Construct debugfs with access to the attached value here
+    /// });
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn build<U, F: for<'a> FnOnce(&'a T, Builder<'a>) -> U>(self: Pin<&Self>, f: F) -> U {
+        // SAFETY: The Builder produced here is technically at the lifetime of self, but is
+        // being used only in a universal context, so that information is immediately erased and
+        // replaced with the universally quantified 'a. By taking a Pin<&Self>, we enforce that
+        // self.backing remains alive for any 'a less than the lifetime of the struct. By not
+        // providing any mutable access to self.backing, we ensure that it's always safe to
+        // materialize a read-only reference to &self.backing for any 'a less than the lifetime of
+        // the struct.
+        f(&self.backing, unsafe { Builder::new(&self.dir) })
+    }
+}
+
+#[pinned_drop]
+impl<T> PinnedDrop for Values<T> {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY: Our internal dir holds its own reference count, so it's always valid.
+        unsafe { kernel::bindings::debugfs_remove(self.dir.as_ptr()) }
+    }
+}
+
+/// A Dir, scoped to the lifetime for which it will exist. Unlike `&'a Dir`, this is equivariant,
+/// preventing the shortening of the lifetime.
+///
+/// # Invariants
+/// Builder will only ever be used with 'static or a universally quantified lifetime that is
+/// unified only with the lifetime of data structures guaranteed to outlive it and not have mutable
+/// references taken.
+#[repr(transparent)]
+#[derive(Copy, Clone)]
+pub struct Builder<'a> {
+    inner: &'a Dir,
+    _equivariant: PhantomData<fn(&'a ()) -> &'a ()>,
+}
+
+impl<'a> Builder<'a> {
+    /// # Safety
+    /// Caller must promise to use this function at static lifetime or only expose it to
+    /// universally quantified functions, unified only with lifetimes guaranteed to extend beyond
+    /// when the directory will be rendered inaccessible.
+    unsafe fn new(inner: &'a Dir) -> Self {
+        Self {
+            inner,
+            _equivariant: PhantomData,
+        }
+    }
+
+    /// Create a file in a DebugFS directory with the provided name, and contents from invoking
+    /// [`Display::fmt`] on the provided reference.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use kernel::{try_pin_init, c_str};
+    /// # use pin_init::stack_try_pin_init;
+    /// # use kernel::debugfs::{Dir, Values};
+    /// let dir = Dir::new(c_str!("foo"), None)?;
+    /// stack_try_pin_init!(let foo =? Values::attach((1, 2), dir));
+    /// foo.as_ref().build(|value, builder| {
+    ///   builder.display_file(c_str!("bar"), &value.0)?;
+    ///   builder.display_file(c_str!("baz"), &value.1)
+    /// })?;
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'a T) -> Result<()> {
+        // We forget the reference because its reference count is implicitly "owned" by the root
+        // builder, which we know will use `debugfs_remove` to clean this up. If we release the
+        // file here, it will be immediately deleted.
+        // SAFETY:
+        // Because `Builder`'s invariant says that our lifetime is how long the directory will
+        // be available, and is equivariant, `'a` will outlive the base directory, which will be
+        // torn down by `debugfs_remove` to prevent access even if an extra refcount is held
+        // somewhere.
+        core::mem::forget(unsafe { self.inner.display_file_raw(name, data)? });
+        Ok(())
+    }
+}
+
+impl<'a> Deref for Builder<'a> {
+    type Target = Dir;
+    fn deref(&self) -> &Self::Target {
+        &self.inner
+    }
+}

-- 
2.49.0.901.g37484f566f-goog

Re: [PATCH 3/8] rust: debugfs: Add scoped builder interface

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 11:15:57PM +0000, Matthew Maurer wrote:
> This adds an interface which allows access to references which may not
> live indefinitely by forcing them to live at least as long as the
> DebugFS directory itself.

They can ONLY live as long as the debugfs directory lives, is that what
you mean here?

> 
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
>  rust/kernel/debugfs.rs | 163 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 163 insertions(+)
> 
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index b20df5fce692b3047c804f7ad5af90fc4248979b..f6240fd056f8598d5ef06bdaf61c5c33eab5b734 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -12,7 +12,12 @@
>  use crate::str::CStr;
>  use crate::types::{ARef, AlwaysRefCounted, Opaque};
>  use core::fmt::Display;
> +use core::marker::{PhantomData, PhantomPinned};
> +use core::mem::ManuallyDrop;
> +use core::ops::Deref;
> +use core::pin::Pin;
>  use core::ptr::NonNull;
> +use pin_init::{pin_data, pinned_drop, PinInit};
>  
>  /// Handle to a DebugFS directory.
>  pub struct Dir {
> @@ -117,6 +122,22 @@ pub fn display_file<T: Display + Sized>(
>          &self,
>          name: &CStr,
>          data: &'static T,
> +    ) -> Result<ARef<Self>> {
> +        // SAFETY: As `data` lives for the static lifetime, it outlives the file.
> +        unsafe { self.display_file_raw(name, data) }
> +    }
> +
> +    /// 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`

Perhaps the refcount should be the thing driving debugfs_remove()?  That
might save you lots of mess overall I think.

thanks,

greg k-h

[PATCH 4/8] rust: debugfs: Allow subdir creation in builder interface

[Matthew Maurer]

Allows creating subdirectories in the builder intended to be cleaned up
by `debugfs_remove` at the same time as the root.

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

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index f6240fd056f8598d5ef06bdaf61c5c33eab5b734..6c7cf7e97741b98d2c0654d01fca3de0d8047e97 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -347,6 +347,32 @@ pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'a T) -> Resu
         core::mem::forget(unsafe { self.inner.display_file_raw(name, data)? });
         Ok(())
     }
+
+    /// Creates a nested directory that may live as long as its parent
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::{Dir, Values};
+    /// let dir = Dir::new(c_str!("foo"), None)?;
+    /// let foo = KBox::pin_init(Values::attach(0, dir), GFP_KERNEL)?;
+    /// foo.as_ref().build(|_value, builder| {
+    ///   builder.dir(c_str!("bar"))?;
+    ///   Ok::<(), Error>(())
+    /// })?;
+    /// // foo/bar will still exist at this point in DebugFS
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn dir(&self, name: &CStr) -> Result<Builder<'a>> {
+        let dir = Dir::new(name, Some(self))?;
+        // SAFETY: We're suppressing the drop of the ARef we received, so we know it will live
+        // until its parent is `debugfs_remove`'d. The lifetime of the parent is 'a, so we can
+        // convert it to a similarly lived reference.
+        let dir: &'a Dir = unsafe { ARef::into_raw(dir).as_ref() };
+        // SAFETY: Since 'a is a builder lifetime, we can propagate our invariants
+        Ok(unsafe { Builder::new(dir) })
+    }
 }
 
 impl<'a> Deref for Builder<'a> {

-- 
2.49.0.901.g37484f566f-goog

Re: [PATCH 4/8] rust: debugfs: Allow subdir creation in builder interface

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 11:15:58PM +0000, Matthew Maurer wrote:
> Allows creating subdirectories in the builder intended to be cleaned up
> by `debugfs_remove` at the same time as the root.
> 
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
>  rust/kernel/debugfs.rs | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
> index f6240fd056f8598d5ef06bdaf61c5c33eab5b734..6c7cf7e97741b98d2c0654d01fca3de0d8047e97 100644
> --- a/rust/kernel/debugfs.rs
> +++ b/rust/kernel/debugfs.rs
> @@ -347,6 +347,32 @@ pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'a T) -> Resu
>          core::mem::forget(unsafe { self.inner.display_file_raw(name, data)? });
>          Ok(())
>      }
> +
> +    /// Creates a nested directory that may live as long as its parent

Ick, why make it complex?

> +    ///
> +    /// # Example
> +    ///
> +    /// ```
> +    /// # use kernel::c_str;
> +    /// # use kernel::debugfs::{Dir, Values};
> +    /// let dir = Dir::new(c_str!("foo"), None)?;

Again, no checking please :)

> +    /// let foo = KBox::pin_init(Values::attach(0, dir), GFP_KERNEL)?;
> +    /// foo.as_ref().build(|_value, builder| {
> +    ///   builder.dir(c_str!("bar"))?;
> +    ///   Ok::<(), Error>(())
> +    /// })?;
> +    /// // foo/bar will still exist at this point in DebugFS
> +    /// # Ok::<(), Error>(())

Again, no errors :)

thanks,

greg k-h

[PATCH 5/8] 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.

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

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 6c7cf7e97741b98d2c0654d01fca3de0d8047e97..2faa59d2dae44ab708cb8fca0d23f06f73a95a3a 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -11,7 +11,8 @@
 use crate::seq_print;
 use crate::str::CStr;
 use crate::types::{ARef, AlwaysRefCounted, Opaque};
-use core::fmt::Display;
+use core::fmt;
+use core::fmt::{Display, Formatter};
 use core::marker::{PhantomData, PhantomPinned};
 use core::mem::ManuallyDrop;
 use core::ops::Deref;
@@ -127,6 +128,35 @@ pub fn display_file<T: Display + Sized>(
         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.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use core::sync::atomic::{AtomicU32, Ordering};
+    /// # use kernel::c_str;
+    /// # use kernel::debugfs::Dir;
+    /// let dir = Dir::new(c_str!("foo"), None)?;
+    /// 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);
+    ///   write!(f, "{out:#010x}\n")
+    /// })?;
+    /// MY_ATOMIC.store(10, Ordering::Relaxed);
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn fmt_file<T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
+        &self,
+        name: &CStr,
+        data: &'static T,
+        f: &'static F,
+    ) -> Result<ARef<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
@@ -162,6 +192,24 @@ unsafe fn display_file_raw<T: Display + Sized>(
         // owning dentry from debugfs_create_dir, so we can wrap it in an ARef
         Ok(unsafe { ARef::from_raw(ptr.cast()) })
     }
+
+    /// 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,
+    ) -> Result<ARef<Self>> {
+        let data_adapted = FormatAdapter::new(data, f);
+        // SAFETY: data outlives the file's accessibility, so data_adapted does too
+        unsafe { self.display_file_raw(name, data_adapted) }
+    }
 }
 
 /// Implements `open` for `file_operations` via `single_open` to fill out a `seq_file`
@@ -373,6 +421,43 @@ pub fn dir(&self, name: &CStr) -> Result<Builder<'a>> {
         // SAFETY: Since 'a is a builder lifetime, we can propagate our invariants
         Ok(unsafe { Builder::new(dir) })
     }
+
+    /// 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.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use kernel::{c_str, new_mutex};
+    /// # use kernel::debugfs::{Dir, Values};
+    /// let dir = Dir::new(c_str!("foo"), None)?;
+    /// let foo = KBox::pin_init(Values::attach(new_mutex!(0), dir), GFP_KERNEL)?;
+    /// foo.as_ref().build(|value, builder| {
+    ///   builder.fmt_file(c_str!("bar"), value, &|val, f| {
+    ///     write!(f, "Mutex read: {}", *val.lock())
+    ///   })
+    /// })?;
+    /// *foo.lock() = 23;
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn fmt_file<T, F: Fn(&T, &mut fmt::Formatter<'_>) -> fmt::Result>(
+        &self,
+        name: &CStr,
+        data: &'a T,
+        f: &'static F,
+    ) -> Result<()> {
+        // We forget the reference because its reference count is implicitly "owned" by the root
+        // builder, which we know will use `debugfs_remove` to clean this up. If we release the
+        // file here, it will be immediately deleted.
+        // SAFETY:
+        // Because `Builder`'s invariant says that our lifetime is how long the directory will
+        // be available, and is equivariant, `'a` will outlive the base directory, which will be
+        // torn down by `debugfs_remove` to prevent access even if an extra refcount is held
+        // somewhere.
+        core::mem::forget(unsafe { self.fmt_file_raw(name, data, f) }?);
+        Ok(())
+    }
 }
 
 impl<'a> Deref for Builder<'a> {
@@ -381,3 +466,41 @@ fn deref(&self) -> &Self::Target {
         &self.inner
     }
 }
+
+// INVARIANT: F is inhabited
+#[repr(transparent)]
+struct FormatAdapter<T, F> {
+    inner: T,
+    _formatter: PhantomData<F>,
+}
+
+impl<T, F> FormatAdapter<T, F> {
+    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: Fn(&T, &mut Formatter<'_>) -> fmt::Result + 'static> Display for FormatAdapter<T, F> {
+    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 {
+    // We don't have generic_const_exprs, and const items inside the function get promoted out and
+    // lose type variables, so we need to do the old-style assert to check for ZSTness
+    [(); 1][core::mem::size_of::<F>()];
+    let zst_dangle: NonNull<F> = NonNull::dangling();
+    // SAFETY:
+    // While the pointer is dangling, it is a dangling pointer to a ZST, based on the array
+    // assertion above. The type is also inhabited, by the caller's assertion. This means
+    // we can materialize it.
+    unsafe { zst_dangle.as_ref() }
+}

-- 
2.49.0.901.g37484f566f-goog

Re: [PATCH 5/8] rust: debugfs: Support format hooks

[Miguel Ojeda]

On Wed, Apr 30, 2025 at 1:16 AM Matthew Maurer <mmaurer@google.com> wrote:
>
> +    /// 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.

The first paragraph of docs are the "title" ("short description"),
which renders differently. Typically it should be kept short, e.g. a
sentence. So I would probably move the second sentence to a second
paragraph.

In any case, is it true that it will fail to compile? Please see below.

> +    /// # Example

We use plurals for these sections.

> +    ) -> Result<()> {

Can be `Result`.

> +        // We forget the reference because its reference count is implicitly "owned" by the root
> +        // builder, which we know will use `debugfs_remove` to clean this up. If we release the
> +        // file here, it will be immediately deleted.
> +        // SAFETY:
> +        // Because `Builder`'s invariant says that our lifetime is how long the directory will

In our usual style, this would be e.g.:

    // file here, it will be immediately deleted.
    //
    // SAFETY: Because `Builder`'s ...

> +// INVARIANT: F is inhabited
> +#[repr(transparent)]
> +struct FormatAdapter<T, F> {

For invariants, we put them in the docs, as a section, e.g.

    /// # Invariants
    ///
    /// `F` is inhabited.

> +        // SAFETY: FormatAdapater is a repr(transparent) wrapper around T, so
> +        // casting a reference is legal

Please use Markdown in comments too.

> +/// # Safety
> +/// The caller asserts that F is inhabited
> +unsafe fn materialize_zst_fmt<F>() -> &'static F {

    /// # Safety
    ///
    /// `F` must be inhabited.

(I simplified the wording)

> +    // We don't have generic_const_exprs, and const items inside the function get promoted out and
> +    // lose type variables, so we need to do the old-style assert to check for ZSTness
> +    [(); 1][core::mem::size_of::<F>()];

Shouldn't this be:

    const { assert!(core::mem::size_of::<F>() == 0) };

to prevent a runtime panic?

Or `build_assert!` otherwise, but the line above catches the issue if
I try to pass a capturing closure from the sample module.

Thanks!

Cheers,
Miguel

[PATCH 6/8] rust: debugfs: Implement display_file in terms of fmt_file

[Matthew Maurer]

fmt_file is fundamentally more flexible, this reduces the number of
unsafe blocks, and allows us to append a newline for display_file which
is a good default.

Previous display_file did not append a newline to ensure that all output
strings, including those without trailing newlines, were an option.

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

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 2faa59d2dae44ab708cb8fca0d23f06f73a95a3a..835df2b5d7311f278d1d15fc8d688809d0ca363f 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -108,7 +108,7 @@ fn as_ptr(&self) -> *mut bindings::dentry {
     }
 
     /// Create a file in a DebugFS directory with the provided name, and contents from invoking
-    /// [`Display::fmt`] on the provided reference.
+    /// [`Display::fmt`] on the provided reference with a trailing newline.
     ///
     /// # Example
     ///
@@ -124,8 +124,7 @@ pub fn display_file<T: Display + Sized>(
         name: &CStr,
         data: &'static T,
     ) -> Result<ARef<Self>> {
-        // SAFETY: As `data` lives for the static lifetime, it outlives the file.
-        unsafe { self.display_file_raw(name, data) }
+        self.fmt_file(name, data, &|val, f| write!(f, "{val}\n"))
     }
 
     /// Create a file in a DebugFS directory with the provided name, and contents from invoking `f`
@@ -367,7 +366,7 @@ unsafe fn new(inner: &'a Dir) -> Self {
     }
 
     /// Create a file in a DebugFS directory with the provided name, and contents from invoking
-    /// [`Display::fmt`] on the provided reference.
+    /// [`Display::fmt`] on the provided reference with a trailing newline.
     ///
     /// # Example
     ///
@@ -384,16 +383,7 @@ unsafe fn new(inner: &'a Dir) -> Self {
     /// # Ok::<(), Error>(())
     /// ```
     pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'a T) -> Result<()> {
-        // We forget the reference because its reference count is implicitly "owned" by the root
-        // builder, which we know will use `debugfs_remove` to clean this up. If we release the
-        // file here, it will be immediately deleted.
-        // SAFETY:
-        // Because `Builder`'s invariant says that our lifetime is how long the directory will
-        // be available, and is equivariant, `'a` will outlive the base directory, which will be
-        // torn down by `debugfs_remove` to prevent access even if an extra refcount is held
-        // somewhere.
-        core::mem::forget(unsafe { self.inner.display_file_raw(name, data)? });
-        Ok(())
+        self.fmt_file(name, data, &|val, f| write!(f, "{val}\n"))
     }
 
     /// Creates a nested directory that may live as long as its parent

-- 
2.49.0.901.g37484f566f-goog

[PATCH 7/8] rust: debugfs: Helper macro for common case implementations

[Matthew Maurer]

The most likely thing someone needs a custom hook for is a simple
wrapper function around `write!`. This macro lets them just specify the
format string directly rather than the fully expanded closure.

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

diff --git a/rust/kernel/debugfs.rs b/rust/kernel/debugfs.rs
index 835df2b5d7311f278d1d15fc8d688809d0ca363f..edc6dd4cc5aedd4d3f1abc1a3793b39fce110d7b 100644
--- a/rust/kernel/debugfs.rs
+++ b/rust/kernel/debugfs.rs
@@ -8,9 +8,9 @@
 
 use crate::error::{from_err_ptr, Result};
 use crate::seq_file::SeqFile;
-use crate::seq_print;
 use crate::str::CStr;
 use crate::types::{ARef, AlwaysRefCounted, Opaque};
+use crate::{debugfs_fmt_file, seq_print};
 use core::fmt;
 use core::fmt::{Display, Formatter};
 use core::marker::{PhantomData, PhantomPinned};
@@ -124,7 +124,7 @@ pub fn display_file<T: Display + Sized>(
         name: &CStr,
         data: &'static T,
     ) -> Result<ARef<Self>> {
-        self.fmt_file(name, data, &|val, f| write!(f, "{val}\n"))
+        debugfs_fmt_file!(self, name, data, "{}\n")
     }
 
     /// Create a file in a DebugFS directory with the provided name, and contents from invoking `f`
@@ -383,7 +383,7 @@ unsafe fn new(inner: &'a Dir) -> Self {
     /// # Ok::<(), Error>(())
     /// ```
     pub fn display_file<T: Display + Sized>(&self, name: &CStr, data: &'a T) -> Result<()> {
-        self.fmt_file(name, data, &|val, f| write!(f, "{val}\n"))
+        debugfs_fmt_file!(self, name, data, "{}\n")
     }
 
     /// Creates a nested directory that may live as long as its parent
@@ -494,3 +494,21 @@ unsafe fn materialize_zst_fmt<F>() -> &'static F {
     // we can materialize it.
     unsafe { zst_dangle.as_ref() }
 }
+
+#[macro_export]
+/// Allows defining a debugfs file with a format string directly
+///
+/// # Example
+///
+/// ```
+/// # use kernel::debugfs::Dir;
+/// # use kernel::{c_str, debugfs_fmt_file};
+/// let dir = Dir::new(c_str!("foo"), None)?;
+/// debugfs_fmt_file!(dir, c_str!("bar"), &3, "bar={}")?;
+/// # Ok::<(), Error>(())
+/// ```
+macro_rules! debugfs_fmt_file {
+    ($dir:expr, $name:expr, $data:expr, $($arg:tt),*) => {
+        $dir.fmt_file($name, $data, &|binding, fmt| write!(fmt, $($arg),*, binding))
+    }
+}

-- 
2.49.0.901.g37484f566f-goog

[PATCH 8/8] rust: samples: Add debugfs sample

[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 | 120 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 133 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..3fd25848f2d096b03fa70679103bd725d0e42fcf
--- /dev/null
+++ b/samples/rust/rust_debugfs.rs
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2025 Google LLC.
+
+//! Sample DebugFS exporting module
+
+use core::fmt;
+use core::fmt::{Display, Formatter};
+use core::sync::atomic::{AtomicU32, Ordering};
+use kernel::c_str;
+use kernel::debugfs::{Builder, Dir, Values};
+use kernel::debugfs_fmt_file;
+use kernel::new_mutex;
+use kernel::prelude::*;
+use kernel::sync::{Arc, Mutex};
+
+module! {
+    type: RustDebugFs,
+    name: "rust_debugfs",
+    authors: ["Matthew Maurer"],
+    description: "Rust DebugFS usage sample",
+    license: "GPL",
+}
+
+struct RustDebugFs {
+    _debugfs: Pin<KBox<Values<Backing>>>,
+}
+
+struct Composite {
+    major: u32,
+    minor: u32,
+}
+
+impl Display for Composite {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        write!(f, "{}.{}", self.major, self.minor)
+    }
+}
+
+struct Record {
+    name: &'static CStr,
+    size: usize,
+    stride: usize,
+}
+
+struct Backing {
+    simple: u32,
+    composite: Composite,
+    custom: u32,
+    many: KVec<Record>,
+    atomic: AtomicU32,
+    locked: Arc<Mutex<u32>>,
+}
+
+impl Backing {
+    fn new() -> Result<Self> {
+        let mut many = KVec::new();
+        many.push(
+            Record {
+                name: c_str!("foo"),
+                size: 1,
+                stride: 2,
+            },
+            GFP_KERNEL,
+        )?;
+        many.push(
+            Record {
+                name: c_str!("bar"),
+                size: 3,
+                stride: 4,
+            },
+            GFP_KERNEL,
+        )?;
+        Ok(Self {
+            simple: 10,
+            composite: Composite { major: 1, minor: 2 },
+            custom: 37,
+            many,
+            atomic: AtomicU32::new(7),
+            locked: Arc::pin_init(new_mutex!(0), GFP_KERNEL)?,
+        })
+    }
+    fn build<'a>(&'a self, builder: Builder<'a>) -> Result<()> {
+        builder.display_file(c_str!("simple"), &self.simple)?;
+        builder.display_file(c_str!("composite"), &self.composite)?;
+        debugfs_fmt_file!(
+            builder,
+            c_str!("custom"),
+            &self.custom,
+            "Foo! {:#010x} Bar!\n"
+        )?;
+        for record in self.many.iter() {
+            let dir = builder.dir(record.name)?;
+            dir.display_file(c_str!("size"), &record.size)?;
+            dir.display_file(c_str!("stride"), &record.stride)?;
+        }
+        builder.fmt_file(c_str!("atomic"), &self.atomic, &|atomic, f| {
+            write!(f, "{}\n", atomic.load(Ordering::Relaxed))
+        })?;
+        builder.fmt_file(c_str!("locked"), &self.locked, &|locked, f| {
+            write!(f, "{}\n", *locked.lock())
+        })?;
+        Ok(())
+    }
+}
+
+impl kernel::Module for RustDebugFs {
+    fn init(_this: &'static ThisModule) -> Result<Self> {
+        let dir = Dir::new(c_str!("sample_debugfs"), None)?;
+        let backing = Backing::new()?;
+        let locked = backing.locked.clone();
+        let debugfs = KBox::pin_init(Values::attach(backing, dir), GFP_KERNEL)?;
+        debugfs.as_ref().build(Backing::build)?;
+        // These mutations could occur at any time in the future, and would remain visible through
+        // debugfs.
+        debugfs.as_ref().atomic.store(8, Ordering::Relaxed);
+        *locked.lock() = 42;
+        Ok(Self { _debugfs: debugfs })
+    }
+}

-- 
2.49.0.901.g37484f566f-goog

Re: [PATCH 8/8] rust: samples: Add debugfs sample

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 11:16:02PM +0000, Matthew Maurer wrote:
> +    fn build<'a>(&'a self, builder: Builder<'a>) -> Result<()> {
> +        builder.display_file(c_str!("simple"), &self.simple)?;
> +        builder.display_file(c_str!("composite"), &self.composite)?;
> +        debugfs_fmt_file!(
> +            builder,
> +            c_str!("custom"),
> +            &self.custom,
> +            "Foo! {:#010x} Bar!\n"
> +        )?;
> +        for record in self.many.iter() {
> +            let dir = builder.dir(record.name)?;
> +            dir.display_file(c_str!("size"), &record.size)?;
> +            dir.display_file(c_str!("stride"), &record.stride)?;
> +        }
> +        builder.fmt_file(c_str!("atomic"), &self.atomic, &|atomic, f| {
> +            write!(f, "{}\n", atomic.load(Ordering::Relaxed))
> +        })?;
> +        builder.fmt_file(c_str!("locked"), &self.locked, &|locked, f| {
> +            write!(f, "{}\n", *locked.lock())
> +        })?;
> +        Ok(())

Good luck removing all of those ? checks, this should work just fine
without any of that.

I'm not set on the "builder" pattern here, it's not a normal one for C
to be following, why do you feel debugfs should be using it?

thanks,

greg k-h

Re: [PATCH 0/8] rust: DebugFS Bindings

[John Hubbard]

On 4/29/25 4:15 PM, Matthew Maurer wrote:
> This series provides safe DebugFS bindings for Rust, with a sample
> driver using them.

Adding Timur Tabi, because he was looking into how to do this.


thanks,
John Hubbard

> 
> The primary intended way to use these bindings is to attach a data
> object to a DebugFS directory handle, and use the contents of that
> attached object to serve what's inside the directory. Through a
> combination of pinning and equivariance, we can ensure that the pointers
> in the attached object will remain valid until after the DebugFS
> directory is made inaccessible.
> 
> Signed-off-by: Matthew Maurer <mmaurer@google.com>
> ---
> Matthew Maurer (8):
>        rust: debugfs: Bind DebugFS directory creation
>        rust: debugfs: Bind file creation for long-lived Display
>        rust: debugfs: Add scoped builder interface
>        rust: debugfs: Allow subdir creation in builder interface
>        rust: debugfs: Support format hooks
>        rust: debugfs: Implement display_file in terms of fmt_file
>        rust: debugfs: Helper macro for common case implementations
>        rust: samples: Add debugfs sample
> 
>   MAINTAINERS                     |   2 +
>   rust/bindings/bindings_helper.h |   1 +
>   rust/helpers/dcache.c           |  12 +
>   rust/helpers/helpers.c          |   1 +
>   rust/kernel/debugfs.rs          | 514 ++++++++++++++++++++++++++++++++++++++++
>   rust/kernel/lib.rs              |   1 +
>   samples/rust/Kconfig            |  11 +
>   samples/rust/Makefile           |   1 +
>   samples/rust/rust_debugfs.rs    | 120 ++++++++++
>   9 files changed, 663 insertions(+)
> ---
> base-commit: b6a7783d306baf3150ac54cd5124f6e85dd375b0
> change-id: 20250428-debugfs-rust-3cd5c97eb7d1
> 
> Best regards,

Re: [PATCH 0/8] rust: DebugFS Bindings

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 11:15:54PM +0000, Matthew Maurer wrote:
> This series provides safe DebugFS bindings for Rust, with a sample
> driver using them.
> 
> The primary intended way to use these bindings is to attach a data
> object to a DebugFS directory handle, and use the contents of that
> attached object to serve what's inside the directory.

That's cool, but really, not the common use for debugfs.  Why not make
the "simple" way work first (i.e. just create/remove directories and
files attached to variables and generic read/write functions) and then,
if really needed, we can add the structure layout mess like you are
wanting to do here.

And yes, I know why you want to tie debugfs layout to a structure
layout, it makes one type of debugfs use really easy to write in rust,
but that's not the common user for what we have today.  Let's address
the common use first please, save the "builder" pattern stuff for after
we nail all of that down.

thanks,

greg k-h

Re: [PATCH 0/8] rust: DebugFS Bindings

[Matthew Maurer]

> And yes, I know why you want to tie debugfs layout to a structure
> layout, it makes one type of debugfs use really easy to write in rust,
> but that's not the common user for what we have today.  Let's address
> the common use first please, save the "builder" pattern stuff for after
> we nail all of that down.
>
> thanks,
>
> greg k-h

I'll remove that API in the next version of the patch series to get
the basics down first, but to give some motivation to what I was
trying to support which *is* done in C today, see qcom-socinfo [1] -
it uses a backing `socinfo_params` struct which is expected to outlive
its whole directory structure.

[1]: https://github.com/torvalds/linux/blob/b6ea1680d0ac0e45157a819c41b46565f4616186/drivers/soc/qcom/socinfo.c#L133-L156

Re: [PATCH 0/8] rust: DebugFS Bindings

[Greg Kroah-Hartman]

On Wed, Apr 30, 2025 at 08:01:38AM -0700, Matthew Maurer wrote:
> > And yes, I know why you want to tie debugfs layout to a structure
> > layout, it makes one type of debugfs use really easy to write in rust,
> > but that's not the common user for what we have today.  Let's address
> > the common use first please, save the "builder" pattern stuff for after
> > we nail all of that down.
> >
> > thanks,
> >
> > greg k-h
> 
> I'll remove that API in the next version of the patch series to get
> the basics down first, but to give some motivation to what I was
> trying to support which *is* done in C today, see qcom-socinfo [1] -
> it uses a backing `socinfo_params` struct which is expected to outlive
> its whole directory structure.

What exactly do you mean by "outlive"?  Right now debugfs has no way to
"own" a structure and it just "has to work" so that the file will always
be there and hope that the backing variable is also still there.  I
guess you are trying to encode that "hope" into something real?  :)

> [1]: https://github.com/torvalds/linux/blob/b6ea1680d0ac0e45157a819c41b46565f4616186/drivers/soc/qcom/socinfo.c#L133-L156
> 

Yes, SOC drivers are a mess of debugfs files, but manually creating them
is "fine" to start with, let's not go wild to start with.  If we see
common patterns outside of the single soc driver use case, then we can
propose exporting structures as a directory structure in debugfs, but I
really think that is a very minor use of the api at the moment.

thanks,

greg k-h

Re: [PATCH 0/8] rust: DebugFS Bindings

[Matthew Maurer]

On Wed, Apr 30, 2025 at 8:21 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Apr 30, 2025 at 08:01:38AM -0700, Matthew Maurer wrote:
> > > And yes, I know why you want to tie debugfs layout to a structure
> > > layout, it makes one type of debugfs use really easy to write in rust,
> > > but that's not the common user for what we have today.  Let's address
> > > the common use first please, save the "builder" pattern stuff for after
> > > we nail all of that down.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > I'll remove that API in the next version of the patch series to get
> > the basics down first, but to give some motivation to what I was
> > trying to support which *is* done in C today, see qcom-socinfo [1] -
> > it uses a backing `socinfo_params` struct which is expected to outlive
> > its whole directory structure.
>
> What exactly do you mean by "outlive"?  Right now debugfs has no way to
> "own" a structure and it just "has to work" so that the file will always
> be there and hope that the backing variable is also still there.  I
> guess you are trying to encode that "hope" into something real?  :)

Yes, the `Values` structure used by the builder interface enforces
that the backing variable must live at least as long as the `Dir`
corresponding to the lowest directory it's going to be used in. To
make DebugFS safe in Rust, we either need to:
1. Have DebugFS files own things (not currently done, doesn't seem to
match intentions).
2. Expose things that live forever (e.g. globals, like the early patches do)
3. Have a pinned structure that is guaranteed to outlive the files
that use it (what the builder interface is trying to support)

>
> > [1]: https://github.com/torvalds/linux/blob/b6ea1680d0ac0e45157a819c41b46565f4616186/drivers/soc/qcom/socinfo.c#L133-L156
> >
>
> Yes, SOC drivers are a mess of debugfs files, but manually creating them
> is "fine" to start with, let's not go wild to start with.  If we see
> common patterns outside of the single soc driver use case, then we can
> propose exporting structures as a directory structure in debugfs, but I
> really think that is a very minor use of the api at the moment.
>
> thanks,
>
> greg k-h

Re: [PATCH 0/8] rust: DebugFS Bindings

[Greg Kroah-Hartman]

On Wed, Apr 30, 2025 at 08:24:14AM -0700, Matthew Maurer wrote:
> On Wed, Apr 30, 2025 at 8:21 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Apr 30, 2025 at 08:01:38AM -0700, Matthew Maurer wrote:
> > > > And yes, I know why you want to tie debugfs layout to a structure
> > > > layout, it makes one type of debugfs use really easy to write in rust,
> > > > but that's not the common user for what we have today.  Let's address
> > > > the common use first please, save the "builder" pattern stuff for after
> > > > we nail all of that down.
> > > >
> > > > thanks,
> > > >
> > > > greg k-h
> > >
> > > I'll remove that API in the next version of the patch series to get
> > > the basics down first, but to give some motivation to what I was
> > > trying to support which *is* done in C today, see qcom-socinfo [1] -
> > > it uses a backing `socinfo_params` struct which is expected to outlive
> > > its whole directory structure.
> >
> > What exactly do you mean by "outlive"?  Right now debugfs has no way to
> > "own" a structure and it just "has to work" so that the file will always
> > be there and hope that the backing variable is also still there.  I
> > guess you are trying to encode that "hope" into something real?  :)
> 
> Yes, the `Values` structure used by the builder interface enforces
> that the backing variable must live at least as long as the `Dir`
> corresponding to the lowest directory it's going to be used in. To
> make DebugFS safe in Rust, we either need to:
> 1. Have DebugFS files own things (not currently done, doesn't seem to
> match intentions).

Agreed, let's not do that.

> 2. Expose things that live forever (e.g. globals, like the early patches do)

Let's start with this for now.

> 3. Have a pinned structure that is guaranteed to outlive the files
> that use it (what the builder interface is trying to support)

Let's add this "after" we get the basics down.  Should make things
simpler for you to work on now, and for us to review.

Baby steps :)

thanks,

greg k-h