[PATCH v2 0/3] rust: configfs abstractions

[PATCH v2 0/3] rust: configfs abstractions

[Andreas Hindborg]

This series adds a safe Rust API that allows Rust modules to interface
the `configfs` machinery.

The series contains an example for the samples folder to demonstrate
usage of the API. As such, there is no inline example in the
documentation.

The last patch adds a maintainer entry for the Rust configfs
abstractions, to make it absolutely clear that I will commit to maintain
these abstractions, if required. Feel free to drop this patch if this is
not required.

The series is a dependency of `rnull`, the Rust null block driver.
Please see [1] for initial `configfs` support in `rnull`.

[1] https://github.com/metaspace/linux/tree/9ac53130f5fb05b9b3074fa261b445b8fde547dd/drivers/block/rnull

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Changes in v2:
- Remove generalization over pointer type and enforce use of `Arc`.
- Use type system to enforce connection between `ItemType` and
  `Subsystem` or `Group`. Differentiate construction of vtables on this
  type difference.
- Move drop logic of child nodes from parent to child.
- Pick `ForeignOwnable::PointedTo` patch as dependency instead of
  including it here.
- Fix some rustdoc warnings.
- Use CamelCase for generic type parameter declaration.
- Destroy mutex in `Subsystem::drop`.
- Move `GroupOperationsVTable` struct definition next to implementation.
- Rebase on v6.14-rc1.
- Link to v1: https://lore.kernel.org/r/20250131-configfs-v1-0-87947611401c@kernel.org

---
Andreas Hindborg (3):
      rust: sync: change `<Arc<T> as ForeignOwnable>::PointedTo` to `T`
      rust: configfs: introduce rust support for configfs
      MAINTAINERS: add entry for configfs Rust abstractions

 MAINTAINERS                     |   7 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/mutex.c            |   5 +
 rust/kernel/configfs.rs         | 860 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   2 +
 rust/kernel/sync/arc.rs         |  21 +-
 samples/rust/Kconfig            |  11 +
 samples/rust/Makefile           |   1 +
 samples/rust/rust_configfs.rs   | 186 +++++++++
 9 files changed, 1089 insertions(+), 5 deletions(-)
---
base-commit: 379487e17ca406b47392e7ab6cf35d1c3bacb371
change-id: 20250131-configfs-b888cd82d84a
prerequisite-patch-id: 275efe8e08839e3a0de28ed26e8de80be9852024

Best regards,
-- 
Andreas Hindborg <a.hindborg@kernel.org>

[PATCH v2 1/3] rust: sync: change `<Arc<T> as ForeignOwnable>::PointedTo` to `T`

[Andreas Hindborg]

Using `ArcInner` as `PoinedTo` in the `ForeignOwnable` implementation for
`Arc` is a bit unfortunate. Using `T` as `PointedTo` does not remove any
functionality, but allows `ArcInner` to be private. Further, it allows
downstream users to write code that is generic over `Box` and `Arc`, when
downstream users need access to `T` after calling `into_foreign`.

Reviewed-by: Fiona Behrens <me@kloenk.dev>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---

This patch is a dependency for Rust `configfs` abstractions. It allows both
`Box` and `Arc` to be used as pointer types in the `configfs` hierarchy.
---
 rust/kernel/sync/arc.rs | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index dfe4abf82c25c..503e318b4c4ef 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -143,7 +143,7 @@ pub struct Arc<T: ?Sized> {
 #[doc(hidden)]
 #[pin_data]
 #[repr(C)]
-pub struct ArcInner<T: ?Sized> {
+struct ArcInner<T: ?Sized> {
     refcount: Opaque<bindings::refcount_t>,
     data: T,
 }
@@ -345,18 +345,25 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
 
 // SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
 unsafe impl<T: 'static> ForeignOwnable for Arc<T> {
-    type PointedTo = ArcInner<T>;
+    type PointedTo = T;
     type Borrowed<'a> = ArcBorrow<'a, T>;
     type BorrowedMut<'a> = Self::Borrowed<'a>;
 
     fn into_foreign(self) -> *mut Self::PointedTo {
-        ManuallyDrop::new(self).ptr.as_ptr()
+        let x = ManuallyDrop::new(self).ptr.as_ptr();
+        // SAFETY: `x` is a valid pointer to `Self` so the projection below is
+        // in bounds of the allocation.
+        unsafe { core::ptr::addr_of_mut!((*x).data) }
     }
 
     unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
+        // SAFETY: We did the reverse offset calculation in `into_foreign`, so
+        // the offset calculation below is in bounds of the allocation.
+        let inner_ptr = unsafe { kernel::container_of!(ptr, ArcInner<T>, data).cast_mut() };
+
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        let inner = unsafe { NonNull::new_unchecked(ptr) };
+        let inner = unsafe { NonNull::new_unchecked(inner_ptr) };
 
         // SAFETY: By the safety requirement of this function, we know that `ptr` came from
         // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and
@@ -365,9 +372,13 @@ unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
     }
 
     unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> ArcBorrow<'a, T> {
+        // SAFETY: We did the reverse offset calculation in `into_foreign`, so
+        // the offset calculation below is in bounds of the allocation.
+        let inner_ptr = unsafe { kernel::container_of!(ptr, ArcInner<T>, data).cast_mut() };
+
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        let inner = unsafe { NonNull::new_unchecked(ptr) };
+        let inner = unsafe { NonNull::new_unchecked(inner_ptr) };
 
         // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
         // for the lifetime of the returned value.

-- 
2.47.0

Re: [PATCH v2 1/3] rust: sync: change `<Arc<T> as ForeignOwnable>::PointedTo` to `T`

[Benno Lossin]

On 07.02.25 15:41, Andreas Hindborg wrote:
> Using `ArcInner` as `PoinedTo` in the `ForeignOwnable` implementation for
> `Arc` is a bit unfortunate. Using `T` as `PointedTo` does not remove any
> functionality, but allows `ArcInner` to be private. Further, it allows
> downstream users to write code that is generic over `Box` and `Arc`, when
> downstream users need access to `T` after calling `into_foreign`.
> 
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Two nits below, but this looks good regardless, so:

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> ---
> 
> This patch is a dependency for Rust `configfs` abstractions. It allows both
> `Box` and `Arc` to be used as pointer types in the `configfs` hierarchy.
> ---
>  rust/kernel/sync/arc.rs | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index dfe4abf82c25c..503e318b4c4ef 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -143,7 +143,7 @@ pub struct Arc<T: ?Sized> {
>  #[doc(hidden)]
>  #[pin_data]
>  #[repr(C)]
> -pub struct ArcInner<T: ?Sized> {
> +struct ArcInner<T: ?Sized> {
>      refcount: Opaque<bindings::refcount_t>,
>      data: T,
>  }
> @@ -345,18 +345,25 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
> 
>  // SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
>  unsafe impl<T: 'static> ForeignOwnable for Arc<T> {
> -    type PointedTo = ArcInner<T>;
> +    type PointedTo = T;
>      type Borrowed<'a> = ArcBorrow<'a, T>;
>      type BorrowedMut<'a> = Self::Borrowed<'a>;
> 
>      fn into_foreign(self) -> *mut Self::PointedTo {
> -        ManuallyDrop::new(self).ptr.as_ptr()
> +        let x = ManuallyDrop::new(self).ptr.as_ptr();

I would normally name such a variable `this`, but it's fine as-is.

> +        // SAFETY: `x` is a valid pointer to `Self` so the projection below is
> +        // in bounds of the allocation.
> +        unsafe { core::ptr::addr_of_mut!((*x).data) }
>      }
> 
>      unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
> +        // SAFETY: We did the reverse offset calculation in `into_foreign`, so
> +        // the offset calculation below is in bounds of the allocation.
> +        let inner_ptr = unsafe { kernel::container_of!(ptr, ArcInner<T>, data).cast_mut() };
> +
>          // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
>          // call to `Self::into_foreign`.
> -        let inner = unsafe { NonNull::new_unchecked(ptr) };
> +        let inner = unsafe { NonNull::new_unchecked(inner_ptr) };
> 
>          // SAFETY: By the safety requirement of this function, we know that `ptr` came from
>          // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and
> @@ -365,9 +372,13 @@ unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
>      }
> 
>      unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> ArcBorrow<'a, T> {
> +        // SAFETY: We did the reverse offset calculation in `into_foreign`, so
> +        // the offset calculation below is in bounds of the allocation.
> +        let inner_ptr = unsafe { kernel::container_of!(ptr, ArcInner<T>, data).cast_mut() };

You could call this variable `ptr` and avoid the change below, but
either way is fine.

---
Cheers,
Benno

> +
>          // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
>          // call to `Self::into_foreign`.
> -        let inner = unsafe { NonNull::new_unchecked(ptr) };
> +        let inner = unsafe { NonNull::new_unchecked(inner_ptr) };
> 
>          // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
>          // for the lifetime of the returned value.
> 
> --
> 2.47.0
> 
> 

Re: [PATCH v2 1/3] rust: sync: change `<Arc<T> as ForeignOwnable>::PointedTo` to `T`

[Tamir Duberstein]

On Fri, Feb 7, 2025 at 9:50 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Using `ArcInner` as `PoinedTo` in the `ForeignOwnable` implementation for
> `Arc` is a bit unfortunate. Using `T` as `PointedTo` does not remove any
> functionality, but allows `ArcInner` to be private. Further, it allows
> downstream users to write code that is generic over `Box` and `Arc`, when
> downstream users need access to `T` after calling `into_foreign`.

I stumbled upon https://github.com/Rust-for-Linux/linux/pull/1036 the
other day. Boqun, are there any plans to revive this work? It might
obviate the need for _this_ patch.

Tamir

Re: [PATCH v2 1/3] rust: sync: change `<Arc<T> as ForeignOwnable>::PointedTo` to `T`

[Andreas Hindborg]

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Fri, Feb 7, 2025 at 9:50 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Using `ArcInner` as `PoinedTo` in the `ForeignOwnable` implementation for
>> `Arc` is a bit unfortunate. Using `T` as `PointedTo` does not remove any
>> functionality, but allows `ArcInner` to be private. Further, it allows
>> downstream users to write code that is generic over `Box` and `Arc`, when
>> downstream users need access to `T` after calling `into_foreign`.
>
> I stumbled upon https://github.com/Rust-for-Linux/linux/pull/1036 the
> other day. Boqun, are there any plans to revive this work? It might
> obviate the need for _this_ patch.

I don't think it would help configfs. The configfs patches rely on
getting access to a `T` pointer from `ForeignOwnable::into_foreign`.

In fact, thinking about this, I should probably mandate this in
`ForeignOwnable` API requirements in order to not have soundness hinge
on the implementation of `ForeignOwnable`.

Also I would appreciate that if we want to use a reference to `ArcInner`
rather than a special borrow type, maybe let's do it down the line and
not stall this series on that work.


Best regards,
Andreas Hindborg

[PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Andreas Hindborg]

This patch adds a rust API for configfs, thus allowing rust modules to use
configfs for configuration. The implementation is a shim on top of the C
configfs implementation allowing safe use of the C infrastructure from
rust.

The patch enables the `const_mut_refs` feature on compilers before rustc
1.83. The feature was stabilized in rustc 1.83 and is not required to be
explicitly enabled on later versions.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

---

This patch is a direct dependency for `rnull`, the rust null block driver.
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/mutex.c            |   5 +
 rust/kernel/configfs.rs         | 860 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   2 +
 samples/rust/Kconfig            |  11 +
 samples/rust/Makefile           |   1 +
 samples/rust/rust_configfs.rs   | 186 +++++++++
 7 files changed, 1066 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55354e4dec14e..d115a770306f6 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -10,6 +10,7 @@
 #include <linux/blk-mq.h>
 #include <linux/blk_types.h>
 #include <linux/blkdev.h>
+#include <linux/configfs.h>
 #include <linux/cred.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
diff --git a/rust/helpers/mutex.c b/rust/helpers/mutex.c
index 06575553eda5c..3e9b910a88e9b 100644
--- a/rust/helpers/mutex.c
+++ b/rust/helpers/mutex.c
@@ -17,3 +17,8 @@ void rust_helper_mutex_assert_is_held(struct mutex *mutex)
 {
 	lockdep_assert_held(mutex);
 }
+
+void rust_helper_mutex_destroy(struct mutex *lock)
+{
+	mutex_destroy(lock);
+}
diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
new file mode 100644
index 0000000000000..9d4b381b9df89
--- /dev/null
+++ b/rust/kernel/configfs.rs
@@ -0,0 +1,860 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! `configfs` interface.
+//!
+//! `configfs` is an in-memory pseudo file system for configuration of kernel
+//! modules. Please see the [C documentation] for details and intended use of
+//! `configfs`.
+//!
+//! This module does not support the following `configfs` features:
+//!
+//! - Items. All group children are groups.
+//! - Symlink support.
+//! - `disconnect_notify` hook.
+//! - Default groups.
+//!
+//! See the [rust_configfs.rs] sample for a full example use of this module.
+//!
+//! C header: [`include/linux/configfs.h`](srctree/include/linux/configfs.h)
+//!
+//! [C documentation]: srctree/Documentation/filesystems/configfs.rst
+//! [rust_configfs.rs]: srctree/samples/rust/rust_configfs.rs
+
+use crate::alloc::flags;
+use crate::container_of;
+use crate::page::PAGE_SIZE;
+use crate::prelude::*;
+use crate::str::CString;
+use crate::sync::Arc;
+use crate::types::ForeignOwnable;
+use crate::types::Opaque;
+use core::cell::UnsafeCell;
+use core::marker::PhantomData;
+use core::ptr::addr_of;
+use core::ptr::addr_of_mut;
+
+/// A `configfs` subsystem.
+///
+/// This is the top level entrypoint for a `configfs` hierarchy. To register
+/// with configfs, embed a field of this type into your kernel module struct.
+#[pin_data(PinnedDrop)]
+pub struct Subsystem<Data> {
+    #[pin]
+    subsystem: Opaque<bindings::configfs_subsystem>,
+    #[pin]
+    data: Data,
+}
+
+// SAFETY: We do not provide any operations on `Subsystem`.
+unsafe impl<Data> Sync for Subsystem<Data> {}
+
+// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
+unsafe impl<Data> Send for Subsystem<Data> {}
+
+impl<Data> Subsystem<Data> {
+    /// Create an initializer for a [`Subsystem`].
+    ///
+    /// The subsystem will appear in configfs as a directory name given by
+    /// `name`. The attributes available in directory are specified by
+    /// `item_type`.
+    pub fn new(
+        name: &'static CStr,
+        item_type: &'static ItemType<Subsystem<Data>, Data>,
+        data: impl PinInit<Data, Error>,
+    ) -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {
+            subsystem <- kernel::init::zeroed().chain(
+                |place: &mut Opaque<bindings::configfs_subsystem>| {
+                    // SAFETY: All of `place` is valid for write.
+                    unsafe {
+                        addr_of_mut!((*place.get()).su_group.cg_item.ci_name )
+                            .write(name.as_ptr().cast_mut().cast())
+                    };
+                    // SAFETY: All of `place` is valid for write.
+                    unsafe {
+                        addr_of_mut!((*place.get()).su_group.cg_item.ci_type)
+                            .write(item_type.as_ptr())
+                    };
+                    // SAFETY: We initialized the required fields of `place.group` above.
+                    unsafe { bindings::config_group_init(&mut (*place.get()).su_group) };
+                    // SAFETY: `place.su_mutex` is valid for use as a mutex.
+                    unsafe { bindings::__mutex_init(
+                        &mut (*place.get()).su_mutex,
+                        kernel::optional_name!().as_char_ptr(),
+                        kernel::static_lock_class!().as_ptr())
+                    }
+                    Ok(())
+                }),
+            data <- data,
+        })
+        .pin_chain(|this| {
+            crate::error::to_result(
+                // SAFETY: We initialized `this.subsystem` according to C API contract above.
+                unsafe { bindings::configfs_register_subsystem(this.subsystem.get()) },
+            )
+        })
+    }
+}
+
+#[pinned_drop]
+impl<Data> PinnedDrop for Subsystem<Data> {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY: We registered `self.subsystem` in the initializer returned by `Self::new`.
+        unsafe { bindings::configfs_unregister_subsystem(self.subsystem.get()) };
+        // SAFETY: We initialized the mutex in `Subsystem::new`.
+        unsafe { bindings::mutex_destroy(addr_of_mut!((*self.subsystem.get()).su_mutex)) };
+    }
+}
+
+/// Trait that allows offset calculations for structs that embed a
+/// `bindings::config_group`.
+///
+/// Users of the `configfs` API should not need to implement this trait.
+///
+/// # Safety
+///
+/// - Implementers of this trait must embed a `bindings::config_group`.
+/// - Methods must be implemented according to method documentation.
+pub unsafe trait HasGroup<Data> {
+    /// Return the address of the `bindings::config_group` embedded in `Self`.
+    ///
+    /// # Safety
+    ///
+    /// - `this` must be a valid allocation of at least the size of `Self`.
+    unsafe fn group(this: *const Self) -> *const bindings::config_group;
+
+    /// Return the address of the `Self` that `group` is embedded in.
+    ///
+    /// # Safety
+    ///
+    /// - `group` must point to the `bindings::config_group` that is embedded in
+    ///   `Self`.
+    unsafe fn container_of(group: *const bindings::config_group) -> *const Self;
+}
+
+// SAFETY: `Subsystem<Data>` embeds a field of type `bindings::config_group`
+// within the `subsystem` field.
+unsafe impl<Data> HasGroup<Data> for Subsystem<Data> {
+    unsafe fn group(this: *const Self) -> *const bindings::config_group {
+        // SAFETY: By impl and function safety requirement this projection is in bounds.
+        unsafe { addr_of!((*(*this).subsystem.get()).su_group) }
+    }
+
+    unsafe fn container_of(group: *const bindings::config_group) -> *const Self {
+        // SAFETY: By impl and function safety requirement this projection is in bounds.
+        let c_subsys_ptr = unsafe { container_of!(group, bindings::configfs_subsystem, su_group) };
+        let opaque_ptr = c_subsys_ptr.cast::<Opaque<bindings::configfs_subsystem>>();
+        // SAFETY: By impl and function safety requirement, `opaque_ptr` and the
+        // pointer it returns, are within the same allocation.
+        unsafe { container_of!(opaque_ptr, Subsystem<Data>, subsystem) }
+    }
+}
+
+/// A `configfs` group.
+///
+/// To add a subgroup to `configfs`, pass this type as `ctype` to
+/// [`crate::configfs_attrs`] when creating a group in [`GroupOperations::make_group`].
+#[pin_data]
+pub struct Group<Data> {
+    #[pin]
+    group: Opaque<bindings::config_group>,
+    #[pin]
+    data: Data,
+}
+
+impl<Data> Group<Data> {
+    /// Create an initializer for a new group.
+    ///
+    /// When instantiated, the group will appear as a directory with the name
+    /// given by `name` and it will contain attributes specified by `item_type`.
+    pub fn new(
+        name: CString,
+        item_type: &'static ItemType<Group<Data>, Data>,
+        data: impl PinInit<Data, Error>,
+    ) -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {
+            group <- kernel::init::zeroed().chain(|v: &mut Opaque<bindings::config_group>| {
+                let place = v.get();
+                let name = name.as_bytes_with_nul().as_ptr();
+                // SAFETY: It is safe to initialize a group once it has been zeroed.
+                unsafe {
+                    bindings::config_group_init_type_name(place, name as _, item_type.as_ptr())
+                };
+                Ok(())
+            }),
+            data <- data,
+        })
+    }
+}
+
+// SAFETY: `Group<Data>` embeds a field of type `bindings::config_group`
+// within the `group` field.
+unsafe impl<Data> HasGroup<Data> for Group<Data> {
+    unsafe fn group(this: *const Self) -> *const bindings::config_group {
+        Opaque::raw_get(
+            // SAFETY: By impl and function safety requirements this field
+            // projection is within bounds of the allocation.
+            unsafe { addr_of!((*this).group) },
+        )
+    }
+
+    unsafe fn container_of(group: *const bindings::config_group) -> *const Self {
+        let opaque_ptr = group.cast::<Opaque<bindings::config_group>>();
+        // SAFETY: By impl and function safety requirement, `opaque_ptr` and
+        // pointer it returns will be in the same allocation.
+        unsafe { container_of!(opaque_ptr, Self, group) }
+    }
+}
+
+/// # Safety
+///
+/// `this` must be a valid pointer.
+///
+/// If `this` does not represent the root group of a `configfs` subsystem,
+/// `this` must be a pointer to a `bindings::config_group` embedded in a
+/// `Group<Parent>`.
+///
+/// Otherwise, `this` must be a pointer to a `bindings::config_group` that
+/// is embedded in a `bindings::configfs_subsystem` that is embedded in a
+/// `Subsystem<Parent>`.
+unsafe fn get_group_data<'a, Parent>(this: *mut bindings::config_group) -> &'a Parent {
+    // TODO
+    // SAFETY: `this` is a valid pointer.
+    let is_root = unsafe { (*this).cg_subsys.is_null() };
+
+    if !is_root {
+        // SAFETY: By C API contact, `this` is a pointer to a
+        // `bindings::config_group` that we passed as a return value in from
+        // `make_group`. Such a pointer is embedded within a `Group<Parent>`.
+        unsafe { &(*Group::<Parent>::container_of(this)).data }
+    } else {
+        // SAFETY: By C API contract, `this` is a pointer to the
+        // `bindings::config_group` field within a `Subsystem<Parent>`.
+        unsafe { &(*Subsystem::container_of(this)).data }
+    }
+}
+
+struct GroupOperationsVTable<Parent, Child>(PhantomData<(Parent, Child)>)
+where
+    Parent: GroupOperations<Child = Child>;
+
+impl<Parent, Child> GroupOperationsVTable<Parent, Child>
+where
+    Parent: GroupOperations<Child = Child>,
+    Child: 'static,
+{
+    /// # Safety
+    ///
+    /// `this` must be a valid pointer.
+    ///
+    /// If `this` does not represent the root group of a `configfs` subsystem,
+    /// `this` must be a pointer to a `bindings::config_group` embedded in a
+    /// `Group<Parent>`.
+    ///
+    /// Otherwise, `this` must be a pointer to a `bindings::config_group` that
+    /// is embedded in a `bindings::configfs_subsystem` that is embedded in a
+    /// `Subsystem<Parent>`.
+    ///
+    /// `name` must point to a null terminated string.
+    unsafe extern "C" fn make_group(
+        this: *mut bindings::config_group,
+        name: *const kernel::ffi::c_char,
+    ) -> *mut bindings::config_group {
+        // SAFETY: By function safety requirements of this function, this call
+        // is safe.
+        let parent_data = unsafe { get_group_data(this) };
+
+        let group_init = match Parent::make_group(
+            parent_data,
+            // SAFETY: By function safety requirements, name points to a null
+            // terminated string.
+            unsafe { CStr::from_char_ptr(name) },
+        ) {
+            Ok(init) => init,
+            Err(e) => return e.to_ptr(),
+        };
+
+        let child_group = <Arc<Group<Child>> as InPlaceInit<Group<Child>>>::try_pin_init(
+            group_init,
+            flags::GFP_KERNEL,
+        );
+
+        match child_group {
+            Ok(child_group) => {
+                let child_group_ptr = child_group.into_foreign();
+                // SAFETY: We allocated the pointee of `child_ptr` above as a
+                // `Group<Child>`.
+                unsafe { Group::<Child>::group(child_group_ptr) }.cast_mut()
+            }
+            Err(e) => e.to_ptr(),
+        }
+    }
+
+    /// # Safety
+    ///
+    /// If `this` does not represent the root group of a `configfs` subsystem,
+    /// `this` must be a pointer to a `bindings::config_group` embedded in a
+    /// `Group<Parent>`.
+    ///
+    /// Otherwise, `this` must be a pointer to a `bindings::config_group` that
+    /// is embedded in a `bindings::configfs_subsystem` that is embedded in a
+    /// `Subsystem<Parent>`.
+    ///
+    /// `item` must point to a `bindings::config_item` within a
+    /// `bindings::config_group` within a `Group<Child>`.
+    unsafe extern "C" fn drop_item(
+        this: *mut bindings::config_group,
+        item: *mut bindings::config_item,
+    ) {
+        // SAFETY: By function safety requirements of this function, this call
+        // is safe.
+        let parent_data = unsafe { get_group_data(this) };
+
+        // SAFETY: By function safety requirements, `item` is embedded in a
+        // `config_group`.
+        let c_child_group_ptr =
+            unsafe { kernel::container_of!(item, bindings::config_group, cg_item) };
+        // SAFETY: By function safety requirements, `c_child_group_ptr` is
+        // embedded within a `Group<CHLD>`.
+        let r_child_group_ptr = unsafe { Group::<Child>::container_of(c_child_group_ptr) };
+
+        if Parent::HAS_DROP_ITEM {
+            Parent::drop_item(
+                parent_data,
+                // SAFETY: We called `into_foreign` to produce `r_child_group_ptr` in
+                // `make_group`. There are not other borrows of this pointer in existence.
+                unsafe {
+                    <Arc<Group<Child>> as ForeignOwnable>::borrow(r_child_group_ptr.cast_mut())
+                },
+            );
+        }
+
+        // SAFETY: By C API contract, we are required to drop a refcount on
+        // `item`.
+        unsafe { bindings::config_item_put(item) };
+    }
+
+    const VTABLE: bindings::configfs_group_operations = bindings::configfs_group_operations {
+        make_item: None,
+        make_group: Some(Self::make_group),
+        disconnect_notify: None,
+        drop_item: Some(Self::drop_item),
+        is_visible: None,
+        is_bin_visible: None,
+    };
+}
+
+struct ItemOperationsVTable<Container, Data>(PhantomData<(Container, Data)>);
+
+impl<Data> ItemOperationsVTable<Group<Data>, Data>
+where
+    Data: 'static,
+{
+    /// # Safety
+    ///
+    /// `this` must be a pointer to a `bindings::config_group` embedded in a
+    /// `Group<Parent>`.
+    ///
+    /// This function will destroy the pointee of `this`. The pointee of `this`
+    /// must not be accessed after the function returns.
+    unsafe extern "C" fn release(this: *mut bindings::config_item) {
+        // SAFETY: By function safety requirements, `this` is embedded in a
+        // `config_group`.
+        let c_group_ptr = unsafe { kernel::container_of!(this, bindings::config_group, cg_item) };
+        // SAFETY: By function safety requirements, `c_group_ptr` is
+        // embedded within a `Group<Data>`.
+        let r_group_ptr = unsafe { Group::<Data>::container_of(c_group_ptr) };
+
+        // SAFETY: We called `into_foreign` on `r_group_ptr` in
+        // `make_group`.
+        let pin_self =
+            unsafe { <Arc<Group<Data>> as ForeignOwnable>::from_foreign(r_group_ptr.cast_mut()) };
+        drop(pin_self);
+    }
+
+    const VTABLE: bindings::configfs_item_operations = bindings::configfs_item_operations {
+        release: Some(Self::release),
+        allow_link: None,
+        drop_link: None,
+    };
+}
+
+impl<Data> ItemOperationsVTable<Subsystem<Data>, Data> {
+    const VTABLE: bindings::configfs_item_operations = bindings::configfs_item_operations {
+        release: None,
+        allow_link: None,
+        drop_link: None,
+    };
+}
+
+/// Operations implemented by `configfs` groups that can create subgroups.
+///
+/// Implement this trait on structs that embed a [`Subsystem`] or a [`Group`].
+#[vtable]
+pub trait GroupOperations {
+    /// The parent data object type.
+    ///
+    /// The implementer of this trait is this kind of data object. Should be set
+    /// to `Self`.
+    type Parent;
+
+    /// The child data object type.
+    ///
+    /// This group will create subgroups (subdirectories) backed by this kind of
+    /// object.
+    type Child: 'static;
+
+    /// The kernel will call this method in response to `mkdir(2)` in the
+    /// directory representing `this`.
+    ///
+    /// To accept the request to create a group, implementations should
+    /// instantiate a `CHLD` and return a `CPTR` to it. To prevent creation,
+    /// return a suitable error.
+    fn make_group(
+        this: &Self::Parent,
+        name: &CStr,
+    ) -> Result<impl PinInit<Group<Self::Child>, Error>>;
+
+    /// The kernel will call this method before the directory representing
+    /// `_child` is removed from `configfs`.
+    ///
+    /// Implementations can use this method to do house keeping before
+    /// `configfs` drops its reference to `Child`.
+    fn drop_item(
+        _this: &Self::Parent,
+        _child: <Arc<Group<Self::Child>> as ForeignOwnable>::Borrowed<'_>,
+    ) {
+        kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
+    }
+}
+
+/// A `configfs` attribute.
+///
+/// An attribute appear as a file in configfs, inside a folder that represent
+/// the group that the attribute belongs to.
+#[repr(transparent)]
+pub struct Attribute<const ID: u64, O, Data> {
+    attribute: Opaque<bindings::configfs_attribute>,
+    _p: PhantomData<(O, Data)>,
+}
+
+// SAFETY: We do not provide any operations on `Attribute`.
+unsafe impl<const ID: u64, O, Data> Sync for Attribute<ID, O, Data> {}
+
+// SAFETY: Ownership of `Attribute` can safely be transferred to other threads.
+unsafe impl<const ID: u64, O, Data> Send for Attribute<ID, O, Data> {}
+
+impl<const ID: u64, O, Data> Attribute<ID, O, Data>
+where
+    O: AttributeOperations<ID, Data = Data>,
+{
+    /// # Safety
+    ///
+    /// `item` must be embedded in a `bindings::config_group`.
+    ///
+    /// If `item` does not represent the root group of a `configfs` subsystem,
+    /// the group must be embedded in a `Group<Data>`.
+    ///
+    /// Otherwise, the group must be a embedded in a
+    /// `bindings::configfs_subsystem` that is embedded in a `Subsystem<Data>`.
+    ///
+    /// `page` must point to a writable buffer of size at least [`PAGE_SIZE`].
+    unsafe extern "C" fn show(
+        item: *mut bindings::config_item,
+        page: *mut kernel::ffi::c_char,
+    ) -> isize {
+        let c_group: *mut bindings::config_group =
+        // SAFETY: By function safety requirements, `item` is embedded in a
+        // `config_group`.
+            unsafe { container_of!(item, bindings::config_group, cg_item) }.cast_mut();
+
+        // SAFETY: The function safety requirements for this function satisfy
+        // the conditions for this call.
+        let data: &Data = unsafe { get_group_data(c_group) };
+
+        // SAFETY: By function safety requirements, `page` is writable for `PAGE_SIZE`.
+        let ret = O::show(data, unsafe { &mut *(page as *mut [u8; PAGE_SIZE]) });
+
+        match ret {
+            Ok(size) => size as isize,
+            Err(err) => err.to_errno() as isize,
+        }
+    }
+
+    /// # Safety
+    ///
+    /// `item` must be embedded in a `bindings::config_group`.
+    ///
+    /// If `item` does not represent the root group of a `configfs` subsystem,
+    /// the group must be embedded in a `Group<Data>`.
+    ///
+    /// Otherwise, the group must be a embedded in a
+    /// `bindings::configfs_subsystem` that is embedded in a `Subsystem<Data>`.
+    ///
+    /// `page` must point to a readable buffer of size at least `size`.
+    unsafe extern "C" fn store(
+        item: *mut bindings::config_item,
+        page: *const kernel::ffi::c_char,
+        size: usize,
+    ) -> isize {
+        let c_group: *mut bindings::config_group =
+        // SAFETY: By function safety requirements, `item` is embedded in a
+        // `config_group`.
+            unsafe { container_of!(item, bindings::config_group, cg_item) }.cast_mut();
+
+        // SAFETY: The function safety requirements for this function satisfy
+        // the conditions for this call.
+        let data: &Data = unsafe { get_group_data(c_group) };
+
+        let ret = O::store(
+            data,
+            // SAFETY: By function safety requirements, `page` is readable
+            // for at least `size`.
+            unsafe { core::slice::from_raw_parts(page.cast(), size) },
+        );
+
+        match ret {
+            Ok(()) => size as isize,
+            Err(err) => err.to_errno() as isize,
+        }
+    }
+
+    /// Create a new attribute.
+    ///
+    /// The attribute will appear as a file with name given by `name`.
+    pub const fn new(name: &'static CStr) -> Self {
+        Self {
+            attribute: Opaque::new(bindings::configfs_attribute {
+                ca_name: name as *const _ as _,
+                ca_owner: core::ptr::null_mut(),
+                ca_mode: 0o660,
+                show: Some(Self::show),
+                store: if O::HAS_STORE {
+                    Some(Self::store)
+                } else {
+                    None
+                },
+            }),
+            _p: PhantomData,
+        }
+    }
+}
+
+/// Operations supported by an attribute.
+///
+/// Implement this trait on type and pass that type as generic parameter when
+/// creating an [`Attribute`]. The type carrying the implementation serve no
+/// purpose other than specifying the attribute operations.
+#[vtable]
+pub trait AttributeOperations<const ID: u64 = 0> {
+    /// The type of the object that contains the field that is backing the
+    /// attribute for this operation.
+    type Data;
+
+    /// This function is called by the kernel to read the value of an attribute.
+    ///
+    /// Implementations should write the rendering of the attribute to `page`
+    /// and return the number of bytes written.
+    fn show(data: &Self::Data, page: &mut [u8; PAGE_SIZE]) -> Result<usize>;
+
+    /// This function is called by the kernel to update the value of an attribute.
+    ///
+    /// Implementations should parse the value from `page` and update internal
+    /// state to reflect the parsed value. Partial writes are not supported and
+    /// implementations should expect the full page to arrive in one write
+    /// operation.
+    fn store(_data: &Self::Data, _page: &[u8]) -> Result {
+        kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
+    }
+}
+
+/// A list of attributes.
+///
+/// This type is used to construct a new [`ItemType`]. It represents a list of
+/// [`Attribute`] that will appear in the directory representing a [`Group`].
+/// Users should not directly instantiate this type, rather they should use the
+/// [`kernel::configfs_attrs`] macro to declare a static set of attributes for a
+/// group.
+#[repr(transparent)]
+pub struct AttributeList<const N: usize, Data>(
+    UnsafeCell<[*mut kernel::ffi::c_void; N]>,
+    PhantomData<Data>,
+);
+
+// SAFETY: Ownership of `AttributeList` can safely be transferred to other threads.
+unsafe impl<const N: usize, Data> Send for AttributeList<N, Data> {}
+
+// SAFETY: We do not provide any operations on `AttributeList` that need synchronization.
+unsafe impl<const N: usize, Data> Sync for AttributeList<N, Data> {}
+
+impl<const N: usize, Data> AttributeList<N, Data> {
+    #[doc(hidden)]
+    /// # Safety
+    ///
+    /// This function can only be called by expanding the `configfs_attrs`
+    /// macro.
+    pub const unsafe fn new() -> Self {
+        Self(UnsafeCell::new([core::ptr::null_mut(); N]), PhantomData)
+    }
+
+    #[doc(hidden)]
+    /// # Safety
+    ///
+    /// This function can only be called by expanding the `configfs_attrs`
+    /// macro.
+    pub const unsafe fn add<
+        const I: usize,
+        const ID: u64,
+        O: AttributeOperations<ID, Data = Data>,
+    >(
+        &'static self,
+        attribute: &'static Attribute<ID, O, Data>,
+    ) {
+        if I >= N - 1 {
+            kernel::build_error!("Invalid attribute index");
+        }
+
+        // SAFETY: This function is only called through `configfs_attrs`. This
+        // ensures that we are evaluating the function in const context when
+        // initializing a static. As such, the reference created below will be
+        // exclusive.
+        unsafe {
+            (&mut *self.0.get())[I] = (attribute as *const Attribute<ID, O, Data>)
+                .cast_mut()
+                .cast()
+        };
+    }
+}
+
+/// A representation of the attributes that will appear in a [`Group`] or
+/// [`Subsystem`].
+///
+/// Users should not directly instantiate objects of this type. Rather, they
+/// should use the [`kernel::configfs_attrs`] macro to statically declare the
+/// shape of a [`Group`] or [`Subsystem`].
+#[pin_data]
+pub struct ItemType<Container, Data> {
+    #[pin]
+    item_type: Opaque<bindings::config_item_type>,
+    _p: PhantomData<(Container, Data)>,
+}
+
+// SAFETY: We do not provide any operations on `ItemType` that need synchronization.
+unsafe impl<Container, Data> Sync for ItemType<Container, Data> {}
+
+// SAFETY: Ownership of `ItemType` can safely be transferred to other threads.
+unsafe impl<Container, Data> Send for ItemType<Container, Data> {}
+
+macro_rules! impl_item_type {
+    ($tpe:ty) => {
+        impl<Data> ItemType<$tpe, Data> {
+            #[doc(hidden)]
+            pub const fn new_with_child_ctor<const N: usize, Child>(
+                owner: &'static ThisModule,
+                attributes: &'static AttributeList<N, Data>,
+            ) -> Self
+            where
+                Data: GroupOperations<Child = Child>,
+                Child: 'static,
+            {
+                Self {
+                    item_type: Opaque::new(bindings::config_item_type {
+                        ct_owner: owner.as_ptr(),
+                        ct_group_ops: (&GroupOperationsVTable::<Data, Child>::VTABLE as *const _)
+                            as *mut _,
+                        ct_item_ops: (&ItemOperationsVTable::<$tpe, Data>::VTABLE as *const _)
+                            as *mut _,
+                        ct_attrs: attributes as *const _ as _,
+                        ct_bin_attrs: core::ptr::null_mut(),
+                    }),
+                    _p: PhantomData,
+                }
+            }
+
+            #[doc(hidden)]
+            pub const fn new<const N: usize>(
+                owner: &'static ThisModule,
+                attributes: &'static AttributeList<N, Data>,
+            ) -> Self {
+                Self {
+                    item_type: Opaque::new(bindings::config_item_type {
+                        ct_owner: owner.as_ptr(),
+                        ct_group_ops: core::ptr::null_mut(),
+                        ct_item_ops: (&ItemOperationsVTable::<$tpe, Data>::VTABLE as *const _)
+                            as *mut _,
+                        ct_attrs: attributes as *const _ as _,
+                        ct_bin_attrs: core::ptr::null_mut(),
+                    }),
+                    _p: PhantomData,
+                }
+            }
+        }
+    };
+}
+
+impl_item_type!(Subsystem<Data>);
+impl_item_type!(Group<Data>);
+
+impl<Container, Data> ItemType<Container, Data> {
+    fn as_ptr(&self) -> *const bindings::config_item_type {
+        self.item_type.get()
+    }
+}
+
+/// Define a list of configfs attributes statically.
+#[macro_export]
+macro_rules! configfs_attrs {
+    (
+        container: $container:ty,
+        data: $data:ty,
+        attributes: [
+            $($name:ident: $attr:literal,)*
+        ],
+    ) => {
+        $crate::configfs_attrs!(
+            count:
+            @container($container),
+            @data($data),
+            @child(),
+            @no_child(x),
+            @attrs($($name $attr)*),
+            @eat($($name $attr,)*),
+            @assign(),
+            @cnt(0usize),
+        )
+    };
+    (
+        container: $container:ty,
+        data: $data:ty,
+        child: $child:ty,
+        attributes: [
+            $($name:ident: $attr:literal,)*
+        ],
+    ) => {
+        $crate::configfs_attrs!(
+            count:
+            @container($container),
+            @data($data),
+            @child($child),
+            @no_child(),
+            @attrs($($name $attr)*),
+            @eat($($name $attr,)*),
+            @assign(),
+            @cnt(0usize),
+        )
+    };
+    (count:
+     @container($container:ty),
+     @data($data:ty),
+     @child($($child:ty)?),
+     @no_child($($no_child:ident)?),
+     @attrs($($aname:ident $aattr:literal)*),
+     @eat($name:ident $attr:literal, $($rname:ident $rattr:literal,)*),
+     @assign($($assign:block)*),
+     @cnt($cnt:expr),
+    ) => {
+        $crate::configfs_attrs!(
+            count:
+            @container($container),
+            @data($data),
+            @child($($child)?),
+            @no_child($($no_child)?),
+            @attrs($($aname $aattr)*),
+            @eat($($rname $rattr,)*),
+            @assign($($assign)* {
+                const N: usize = $cnt;
+                // SAFETY: We are expanding `configfs_attrs`.
+                unsafe {
+                    $crate::macros::paste!( [< $data:upper _ATTRS >])
+                        .add::<N, $attr, _>(
+                            & $crate::macros::paste!( [< $data:upper _ $name:upper _ATTR >])
+                        )
+                };
+            }),
+            @cnt(1usize + $cnt),
+        )
+    };
+    (count:
+     @container($container:ty),
+     @data($data:ty),
+     @child($($child:ty)?),
+     @no_child($($no_child:ident)?),
+     @attrs($($aname:ident $aattr:literal)*),
+     @eat(),
+     @assign($($assign:block)*),
+     @cnt($cnt:expr),
+    ) =>
+    {
+        $crate::configfs_attrs!(
+            final:
+            @container($container),
+            @data($data),
+            @child($($child)?),
+            @no_child($($no_child)?),
+            @attrs($($aname $aattr)*),
+            @assign($($assign)*),
+            @cnt($cnt),
+        )
+    };
+    (final:
+     @container($container:ty),
+     @data($data:ty),
+     @child($($child:ty)?),
+     @no_child($($no_child:ident)?),
+     @attrs($($name:ident $attr:literal)*),
+     @assign($($assign:block)*),
+     @cnt($cnt:expr),
+    ) =>
+    {
+        {
+            $(
+                $crate::macros::paste!{
+                    // SAFETY: We are expanding `configfs_attrs`.
+                    static [< $data:upper _ $name:upper _ATTR >]:
+                      $crate::configfs::Attribute<$attr, $data, $data> =
+                        unsafe {
+                            $crate::configfs::Attribute::new(c_str!(::core::stringify!($name)))
+                        };
+                }
+            )*
+
+
+            const N: usize = $cnt + 1usize;
+            $crate::macros::paste!{
+                // SAFETY: We are expanding `configfs_attrs`.
+                static [< $data:upper _ATTRS >]:
+                  $crate::configfs::AttributeList<N, $data> =
+                    unsafe { $crate::configfs::AttributeList::new() };
+            }
+
+            $($assign)*
+
+            $(
+                $crate::macros::paste!{
+                    const [<$no_child:upper>]: bool = true;
+                };
+
+                $crate::macros::paste!{
+                    static [< $data:upper _TPE >] : $crate::configfs::ItemType<$container, $data>  =
+                        $crate::configfs::ItemType::<$container, $data>::new::<N>(
+                            &THIS_MODULE, &[<$ data:upper _ATTRS >]
+                        );
+                }
+            )?
+
+            $(
+                $crate::macros::paste!{
+                    static [< $data:upper _TPE >]:
+                      $crate::configfs::ItemType<$container, $data>  =
+                        $crate::configfs::ItemType::<$container, $data>::new_with_child_ctor::
+                    <N, $child>(
+                        &THIS_MODULE, &[<$ data:upper _ATTRS >]
+                    );
+                }
+            )?
+
+            &$crate::macros::paste!( [< $data:upper _TPE >] )
+        }
+    };
+
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911a..ec84653ab8c7a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,8 @@
 pub mod block;
 #[doc(hidden)]
 pub mod build_assert;
+#[cfg(CONFIG_CONFIGFS_FS)]
+pub mod configfs;
 pub mod cred;
 pub mod device;
 pub mod device_id;
diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 918dbead2c0b4..2f97bf9a7b4cc 100644
--- a/samples/rust/Kconfig
+++ b/samples/rust/Kconfig
@@ -10,6 +10,17 @@ menuconfig SAMPLES_RUST
 
 if SAMPLES_RUST
 
+config SAMPLE_RUST_CONFIGFS
+	tristate "Configfs sample"
+	depends on CONFIGFS_FS
+	help
+	  This option builds the Rust configfs sample.
+
+	  To compile this as a module, choose M here:
+	  the module will be called rust_configfs.
+
+	  If unsure, say N.
+
 config SAMPLE_RUST_MINIMAL
 	tristate "Minimal"
 	help
diff --git a/samples/rust/Makefile b/samples/rust/Makefile
index 5a8ab0df0567c..72122f010cafc 100644
--- a/samples/rust/Makefile
+++ b/samples/rust/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
 obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
 obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
+obj-$(CONFIG_SAMPLE_RUST_CONFIGFS)		+= rust_configfs.o
 
 rust_print-y := rust_print_main.o rust_print_events.o
 
diff --git a/samples/rust/rust_configfs.rs b/samples/rust/rust_configfs.rs
new file mode 100644
index 0000000000000..fe896e66efb41
--- /dev/null
+++ b/samples/rust/rust_configfs.rs
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust configfs sample.
+
+use kernel::alloc::flags;
+use kernel::c_str;
+use kernel::configfs;
+use kernel::configfs_attrs;
+use kernel::new_mutex;
+use kernel::prelude::*;
+use kernel::sync::Mutex;
+
+module! {
+    type: RustConfigfs,
+    name: "rust_configfs",
+    author: "Rust for Linux Contributors",
+    description: "Rust configfs sample",
+    license: "GPL",
+}
+
+#[pin_data]
+struct RustConfigfs {
+    #[pin]
+    config: configfs::Subsystem<Configuration>,
+}
+
+#[pin_data]
+struct Configuration {
+    message: &'static CStr,
+    #[pin]
+    bar: Mutex<(KBox<[u8; 4096]>, usize)>,
+}
+
+impl Configuration {
+    fn new() -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {
+            message: c_str!("Hello World\n"),
+            bar <- new_mutex!((KBox::new([0;4096], flags::GFP_KERNEL)?,0)),
+        })
+    }
+}
+
+impl kernel::InPlaceModule for RustConfigfs {
+    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
+        pr_info!("Rust configfs sample (init)\n");
+
+        let item_type = configfs_attrs! {
+            container: configfs::Subsystem<Configuration>,
+            data: Configuration,
+            child: Child,
+            attributes: [
+                message: 0,
+                bar: 1,
+            ],
+        };
+
+        try_pin_init!(Self {
+            config <- configfs::Subsystem::new(
+                kernel::c_str!("rust_configfs"), item_type, Configuration::new()
+            ),
+        })
+    }
+}
+
+#[vtable]
+impl configfs::GroupOperations for Configuration {
+    type Parent = Self;
+    type Child = Child;
+
+    fn make_group(
+        _this: &Self,
+        name: &CStr,
+    ) -> Result<impl PinInit<configfs::Group<Child>, Error>> {
+        let tpe = configfs_attrs! {
+            container: configfs::Group<Child>,
+            data: Child,
+            child: GrandChild,
+            attributes: [
+                baz: 0,
+            ],
+        };
+
+        Ok(configfs::Group::new(name.try_into()?, tpe, Child::new()))
+    }
+}
+
+#[vtable]
+impl configfs::AttributeOperations<0> for Configuration {
+    type Data = Configuration;
+
+    fn show(container: &Configuration, page: &mut [u8; 4096]) -> Result<usize> {
+        pr_info!("Show message\n");
+        let data = container.message;
+        page[0..data.len()].copy_from_slice(data);
+        Ok(data.len())
+    }
+}
+
+#[vtable]
+impl configfs::AttributeOperations<1> for Configuration {
+    type Data = Configuration;
+
+    fn show(container: &Configuration, page: &mut [u8; 4096]) -> Result<usize> {
+        pr_info!("Show bar\n");
+        let guard = container.bar.lock();
+        let data = guard.0.as_slice();
+        let len = guard.1;
+        page[0..len].copy_from_slice(&data[0..len]);
+        Ok(len)
+    }
+
+    fn store(container: &Configuration, page: &[u8]) -> Result {
+        pr_info!("Store bar\n");
+        let mut guard = container.bar.lock();
+        guard.0[0..page.len()].copy_from_slice(page);
+        guard.1 = page.len();
+        Ok(())
+    }
+}
+
+#[pin_data]
+struct Child {}
+
+impl Child {
+    fn new() -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {})
+    }
+}
+
+#[vtable]
+impl configfs::GroupOperations for Child {
+    type Parent = Self;
+    type Child = GrandChild;
+
+    fn make_group(
+        _this: &Self,
+        name: &CStr,
+    ) -> Result<impl PinInit<configfs::Group<GrandChild>, Error>> {
+        let tpe = configfs_attrs! {
+            container: configfs::Group<GrandChild>,
+            data: GrandChild,
+            attributes: [
+                gc: 0,
+            ],
+        };
+
+        Ok(configfs::Group::new(
+            name.try_into()?,
+            tpe,
+            GrandChild::new(),
+        ))
+    }
+}
+
+#[vtable]
+impl configfs::AttributeOperations<0> for Child {
+    type Data = Child;
+
+    fn show(_container: &Child, page: &mut [u8; 4096]) -> Result<usize> {
+        pr_info!("Show baz\n");
+        let data = c"Hello Baz\n".to_bytes();
+        page[0..data.len()].copy_from_slice(data);
+        Ok(data.len())
+    }
+}
+
+#[pin_data]
+struct GrandChild {}
+
+impl GrandChild {
+    fn new() -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {})
+    }
+}
+
+#[vtable]
+impl configfs::AttributeOperations<0> for GrandChild {
+    type Data = GrandChild;
+
+    fn show(_container: &GrandChild, page: &mut [u8; 4096]) -> Result<usize> {
+        pr_info!("Show baz\n");
+        let data = c"Hello GC\n".to_bytes();
+        page[0..data.len()].copy_from_slice(data);
+        Ok(data.len())
+    }
+}

-- 
2.47.0

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Charalampos Mitrodimas]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> This patch adds a rust API for configfs, thus allowing rust modules to use
> configfs for configuration. The implementation is a shim on top of the C
> configfs implementation allowing safe use of the C infrastructure from
> rust.
>
> The patch enables the `const_mut_refs` feature on compilers before rustc
> 1.83. The feature was stabilized in rustc 1.83 and is not required to be
> explicitly enabled on later versions.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> ---
>
> This patch is a direct dependency for `rnull`, the rust null block driver.
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/mutex.c            |   5 +
>  rust/kernel/configfs.rs         | 860 ++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   2 +
>  samples/rust/Kconfig            |  11 +
>  samples/rust/Makefile           |   1 +
>  samples/rust/rust_configfs.rs   | 186 +++++++++
>  7 files changed, 1066 insertions(+)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 55354e4dec14e..d115a770306f6 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -10,6 +10,7 @@
>  #include <linux/blk-mq.h>
>  #include <linux/blk_types.h>
>  #include <linux/blkdev.h>
> +#include <linux/configfs.h>
>  #include <linux/cred.h>
>  #include <linux/errname.h>
>  #include <linux/ethtool.h>
> diff --git a/rust/helpers/mutex.c b/rust/helpers/mutex.c
> index 06575553eda5c..3e9b910a88e9b 100644
> --- a/rust/helpers/mutex.c
> +++ b/rust/helpers/mutex.c
> @@ -17,3 +17,8 @@ void rust_helper_mutex_assert_is_held(struct mutex *mutex)
>  {
>  	lockdep_assert_held(mutex);
>  }
> +
> +void rust_helper_mutex_destroy(struct mutex *lock)
> +{
> +	mutex_destroy(lock);
> +}
> diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
> new file mode 100644
> index 0000000000000..9d4b381b9df89
> --- /dev/null
> +++ b/rust/kernel/configfs.rs
> @@ -0,0 +1,860 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! `configfs` interface.
> +//!
> +//! `configfs` is an in-memory pseudo file system for configuration of kernel
> +//! modules. Please see the [C documentation] for details and intended use of
> +//! `configfs`.
> +//!
> +//! This module does not support the following `configfs` features:
> +//!
> +//! - Items. All group children are groups.
> +//! - Symlink support.
> +//! - `disconnect_notify` hook.
> +//! - Default groups.
> +//!
> +//! See the [rust_configfs.rs] sample for a full example use of this module.
> +//!
> +//! C header: [`include/linux/configfs.h`](srctree/include/linux/configfs.h)
> +//!
> +//! [C documentation]: srctree/Documentation/filesystems/configfs.rst
> +//! [rust_configfs.rs]: srctree/samples/rust/rust_configfs.rs
> +
> +use crate::alloc::flags;
> +use crate::container_of;
> +use crate::page::PAGE_SIZE;
> +use crate::prelude::*;
> +use crate::str::CString;
> +use crate::sync::Arc;
> +use crate::types::ForeignOwnable;
> +use crate::types::Opaque;
> +use core::cell::UnsafeCell;
> +use core::marker::PhantomData;
> +use core::ptr::addr_of;
> +use core::ptr::addr_of_mut;
> +
> +/// A `configfs` subsystem.
> +///
> +/// This is the top level entrypoint for a `configfs` hierarchy. To register
> +/// with configfs, embed a field of this type into your kernel module struct.
> +#[pin_data(PinnedDrop)]
> +pub struct Subsystem<Data> {
> +    #[pin]
> +    subsystem: Opaque<bindings::configfs_subsystem>,
> +    #[pin]
> +    data: Data,
> +}
> +
> +// SAFETY: We do not provide any operations on `Subsystem`.
> +unsafe impl<Data> Sync for Subsystem<Data> {}
> +
> +// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
> +unsafe impl<Data> Send for Subsystem<Data> {}
> +
> +impl<Data> Subsystem<Data> {
> +    /// Create an initializer for a [`Subsystem`].
> +    ///
> +    /// The subsystem will appear in configfs as a directory name given by
> +    /// `name`. The attributes available in directory are specified by
> +    /// `item_type`.
> +    pub fn new(
> +        name: &'static CStr,
> +        item_type: &'static ItemType<Subsystem<Data>, Data>,
> +        data: impl PinInit<Data, Error>,
> +    ) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            subsystem <- kernel::init::zeroed().chain(
> +                |place: &mut Opaque<bindings::configfs_subsystem>| {
> +                    // SAFETY: All of `place` is valid for write.
> +                    unsafe {
> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_name )
> +                            .write(name.as_ptr().cast_mut().cast())
> +                    };
> +                    // SAFETY: All of `place` is valid for write.
> +                    unsafe {
> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_type)
> +                            .write(item_type.as_ptr())
> +                    };
> +                    // SAFETY: We initialized the required fields of `place.group` above.
> +                    unsafe { bindings::config_group_init(&mut (*place.get()).su_group) };
> +                    // SAFETY: `place.su_mutex` is valid for use as a mutex.
> +                    unsafe { bindings::__mutex_init(
> +                        &mut (*place.get()).su_mutex,
> +                        kernel::optional_name!().as_char_ptr(),
> +                        kernel::static_lock_class!().as_ptr())
> +                    }
> +                    Ok(())
> +                }),
> +            data <- data,
> +        })
> +        .pin_chain(|this| {
> +            crate::error::to_result(
> +                // SAFETY: We initialized `this.subsystem` according to C API contract above.
> +                unsafe { bindings::configfs_register_subsystem(this.subsystem.get()) },
> +            )
> +        })
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<Data> PinnedDrop for Subsystem<Data> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: We registered `self.subsystem` in the initializer returned by `Self::new`.
> +        unsafe { bindings::configfs_unregister_subsystem(self.subsystem.get()) };
> +        // SAFETY: We initialized the mutex in `Subsystem::new`.
> +        unsafe { bindings::mutex_destroy(addr_of_mut!((*self.subsystem.get()).su_mutex)) };
> +    }
> +}
> +
> +/// Trait that allows offset calculations for structs that embed a
> +/// `bindings::config_group`.
> +///
> +/// Users of the `configfs` API should not need to implement this trait.
> +///
> +/// # Safety
> +///
> +/// - Implementers of this trait must embed a `bindings::config_group`.
> +/// - Methods must be implemented according to method documentation.
> +pub unsafe trait HasGroup<Data> {
> +    /// Return the address of the `bindings::config_group` embedded in `Self`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `this` must be a valid allocation of at least the size of `Self`.
> +    unsafe fn group(this: *const Self) -> *const bindings::config_group;
> +
> +    /// Return the address of the `Self` that `group` is embedded in.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `group` must point to the `bindings::config_group` that is embedded in
> +    ///   `Self`.
> +    unsafe fn container_of(group: *const bindings::config_group) -> *const Self;
> +}
> +
> +// SAFETY: `Subsystem<Data>` embeds a field of type `bindings::config_group`
> +// within the `subsystem` field.
> +unsafe impl<Data> HasGroup<Data> for Subsystem<Data> {
> +    unsafe fn group(this: *const Self) -> *const bindings::config_group {
> +        // SAFETY: By impl and function safety requirement this projection is in bounds.
> +        unsafe { addr_of!((*(*this).subsystem.get()).su_group) }
> +    }
> +
> +    unsafe fn container_of(group: *const bindings::config_group) -> *const Self {
> +        // SAFETY: By impl and function safety requirement this projection is in bounds.
> +        let c_subsys_ptr = unsafe { container_of!(group, bindings::configfs_subsystem, su_group) };
> +        let opaque_ptr = c_subsys_ptr.cast::<Opaque<bindings::configfs_subsystem>>();
> +        // SAFETY: By impl and function safety requirement, `opaque_ptr` and the
> +        // pointer it returns, are within the same allocation.
> +        unsafe { container_of!(opaque_ptr, Subsystem<Data>, subsystem) }
> +    }
> +}
> +
> +/// A `configfs` group.
> +///
> +/// To add a subgroup to `configfs`, pass this type as `ctype` to
> +/// [`crate::configfs_attrs`] when creating a group in [`GroupOperations::make_group`].
> +#[pin_data]
> +pub struct Group<Data> {
> +    #[pin]
> +    group: Opaque<bindings::config_group>,
> +    #[pin]
> +    data: Data,
> +}
> +
> +impl<Data> Group<Data> {
> +    /// Create an initializer for a new group.
> +    ///
> +    /// When instantiated, the group will appear as a directory with the name
> +    /// given by `name` and it will contain attributes specified by `item_type`.
> +    pub fn new(
> +        name: CString,
> +        item_type: &'static ItemType<Group<Data>, Data>,
> +        data: impl PinInit<Data, Error>,
> +    ) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            group <- kernel::init::zeroed().chain(|v: &mut Opaque<bindings::config_group>| {
> +                let place = v.get();
> +                let name = name.as_bytes_with_nul().as_ptr();
> +                // SAFETY: It is safe to initialize a group once it has been zeroed.
> +                unsafe {
> +                    bindings::config_group_init_type_name(place, name as _, item_type.as_ptr())
> +                };
> +                Ok(())
> +            }),
> +            data <- data,
> +        })
> +    }
> +}
> +
> +// SAFETY: `Group<Data>` embeds a field of type `bindings::config_group`
> +// within the `group` field.
> +unsafe impl<Data> HasGroup<Data> for Group<Data> {
> +    unsafe fn group(this: *const Self) -> *const bindings::config_group {
> +        Opaque::raw_get(
> +            // SAFETY: By impl and function safety requirements this field
> +            // projection is within bounds of the allocation.
> +            unsafe { addr_of!((*this).group) },
> +        )
> +    }
> +
> +    unsafe fn container_of(group: *const bindings::config_group) -> *const Self {
> +        let opaque_ptr = group.cast::<Opaque<bindings::config_group>>();
> +        // SAFETY: By impl and function safety requirement, `opaque_ptr` and
> +        // pointer it returns will be in the same allocation.
> +        unsafe { container_of!(opaque_ptr, Self, group) }
> +    }
> +}
> +
> +/// # Safety
> +///
> +/// `this` must be a valid pointer.
> +///
> +/// If `this` does not represent the root group of a `configfs` subsystem,
> +/// `this` must be a pointer to a `bindings::config_group` embedded in a
> +/// `Group<Parent>`.
> +///
> +/// Otherwise, `this` must be a pointer to a `bindings::config_group` that
> +/// is embedded in a `bindings::configfs_subsystem` that is embedded in a
> +/// `Subsystem<Parent>`.
> +unsafe fn get_group_data<'a, Parent>(this: *mut bindings::config_group) -> &'a Parent {
> +    // TODO
> +    // SAFETY: `this` is a valid pointer.
> +    let is_root = unsafe { (*this).cg_subsys.is_null() };
> +
> +    if !is_root {
> +        // SAFETY: By C API contact, `this` is a pointer to a
> +        // `bindings::config_group` that we passed as a return value in from
> +        // `make_group`. Such a pointer is embedded within a `Group<Parent>`.
> +        unsafe { &(*Group::<Parent>::container_of(this)).data }
> +    } else {
> +        // SAFETY: By C API contract, `this` is a pointer to the
> +        // `bindings::config_group` field within a `Subsystem<Parent>`.
> +        unsafe { &(*Subsystem::container_of(this)).data }
> +    }
> +}
> +
> +struct GroupOperationsVTable<Parent, Child>(PhantomData<(Parent, Child)>)
> +where
> +    Parent: GroupOperations<Child = Child>;
> +
> +impl<Parent, Child> GroupOperationsVTable<Parent, Child>
> +where
> +    Parent: GroupOperations<Child = Child>,
> +    Child: 'static,
> +{
> +    /// # Safety
> +    ///
> +    /// `this` must be a valid pointer.
> +    ///
> +    /// If `this` does not represent the root group of a `configfs` subsystem,
> +    /// `this` must be a pointer to a `bindings::config_group` embedded in a
> +    /// `Group<Parent>`.
> +    ///
> +    /// Otherwise, `this` must be a pointer to a `bindings::config_group` that
> +    /// is embedded in a `bindings::configfs_subsystem` that is embedded in a
> +    /// `Subsystem<Parent>`.
> +    ///
> +    /// `name` must point to a null terminated string.
> +    unsafe extern "C" fn make_group(
> +        this: *mut bindings::config_group,
> +        name: *const kernel::ffi::c_char,
> +    ) -> *mut bindings::config_group {
> +        // SAFETY: By function safety requirements of this function, this call
> +        // is safe.
> +        let parent_data = unsafe { get_group_data(this) };
> +
> +        let group_init = match Parent::make_group(
> +            parent_data,
> +            // SAFETY: By function safety requirements, name points to a null
> +            // terminated string.
> +            unsafe { CStr::from_char_ptr(name) },
> +        ) {
> +            Ok(init) => init,
> +            Err(e) => return e.to_ptr(),
> +        };
> +
> +        let child_group = <Arc<Group<Child>> as InPlaceInit<Group<Child>>>::try_pin_init(
> +            group_init,
> +            flags::GFP_KERNEL,
> +        );
> +
> +        match child_group {
> +            Ok(child_group) => {
> +                let child_group_ptr = child_group.into_foreign();
> +                // SAFETY: We allocated the pointee of `child_ptr` above as a
> +                // `Group<Child>`.
> +                unsafe { Group::<Child>::group(child_group_ptr) }.cast_mut()
> +            }
> +            Err(e) => e.to_ptr(),
> +        }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// If `this` does not represent the root group of a `configfs` subsystem,
> +    /// `this` must be a pointer to a `bindings::config_group` embedded in a
> +    /// `Group<Parent>`.
> +    ///
> +    /// Otherwise, `this` must be a pointer to a `bindings::config_group` that
> +    /// is embedded in a `bindings::configfs_subsystem` that is embedded in a
> +    /// `Subsystem<Parent>`.
> +    ///
> +    /// `item` must point to a `bindings::config_item` within a
> +    /// `bindings::config_group` within a `Group<Child>`.
> +    unsafe extern "C" fn drop_item(
> +        this: *mut bindings::config_group,
> +        item: *mut bindings::config_item,
> +    ) {
> +        // SAFETY: By function safety requirements of this function, this call
> +        // is safe.
> +        let parent_data = unsafe { get_group_data(this) };
> +
> +        // SAFETY: By function safety requirements, `item` is embedded in a
> +        // `config_group`.
> +        let c_child_group_ptr =
> +            unsafe { kernel::container_of!(item, bindings::config_group, cg_item) };
> +        // SAFETY: By function safety requirements, `c_child_group_ptr` is
> +        // embedded within a `Group<CHLD>`.
> +        let r_child_group_ptr = unsafe { Group::<Child>::container_of(c_child_group_ptr) };
> +
> +        if Parent::HAS_DROP_ITEM {
> +            Parent::drop_item(
> +                parent_data,
> +                // SAFETY: We called `into_foreign` to produce `r_child_group_ptr` in
> +                // `make_group`. There are not other borrows of this pointer in existence.
> +                unsafe {
> +                    <Arc<Group<Child>> as ForeignOwnable>::borrow(r_child_group_ptr.cast_mut())
> +                },
> +            );
> +        }
> +
> +        // SAFETY: By C API contract, we are required to drop a refcount on
> +        // `item`.
> +        unsafe { bindings::config_item_put(item) };
> +    }
> +
> +    const VTABLE: bindings::configfs_group_operations = bindings::configfs_group_operations {
> +        make_item: None,
> +        make_group: Some(Self::make_group),
> +        disconnect_notify: None,
> +        drop_item: Some(Self::drop_item),
> +        is_visible: None,
> +        is_bin_visible: None,
> +    };
> +}
> +
> +struct ItemOperationsVTable<Container, Data>(PhantomData<(Container, Data)>);
> +
> +impl<Data> ItemOperationsVTable<Group<Data>, Data>
> +where
> +    Data: 'static,
> +{
> +    /// # Safety
> +    ///
> +    /// `this` must be a pointer to a `bindings::config_group` embedded in a
> +    /// `Group<Parent>`.
> +    ///
> +    /// This function will destroy the pointee of `this`. The pointee of `this`
> +    /// must not be accessed after the function returns.
> +    unsafe extern "C" fn release(this: *mut bindings::config_item) {
> +        // SAFETY: By function safety requirements, `this` is embedded in a
> +        // `config_group`.
> +        let c_group_ptr = unsafe { kernel::container_of!(this, bindings::config_group, cg_item) };
> +        // SAFETY: By function safety requirements, `c_group_ptr` is
> +        // embedded within a `Group<Data>`.
> +        let r_group_ptr = unsafe { Group::<Data>::container_of(c_group_ptr) };
> +
> +        // SAFETY: We called `into_foreign` on `r_group_ptr` in
> +        // `make_group`.
> +        let pin_self =
> +            unsafe { <Arc<Group<Data>> as ForeignOwnable>::from_foreign(r_group_ptr.cast_mut()) };
> +        drop(pin_self);
> +    }
> +
> +    const VTABLE: bindings::configfs_item_operations = bindings::configfs_item_operations {
> +        release: Some(Self::release),
> +        allow_link: None,
> +        drop_link: None,
> +    };
> +}
> +
> +impl<Data> ItemOperationsVTable<Subsystem<Data>, Data> {
> +    const VTABLE: bindings::configfs_item_operations = bindings::configfs_item_operations {
> +        release: None,
> +        allow_link: None,
> +        drop_link: None,
> +    };
> +}
> +
> +/// Operations implemented by `configfs` groups that can create subgroups.
> +///
> +/// Implement this trait on structs that embed a [`Subsystem`] or a [`Group`].
> +#[vtable]
> +pub trait GroupOperations {
> +    /// The parent data object type.
> +    ///
> +    /// The implementer of this trait is this kind of data object. Should be set
> +    /// to `Self`.
> +    type Parent;
> +
> +    /// The child data object type.
> +    ///
> +    /// This group will create subgroups (subdirectories) backed by this kind of
> +    /// object.
> +    type Child: 'static;
> +
> +    /// The kernel will call this method in response to `mkdir(2)` in the
> +    /// directory representing `this`.
> +    ///
> +    /// To accept the request to create a group, implementations should
> +    /// instantiate a `CHLD` and return a `CPTR` to it. To prevent creation,
> +    /// return a suitable error.
> +    fn make_group(
> +        this: &Self::Parent,
> +        name: &CStr,
> +    ) -> Result<impl PinInit<Group<Self::Child>, Error>>;
> +
> +    /// The kernel will call this method before the directory representing
> +    /// `_child` is removed from `configfs`.
> +    ///
> +    /// Implementations can use this method to do house keeping before
> +    /// `configfs` drops its reference to `Child`.
> +    fn drop_item(
> +        _this: &Self::Parent,
> +        _child: <Arc<Group<Self::Child>> as ForeignOwnable>::Borrowed<'_>,
> +    ) {
> +        kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
> +    }
> +}
> +
> +/// A `configfs` attribute.
> +///
> +/// An attribute appear as a file in configfs, inside a folder that represent
> +/// the group that the attribute belongs to.
> +#[repr(transparent)]
> +pub struct Attribute<const ID: u64, O, Data> {
> +    attribute: Opaque<bindings::configfs_attribute>,
> +    _p: PhantomData<(O, Data)>,
> +}
> +
> +// SAFETY: We do not provide any operations on `Attribute`.
> +unsafe impl<const ID: u64, O, Data> Sync for Attribute<ID, O, Data> {}
> +
> +// SAFETY: Ownership of `Attribute` can safely be transferred to other threads.
> +unsafe impl<const ID: u64, O, Data> Send for Attribute<ID, O, Data> {}
> +
> +impl<const ID: u64, O, Data> Attribute<ID, O, Data>
> +where
> +    O: AttributeOperations<ID, Data = Data>,
> +{
> +    /// # Safety
> +    ///
> +    /// `item` must be embedded in a `bindings::config_group`.
> +    ///
> +    /// If `item` does not represent the root group of a `configfs` subsystem,
> +    /// the group must be embedded in a `Group<Data>`.
> +    ///
> +    /// Otherwise, the group must be a embedded in a
> +    /// `bindings::configfs_subsystem` that is embedded in a `Subsystem<Data>`.
> +    ///
> +    /// `page` must point to a writable buffer of size at least [`PAGE_SIZE`].
> +    unsafe extern "C" fn show(
> +        item: *mut bindings::config_item,
> +        page: *mut kernel::ffi::c_char,
> +    ) -> isize {
> +        let c_group: *mut bindings::config_group =
> +        // SAFETY: By function safety requirements, `item` is embedded in a
> +        // `config_group`.
> +            unsafe { container_of!(item, bindings::config_group, cg_item) }.cast_mut();
> +
> +        // SAFETY: The function safety requirements for this function satisfy
> +        // the conditions for this call.
> +        let data: &Data = unsafe { get_group_data(c_group) };
> +
> +        // SAFETY: By function safety requirements, `page` is writable for `PAGE_SIZE`.
> +        let ret = O::show(data, unsafe { &mut *(page as *mut [u8; PAGE_SIZE]) });
> +
> +        match ret {
> +            Ok(size) => size as isize,
> +            Err(err) => err.to_errno() as isize,
> +        }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `item` must be embedded in a `bindings::config_group`.
> +    ///
> +    /// If `item` does not represent the root group of a `configfs` subsystem,
> +    /// the group must be embedded in a `Group<Data>`.
> +    ///
> +    /// Otherwise, the group must be a embedded in a
> +    /// `bindings::configfs_subsystem` that is embedded in a `Subsystem<Data>`.
> +    ///
> +    /// `page` must point to a readable buffer of size at least `size`.
> +    unsafe extern "C" fn store(
> +        item: *mut bindings::config_item,
> +        page: *const kernel::ffi::c_char,
> +        size: usize,
> +    ) -> isize {
> +        let c_group: *mut bindings::config_group =
> +        // SAFETY: By function safety requirements, `item` is embedded in a
> +        // `config_group`.
> +            unsafe { container_of!(item, bindings::config_group, cg_item) }.cast_mut();
> +
> +        // SAFETY: The function safety requirements for this function satisfy
> +        // the conditions for this call.
> +        let data: &Data = unsafe { get_group_data(c_group) };
> +
> +        let ret = O::store(
> +            data,
> +            // SAFETY: By function safety requirements, `page` is readable
> +            // for at least `size`.
> +            unsafe { core::slice::from_raw_parts(page.cast(), size) },
> +        );
> +
> +        match ret {
> +            Ok(()) => size as isize,
> +            Err(err) => err.to_errno() as isize,
> +        }
> +    }
> +
> +    /// Create a new attribute.
> +    ///
> +    /// The attribute will appear as a file with name given by `name`.
> +    pub const fn new(name: &'static CStr) -> Self {
> +        Self {
> +            attribute: Opaque::new(bindings::configfs_attribute {
> +                ca_name: name as *const _ as _,
> +                ca_owner: core::ptr::null_mut(),
> +                ca_mode: 0o660,
> +                show: Some(Self::show),
> +                store: if O::HAS_STORE {
> +                    Some(Self::store)
> +                } else {
> +                    None
> +                },
> +            }),
> +            _p: PhantomData,
> +        }
> +    }
> +}
> +
> +/// Operations supported by an attribute.
> +///
> +/// Implement this trait on type and pass that type as generic parameter when
> +/// creating an [`Attribute`]. The type carrying the implementation serve no
> +/// purpose other than specifying the attribute operations.
> +#[vtable]
> +pub trait AttributeOperations<const ID: u64 = 0> {
> +    /// The type of the object that contains the field that is backing the
> +    /// attribute for this operation.
> +    type Data;
> +
> +    /// This function is called by the kernel to read the value of an attribute.
> +    ///
> +    /// Implementations should write the rendering of the attribute to `page`
> +    /// and return the number of bytes written.
> +    fn show(data: &Self::Data, page: &mut [u8; PAGE_SIZE]) -> Result<usize>;
> +
> +    /// This function is called by the kernel to update the value of an attribute.
> +    ///
> +    /// Implementations should parse the value from `page` and update internal
> +    /// state to reflect the parsed value. Partial writes are not supported and
> +    /// implementations should expect the full page to arrive in one write
> +    /// operation.
> +    fn store(_data: &Self::Data, _page: &[u8]) -> Result {
> +        kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
> +    }
> +}
> +
> +/// A list of attributes.
> +///
> +/// This type is used to construct a new [`ItemType`]. It represents a list of
> +/// [`Attribute`] that will appear in the directory representing a [`Group`].
> +/// Users should not directly instantiate this type, rather they should use the
> +/// [`kernel::configfs_attrs`] macro to declare a static set of attributes for a
> +/// group.
> +#[repr(transparent)]
> +pub struct AttributeList<const N: usize, Data>(
> +    UnsafeCell<[*mut kernel::ffi::c_void; N]>,
> +    PhantomData<Data>,
> +);
> +
> +// SAFETY: Ownership of `AttributeList` can safely be transferred to other threads.
> +unsafe impl<const N: usize, Data> Send for AttributeList<N, Data> {}
> +
> +// SAFETY: We do not provide any operations on `AttributeList` that need synchronization.
> +unsafe impl<const N: usize, Data> Sync for AttributeList<N, Data> {}
> +
> +impl<const N: usize, Data> AttributeList<N, Data> {
> +    #[doc(hidden)]
> +    /// # Safety
> +    ///
> +    /// This function can only be called by expanding the `configfs_attrs`
> +    /// macro.
> +    pub const unsafe fn new() -> Self {
> +        Self(UnsafeCell::new([core::ptr::null_mut(); N]), PhantomData)
> +    }
> +
> +    #[doc(hidden)]
> +    /// # Safety
> +    ///
> +    /// This function can only be called by expanding the `configfs_attrs`
> +    /// macro.
> +    pub const unsafe fn add<
> +        const I: usize,
> +        const ID: u64,
> +        O: AttributeOperations<ID, Data = Data>,
> +    >(
> +        &'static self,
> +        attribute: &'static Attribute<ID, O, Data>,
> +    ) {
> +        if I >= N - 1 {
> +            kernel::build_error!("Invalid attribute index");
> +        }
> +
> +        // SAFETY: This function is only called through `configfs_attrs`. This
> +        // ensures that we are evaluating the function in const context when
> +        // initializing a static. As such, the reference created below will be
> +        // exclusive.
> +        unsafe {
> +            (&mut *self.0.get())[I] = (attribute as *const Attribute<ID, O, Data>)
> +                .cast_mut()
> +                .cast()
> +        };
> +    }
> +}
> +
> +/// A representation of the attributes that will appear in a [`Group`] or
> +/// [`Subsystem`].
> +///
> +/// Users should not directly instantiate objects of this type. Rather, they
> +/// should use the [`kernel::configfs_attrs`] macro to statically declare the
> +/// shape of a [`Group`] or [`Subsystem`].
> +#[pin_data]
> +pub struct ItemType<Container, Data> {
> +    #[pin]
> +    item_type: Opaque<bindings::config_item_type>,
> +    _p: PhantomData<(Container, Data)>,
> +}
> +
> +// SAFETY: We do not provide any operations on `ItemType` that need synchronization.
> +unsafe impl<Container, Data> Sync for ItemType<Container, Data> {}
> +
> +// SAFETY: Ownership of `ItemType` can safely be transferred to other threads.
> +unsafe impl<Container, Data> Send for ItemType<Container, Data> {}
> +
> +macro_rules! impl_item_type {
> +    ($tpe:ty) => {
> +        impl<Data> ItemType<$tpe, Data> {
> +            #[doc(hidden)]
> +            pub const fn new_with_child_ctor<const N: usize, Child>(
> +                owner: &'static ThisModule,
> +                attributes: &'static AttributeList<N, Data>,
> +            ) -> Self
> +            where
> +                Data: GroupOperations<Child = Child>,
> +                Child: 'static,
> +            {
> +                Self {
> +                    item_type: Opaque::new(bindings::config_item_type {
> +                        ct_owner: owner.as_ptr(),
> +                        ct_group_ops: (&GroupOperationsVTable::<Data, Child>::VTABLE as *const _)
> +                            as *mut _,
> +                        ct_item_ops: (&ItemOperationsVTable::<$tpe, Data>::VTABLE as *const _)
> +                            as *mut _,
> +                        ct_attrs: attributes as *const _ as _,
> +                        ct_bin_attrs: core::ptr::null_mut(),
> +                    }),
> +                    _p: PhantomData,
> +                }
> +            }
> +
> +            #[doc(hidden)]
> +            pub const fn new<const N: usize>(
> +                owner: &'static ThisModule,
> +                attributes: &'static AttributeList<N, Data>,
> +            ) -> Self {
> +                Self {
> +                    item_type: Opaque::new(bindings::config_item_type {
> +                        ct_owner: owner.as_ptr(),
> +                        ct_group_ops: core::ptr::null_mut(),
> +                        ct_item_ops: (&ItemOperationsVTable::<$tpe, Data>::VTABLE as *const _)
> +                            as *mut _,
> +                        ct_attrs: attributes as *const _ as _,
> +                        ct_bin_attrs: core::ptr::null_mut(),
> +                    }),
> +                    _p: PhantomData,
> +                }
> +            }
> +        }
> +    };
> +}
> +
> +impl_item_type!(Subsystem<Data>);
> +impl_item_type!(Group<Data>);
> +
> +impl<Container, Data> ItemType<Container, Data> {
> +    fn as_ptr(&self) -> *const bindings::config_item_type {
> +        self.item_type.get()
> +    }
> +}
> +
> +/// Define a list of configfs attributes statically.
> +#[macro_export]
> +macro_rules! configfs_attrs {
> +    (
> +        container: $container:ty,
> +        data: $data:ty,
> +        attributes: [
> +            $($name:ident: $attr:literal,)*
> +        ],
> +    ) => {
> +        $crate::configfs_attrs!(
> +            count:
> +            @container($container),
> +            @data($data),
> +            @child(),
> +            @no_child(x),
> +            @attrs($($name $attr)*),
> +            @eat($($name $attr,)*),
> +            @assign(),
> +            @cnt(0usize),
> +        )
> +    };
> +    (
> +        container: $container:ty,
> +        data: $data:ty,
> +        child: $child:ty,
> +        attributes: [
> +            $($name:ident: $attr:literal,)*
> +        ],
> +    ) => {
> +        $crate::configfs_attrs!(
> +            count:
> +            @container($container),
> +            @data($data),
> +            @child($child),
> +            @no_child(),
> +            @attrs($($name $attr)*),
> +            @eat($($name $attr,)*),
> +            @assign(),
> +            @cnt(0usize),
> +        )
> +    };
> +    (count:
> +     @container($container:ty),
> +     @data($data:ty),
> +     @child($($child:ty)?),
> +     @no_child($($no_child:ident)?),
> +     @attrs($($aname:ident $aattr:literal)*),
> +     @eat($name:ident $attr:literal, $($rname:ident $rattr:literal,)*),
> +     @assign($($assign:block)*),
> +     @cnt($cnt:expr),
> +    ) => {
> +        $crate::configfs_attrs!(
> +            count:
> +            @container($container),
> +            @data($data),
> +            @child($($child)?),
> +            @no_child($($no_child)?),
> +            @attrs($($aname $aattr)*),
> +            @eat($($rname $rattr,)*),
> +            @assign($($assign)* {
> +                const N: usize = $cnt;
> +                // SAFETY: We are expanding `configfs_attrs`.
> +                unsafe {
> +                    $crate::macros::paste!( [< $data:upper _ATTRS >])
> +                        .add::<N, $attr, _>(
> +                            & $crate::macros::paste!( [< $data:upper _ $name:upper _ATTR >])
> +                        )
> +                };
> +            }),
> +            @cnt(1usize + $cnt),
> +        )
> +    };
> +    (count:
> +     @container($container:ty),
> +     @data($data:ty),
> +     @child($($child:ty)?),
> +     @no_child($($no_child:ident)?),
> +     @attrs($($aname:ident $aattr:literal)*),
> +     @eat(),
> +     @assign($($assign:block)*),
> +     @cnt($cnt:expr),
> +    ) =>
> +    {
> +        $crate::configfs_attrs!(
> +            final:
> +            @container($container),
> +            @data($data),
> +            @child($($child)?),
> +            @no_child($($no_child)?),
> +            @attrs($($aname $aattr)*),
> +            @assign($($assign)*),
> +            @cnt($cnt),
> +        )
> +    };
> +    (final:
> +     @container($container:ty),
> +     @data($data:ty),
> +     @child($($child:ty)?),
> +     @no_child($($no_child:ident)?),
> +     @attrs($($name:ident $attr:literal)*),
> +     @assign($($assign:block)*),
> +     @cnt($cnt:expr),
> +    ) =>
> +    {
> +        {
> +            $(
> +                $crate::macros::paste!{
> +                    // SAFETY: We are expanding `configfs_attrs`.
> +                    static [< $data:upper _ $name:upper _ATTR >]:
> +                      $crate::configfs::Attribute<$attr, $data, $data> =
> +                        unsafe {
> +                            $crate::configfs::Attribute::new(c_str!(::core::stringify!($name)))
> +                        };
> +                }
> +            )*
> +
> +
> +            const N: usize = $cnt + 1usize;
> +            $crate::macros::paste!{
> +                // SAFETY: We are expanding `configfs_attrs`.
> +                static [< $data:upper _ATTRS >]:
> +                  $crate::configfs::AttributeList<N, $data> =
> +                    unsafe { $crate::configfs::AttributeList::new() };
> +            }
> +
> +            $($assign)*
> +
> +            $(
> +                $crate::macros::paste!{
> +                    const [<$no_child:upper>]: bool = true;
> +                };
> +
> +                $crate::macros::paste!{
> +                    static [< $data:upper _TPE >] : $crate::configfs::ItemType<$container, $data>  =
> +                        $crate::configfs::ItemType::<$container, $data>::new::<N>(
> +                            &THIS_MODULE, &[<$ data:upper _ATTRS >]
> +                        );
> +                }
> +            )?
> +
> +            $(
> +                $crate::macros::paste!{
> +                    static [< $data:upper _TPE >]:
> +                      $crate::configfs::ItemType<$container, $data>  =
> +                        $crate::configfs::ItemType::<$container, $data>::new_with_child_ctor::
> +                    <N, $child>(
> +                        &THIS_MODULE, &[<$ data:upper _ATTRS >]
> +                    );
> +                }
> +            )?
> +
> +            &$crate::macros::paste!( [< $data:upper _TPE >] )
> +        }
> +    };
> +
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911a..ec84653ab8c7a 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -40,6 +40,8 @@
>  pub mod block;
>  #[doc(hidden)]
>  pub mod build_assert;
> +#[cfg(CONFIG_CONFIGFS_FS)]
> +pub mod configfs;
>  pub mod cred;
>  pub mod device;
>  pub mod device_id;
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index 918dbead2c0b4..2f97bf9a7b4cc 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -10,6 +10,17 @@ menuconfig SAMPLES_RUST
>  
>  if SAMPLES_RUST
>  
> +config SAMPLE_RUST_CONFIGFS
> +	tristate "Configfs sample"
> +	depends on CONFIGFS_FS
> +	help
> +	  This option builds the Rust configfs sample.
> +
> +	  To compile this as a module, choose M here:
> +	  the module will be called rust_configfs.
> +
> +	  If unsure, say N.
> +
>  config SAMPLE_RUST_MINIMAL
>  	tristate "Minimal"
>  	help
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index 5a8ab0df0567c..72122f010cafc 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
>  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
> +obj-$(CONFIG_SAMPLE_RUST_CONFIGFS)		+= rust_configfs.o
>  
>  rust_print-y := rust_print_main.o rust_print_events.o
>  
> diff --git a/samples/rust/rust_configfs.rs b/samples/rust/rust_configfs.rs
> new file mode 100644
> index 0000000000000..fe896e66efb41
> --- /dev/null
> +++ b/samples/rust/rust_configfs.rs
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Rust configfs sample.
> +
> +use kernel::alloc::flags;
> +use kernel::c_str;
> +use kernel::configfs;
> +use kernel::configfs_attrs;
> +use kernel::new_mutex;
> +use kernel::prelude::*;
> +use kernel::sync::Mutex;
> +
> +module! {
> +    type: RustConfigfs,
> +    name: "rust_configfs",
> +    author: "Rust for Linux Contributors",
> +    description: "Rust configfs sample",
> +    license: "GPL",
> +}
> +
> +#[pin_data]
> +struct RustConfigfs {
> +    #[pin]
> +    config: configfs::Subsystem<Configuration>,
> +}
> +
> +#[pin_data]
> +struct Configuration {
> +    message: &'static CStr,
> +    #[pin]
> +    bar: Mutex<(KBox<[u8; 4096]>, usize)>,
> +}
> +
> +impl Configuration {
> +    fn new() -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            message: c_str!("Hello World\n"),
> +            bar <- new_mutex!((KBox::new([0;4096], flags::GFP_KERNEL)?,0)),
> +        })
> +    }
> +}
> +
> +impl kernel::InPlaceModule for RustConfigfs {
> +    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> +        pr_info!("Rust configfs sample (init)\n");
> +
> +        let item_type = configfs_attrs! {
> +            container: configfs::Subsystem<Configuration>,
> +            data: Configuration,
> +            child: Child,
> +            attributes: [
> +                message: 0,
> +                bar: 1,
> +            ],
> +        };
> +
> +        try_pin_init!(Self {
> +            config <- configfs::Subsystem::new(
> +                kernel::c_str!("rust_configfs"), item_type, Configuration::new()
> +            ),
> +        })
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::GroupOperations for Configuration {
> +    type Parent = Self;
> +    type Child = Child;
> +
> +    fn make_group(
> +        _this: &Self,
> +        name: &CStr,
> +    ) -> Result<impl PinInit<configfs::Group<Child>, Error>> {
> +        let tpe = configfs_attrs! {
> +            container: configfs::Group<Child>,
> +            data: Child,
> +            child: GrandChild,
> +            attributes: [
> +                baz: 0,
> +            ],
> +        };
> +
> +        Ok(configfs::Group::new(name.try_into()?, tpe, Child::new()))
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<0> for Configuration {
> +    type Data = Configuration;
> +
> +    fn show(container: &Configuration, page: &mut [u8; 4096]) -> Result<usize> {
> +        pr_info!("Show message\n");
> +        let data = container.message;
> +        page[0..data.len()].copy_from_slice(data);
> +        Ok(data.len())
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<1> for Configuration {
> +    type Data = Configuration;
> +
> +    fn show(container: &Configuration, page: &mut [u8; 4096]) -> Result<usize> {
> +        pr_info!("Show bar\n");
> +        let guard = container.bar.lock();
> +        let data = guard.0.as_slice();
> +        let len = guard.1;
> +        page[0..len].copy_from_slice(&data[0..len]);
> +        Ok(len)
> +    }
> +
> +    fn store(container: &Configuration, page: &[u8]) -> Result {
> +        pr_info!("Store bar\n");
> +        let mut guard = container.bar.lock();
> +        guard.0[0..page.len()].copy_from_slice(page);
> +        guard.1 = page.len();
> +        Ok(())
> +    }
> +}
> +
> +#[pin_data]
> +struct Child {}
> +
> +impl Child {
> +    fn new() -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {})
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::GroupOperations for Child {
> +    type Parent = Self;
> +    type Child = GrandChild;
> +
> +    fn make_group(
> +        _this: &Self,
> +        name: &CStr,
> +    ) -> Result<impl PinInit<configfs::Group<GrandChild>, Error>> {
> +        let tpe = configfs_attrs! {
> +            container: configfs::Group<GrandChild>,
> +            data: GrandChild,
> +            attributes: [
> +                gc: 0,
> +            ],
> +        };
> +
> +        Ok(configfs::Group::new(
> +            name.try_into()?,
> +            tpe,
> +            GrandChild::new(),
> +        ))
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<0> for Child {
> +    type Data = Child;
> +
> +    fn show(_container: &Child, page: &mut [u8; 4096]) -> Result<usize> {
> +        pr_info!("Show baz\n");
> +        let data = c"Hello Baz\n".to_bytes();
> +        page[0..data.len()].copy_from_slice(data);
> +        Ok(data.len())
> +    }
> +}
> +
> +#[pin_data]
> +struct GrandChild {}
> +
> +impl GrandChild {
> +    fn new() -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {})
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<0> for GrandChild {
> +    type Data = GrandChild;
> +
> +    fn show(_container: &GrandChild, page: &mut [u8; 4096]) -> Result<usize> {
> +        pr_info!("Show baz\n");
> +        let data = c"Hello GC\n".to_bytes();
> +        page[0..data.len()].copy_from_slice(data);
> +        Ok(data.len())
> +    }
> +}

Tested-by: Charalampos Mitrodimas <charmitro@posteo.net>

One minor nit, "4096" is mentioned a lot in the sample, which is normal,
should we have it as PAGE_SIZE (from kernel::page)?

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Andreas Hindborg]

"Charalampos Mitrodimas" <charmitro@posteo.net> writes:

> Andreas Hindborg <a.hindborg@kernel.org> writes:
>
>> This patch adds a rust API for configfs, thus allowing rust modules to use
>> configfs for configuration. The implementation is a shim on top of the C
>> configfs implementation allowing safe use of the C infrastructure from
>> rust.
>>
>> The patch enables the `const_mut_refs` feature on compilers before rustc
>> 1.83. The feature was stabilized in rustc 1.83 and is not required to be
>> explicitly enabled on later versions.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>

[...]

>
> Tested-by: Charalampos Mitrodimas <charmitro@posteo.net>

Thanks for testing!

>
> One minor nit, "4096" is mentioned a lot in the sample, which is normal,
> should we have it as PAGE_SIZE (from kernel::page)?

Yes, I forgot. I will fix that.


Best regards,
Andreas Hindborg

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Charalampos Mitrodimas]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> diff --git a/samples/rust/rust_configfs.rs b/samples/rust/rust_configfs.rs
> new file mode 100644
> index 0000000000000..fe896e66efb41
> --- /dev/null
> +++ b/samples/rust/rust_configfs.rs
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Rust configfs sample.
> +
> +use kernel::alloc::flags;
> +use kernel::c_str;

[...snip...]

> +impl Configuration {
> +    fn new() -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            message: c_str!("Hello World\n"),
> +            bar <- new_mutex!((KBox::new([0;4096], flags::GFP_KERNEL)?,0)),
> +        })
> +    }
> +}
> +
> +impl kernel::InPlaceModule for RustConfigfs {
> +    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> +        pr_info!("Rust configfs sample (init)\n");
> +
> +        let item_type = configfs_attrs! {
> +            container: configfs::Subsystem<Configuration>,
> +            data: Configuration,
> +            child: Child,
> +            attributes: [
> +                message: 0,
> +                bar: 1,
> +            ],
> +        };
> +
> +        try_pin_init!(Self {
> +            config <- configfs::Subsystem::new(
> +                kernel::c_str!("rust_configfs"), item_type, Configuration::new()

Hi Andreas,

One more nit I found, "kernel::c_str" is already imported in the
sample. So this can be changed to "c_str!("rust_configfs")".

C. Mitrodimas

> +            ),
> +        })
> +    }
> +}
> +

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Andreas Hindborg]

"Charalampos Mitrodimas" <charmitro@posteo.net> writes:

> Andreas Hindborg <a.hindborg@kernel.org> writes:
>
>> diff --git a/samples/rust/rust_configfs.rs b/samples/rust/rust_configfs.rs
>> new file mode 100644
>> index 0000000000000..fe896e66efb41
>> --- /dev/null
>> +++ b/samples/rust/rust_configfs.rs
>> @@ -0,0 +1,186 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Rust configfs sample.
>> +
>> +use kernel::alloc::flags;
>> +use kernel::c_str;
>
> [...snip...]
>
>> +impl Configuration {
>> +    fn new() -> impl PinInit<Self, Error> {
>> +        try_pin_init!(Self {
>> +            message: c_str!("Hello World\n"),
>> +            bar <- new_mutex!((KBox::new([0;4096], flags::GFP_KERNEL)?,0)),
>> +        })
>> +    }
>> +}
>> +
>> +impl kernel::InPlaceModule for RustConfigfs {
>> +    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>> +        pr_info!("Rust configfs sample (init)\n");
>> +
>> +        let item_type = configfs_attrs! {
>> +            container: configfs::Subsystem<Configuration>,
>> +            data: Configuration,
>> +            child: Child,
>> +            attributes: [
>> +                message: 0,
>> +                bar: 1,
>> +            ],
>> +        };
>> +
>> +        try_pin_init!(Self {
>> +            config <- configfs::Subsystem::new(
>> +                kernel::c_str!("rust_configfs"), item_type, Configuration::new()
>
> Hi Andreas,
>
> One more nit I found, "kernel::c_str" is already imported in the
> sample. So this can be changed to "c_str!("rust_configfs")".

Thanks!


Best regards,
Andreas Hindborg

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Benno Lossin]

On 07.02.25 15:41, Andreas Hindborg wrote:
> This patch adds a rust API for configfs, thus allowing rust modules to use
> configfs for configuration. The implementation is a shim on top of the C
> configfs implementation allowing safe use of the C infrastructure from
> rust.
> 
> The patch enables the `const_mut_refs` feature on compilers before rustc
> 1.83. The feature was stabilized in rustc 1.83 and is not required to be
> explicitly enabled on later versions.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> 
> ---
> 
> This patch is a direct dependency for `rnull`, the rust null block driver.
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/helpers/mutex.c            |   5 +
>  rust/kernel/configfs.rs         | 860 ++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs              |   2 +
>  samples/rust/Kconfig            |  11 +
>  samples/rust/Makefile           |   1 +
>  samples/rust/rust_configfs.rs   | 186 +++++++++

Can you move the sample into its own patch?

>  7 files changed, 1066 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 55354e4dec14e..d115a770306f6 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -10,6 +10,7 @@
>  #include <linux/blk-mq.h>
>  #include <linux/blk_types.h>
>  #include <linux/blkdev.h>
> +#include <linux/configfs.h>
>  #include <linux/cred.h>
>  #include <linux/errname.h>
>  #include <linux/ethtool.h>
> diff --git a/rust/helpers/mutex.c b/rust/helpers/mutex.c
> index 06575553eda5c..3e9b910a88e9b 100644
> --- a/rust/helpers/mutex.c
> +++ b/rust/helpers/mutex.c
> @@ -17,3 +17,8 @@ void rust_helper_mutex_assert_is_held(struct mutex *mutex)
>  {
>  	lockdep_assert_held(mutex);
>  }
> +
> +void rust_helper_mutex_destroy(struct mutex *lock)
> +{
> +	mutex_destroy(lock);
> +}
> diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
> new file mode 100644
> index 0000000000000..9d4b381b9df89
> --- /dev/null
> +++ b/rust/kernel/configfs.rs
> @@ -0,0 +1,860 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! `configfs` interface.
> +//!
> +//! `configfs` is an in-memory pseudo file system for configuration of kernel
> +//! modules. Please see the [C documentation] for details and intended use of
> +//! `configfs`.
> +//!
> +//! This module does not support the following `configfs` features:
> +//!
> +//! - Items. All group children are groups.
> +//! - Symlink support.
> +//! - `disconnect_notify` hook.
> +//! - Default groups.

For lists like this, I usually end items except the last one with a
comma instead of a period.

> +//!
> +//! See the [rust_configfs.rs] sample for a full example use of this module.

It could also be useful to just put the example directly here into the
docs instead of/additionally to having it as a sample.

> +//!
> +//! C header: [`include/linux/configfs.h`](srctree/include/linux/configfs.h)
> +//!
> +//! [C documentation]: srctree/Documentation/filesystems/configfs.rst
> +//! [rust_configfs.rs]: srctree/samples/rust/rust_configfs.rs
> +
> +use crate::alloc::flags;
> +use crate::container_of;
> +use crate::page::PAGE_SIZE;
> +use crate::prelude::*;
> +use crate::str::CString;
> +use crate::sync::Arc;
> +use crate::types::ForeignOwnable;
> +use crate::types::Opaque;
> +use core::cell::UnsafeCell;
> +use core::marker::PhantomData;
> +use core::ptr::addr_of;
> +use core::ptr::addr_of_mut;

I usually would import this like so:

    use crate::{
        alloc::flags,
        container_of,
        page::PAGE_SIZE,
        prelude::*,
        str::CString,
        sync::Arc,
        types::{ForeignOwnable, Opaque},
    };
    use core::{
        cell::UnsafeCell,
        marker::PhantomData,
        ptr::{addr_of, addr_of_mut},
    };

To me this is more readable.

> +
> +/// A `configfs` subsystem.
> +///
> +/// This is the top level entrypoint for a `configfs` hierarchy. To register
> +/// with configfs, embed a field of this type into your kernel module struct.
> +#[pin_data(PinnedDrop)]
> +pub struct Subsystem<Data> {

Usually, we don't have multi-character generics, any specific reason
that you chose `Data` here over `T` or `D`?

> +    #[pin]
> +    subsystem: Opaque<bindings::configfs_subsystem>,
> +    #[pin]
> +    data: Data,
> +}
> +
> +// SAFETY: We do not provide any operations on `Subsystem`.
> +unsafe impl<Data> Sync for Subsystem<Data> {}
> +
> +// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
> +unsafe impl<Data> Send for Subsystem<Data> {}
> +
> +impl<Data> Subsystem<Data> {
> +    /// Create an initializer for a [`Subsystem`].
> +    ///
> +    /// The subsystem will appear in configfs as a directory name given by
> +    /// `name`. The attributes available in directory are specified by
> +    /// `item_type`.
> +    pub fn new(
> +        name: &'static CStr,
> +        item_type: &'static ItemType<Subsystem<Data>, Data>,
> +        data: impl PinInit<Data, Error>,
> +    ) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            subsystem <- kernel::init::zeroed().chain(
> +                |place: &mut Opaque<bindings::configfs_subsystem>| {
> +                    // SAFETY: All of `place` is valid for write.
> +                    unsafe {
> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_name )
> +                            .write(name.as_ptr().cast_mut().cast())
> +                    };
> +                    // SAFETY: All of `place` is valid for write.
> +                    unsafe {
> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_type)
> +                            .write(item_type.as_ptr())
> +                    };
> +                    // SAFETY: We initialized the required fields of `place.group` above.
> +                    unsafe { bindings::config_group_init(&mut (*place.get()).su_group) };
> +                    // SAFETY: `place.su_mutex` is valid for use as a mutex.
> +                    unsafe { bindings::__mutex_init(
> +                        &mut (*place.get()).su_mutex,
> +                        kernel::optional_name!().as_char_ptr(),
> +                        kernel::static_lock_class!().as_ptr())

Formatting for this code is weird.

(since this is inside of the `try_pin_init!` macro, rustfmt doesn't
format it, since `<-` isn't part of rust syntax, so it doesn't know what
to do. I usually fix this by replacing all `<-` with `:`, format and
then change things back)

Also, is there no function in C that does all of this initialization for
you?

> +                    }
> +                    Ok(())
> +                }),
> +            data <- data,
> +        })
> +        .pin_chain(|this| {
> +            crate::error::to_result(
> +                // SAFETY: We initialized `this.subsystem` according to C API contract above.
> +                unsafe { bindings::configfs_register_subsystem(this.subsystem.get()) },
> +            )
> +        })
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<Data> PinnedDrop for Subsystem<Data> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: We registered `self.subsystem` in the initializer returned by `Self::new`.
> +        unsafe { bindings::configfs_unregister_subsystem(self.subsystem.get()) };
> +        // SAFETY: We initialized the mutex in `Subsystem::new`.
> +        unsafe { bindings::mutex_destroy(addr_of_mut!((*self.subsystem.get()).su_mutex)) };
> +    }
> +}
> +
> +/// Trait that allows offset calculations for structs that embed a
> +/// `bindings::config_group`.
> +///
> +/// Users of the `configfs` API should not need to implement this trait.
> +///
> +/// # Safety
> +///
> +/// - Implementers of this trait must embed a `bindings::config_group`.
> +/// - Methods must be implemented according to method documentation.
> +pub unsafe trait HasGroup<Data> {
> +    /// Return the address of the `bindings::config_group` embedded in `Self`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `this` must be a valid allocation of at least the size of `Self`.
> +    unsafe fn group(this: *const Self) -> *const bindings::config_group;
> +
> +    /// Return the address of the `Self` that `group` is embedded in.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `group` must point to the `bindings::config_group` that is embedded in
> +    ///   `Self`.
> +    unsafe fn container_of(group: *const bindings::config_group) -> *const Self;
> +}
> +
> +// SAFETY: `Subsystem<Data>` embeds a field of type `bindings::config_group`
> +// within the `subsystem` field.
> +unsafe impl<Data> HasGroup<Data> for Subsystem<Data> {
> +    unsafe fn group(this: *const Self) -> *const bindings::config_group {
> +        // SAFETY: By impl and function safety requirement this projection is in bounds.
> +        unsafe { addr_of!((*(*this).subsystem.get()).su_group) }
> +    }
> +
> +    unsafe fn container_of(group: *const bindings::config_group) -> *const Self {
> +        // SAFETY: By impl and function safety requirement this projection is in bounds.
> +        let c_subsys_ptr = unsafe { container_of!(group, bindings::configfs_subsystem, su_group) };
> +        let opaque_ptr = c_subsys_ptr.cast::<Opaque<bindings::configfs_subsystem>>();
> +        // SAFETY: By impl and function safety requirement, `opaque_ptr` and the
> +        // pointer it returns, are within the same allocation.
> +        unsafe { container_of!(opaque_ptr, Subsystem<Data>, subsystem) }
> +    }
> +}
> +
> +/// A `configfs` group.
> +///
> +/// To add a subgroup to `configfs`, pass this type as `ctype` to
> +/// [`crate::configfs_attrs`] when creating a group in [`GroupOperations::make_group`].
> +#[pin_data]
> +pub struct Group<Data> {

Same question about the generic name here.

> +    #[pin]
> +    group: Opaque<bindings::config_group>,
> +    #[pin]
> +    data: Data,
> +}
> +
> +impl<Data> Group<Data> {
> +    /// Create an initializer for a new group.
> +    ///
> +    /// When instantiated, the group will appear as a directory with the name
> +    /// given by `name` and it will contain attributes specified by `item_type`.
> +    pub fn new(
> +        name: CString,
> +        item_type: &'static ItemType<Group<Data>, Data>,
> +        data: impl PinInit<Data, Error>,
> +    ) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            group <- kernel::init::zeroed().chain(|v: &mut Opaque<bindings::config_group>| {
> +                let place = v.get();
> +                let name = name.as_bytes_with_nul().as_ptr();
> +                // SAFETY: It is safe to initialize a group once it has been zeroed.
> +                unsafe {
> +                    bindings::config_group_init_type_name(place, name as _, item_type.as_ptr())

Can you replace the `as _` cast with a `.cast()`?

> +                };
> +                Ok(())
> +            }),
> +            data <- data,
> +        })
> +    }
> +}
> +
> +// SAFETY: `Group<Data>` embeds a field of type `bindings::config_group`
> +// within the `group` field.
> +unsafe impl<Data> HasGroup<Data> for Group<Data> {
> +    unsafe fn group(this: *const Self) -> *const bindings::config_group {
> +        Opaque::raw_get(
> +            // SAFETY: By impl and function safety requirements this field
> +            // projection is within bounds of the allocation.
> +            unsafe { addr_of!((*this).group) },
> +        )
> +    }
> +
> +    unsafe fn container_of(group: *const bindings::config_group) -> *const Self {
> +        let opaque_ptr = group.cast::<Opaque<bindings::config_group>>();
> +        // SAFETY: By impl and function safety requirement, `opaque_ptr` and
> +        // pointer it returns will be in the same allocation.
> +        unsafe { container_of!(opaque_ptr, Self, group) }
> +    }
> +}
> +
> +/// # Safety
> +///
> +/// `this` must be a valid pointer.
> +///
> +/// If `this` does not represent the root group of a `configfs` subsystem,
> +/// `this` must be a pointer to a `bindings::config_group` embedded in a
> +/// `Group<Parent>`.
> +///
> +/// Otherwise, `this` must be a pointer to a `bindings::config_group` that
> +/// is embedded in a `bindings::configfs_subsystem` that is embedded in a
> +/// `Subsystem<Parent>`.
> +unsafe fn get_group_data<'a, Parent>(this: *mut bindings::config_group) -> &'a Parent {
> +    // TODO

Missed this todo?

> +    // SAFETY: `this` is a valid pointer.
> +    let is_root = unsafe { (*this).cg_subsys.is_null() };
> +
> +    if !is_root {
> +        // SAFETY: By C API contact, `this` is a pointer to a
> +        // `bindings::config_group` that we passed as a return value in from
> +        // `make_group`. Such a pointer is embedded within a `Group<Parent>`.
> +        unsafe { &(*Group::<Parent>::container_of(this)).data }
> +    } else {
> +        // SAFETY: By C API contract, `this` is a pointer to the
> +        // `bindings::config_group` field within a `Subsystem<Parent>`.
> +        unsafe { &(*Subsystem::container_of(this)).data }
> +    }
> +}
> +
> +struct GroupOperationsVTable<Parent, Child>(PhantomData<(Parent, Child)>)

Generic names?

> +where
> +    Parent: GroupOperations<Child = Child>;

No need to put this where bound on the struct definition (it is only
needed if the struct impls `Drop`).

> +
> +impl<Parent, Child> GroupOperationsVTable<Parent, Child>
> +where
> +    Parent: GroupOperations<Child = Child>,
> +    Child: 'static,
> +{
> +    /// # Safety
> +    ///
> +    /// `this` must be a valid pointer.
> +    ///
> +    /// If `this` does not represent the root group of a `configfs` subsystem,
> +    /// `this` must be a pointer to a `bindings::config_group` embedded in a
> +    /// `Group<Parent>`.
> +    ///
> +    /// Otherwise, `this` must be a pointer to a `bindings::config_group` that
> +    /// is embedded in a `bindings::configfs_subsystem` that is embedded in a
> +    /// `Subsystem<Parent>`.
> +    ///
> +    /// `name` must point to a null terminated string.
> +    unsafe extern "C" fn make_group(
> +        this: *mut bindings::config_group,
> +        name: *const kernel::ffi::c_char,
> +    ) -> *mut bindings::config_group {
> +        // SAFETY: By function safety requirements of this function, this call
> +        // is safe.
> +        let parent_data = unsafe { get_group_data(this) };
> +
> +        let group_init = match Parent::make_group(
> +            parent_data,
> +            // SAFETY: By function safety requirements, name points to a null
> +            // terminated string.
> +            unsafe { CStr::from_char_ptr(name) },
> +        ) {
> +            Ok(init) => init,
> +            Err(e) => return e.to_ptr(),
> +        };
> +
> +        let child_group = <Arc<Group<Child>> as InPlaceInit<Group<Child>>>::try_pin_init(
> +            group_init,
> +            flags::GFP_KERNEL,
> +        );
> +
> +        match child_group {
> +            Ok(child_group) => {
> +                let child_group_ptr = child_group.into_foreign();
> +                // SAFETY: We allocated the pointee of `child_ptr` above as a
> +                // `Group<Child>`.
> +                unsafe { Group::<Child>::group(child_group_ptr) }.cast_mut()
> +            }
> +            Err(e) => e.to_ptr(),
> +        }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// If `this` does not represent the root group of a `configfs` subsystem,
> +    /// `this` must be a pointer to a `bindings::config_group` embedded in a
> +    /// `Group<Parent>`.
> +    ///
> +    /// Otherwise, `this` must be a pointer to a `bindings::config_group` that
> +    /// is embedded in a `bindings::configfs_subsystem` that is embedded in a
> +    /// `Subsystem<Parent>`.
> +    ///
> +    /// `item` must point to a `bindings::config_item` within a
> +    /// `bindings::config_group` within a `Group<Child>`.
> +    unsafe extern "C" fn drop_item(
> +        this: *mut bindings::config_group,
> +        item: *mut bindings::config_item,
> +    ) {
> +        // SAFETY: By function safety requirements of this function, this call
> +        // is safe.
> +        let parent_data = unsafe { get_group_data(this) };
> +
> +        // SAFETY: By function safety requirements, `item` is embedded in a
> +        // `config_group`.
> +        let c_child_group_ptr =
> +            unsafe { kernel::container_of!(item, bindings::config_group, cg_item) };
> +        // SAFETY: By function safety requirements, `c_child_group_ptr` is
> +        // embedded within a `Group<CHLD>`.
> +        let r_child_group_ptr = unsafe { Group::<Child>::container_of(c_child_group_ptr) };
> +
> +        if Parent::HAS_DROP_ITEM {
> +            Parent::drop_item(
> +                parent_data,
> +                // SAFETY: We called `into_foreign` to produce `r_child_group_ptr` in
> +                // `make_group`. There are not other borrows of this pointer in existence.
> +                unsafe {
> +                    <Arc<Group<Child>> as ForeignOwnable>::borrow(r_child_group_ptr.cast_mut())
> +                },
> +            );
> +        }
> +
> +        // SAFETY: By C API contract, we are required to drop a refcount on
> +        // `item`.
> +        unsafe { bindings::config_item_put(item) };
> +    }
> +
> +    const VTABLE: bindings::configfs_group_operations = bindings::configfs_group_operations {
> +        make_item: None,
> +        make_group: Some(Self::make_group),
> +        disconnect_notify: None,
> +        drop_item: Some(Self::drop_item),
> +        is_visible: None,
> +        is_bin_visible: None,
> +    };
> +}
> +
> +struct ItemOperationsVTable<Container, Data>(PhantomData<(Container, Data)>);
> +
> +impl<Data> ItemOperationsVTable<Group<Data>, Data>
> +where
> +    Data: 'static,
> +{
> +    /// # Safety
> +    ///
> +    /// `this` must be a pointer to a `bindings::config_group` embedded in a
> +    /// `Group<Parent>`.
> +    ///
> +    /// This function will destroy the pointee of `this`. The pointee of `this`
> +    /// must not be accessed after the function returns.
> +    unsafe extern "C" fn release(this: *mut bindings::config_item) {
> +        // SAFETY: By function safety requirements, `this` is embedded in a
> +        // `config_group`.
> +        let c_group_ptr = unsafe { kernel::container_of!(this, bindings::config_group, cg_item) };
> +        // SAFETY: By function safety requirements, `c_group_ptr` is
> +        // embedded within a `Group<Data>`.
> +        let r_group_ptr = unsafe { Group::<Data>::container_of(c_group_ptr) };
> +
> +        // SAFETY: We called `into_foreign` on `r_group_ptr` in
> +        // `make_group`.
> +        let pin_self =
> +            unsafe { <Arc<Group<Data>> as ForeignOwnable>::from_foreign(r_group_ptr.cast_mut()) };
> +        drop(pin_self);
> +    }
> +
> +    const VTABLE: bindings::configfs_item_operations = bindings::configfs_item_operations {
> +        release: Some(Self::release),
> +        allow_link: None,
> +        drop_link: None,
> +    };
> +}
> +
> +impl<Data> ItemOperationsVTable<Subsystem<Data>, Data> {
> +    const VTABLE: bindings::configfs_item_operations = bindings::configfs_item_operations {
> +        release: None,
> +        allow_link: None,
> +        drop_link: None,
> +    };
> +}
> +
> +/// Operations implemented by `configfs` groups that can create subgroups.
> +///
> +/// Implement this trait on structs that embed a [`Subsystem`] or a [`Group`].
> +#[vtable]
> +pub trait GroupOperations {
> +    /// The parent data object type.
> +    ///
> +    /// The implementer of this trait is this kind of data object. Should be set
> +    /// to `Self`.
> +    type Parent;

If it should be set to `Self`, why does this even exist? If there are
cases where it isn't supposed to be `Self`, it would be good to put them
into the docs.

> +
> +    /// The child data object type.
> +    ///
> +    /// This group will create subgroups (subdirectories) backed by this kind of
> +    /// object.
> +    type Child: 'static;
> +
> +    /// The kernel will call this method in response to `mkdir(2)` in the
> +    /// directory representing `this`.

This doesn't really read like a first line description of this function,
how about putting "Creates a new subgroup." as the first line?

> +    ///
> +    /// To accept the request to create a group, implementations should
> +    /// instantiate a `CHLD` and return a `CPTR` to it. To prevent creation,

Is there a typo in `CHLD`? What do you mean by "return a `CPTR` to it"?

> +    /// return a suitable error.
> +    fn make_group(
> +        this: &Self::Parent,
> +        name: &CStr,
> +    ) -> Result<impl PinInit<Group<Self::Child>, Error>>;
> +
> +    /// The kernel will call this method before the directory representing
> +    /// `_child` is removed from `configfs`.

Same thing about the one-line description, how about (with the name
changed below): "Prepares the given group for removal from configfs.".

> +    ///
> +    /// Implementations can use this method to do house keeping before
> +    /// `configfs` drops its reference to `Child`.
> +    fn drop_item(

`drop` doesn't really fit here, I think something like `unlink_item`
fits better, since the child isn't actually dropped after this function
returns.

> +        _this: &Self::Parent,
> +        _child: <Arc<Group<Self::Child>> as ForeignOwnable>::Borrowed<'_>,

Just write ArcBorrow<'_, Group<Self::Child>> instead of the above?

> +    ) {
> +        kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
> +    }
> +}
> +
> +/// A `configfs` attribute.
> +///
> +/// An attribute appear as a file in configfs, inside a folder that represent

Typo: appear -> appears

> +/// the group that the attribute belongs to.
> +#[repr(transparent)]
> +pub struct Attribute<const ID: u64, O, Data> {
> +    attribute: Opaque<bindings::configfs_attribute>,
> +    _p: PhantomData<(O, Data)>,
> +}
> +
> +// SAFETY: We do not provide any operations on `Attribute`.
> +unsafe impl<const ID: u64, O, Data> Sync for Attribute<ID, O, Data> {}
> +
> +// SAFETY: Ownership of `Attribute` can safely be transferred to other threads.
> +unsafe impl<const ID: u64, O, Data> Send for Attribute<ID, O, Data> {}
> +
> +impl<const ID: u64, O, Data> Attribute<ID, O, Data>
> +where
> +    O: AttributeOperations<ID, Data = Data>,
> +{
> +    /// # Safety
> +    ///
> +    /// `item` must be embedded in a `bindings::config_group`.
> +    ///
> +    /// If `item` does not represent the root group of a `configfs` subsystem,
> +    /// the group must be embedded in a `Group<Data>`.
> +    ///
> +    /// Otherwise, the group must be a embedded in a
> +    /// `bindings::configfs_subsystem` that is embedded in a `Subsystem<Data>`.
> +    ///
> +    /// `page` must point to a writable buffer of size at least [`PAGE_SIZE`].
> +    unsafe extern "C" fn show(
> +        item: *mut bindings::config_item,
> +        page: *mut kernel::ffi::c_char,
> +    ) -> isize {
> +        let c_group: *mut bindings::config_group =
> +        // SAFETY: By function safety requirements, `item` is embedded in a
> +        // `config_group`.
> +            unsafe { container_of!(item, bindings::config_group, cg_item) }.cast_mut();
> +
> +        // SAFETY: The function safety requirements for this function satisfy
> +        // the conditions for this call.
> +        let data: &Data = unsafe { get_group_data(c_group) };
> +
> +        // SAFETY: By function safety requirements, `page` is writable for `PAGE_SIZE`.
> +        let ret = O::show(data, unsafe { &mut *(page as *mut [u8; PAGE_SIZE]) });
> +
> +        match ret {
> +            Ok(size) => size as isize,
> +            Err(err) => err.to_errno() as isize,
> +        }
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// `item` must be embedded in a `bindings::config_group`.
> +    ///
> +    /// If `item` does not represent the root group of a `configfs` subsystem,
> +    /// the group must be embedded in a `Group<Data>`.
> +    ///
> +    /// Otherwise, the group must be a embedded in a
> +    /// `bindings::configfs_subsystem` that is embedded in a `Subsystem<Data>`.
> +    ///
> +    /// `page` must point to a readable buffer of size at least `size`.
> +    unsafe extern "C" fn store(
> +        item: *mut bindings::config_item,
> +        page: *const kernel::ffi::c_char,
> +        size: usize,
> +    ) -> isize {
> +        let c_group: *mut bindings::config_group =
> +        // SAFETY: By function safety requirements, `item` is embedded in a
> +        // `config_group`.
> +            unsafe { container_of!(item, bindings::config_group, cg_item) }.cast_mut();
> +
> +        // SAFETY: The function safety requirements for this function satisfy
> +        // the conditions for this call.
> +        let data: &Data = unsafe { get_group_data(c_group) };
> +
> +        let ret = O::store(
> +            data,
> +            // SAFETY: By function safety requirements, `page` is readable
> +            // for at least `size`.
> +            unsafe { core::slice::from_raw_parts(page.cast(), size) },
> +        );
> +
> +        match ret {
> +            Ok(()) => size as isize,
> +            Err(err) => err.to_errno() as isize,
> +        }
> +    }
> +
> +    /// Create a new attribute.
> +    ///
> +    /// The attribute will appear as a file with name given by `name`.
> +    pub const fn new(name: &'static CStr) -> Self {
> +        Self {
> +            attribute: Opaque::new(bindings::configfs_attribute {
> +                ca_name: name as *const _ as _,
> +                ca_owner: core::ptr::null_mut(),
> +                ca_mode: 0o660,
> +                show: Some(Self::show),
> +                store: if O::HAS_STORE {
> +                    Some(Self::store)
> +                } else {
> +                    None
> +                },
> +            }),
> +            _p: PhantomData,
> +        }
> +    }
> +}
> +
> +/// Operations supported by an attribute.
> +///
> +/// Implement this trait on type and pass that type as generic parameter when
> +/// creating an [`Attribute`]. The type carrying the implementation serve no
> +/// purpose other than specifying the attribute operations.
> +#[vtable]
> +pub trait AttributeOperations<const ID: u64 = 0> {
> +    /// The type of the object that contains the field that is backing the
> +    /// attribute for this operation.
> +    type Data;
> +
> +    /// This function is called by the kernel to read the value of an attribute.

"Reads the value of an attribute.".

> +    ///
> +    /// Implementations should write the rendering of the attribute to `page`
> +    /// and return the number of bytes written.
> +    fn show(data: &Self::Data, page: &mut [u8; PAGE_SIZE]) -> Result<usize>;

Why is this not named `read` or `load`? If the C equivalent is `show`,
then it's fine, but otherwise I wouldn't understand why it's show/store
as opposed to load/store or read/write.

> +
> +    /// This function is called by the kernel to update the value of an attribute.

Again first line doc here.

> +    ///
> +    /// Implementations should parse the value from `page` and update internal
> +    /// state to reflect the parsed value. Partial writes are not supported and
> +    /// implementations should expect the full page to arrive in one write
> +    /// operation.

I don't understand what you're trying to say with the last sentence.

> +    fn store(_data: &Self::Data, _page: &[u8]) -> Result {
> +        kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
> +    }
> +}
> +
> +/// A list of attributes.
> +///
> +/// This type is used to construct a new [`ItemType`]. It represents a list of
> +/// [`Attribute`] that will appear in the directory representing a [`Group`].
> +/// Users should not directly instantiate this type, rather they should use the
> +/// [`kernel::configfs_attrs`] macro to declare a static set of attributes for a
> +/// group.
> +#[repr(transparent)]
> +pub struct AttributeList<const N: usize, Data>(
> +    UnsafeCell<[*mut kernel::ffi::c_void; N]>,
> +    PhantomData<Data>,
> +);
> +
> +// SAFETY: Ownership of `AttributeList` can safely be transferred to other threads.
> +unsafe impl<const N: usize, Data> Send for AttributeList<N, Data> {}
> +
> +// SAFETY: We do not provide any operations on `AttributeList` that need synchronization.
> +unsafe impl<const N: usize, Data> Sync for AttributeList<N, Data> {}
> +
> +impl<const N: usize, Data> AttributeList<N, Data> {
> +    #[doc(hidden)]

I normally put attributes after the documentation.

> +    /// # Safety
> +    ///
> +    /// This function can only be called by expanding the `configfs_attrs`

s/expanding//

> +    /// macro.
> +    pub const unsafe fn new() -> Self {
> +        Self(UnsafeCell::new([core::ptr::null_mut(); N]), PhantomData)
> +    }
> +
> +    #[doc(hidden)]
> +    /// # Safety
> +    ///
> +    /// This function can only be called by expanding the `configfs_attrs`

s/expanding//

> +    /// macro.
> +    pub const unsafe fn add<
> +        const I: usize,
> +        const ID: u64,
> +        O: AttributeOperations<ID, Data = Data>,
> +    >(
> +        &'static self,
> +        attribute: &'static Attribute<ID, O, Data>,
> +    ) {
> +        if I >= N - 1 {
> +            kernel::build_error!("Invalid attribute index");
> +        }
> +
> +        // SAFETY: This function is only called through `configfs_attrs`. This

s/through `configfs_attrs`/through the `configfs_attrs` macro/

> +        // ensures that we are evaluating the function in const context when
> +        // initializing a static. As such, the reference created below will be
> +        // exclusive.
> +        unsafe {
> +            (&mut *self.0.get())[I] = (attribute as *const Attribute<ID, O, Data>)
> +                .cast_mut()
> +                .cast()
> +        };
> +    }
> +}
> +
> +/// A representation of the attributes that will appear in a [`Group`] or
> +/// [`Subsystem`].
> +///
> +/// Users should not directly instantiate objects of this type. Rather, they
> +/// should use the [`kernel::configfs_attrs`] macro to statically declare the
> +/// shape of a [`Group`] or [`Subsystem`].
> +#[pin_data]
> +pub struct ItemType<Container, Data> {
> +    #[pin]
> +    item_type: Opaque<bindings::config_item_type>,
> +    _p: PhantomData<(Container, Data)>,
> +}
> +
> +// SAFETY: We do not provide any operations on `ItemType` that need synchronization.
> +unsafe impl<Container, Data> Sync for ItemType<Container, Data> {}
> +
> +// SAFETY: Ownership of `ItemType` can safely be transferred to other threads.
> +unsafe impl<Container, Data> Send for ItemType<Container, Data> {}
> +
> +macro_rules! impl_item_type {
> +    ($tpe:ty) => {
> +        impl<Data> ItemType<$tpe, Data> {
> +            #[doc(hidden)]
> +            pub const fn new_with_child_ctor<const N: usize, Child>(
> +                owner: &'static ThisModule,
> +                attributes: &'static AttributeList<N, Data>,
> +            ) -> Self
> +            where
> +                Data: GroupOperations<Child = Child>,
> +                Child: 'static,
> +            {
> +                Self {
> +                    item_type: Opaque::new(bindings::config_item_type {
> +                        ct_owner: owner.as_ptr(),
> +                        ct_group_ops: (&GroupOperationsVTable::<Data, Child>::VTABLE as *const _)
> +                            as *mut _,
> +                        ct_item_ops: (&ItemOperationsVTable::<$tpe, Data>::VTABLE as *const _)
> +                            as *mut _,
> +                        ct_attrs: attributes as *const _ as _,
> +                        ct_bin_attrs: core::ptr::null_mut(),
> +                    }),
> +                    _p: PhantomData,
> +                }
> +            }
> +
> +            #[doc(hidden)]
> +            pub const fn new<const N: usize>(
> +                owner: &'static ThisModule,
> +                attributes: &'static AttributeList<N, Data>,
> +            ) -> Self {
> +                Self {
> +                    item_type: Opaque::new(bindings::config_item_type {
> +                        ct_owner: owner.as_ptr(),
> +                        ct_group_ops: core::ptr::null_mut(),
> +                        ct_item_ops: (&ItemOperationsVTable::<$tpe, Data>::VTABLE as *const _)
> +                            as *mut _,
> +                        ct_attrs: attributes as *const _ as _,
> +                        ct_bin_attrs: core::ptr::null_mut(),
> +                    }),
> +                    _p: PhantomData,
> +                }
> +            }
> +        }
> +    };
> +}
> +
> +impl_item_type!(Subsystem<Data>);
> +impl_item_type!(Group<Data>);
> +
> +impl<Container, Data> ItemType<Container, Data> {
> +    fn as_ptr(&self) -> *const bindings::config_item_type {
> +        self.item_type.get()
> +    }
> +}
> +
> +/// Define a list of configfs attributes statically.
> +#[macro_export]
> +macro_rules! configfs_attrs {

I see you've joined the dark side of declarative macros!

This seems like a prime candidate for replacing with a proc-macro when
we have syn :)

> +    (
> +        container: $container:ty,
> +        data: $data:ty,
> +        attributes: [
> +            $($name:ident: $attr:literal,)*

This syntax always requires a trailing comma. Most (IIRC all, but not
100% sure) Rust syntax allows you to omit it, so it would be odd if it
were not the case here. You can have an optional trailing comma via:

    $($name:ident: $attr:literal),* $(,)?

But as soon as you give the tokens off to the internals of the macro, I
would recommend sticking to always having a trailing comma or no
trailing comma.

> +        ],
> +    ) => {
> +        $crate::configfs_attrs!(
> +            count:
> +            @container($container),
> +            @data($data),
> +            @child(),
> +            @no_child(x),
> +            @attrs($($name $attr)*),
> +            @eat($($name $attr,)*),
> +            @assign(),
> +            @cnt(0usize),
> +        )
> +    };
> +    (
> +        container: $container:ty,
> +        data: $data:ty,
> +        child: $child:ty,
> +        attributes: [
> +            $($name:ident: $attr:literal,)*

Ditto.

> +        ],
> +    ) => {
> +        $crate::configfs_attrs!(
> +            count:
> +            @container($container),
> +            @data($data),
> +            @child($child),
> +            @no_child(),
> +            @attrs($($name $attr)*),
> +            @eat($($name $attr,)*),
> +            @assign(),
> +            @cnt(0usize),
> +        )
> +    };
> +    (count:
> +     @container($container:ty),
> +     @data($data:ty),
> +     @child($($child:ty)?),
> +     @no_child($($no_child:ident)?),
> +     @attrs($($aname:ident $aattr:literal)*),
> +     @eat($name:ident $attr:literal, $($rname:ident $rattr:literal,)*),
> +     @assign($($assign:block)*),
> +     @cnt($cnt:expr),
> +    ) => {
> +        $crate::configfs_attrs!(
> +            count:
> +            @container($container),
> +            @data($data),
> +            @child($($child)?),
> +            @no_child($($no_child)?),
> +            @attrs($($aname $aattr)*),
> +            @eat($($rname $rattr,)*),
> +            @assign($($assign)* {
> +                const N: usize = $cnt;
> +                // SAFETY: We are expanding `configfs_attrs`.

This safety comment is doing a lot of heavy lifting, since it is not at
all obvious what the below unsafe function will resolve to... Seems also
a hassle to put a full comment here explaining that
`[< $data:upper _ATTRS >]` is defined by the macro below and that it is
of type `AttributeList` etc... But maybe we should.

> +                unsafe {
> +                    $crate::macros::paste!( [< $data:upper _ATTRS >])
> +                        .add::<N, $attr, _>(
> +                            & $crate::macros::paste!( [< $data:upper _ $name:upper _ATTR >])
> +                        )
> +                };
> +            }),

You can merge the two `paste!` invocations into one:

    @assign($($assign)* {
        const N: usize = $cnt;
        $crate::macros::paste! {
            // SAFETY: see above comment
            unsafe {
                [< $data:upper _ATTRS >].add::<N, $attr, _>(
                    &[< $data:upper _ $name:upper _ATTR >]
                );
            }
        }
    }),

> +            @cnt(1usize + $cnt),
> +        )
> +    };
> +    (count:
> +     @container($container:ty),
> +     @data($data:ty),
> +     @child($($child:ty)?),
> +     @no_child($($no_child:ident)?),
> +     @attrs($($aname:ident $aattr:literal)*),
> +     @eat(),
> +     @assign($($assign:block)*),
> +     @cnt($cnt:expr),
> +    ) =>
> +    {
> +        $crate::configfs_attrs!(
> +            final:
> +            @container($container),
> +            @data($data),
> +            @child($($child)?),
> +            @no_child($($no_child)?),
> +            @attrs($($aname $aattr)*),
> +            @assign($($assign)*),
> +            @cnt($cnt),
> +        )
> +    };
> +    (final:
> +     @container($container:ty),
> +     @data($data:ty),
> +     @child($($child:ty)?),
> +     @no_child($($no_child:ident)?),
> +     @attrs($($name:ident $attr:literal)*),
> +     @assign($($assign:block)*),
> +     @cnt($cnt:expr),
> +    ) =>
> +    {
> +        {
> +            $(
> +                $crate::macros::paste!{

Again you can coalesce all of the `paste!` invocations into a single one
spanning the entire output of this macro branch.

> +                    // SAFETY: We are expanding `configfs_attrs`.
> +                    static [< $data:upper _ $name:upper _ATTR >]:
> +                      $crate::configfs::Attribute<$attr, $data, $data> =
> +                        unsafe {
> +                            $crate::configfs::Attribute::new(c_str!(::core::stringify!($name)))
> +                        };
> +                }
> +            )*
> +
> +
> +            const N: usize = $cnt + 1usize;

Why do we need an additional copy? To have a zero entry at the end for C
to know it's the end of the list? If so, a comment here would be very
helpful.

> +            $crate::macros::paste!{
> +                // SAFETY: We are expanding `configfs_attrs`.
> +                static [< $data:upper _ATTRS >]:
> +                  $crate::configfs::AttributeList<N, $data> =
> +                    unsafe { $crate::configfs::AttributeList::new() };
> +            }
> +
> +            $($assign)*
> +
> +            $(
> +                $crate::macros::paste!{
> +                    const [<$no_child:upper>]: bool = true;
> +                };
> +
> +                $crate::macros::paste!{
> +                    static [< $data:upper _TPE >] : $crate::configfs::ItemType<$container, $data>  =
> +                        $crate::configfs::ItemType::<$container, $data>::new::<N>(
> +                            &THIS_MODULE, &[<$ data:upper _ATTRS >]
> +                        );
> +                }
> +            )?
> +
> +            $(
> +                $crate::macros::paste!{
> +                    static [< $data:upper _TPE >]:
> +                      $crate::configfs::ItemType<$container, $data>  =
> +                        $crate::configfs::ItemType::<$container, $data>::new_with_child_ctor::
> +                    <N, $child>(
> +                        &THIS_MODULE, &[<$ data:upper _ATTRS >]
> +                    );
> +                }
> +            )?
> +
> +            &$crate::macros::paste!( [< $data:upper _TPE >] )
> +        }
> +    };
> +
> +}

I tested if multiple invocations of this macro can shadow each other and
the answer is no. So wrapping a const with `{}` makes it inaccessible to
the outside which is exactly what we need here.
The macro looks quite good!

> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911a..ec84653ab8c7a 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -40,6 +40,8 @@
>  pub mod block;
>  #[doc(hidden)]
>  pub mod build_assert;
> +#[cfg(CONFIG_CONFIGFS_FS)]
> +pub mod configfs;
>  pub mod cred;
>  pub mod device;
>  pub mod device_id;
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index 918dbead2c0b4..2f97bf9a7b4cc 100644
> --- a/samples/rust/Kconfig
> +++ b/samples/rust/Kconfig
> @@ -10,6 +10,17 @@ menuconfig SAMPLES_RUST
> 
>  if SAMPLES_RUST
> 
> +config SAMPLE_RUST_CONFIGFS
> +	tristate "Configfs sample"
> +	depends on CONFIGFS_FS
> +	help
> +	  This option builds the Rust configfs sample.
> +
> +	  To compile this as a module, choose M here:
> +	  the module will be called rust_configfs.
> +
> +	  If unsure, say N.
> +
>  config SAMPLE_RUST_MINIMAL
>  	tristate "Minimal"
>  	help
> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
> index 5a8ab0df0567c..72122f010cafc 100644
> --- a/samples/rust/Makefile
> +++ b/samples/rust/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
>  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
> +obj-$(CONFIG_SAMPLE_RUST_CONFIGFS)		+= rust_configfs.o
> 
>  rust_print-y := rust_print_main.o rust_print_events.o
> 
> diff --git a/samples/rust/rust_configfs.rs b/samples/rust/rust_configfs.rs
> new file mode 100644
> index 0000000000000..fe896e66efb41
> --- /dev/null
> +++ b/samples/rust/rust_configfs.rs
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Rust configfs sample.
> +
> +use kernel::alloc::flags;
> +use kernel::c_str;
> +use kernel::configfs;
> +use kernel::configfs_attrs;
> +use kernel::new_mutex;
> +use kernel::prelude::*;
> +use kernel::sync::Mutex;

Would merge the imports here too (rust-analyzer has a code-action for
that btw).

> +
> +module! {
> +    type: RustConfigfs,
> +    name: "rust_configfs",
> +    author: "Rust for Linux Contributors",
> +    description: "Rust configfs sample",
> +    license: "GPL",
> +}
> +
> +#[pin_data]
> +struct RustConfigfs {
> +    #[pin]
> +    config: configfs::Subsystem<Configuration>,
> +}
> +
> +#[pin_data]
> +struct Configuration {
> +    message: &'static CStr,
> +    #[pin]
> +    bar: Mutex<(KBox<[u8; 4096]>, usize)>,
> +}
> +
> +impl Configuration {
> +    fn new() -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            message: c_str!("Hello World\n"),
> +            bar <- new_mutex!((KBox::new([0;4096], flags::GFP_KERNEL)?,0)),

s/;/; /
s/,0/, 0/

---
Cheers,
Benno

> +        })
> +    }
> +}
> +
> +impl kernel::InPlaceModule for RustConfigfs {
> +    fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
> +        pr_info!("Rust configfs sample (init)\n");
> +
> +        let item_type = configfs_attrs! {
> +            container: configfs::Subsystem<Configuration>,
> +            data: Configuration,
> +            child: Child,
> +            attributes: [
> +                message: 0,
> +                bar: 1,
> +            ],
> +        };
> +
> +        try_pin_init!(Self {
> +            config <- configfs::Subsystem::new(
> +                kernel::c_str!("rust_configfs"), item_type, Configuration::new()
> +            ),
> +        })
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::GroupOperations for Configuration {
> +    type Parent = Self;
> +    type Child = Child;
> +
> +    fn make_group(
> +        _this: &Self,
> +        name: &CStr,
> +    ) -> Result<impl PinInit<configfs::Group<Child>, Error>> {
> +        let tpe = configfs_attrs! {
> +            container: configfs::Group<Child>,
> +            data: Child,
> +            child: GrandChild,
> +            attributes: [
> +                baz: 0,
> +            ],
> +        };
> +
> +        Ok(configfs::Group::new(name.try_into()?, tpe, Child::new()))
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<0> for Configuration {
> +    type Data = Configuration;
> +
> +    fn show(container: &Configuration, page: &mut [u8; 4096]) -> Result<usize> {
> +        pr_info!("Show message\n");
> +        let data = container.message;
> +        page[0..data.len()].copy_from_slice(data);
> +        Ok(data.len())
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<1> for Configuration {
> +    type Data = Configuration;
> +
> +    fn show(container: &Configuration, page: &mut [u8; 4096]) -> Result<usize> {
> +        pr_info!("Show bar\n");
> +        let guard = container.bar.lock();
> +        let data = guard.0.as_slice();
> +        let len = guard.1;
> +        page[0..len].copy_from_slice(&data[0..len]);
> +        Ok(len)
> +    }
> +
> +    fn store(container: &Configuration, page: &[u8]) -> Result {
> +        pr_info!("Store bar\n");
> +        let mut guard = container.bar.lock();
> +        guard.0[0..page.len()].copy_from_slice(page);
> +        guard.1 = page.len();
> +        Ok(())
> +    }
> +}
> +
> +#[pin_data]
> +struct Child {}
> +
> +impl Child {
> +    fn new() -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {})
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::GroupOperations for Child {
> +    type Parent = Self;
> +    type Child = GrandChild;
> +
> +    fn make_group(
> +        _this: &Self,
> +        name: &CStr,
> +    ) -> Result<impl PinInit<configfs::Group<GrandChild>, Error>> {
> +        let tpe = configfs_attrs! {
> +            container: configfs::Group<GrandChild>,
> +            data: GrandChild,
> +            attributes: [
> +                gc: 0,
> +            ],
> +        };
> +
> +        Ok(configfs::Group::new(
> +            name.try_into()?,
> +            tpe,
> +            GrandChild::new(),
> +        ))
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<0> for Child {
> +    type Data = Child;
> +
> +    fn show(_container: &Child, page: &mut [u8; 4096]) -> Result<usize> {
> +        pr_info!("Show baz\n");
> +        let data = c"Hello Baz\n".to_bytes();
> +        page[0..data.len()].copy_from_slice(data);
> +        Ok(data.len())
> +    }
> +}
> +
> +#[pin_data]
> +struct GrandChild {}
> +
> +impl GrandChild {
> +    fn new() -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {})
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::AttributeOperations<0> for GrandChild {
> +    type Data = GrandChild;
> +
> +    fn show(_container: &GrandChild, page: &mut [u8; 4096]) -> Result<usize> {
> +        pr_info!("Show baz\n");
> +        let data = c"Hello GC\n".to_bytes();
> +        page[0..data.len()].copy_from_slice(data);
> +        Ok(data.len())
> +    }
> +}
> 
> --
> 2.47.0
> 
> 

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 07.02.25 15:41, Andreas Hindborg wrote:
>> This patch adds a rust API for configfs, thus allowing rust modules to use
>> configfs for configuration. The implementation is a shim on top of the C
>> configfs implementation allowing safe use of the C infrastructure from
>> rust.
>>
>> The patch enables the `const_mut_refs` feature on compilers before rustc
>> 1.83. The feature was stabilized in rustc 1.83 and is not required to be
>> explicitly enabled on later versions.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>
>> ---
>>
>> This patch is a direct dependency for `rnull`, the rust null block driver.
>> ---
>>  rust/bindings/bindings_helper.h |   1 +
>>  rust/helpers/mutex.c            |   5 +
>>  rust/kernel/configfs.rs         | 860 ++++++++++++++++++++++++++++++++++++++++
>>  rust/kernel/lib.rs              |   2 +
>>  samples/rust/Kconfig            |  11 +
>>  samples/rust/Makefile           |   1 +
>>  samples/rust/rust_configfs.rs   | 186 +++++++++
>
> Can you move the sample into its own patch?

Yes.

[...]

>> diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
>> new file mode 100644
>> index 0000000000000..9d4b381b9df89
>> --- /dev/null
>> +++ b/rust/kernel/configfs.rs
>> @@ -0,0 +1,860 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! `configfs` interface.
>> +//!
>> +//! `configfs` is an in-memory pseudo file system for configuration of kernel
>> +//! modules. Please see the [C documentation] for details and intended use of
>> +//! `configfs`.
>> +//!
>> +//! This module does not support the following `configfs` features:
>> +//!
>> +//! - Items. All group children are groups.
>> +//! - Symlink support.
>> +//! - `disconnect_notify` hook.
>> +//! - Default groups.
>
> For lists like this, I usually end items except the last one with a
> comma instead of a period.

If that is the right way to do it, sure. It is actually funny that you
notice, because I searched for input on how to typeset such a list, and
some Microsoft typesetting site told me to do it like this when some
items are sentences [1]. I am not a native English speaker, and I have
no idea what the correct formatting is. Commas are not mentioned at the
resource I found.

[1] https://www.microsoft.com/en-us/microsoft-365-life-hacks/writing/punctuating-bullet-point-lists

>
>> +//!
>> +//! See the [rust_configfs.rs] sample for a full example use of this module.
>
> It could also be useful to just put the example directly here into the
> docs instead of/additionally to having it as a sample.

I don't think we should duplicate the code. As long as the link works, I
think having it separately is fine.

>
>> +//!
>> +//! C header: [`include/linux/configfs.h`](srctree/include/linux/configfs.h)
>> +//!
>> +//! [C documentation]: srctree/Documentation/filesystems/configfs.rst
>> +//! [rust_configfs.rs]: srctree/samples/rust/rust_configfs.rs
>> +
>> +use crate::alloc::flags;
>> +use crate::container_of;
>> +use crate::page::PAGE_SIZE;
>> +use crate::prelude::*;
>> +use crate::str::CString;
>> +use crate::sync::Arc;
>> +use crate::types::ForeignOwnable;
>> +use crate::types::Opaque;
>> +use core::cell::UnsafeCell;
>> +use core::marker::PhantomData;
>> +use core::ptr::addr_of;
>> +use core::ptr::addr_of_mut;
>
> I usually would import this like so:
>
>     use crate::{
>         alloc::flags,
>         container_of,
>         page::PAGE_SIZE,
>         prelude::*,
>         str::CString,
>         sync::Arc,
>         types::{ForeignOwnable, Opaque},
>     };
>     use core::{
>         cell::UnsafeCell,
>         marker::PhantomData,
>         ptr::{addr_of, addr_of_mut},
>     };
>
> To me this is more readable.

I disagree with that. I don't think what you suggest is easier to read,
and it is much more difficult to work with when rebasing and merging
things. This was discussed elsewhere in the past without reaching a
conclusion. I think we should come to a consensus on what style we
should adopt for the imports.

>
>> +
>> +/// A `configfs` subsystem.
>> +///
>> +/// This is the top level entrypoint for a `configfs` hierarchy. To register
>> +/// with configfs, embed a field of this type into your kernel module struct.
>> +#[pin_data(PinnedDrop)]
>> +pub struct Subsystem<Data> {
>
> Usually, we don't have multi-character generics, any specific reason
> that you chose `Data` here over `T` or `D`?

Yes, I find it more descriptive. The patch set went through quite a bit
of evolution, and the generics got a bit complicated in earlier
iterations, which necessitated more descriptive generic type parameter
names. It's not so bad in this version after I restricted the pointer
type to just `Arc`, but I still think that using a word rather a single
letter makes the code easier to comprehend at first pass.

Also, using a word is allowed as per the API guideline document [2]:

      > concise UpperCamelCase, usually single uppercase letter: T

https://rust-lang.github.io/api-guidelines/naming.html

>
>> +    #[pin]
>> +    subsystem: Opaque<bindings::configfs_subsystem>,
>> +    #[pin]
>> +    data: Data,
>> +}
>> +
>> +// SAFETY: We do not provide any operations on `Subsystem`.
>> +unsafe impl<Data> Sync for Subsystem<Data> {}
>> +
>> +// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
>> +unsafe impl<Data> Send for Subsystem<Data> {}
>> +
>> +impl<Data> Subsystem<Data> {
>> +    /// Create an initializer for a [`Subsystem`].
>> +    ///
>> +    /// The subsystem will appear in configfs as a directory name given by
>> +    /// `name`. The attributes available in directory are specified by
>> +    /// `item_type`.
>> +    pub fn new(
>> +        name: &'static CStr,
>> +        item_type: &'static ItemType<Subsystem<Data>, Data>,
>> +        data: impl PinInit<Data, Error>,
>> +    ) -> impl PinInit<Self, Error> {
>> +        try_pin_init!(Self {
>> +            subsystem <- kernel::init::zeroed().chain(
>> +                |place: &mut Opaque<bindings::configfs_subsystem>| {
>> +                    // SAFETY: All of `place` is valid for write.
>> +                    unsafe {
>> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_name )
>> +                            .write(name.as_ptr().cast_mut().cast())
>> +                    };
>> +                    // SAFETY: All of `place` is valid for write.
>> +                    unsafe {
>> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_type)
>> +                            .write(item_type.as_ptr())
>> +                    };
>> +                    // SAFETY: We initialized the required fields of `place.group` above.
>> +                    unsafe { bindings::config_group_init(&mut (*place.get()).su_group) };
>> +                    // SAFETY: `place.su_mutex` is valid for use as a mutex.
>> +                    unsafe { bindings::__mutex_init(
>> +                        &mut (*place.get()).su_mutex,
>> +                        kernel::optional_name!().as_char_ptr(),
>> +                        kernel::static_lock_class!().as_ptr())
>
> Formatting for this code is weird.
>
> (since this is inside of the `try_pin_init!` macro, rustfmt doesn't
> format it, since `<-` isn't part of rust syntax, so it doesn't know what
> to do. I usually fix this by replacing all `<-` with `:`, format and
> then change things back)

Such is the perils of macros. I'll try to go over it again. Perhaps we
could make `rustfmt` understand `<-`?

>
> Also, is there no function in C that does all of this initialization for
> you?

I might be able to do a little better. There is a C function that takes
care of initialization of `ci_name` and `ci_type` as well. I can't
recall if there was a particular reason for not using it, but I'll
check.

We have to initialize the mutex explicitly. I think the reason for not
doing that implicitly C side is to allow it to be statically
initialized.

[...]

>> +
>> +impl<Data> Group<Data> {
>> +    /// Create an initializer for a new group.
>> +    ///
>> +    /// When instantiated, the group will appear as a directory with the name
>> +    /// given by `name` and it will contain attributes specified by `item_type`.
>> +    pub fn new(
>> +        name: CString,
>> +        item_type: &'static ItemType<Group<Data>, Data>,
>> +        data: impl PinInit<Data, Error>,
>> +    ) -> impl PinInit<Self, Error> {
>> +        try_pin_init!(Self {
>> +            group <- kernel::init::zeroed().chain(|v: &mut Opaque<bindings::config_group>| {
>> +                let place = v.get();
>> +                let name = name.as_bytes_with_nul().as_ptr();
>> +                // SAFETY: It is safe to initialize a group once it has been zeroed.
>> +                unsafe {
>> +                    bindings::config_group_init_type_name(place, name as _, item_type.as_ptr())
>
> Can you replace the `as _` cast with a `.cast()`?

👍

[...]

>> +unsafe fn get_group_data<'a, Parent>(this: *mut bindings::config_group) -> &'a Parent {
>> +    // TODO
>
> Missed this todo?

Thanks. It was referring to a missing safety comment that was later put
in place.

>
>> +    // SAFETY: `this` is a valid pointer.
>> +    let is_root = unsafe { (*this).cg_subsys.is_null() };
>> +
>> +    if !is_root {
>> +        // SAFETY: By C API contact, `this` is a pointer to a
>> +        // `bindings::config_group` that we passed as a return value in from
>> +        // `make_group`. Such a pointer is embedded within a `Group<Parent>`.
>> +        unsafe { &(*Group::<Parent>::container_of(this)).data }
>> +    } else {
>> +        // SAFETY: By C API contract, `this` is a pointer to the
>> +        // `bindings::config_group` field within a `Subsystem<Parent>`.
>> +        unsafe { &(*Subsystem::container_of(this)).data }
>> +    }
>> +}
>> +
>> +struct GroupOperationsVTable<Parent, Child>(PhantomData<(Parent, Child)>)
>
> Generic names?

I prefer descriptive names. I don't really see the point of replacing
with `P,C`. It would not be better.

>
>> +where
>> +    Parent: GroupOperations<Child = Child>;
>
> No need to put this where bound on the struct definition (it is only
> needed if the struct impls `Drop`).

Right.

[...]

>> +
>> +/// Operations implemented by `configfs` groups that can create subgroups.
>> +///
>> +/// Implement this trait on structs that embed a [`Subsystem`] or a [`Group`].
>> +#[vtable]
>> +pub trait GroupOperations {
>> +    /// The parent data object type.
>> +    ///
>> +    /// The implementer of this trait is this kind of data object. Should be set
>> +    /// to `Self`.
>> +    type Parent;
>
> If it should be set to `Self`, why does this even exist? If there are
> cases where it isn't supposed to be `Self`, it would be good to put them
> into the docs.

Good point. I'll remove the type and use `&self` at relevant places
instead.

>
>> +
>> +    /// The child data object type.
>> +    ///
>> +    /// This group will create subgroups (subdirectories) backed by this kind of
>> +    /// object.
>> +    type Child: 'static;
>> +
>> +    /// The kernel will call this method in response to `mkdir(2)` in the
>> +    /// directory representing `this`.
>
> This doesn't really read like a first line description of this function,
> how about putting "Creates a new subgroup." as the first line?

Good call.

>
>> +    ///
>> +    /// To accept the request to create a group, implementations should
>> +    /// instantiate a `CHLD` and return a `CPTR` to it. To prevent creation,
>
> Is there a typo in `CHLD`? What do you mean by "return a `CPTR` to it"?

Sorry, it's from an older iteration of the patch set. I'll update the
sentence. Looking forward to the day where the compiler can use AI to
type check the docs :)

>
>> +    /// return a suitable error.
>> +    fn make_group(
>> +        this: &Self::Parent,
>> +        name: &CStr,
>> +    ) -> Result<impl PinInit<Group<Self::Child>, Error>>;
>> +
>> +    /// The kernel will call this method before the directory representing
>> +    /// `_child` is removed from `configfs`.
>
> Same thing about the one-line description, how about (with the name
> changed below): "Prepares the given group for removal from configfs.".

👍

>
>> +    ///
>> +    /// Implementations can use this method to do house keeping before
>> +    /// `configfs` drops its reference to `Child`.
>> +    fn drop_item(
>
> `drop` doesn't really fit here, I think something like `unlink_item`
> fits better, since the child isn't actually dropped after this function
> returns.

Yea, I know. But the function is called `drop_item` on the C side of
things. Usually we keep the C names.

>
>> +        _this: &Self::Parent,
>> +        _child: <Arc<Group<Self::Child>> as ForeignOwnable>::Borrowed<'_>,
>
> Just write ArcBorrow<'_, Group<Self::Child>> instead of the above?

Right. An earlier version of the patch set was generic over the pointer
type, allowing use of Box as well. The bounds became sort of wild, so I
figured limiting to `Arc` is probably fine.

>
>> +    ) {
>> +        kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
>> +    }
>> +}
>> +
>> +/// A `configfs` attribute.
>> +///
>> +/// An attribute appear as a file in configfs, inside a folder that represent
>
> Typo: appear -> appears

👍

>

[...]

>> +/// Operations supported by an attribute.
>> +///
>> +/// Implement this trait on type and pass that type as generic parameter when
>> +/// creating an [`Attribute`]. The type carrying the implementation serve no
>> +/// purpose other than specifying the attribute operations.
>> +#[vtable]
>> +pub trait AttributeOperations<const ID: u64 = 0> {
>> +    /// The type of the object that contains the field that is backing the
>> +    /// attribute for this operation.
>> +    type Data;
>> +
>> +    /// This function is called by the kernel to read the value of an attribute.
>
> "Reads the value of an attribute.".

👍

>
>> +    ///
>> +    /// Implementations should write the rendering of the attribute to `page`
>> +    /// and return the number of bytes written.
>> +    fn show(data: &Self::Data, page: &mut [u8; PAGE_SIZE]) -> Result<usize>;
>
> Why is this not named `read` or `load`? If the C equivalent is `show`,
> then it's fine, but otherwise I wouldn't understand why it's show/store
> as opposed to load/store or read/write.

Yes, also here naming is derived from the C counterpart. The vtable item
is called `show`.

>
>> +
>> +    /// This function is called by the kernel to update the value of an attribute.
>
> Again first line doc here.

👍

>
>> +    ///
>> +    /// Implementations should parse the value from `page` and update internal
>> +    /// state to reflect the parsed value. Partial writes are not supported and
>> +    /// implementations should expect the full page to arrive in one write
>> +    /// operation.
>
> I don't understand what you're trying to say with the last sentence.

I will remove the comment, it does not make sense here. I picked it up
from C docs. It refers to how the function is exposed to user space.
Usually writes to files are processed in chunks, and file systems cannot
expect that all data written to a file will be passed in a single
function call.

>
>> +    fn store(_data: &Self::Data, _page: &[u8]) -> Result {
>> +        kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
>> +    }
>> +}
>> +
>> +/// A list of attributes.
>> +///
>> +/// This type is used to construct a new [`ItemType`]. It represents a list of
>> +/// [`Attribute`] that will appear in the directory representing a [`Group`].
>> +/// Users should not directly instantiate this type, rather they should use the
>> +/// [`kernel::configfs_attrs`] macro to declare a static set of attributes for a
>> +/// group.
>> +#[repr(transparent)]
>> +pub struct AttributeList<const N: usize, Data>(
>> +    UnsafeCell<[*mut kernel::ffi::c_void; N]>,
>> +    PhantomData<Data>,
>> +);
>> +
>> +// SAFETY: Ownership of `AttributeList` can safely be transferred to other threads.
>> +unsafe impl<const N: usize, Data> Send for AttributeList<N, Data> {}
>> +
>> +// SAFETY: We do not provide any operations on `AttributeList` that need synchronization.
>> +unsafe impl<const N: usize, Data> Sync for AttributeList<N, Data> {}
>> +
>> +impl<const N: usize, Data> AttributeList<N, Data> {
>> +    #[doc(hidden)]
>
> I normally put attributes after the documentation.

Ok 👍

>
>> +    /// # Safety
>> +    ///
>> +    /// This function can only be called by expanding the `configfs_attrs`
>
> s/expanding//

Ok.

>
>> +    /// macro.
>> +    pub const unsafe fn new() -> Self {
>> +        Self(UnsafeCell::new([core::ptr::null_mut(); N]), PhantomData)
>> +    }
>> +
>> +    #[doc(hidden)]
>> +    /// # Safety
>> +    ///
>> +    /// This function can only be called by expanding the `configfs_attrs`
>
> s/expanding//

Ok.

>
>> +    /// macro.
>> +    pub const unsafe fn add<
>> +        const I: usize,
>> +        const ID: u64,
>> +        O: AttributeOperations<ID, Data = Data>,
>> +    >(
>> +        &'static self,
>> +        attribute: &'static Attribute<ID, O, Data>,
>> +    ) {
>> +        if I >= N - 1 {
>> +            kernel::build_error!("Invalid attribute index");
>> +        }
>> +
>> +        // SAFETY: This function is only called through `configfs_attrs`. This
>
> s/through `configfs_attrs`/through the `configfs_attrs` macro/

Ok.

>

[...]

>> +impl_item_type!(Subsystem<Data>);
>> +impl_item_type!(Group<Data>);
>> +
>> +impl<Container, Data> ItemType<Container, Data> {
>> +    fn as_ptr(&self) -> *const bindings::config_item_type {
>> +        self.item_type.get()
>> +    }
>> +}
>> +
>> +/// Define a list of configfs attributes statically.
>> +#[macro_export]
>> +macro_rules! configfs_attrs {
>
> I see you've joined the dark side of declarative macros!
>
> This seems like a prime candidate for replacing with a proc-macro when
> we have syn :)

Yes, I found myself wishing for `syn` very much while writing this.

>
>> +    (
>> +        container: $container:ty,
>> +        data: $data:ty,
>> +        attributes: [
>> +            $($name:ident: $attr:literal,)*
>
> This syntax always requires a trailing comma. Most (IIRC all, but not
> 100% sure) Rust syntax allows you to omit it, so it would be odd if it
> were not the case here. You can have an optional trailing comma via:
>
>     $($name:ident: $attr:literal),* $(,)?
>
> But as soon as you give the tokens off to the internals of the macro, I
> would recommend sticking to always having a trailing comma or no
> trailing comma.

Makes sense, I will fix that. Perhaps after the array square brackets as well.

>
>> +        ],
>> +    ) => {
>> +        $crate::configfs_attrs!(
>> +            count:
>> +            @container($container),
>> +            @data($data),
>> +            @child(),
>> +            @no_child(x),
>> +            @attrs($($name $attr)*),
>> +            @eat($($name $attr,)*),
>> +            @assign(),
>> +            @cnt(0usize),
>> +        )
>> +    };
>> +    (
>> +        container: $container:ty,
>> +        data: $data:ty,
>> +        child: $child:ty,
>> +        attributes: [
>> +            $($name:ident: $attr:literal,)*
>
> Ditto.
>
>> +        ],
>> +    ) => {
>> +        $crate::configfs_attrs!(
>> +            count:
>> +            @container($container),
>> +            @data($data),
>> +            @child($child),
>> +            @no_child(),
>> +            @attrs($($name $attr)*),
>> +            @eat($($name $attr,)*),
>> +            @assign(),
>> +            @cnt(0usize),
>> +        )
>> +    };
>> +    (count:
>> +     @container($container:ty),
>> +     @data($data:ty),
>> +     @child($($child:ty)?),
>> +     @no_child($($no_child:ident)?),
>> +     @attrs($($aname:ident $aattr:literal)*),
>> +     @eat($name:ident $attr:literal, $($rname:ident $rattr:literal,)*),
>> +     @assign($($assign:block)*),
>> +     @cnt($cnt:expr),
>> +    ) => {
>> +        $crate::configfs_attrs!(
>> +            count:
>> +            @container($container),
>> +            @data($data),
>> +            @child($($child)?),
>> +            @no_child($($no_child)?),
>> +            @attrs($($aname $aattr)*),
>> +            @eat($($rname $rattr,)*),
>> +            @assign($($assign)* {
>> +                const N: usize = $cnt;
>> +                // SAFETY: We are expanding `configfs_attrs`.
>
> This safety comment is doing a lot of heavy lifting, since it is not at
> all obvious what the below unsafe function will resolve to... Seems also
> a hassle to put a full comment here explaining that
> `[< $data:upper _ATTRS >]` is defined by the macro below and that it is
> of type `AttributeList` etc... But maybe we should.

I mean, I can put a comment saying that this will expand to a call to `Attribute::add`?

>
>> +                unsafe {
>> +                    $crate::macros::paste!( [< $data:upper _ATTRS >])
>> +                        .add::<N, $attr, _>(
>> +                            & $crate::macros::paste!( [< $data:upper _ $name:upper _ATTR >])
>> +                        )
>> +                };
>> +            }),
>
> You can merge the two `paste!` invocations into one:

Is that better?

>
>     @assign($($assign)* {
>         const N: usize = $cnt;
>         $crate::macros::paste! {
>             // SAFETY: see above comment
>             unsafe {
>                 [< $data:upper _ATTRS >].add::<N, $attr, _>(
>                     &[< $data:upper _ $name:upper _ATTR >]
>                 );
>             }
>         }
>     }),
>
>> +            @cnt(1usize + $cnt),
>> +        )
>> +    };
>> +    (count:
>> +     @container($container:ty),
>> +     @data($data:ty),
>> +     @child($($child:ty)?),
>> +     @no_child($($no_child:ident)?),
>> +     @attrs($($aname:ident $aattr:literal)*),
>> +     @eat(),
>> +     @assign($($assign:block)*),
>> +     @cnt($cnt:expr),
>> +    ) =>
>> +    {
>> +        $crate::configfs_attrs!(
>> +            final:
>> +            @container($container),
>> +            @data($data),
>> +            @child($($child)?),
>> +            @no_child($($no_child)?),
>> +            @attrs($($aname $aattr)*),
>> +            @assign($($assign)*),
>> +            @cnt($cnt),
>> +        )
>> +    };
>> +    (final:
>> +     @container($container:ty),
>> +     @data($data:ty),
>> +     @child($($child:ty)?),
>> +     @no_child($($no_child:ident)?),
>> +     @attrs($($name:ident $attr:literal)*),
>> +     @assign($($assign:block)*),
>> +     @cnt($cnt:expr),
>> +    ) =>
>> +    {
>> +        {
>> +            $(
>> +                $crate::macros::paste!{
>
> Again you can coalesce all of the `paste!` invocations into a single one
> spanning the entire output of this macro branch.

Is that better? I actually tried to keep them smaller.

>
>> +                    // SAFETY: We are expanding `configfs_attrs`.
>> +                    static [< $data:upper _ $name:upper _ATTR >]:
>> +                      $crate::configfs::Attribute<$attr, $data, $data> =
>> +                        unsafe {
>> +                            $crate::configfs::Attribute::new(c_str!(::core::stringify!($name)))
>> +                        };
>> +                }
>> +            )*
>> +
>> +
>> +            const N: usize = $cnt + 1usize;
>
> Why do we need an additional copy? To have a zero entry at the end for C
> to know it's the end of the list? If so, a comment here would be very
> helpful.

Yes, we need space for a null terminator. I'll add a comment.

We actually have a static check to make sure that we not missing this.

>
>> +            $crate::macros::paste!{
>> +                // SAFETY: We are expanding `configfs_attrs`.
>> +                static [< $data:upper _ATTRS >]:
>> +                  $crate::configfs::AttributeList<N, $data> =
>> +                    unsafe { $crate::configfs::AttributeList::new() };
>> +            }
>> +
>> +            $($assign)*
>> +
>> +            $(
>> +                $crate::macros::paste!{
>> +                    const [<$no_child:upper>]: bool = true;
>> +                };
>> +
>> +                $crate::macros::paste!{
>> +                    static [< $data:upper _TPE >] : $crate::configfs::ItemType<$container, $data>  =
>> +                        $crate::configfs::ItemType::<$container, $data>::new::<N>(
>> +                            &THIS_MODULE, &[<$ data:upper _ATTRS >]
>> +                        );
>> +                }
>> +            )?
>> +
>> +            $(
>> +                $crate::macros::paste!{
>> +                    static [< $data:upper _TPE >]:
>> +                      $crate::configfs::ItemType<$container, $data>  =
>> +                        $crate::configfs::ItemType::<$container, $data>::new_with_child_ctor::
>> +                    <N, $child>(
>> +                        &THIS_MODULE, &[<$ data:upper _ATTRS >]
>> +                    );
>> +                }
>> +            )?
>> +
>> +            &$crate::macros::paste!( [< $data:upper _TPE >] )
>> +        }
>> +    };
>> +
>> +}
>
> I tested if multiple invocations of this macro can shadow each other and
> the answer is no. So wrapping a const with `{}` makes it inaccessible to
> the outside which is exactly what we need here.
> The macro looks quite good!

Well you can guess where the inspiration came from :)

>

[...]

>> diff --git a/samples/rust/rust_configfs.rs b/samples/rust/rust_configfs.rs
>> new file mode 100644
>> index 0000000000000..fe896e66efb41
>> --- /dev/null
>> +++ b/samples/rust/rust_configfs.rs
>> @@ -0,0 +1,186 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Rust configfs sample.
>> +
>> +use kernel::alloc::flags;
>> +use kernel::c_str;
>> +use kernel::configfs;
>> +use kernel::configfs_attrs;
>> +use kernel::new_mutex;
>> +use kernel::prelude::*;
>> +use kernel::sync::Mutex;
>
> Would merge the imports here too (rust-analyzer has a code-action for
> that btw).

I prefer it like this.

>
>> +
>> +module! {
>> +    type: RustConfigfs,
>> +    name: "rust_configfs",
>> +    author: "Rust for Linux Contributors",
>> +    description: "Rust configfs sample",
>> +    license: "GPL",
>> +}
>> +
>> +#[pin_data]
>> +struct RustConfigfs {
>> +    #[pin]
>> +    config: configfs::Subsystem<Configuration>,
>> +}
>> +
>> +#[pin_data]
>> +struct Configuration {
>> +    message: &'static CStr,
>> +    #[pin]
>> +    bar: Mutex<(KBox<[u8; 4096]>, usize)>,
>> +}
>> +
>> +impl Configuration {
>> +    fn new() -> impl PinInit<Self, Error> {
>> +        try_pin_init!(Self {
>> +            message: c_str!("Hello World\n"),
>> +            bar <- new_mutex!((KBox::new([0;4096], flags::GFP_KERNEL)?,0)),
>
> s/;/; /
> s/,0/, 0/

👍


Thanks for the detailed review!


Best regards,
Andreas Hindborg

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Benno Lossin]

On 17.02.25 12:08, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>> On 07.02.25 15:41, Andreas Hindborg wrote:
>>> diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
>>> new file mode 100644
>>> index 0000000000000..9d4b381b9df89
>>> --- /dev/null
>>> +++ b/rust/kernel/configfs.rs
>>> @@ -0,0 +1,860 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! `configfs` interface.
>>> +//!
>>> +//! `configfs` is an in-memory pseudo file system for configuration of kernel
>>> +//! modules. Please see the [C documentation] for details and intended use of
>>> +//! `configfs`.
>>> +//!
>>> +//! This module does not support the following `configfs` features:
>>> +//!
>>> +//! - Items. All group children are groups.
>>> +//! - Symlink support.
>>> +//! - `disconnect_notify` hook.
>>> +//! - Default groups.
>>
>> For lists like this, I usually end items except the last one with a
>> comma instead of a period.
> 
> If that is the right way to do it, sure. It is actually funny that you
> notice, because I searched for input on how to typeset such a list, and
> some Microsoft typesetting site told me to do it like this when some
> items are sentences [1]. I am not a native English speaker, and I have
> no idea what the correct formatting is. Commas are not mentioned at the
> resource I found.

Since I'm also not a native speaker (and I don't really remember the
rules from school by name, but rather by heart and intuition only), I
don't really know. It just looked a bit strange to me (intuition
speaking) and so I commented on it. I don't really mind one way or the
other.

> [1] https://www.microsoft.com/en-us/microsoft-365-life-hacks/writing/punctuating-bullet-point-lists
> 
>>
>>> +//!
>>> +//! See the [rust_configfs.rs] sample for a full example use of this module.
>>
>> It could also be useful to just put the example directly here into the
>> docs instead of/additionally to having it as a sample.
> 
> I don't think we should duplicate the code. As long as the link works, I
> think having it separately is fine.

When I'm coding in my editor and read some docs directly in it, it often
is annoying to find a link, because then I have to open the docs in my
web-browser.
I understand that you don't want to duplicate the code (and it also is a
bit too much for a short example), so how about having a simpler
example? Maybe with only a single operation that has no associated data
(use `()`)?

>>> +//!
>>> +//! C header: [`include/linux/configfs.h`](srctree/include/linux/configfs.h)
>>> +//!
>>> +//! [C documentation]: srctree/Documentation/filesystems/configfs.rst
>>> +//! [rust_configfs.rs]: srctree/samples/rust/rust_configfs.rs
>>> +
>>> +use crate::alloc::flags;
>>> +use crate::container_of;
>>> +use crate::page::PAGE_SIZE;
>>> +use crate::prelude::*;
>>> +use crate::str::CString;
>>> +use crate::sync::Arc;
>>> +use crate::types::ForeignOwnable;
>>> +use crate::types::Opaque;
>>> +use core::cell::UnsafeCell;
>>> +use core::marker::PhantomData;
>>> +use core::ptr::addr_of;
>>> +use core::ptr::addr_of_mut;
>>
>> I usually would import this like so:
>>
>>     use crate::{
>>         alloc::flags,
>>         container_of,
>>         page::PAGE_SIZE,
>>         prelude::*,
>>         str::CString,
>>         sync::Arc,
>>         types::{ForeignOwnable, Opaque},
>>     };
>>     use core::{
>>         cell::UnsafeCell,
>>         marker::PhantomData,
>>         ptr::{addr_of, addr_of_mut},
>>     };
>>
>> To me this is more readable.
> 
> I disagree with that. I don't think what you suggest is easier to read,
> and it is much more difficult to work with when rebasing and merging
> things. This was discussed elsewhere in the past without reaching a
> conclusion. I think we should come to a consensus on what style we
> should adopt for the imports.

Yeah for rebasing it is annoying... I think we discussed at some point
of maybe having a script that automatically merges imports, but that
runs into the issue of keeping all of them, which might not be
necessary, because the code below doesn't use everything...
We should discuss this on a more general basis. 

To me the merged form is more readable, because I can better see at a
glance what things are used from where. But maybe that is just due to
familiarity with it.

Created an issue to track this: https://github.com/Rust-for-Linux/linux/issues/1143

>>> +
>>> +/// A `configfs` subsystem.
>>> +///
>>> +/// This is the top level entrypoint for a `configfs` hierarchy. To register
>>> +/// with configfs, embed a field of this type into your kernel module struct.
>>> +#[pin_data(PinnedDrop)]
>>> +pub struct Subsystem<Data> {
>>
>> Usually, we don't have multi-character generics, any specific reason
>> that you chose `Data` here over `T` or `D`?
> 
> Yes, I find it more descriptive. The patch set went through quite a bit
> of evolution, and the generics got a bit complicated in earlier
> iterations, which necessitated more descriptive generic type parameter
> names. It's not so bad in this version after I restricted the pointer
> type to just `Arc`, but I still think that using a word rather a single
> letter makes the code easier to comprehend at first pass.

Makes sense. I'm not opposed to it, but I am a bit cautious, because one
disadvantage with using multi-character names for generics is that one
cannot easily see if a type is a generic or not. Maybe that is not as
important as I think it could be, but to me it seems useful.

What do the others think?

> Also, using a word is allowed as per the API guideline document [2]:
> 
>       > concise UpperCamelCase, usually single uppercase letter: T
> 
> https://rust-lang.github.io/api-guidelines/naming.html
> 
>>
>>> +    #[pin]
>>> +    subsystem: Opaque<bindings::configfs_subsystem>,
>>> +    #[pin]
>>> +    data: Data,
>>> +}
>>> +
>>> +// SAFETY: We do not provide any operations on `Subsystem`.
>>> +unsafe impl<Data> Sync for Subsystem<Data> {}
>>> +
>>> +// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
>>> +unsafe impl<Data> Send for Subsystem<Data> {}
>>> +
>>> +impl<Data> Subsystem<Data> {
>>> +    /// Create an initializer for a [`Subsystem`].
>>> +    ///
>>> +    /// The subsystem will appear in configfs as a directory name given by
>>> +    /// `name`. The attributes available in directory are specified by
>>> +    /// `item_type`.
>>> +    pub fn new(
>>> +        name: &'static CStr,
>>> +        item_type: &'static ItemType<Subsystem<Data>, Data>,
>>> +        data: impl PinInit<Data, Error>,
>>> +    ) -> impl PinInit<Self, Error> {
>>> +        try_pin_init!(Self {
>>> +            subsystem <- kernel::init::zeroed().chain(
>>> +                |place: &mut Opaque<bindings::configfs_subsystem>| {
>>> +                    // SAFETY: All of `place` is valid for write.
>>> +                    unsafe {
>>> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_name )
>>> +                            .write(name.as_ptr().cast_mut().cast())
>>> +                    };
>>> +                    // SAFETY: All of `place` is valid for write.
>>> +                    unsafe {
>>> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_type)
>>> +                            .write(item_type.as_ptr())
>>> +                    };
>>> +                    // SAFETY: We initialized the required fields of `place.group` above.
>>> +                    unsafe { bindings::config_group_init(&mut (*place.get()).su_group) };
>>> +                    // SAFETY: `place.su_mutex` is valid for use as a mutex.
>>> +                    unsafe { bindings::__mutex_init(
>>> +                        &mut (*place.get()).su_mutex,
>>> +                        kernel::optional_name!().as_char_ptr(),
>>> +                        kernel::static_lock_class!().as_ptr())
>>
>> Formatting for this code is weird.
>>
>> (since this is inside of the `try_pin_init!` macro, rustfmt doesn't
>> format it, since `<-` isn't part of rust syntax, so it doesn't know what
>> to do. I usually fix this by replacing all `<-` with `:`, format and
>> then change things back)
> 
> Such is the perils of macros. I'll try to go over it again. Perhaps we
> could make `rustfmt` understand `<-`?

There have been several discussions about teaching rustfmt custom macro
rules, but I don't think that such a feature exists.

>> Also, is there no function in C that does all of this initialization for
>> you?
> 
> I might be able to do a little better. There is a C function that takes
> care of initialization of `ci_name` and `ci_type` as well. I can't
> recall if there was a particular reason for not using it, but I'll
> check.

Just checking that we don't miss an initialization function, since that
makes it easier to maintain the code.

> We have to initialize the mutex explicitly. I think the reason for not
> doing that implicitly C side is to allow it to be statically
> initialized.

I see.

>>> +    ///
>>> +    /// Implementations can use this method to do house keeping before
>>> +    /// `configfs` drops its reference to `Child`.
>>> +    fn drop_item(
>>
>> `drop` doesn't really fit here, I think something like `unlink_item`
>> fits better, since the child isn't actually dropped after this function
>> returns.
> 
> Yea, I know. But the function is called `drop_item` on the C side of
> things. Usually we keep the C names.

Makes sense.

>>> +impl_item_type!(Subsystem<Data>);
>>> +impl_item_type!(Group<Data>);
>>> +
>>> +impl<Container, Data> ItemType<Container, Data> {
>>> +    fn as_ptr(&self) -> *const bindings::config_item_type {
>>> +        self.item_type.get()
>>> +    }
>>> +}
>>> +
>>> +/// Define a list of configfs attributes statically.
>>> +#[macro_export]
>>> +macro_rules! configfs_attrs {
>>
>> I see you've joined the dark side of declarative macros!
>>
>> This seems like a prime candidate for replacing with a proc-macro when
>> we have syn :)
> 
> Yes, I found myself wishing for `syn` very much while writing this.

Now you know my pain :) :) :)

>>> +    (
>>> +        container: $container:ty,
>>> +        data: $data:ty,
>>> +        attributes: [
>>> +            $($name:ident: $attr:literal,)*
>>
>> This syntax always requires a trailing comma. Most (IIRC all, but not
>> 100% sure) Rust syntax allows you to omit it, so it would be odd if it
>> were not the case here. You can have an optional trailing comma via:
>>
>>     $($name:ident: $attr:literal),* $(,)?
>>
>> But as soon as you give the tokens off to the internals of the macro, I
>> would recommend sticking to always having a trailing comma or no
>> trailing comma.
> 
> Makes sense, I will fix that. Perhaps after the array square brackets as well.

Yeah that too.

>>> +        ],
>>> +    ) => {
>>> +        $crate::configfs_attrs!(
>>> +            count:
>>> +            @container($container),
>>> +            @data($data),
>>> +            @child(),
>>> +            @no_child(x),
>>> +            @attrs($($name $attr)*),
>>> +            @eat($($name $attr,)*),
>>> +            @assign(),
>>> +            @cnt(0usize),
>>> +        )
>>> +    };
>>> +    (
>>> +        container: $container:ty,
>>> +        data: $data:ty,
>>> +        child: $child:ty,
>>> +        attributes: [
>>> +            $($name:ident: $attr:literal,)*
>>
>> Ditto.
>>
>>> +        ],
>>> +    ) => {
>>> +        $crate::configfs_attrs!(
>>> +            count:
>>> +            @container($container),
>>> +            @data($data),
>>> +            @child($child),
>>> +            @no_child(),
>>> +            @attrs($($name $attr)*),
>>> +            @eat($($name $attr,)*),
>>> +            @assign(),
>>> +            @cnt(0usize),
>>> +        )
>>> +    };
>>> +    (count:
>>> +     @container($container:ty),
>>> +     @data($data:ty),
>>> +     @child($($child:ty)?),
>>> +     @no_child($($no_child:ident)?),
>>> +     @attrs($($aname:ident $aattr:literal)*),
>>> +     @eat($name:ident $attr:literal, $($rname:ident $rattr:literal,)*),
>>> +     @assign($($assign:block)*),
>>> +     @cnt($cnt:expr),
>>> +    ) => {
>>> +        $crate::configfs_attrs!(
>>> +            count:
>>> +            @container($container),
>>> +            @data($data),
>>> +            @child($($child)?),
>>> +            @no_child($($no_child)?),
>>> +            @attrs($($aname $aattr)*),
>>> +            @eat($($rname $rattr,)*),
>>> +            @assign($($assign)* {
>>> +                const N: usize = $cnt;
>>> +                // SAFETY: We are expanding `configfs_attrs`.
>>
>> This safety comment is doing a lot of heavy lifting, since it is not at
>> all obvious what the below unsafe function will resolve to... Seems also
>> a hassle to put a full comment here explaining that
>> `[< $data:upper _ATTRS >]` is defined by the macro below and that it is
>> of type `AttributeList` etc... But maybe we should.
> 
> I mean, I can put a comment saying that this will expand to a call to `Attribute::add`?

Yeah that would be good.

>>> +                unsafe {
>>> +                    $crate::macros::paste!( [< $data:upper _ATTRS >])
>>> +                        .add::<N, $attr, _>(
>>> +                            & $crate::macros::paste!( [< $data:upper _ $name:upper _ATTR >])
>>> +                        )
>>> +                };
>>> +            }),
>>
>> You can merge the two `paste!` invocations into one:
> 
> Is that better?

I feel it is. You trade one indentation level for having less characters
in the body. To me that is worth it, because then I don't have to ignore
the `$crate::macros::paste!` characters.

> 
>>
>>     @assign($($assign)* {
>>         const N: usize = $cnt;
>>         $crate::macros::paste! {
>>             // SAFETY: see above comment
>>             unsafe {
>>                 [< $data:upper _ATTRS >].add::<N, $attr, _>(
>>                     &[< $data:upper _ $name:upper _ATTR >]
>>                 );
>>             }
>>         }
>>     }),
>>
>>> +            @cnt(1usize + $cnt),
>>> +        )
>>> +    };
>>> +    (count:
>>> +     @container($container:ty),
>>> +     @data($data:ty),
>>> +     @child($($child:ty)?),
>>> +     @no_child($($no_child:ident)?),
>>> +     @attrs($($aname:ident $aattr:literal)*),
>>> +     @eat(),
>>> +     @assign($($assign:block)*),
>>> +     @cnt($cnt:expr),
>>> +    ) =>
>>> +    {
>>> +        $crate::configfs_attrs!(
>>> +            final:
>>> +            @container($container),
>>> +            @data($data),
>>> +            @child($($child)?),
>>> +            @no_child($($no_child)?),
>>> +            @attrs($($aname $aattr)*),
>>> +            @assign($($assign)*),
>>> +            @cnt($cnt),
>>> +        )
>>> +    };
>>> +    (final:
>>> +     @container($container:ty),
>>> +     @data($data:ty),
>>> +     @child($($child:ty)?),
>>> +     @no_child($($no_child:ident)?),
>>> +     @attrs($($name:ident $attr:literal)*),
>>> +     @assign($($assign:block)*),
>>> +     @cnt($cnt:expr),
>>> +    ) =>
>>> +    {
>>> +        {
>>> +            $(
>>> +                $crate::macros::paste!{
>>
>> Again you can coalesce all of the `paste!` invocations into a single one
>> spanning the entire output of this macro branch.
> 
> Is that better? I actually tried to keep them smaller.

Yeah I think so (see above).

>>> +                    // SAFETY: We are expanding `configfs_attrs`.
>>> +                    static [< $data:upper _ $name:upper _ATTR >]:
>>> +                      $crate::configfs::Attribute<$attr, $data, $data> =
>>> +                        unsafe {
>>> +                            $crate::configfs::Attribute::new(c_str!(::core::stringify!($name)))
>>> +                        };
>>> +                }
>>> +            )*
>>> +
>>> +
>>> +            const N: usize = $cnt + 1usize;
>>
>> Why do we need an additional copy? To have a zero entry at the end for C
>> to know it's the end of the list? If so, a comment here would be very
>> helpful.
> 
> Yes, we need space for a null terminator. I'll add a comment.
> 
> We actually have a static check to make sure that we not missing this.

Where is this static check?

>>> +            $crate::macros::paste!{
>>> +                // SAFETY: We are expanding `configfs_attrs`.
>>> +                static [< $data:upper _ATTRS >]:
>>> +                  $crate::configfs::AttributeList<N, $data> =
>>> +                    unsafe { $crate::configfs::AttributeList::new() };
>>> +            }
>>> +
>>> +            $($assign)*
>>> +
>>> +            $(
>>> +                $crate::macros::paste!{
>>> +                    const [<$no_child:upper>]: bool = true;
>>> +                };
>>> +
>>> +                $crate::macros::paste!{
>>> +                    static [< $data:upper _TPE >] : $crate::configfs::ItemType<$container, $data>  =
>>> +                        $crate::configfs::ItemType::<$container, $data>::new::<N>(
>>> +                            &THIS_MODULE, &[<$ data:upper _ATTRS >]
>>> +                        );
>>> +                }
>>> +            )?
>>> +
>>> +            $(
>>> +                $crate::macros::paste!{
>>> +                    static [< $data:upper _TPE >]:
>>> +                      $crate::configfs::ItemType<$container, $data>  =
>>> +                        $crate::configfs::ItemType::<$container, $data>::new_with_child_ctor::
>>> +                    <N, $child>(
>>> +                        &THIS_MODULE, &[<$ data:upper _ATTRS >]
>>> +                    );
>>> +                }
>>> +            )?
>>> +
>>> +            &$crate::macros::paste!( [< $data:upper _TPE >] )
>>> +        }
>>> +    };
>>> +
>>> +}
>>
>> I tested if multiple invocations of this macro can shadow each other and
>> the answer is no. So wrapping a const with `{}` makes it inaccessible to
>> the outside which is exactly what we need here.
>> The macro looks quite good!
> 
> Well you can guess where the inspiration came from :)

Glad that that was helpful to you :)

---
Cheers,
Benno

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 17.02.25 12:08, Andreas Hindborg wrote:

[...]

>>>
>>>> +//!
>>>> +//! See the [rust_configfs.rs] sample for a full example use of this module.
>>>
>>> It could also be useful to just put the example directly here into the
>>> docs instead of/additionally to having it as a sample.
>>
>> I don't think we should duplicate the code. As long as the link works, I
>> think having it separately is fine.
>
> When I'm coding in my editor and read some docs directly in it, it often
> is annoying to find a link, because then I have to open the docs in my
> web-browser.
> I understand that you don't want to duplicate the code (and it also is a
> bit too much for a short example), so how about having a simpler
> example? Maybe with only a single operation that has no associated data
> (use `()`)?

Sure, we can do that.

>
>>>> +//!
>>>> +//! C header: [`include/linux/configfs.h`](srctree/include/linux/configfs.h)
>>>> +//!
>>>> +//! [C documentation]: srctree/Documentation/filesystems/configfs.rst
>>>> +//! [rust_configfs.rs]: srctree/samples/rust/rust_configfs.rs
>>>> +
>>>> +use crate::alloc::flags;
>>>> +use crate::container_of;
>>>> +use crate::page::PAGE_SIZE;
>>>> +use crate::prelude::*;
>>>> +use crate::str::CString;
>>>> +use crate::sync::Arc;
>>>> +use crate::types::ForeignOwnable;
>>>> +use crate::types::Opaque;
>>>> +use core::cell::UnsafeCell;
>>>> +use core::marker::PhantomData;
>>>> +use core::ptr::addr_of;
>>>> +use core::ptr::addr_of_mut;
>>>
>>> I usually would import this like so:
>>>
>>>     use crate::{
>>>         alloc::flags,
>>>         container_of,
>>>         page::PAGE_SIZE,
>>>         prelude::*,
>>>         str::CString,
>>>         sync::Arc,
>>>         types::{ForeignOwnable, Opaque},
>>>     };
>>>     use core::{
>>>         cell::UnsafeCell,
>>>         marker::PhantomData,
>>>         ptr::{addr_of, addr_of_mut},
>>>     };
>>>
>>> To me this is more readable.
>>
>> I disagree with that. I don't think what you suggest is easier to read,
>> and it is much more difficult to work with when rebasing and merging
>> things. This was discussed elsewhere in the past without reaching a
>> conclusion. I think we should come to a consensus on what style we
>> should adopt for the imports.
>
> Yeah for rebasing it is annoying... I think we discussed at some point
> of maybe having a script that automatically merges imports, but that
> runs into the issue of keeping all of them, which might not be
> necessary, because the code below doesn't use everything...
> We should discuss this on a more general basis.
>
> To me the merged form is more readable, because I can better see at a
> glance what things are used from where. But maybe that is just due to
> familiarity with it.
>
> Created an issue to track this: https://github.com/Rust-for-Linux/linux/issues/1143

Cool 👍

>
>>>> +
>>>> +/// A `configfs` subsystem.
>>>> +///
>>>> +/// This is the top level entrypoint for a `configfs` hierarchy. To register
>>>> +/// with configfs, embed a field of this type into your kernel module struct.
>>>> +#[pin_data(PinnedDrop)]
>>>> +pub struct Subsystem<Data> {
>>>
>>> Usually, we don't have multi-character generics, any specific reason
>>> that you chose `Data` here over `T` or `D`?
>>
>> Yes, I find it more descriptive. The patch set went through quite a bit
>> of evolution, and the generics got a bit complicated in earlier
>> iterations, which necessitated more descriptive generic type parameter
>> names. It's not so bad in this version after I restricted the pointer
>> type to just `Arc`, but I still think that using a word rather a single
>> letter makes the code easier to comprehend at first pass.
>
> Makes sense. I'm not opposed to it, but I am a bit cautious, because one
> disadvantage with using multi-character names for generics is that one
> cannot easily see if a type is a generic or not. Maybe that is not as
> important as I think it could be, but to me it seems useful.

If you use an editor with semantic highlighting, you can style the
generic identifiers. I am currently trying out Helix, and that is
unfortunately on of the features it is missing. Can't have it all I
guess.

>
> What do the others think?
>
>> Also, using a word is allowed as per the API guideline document [2]:
>>
>>       > concise UpperCamelCase, usually single uppercase letter: T
>>
>> https://rust-lang.github.io/api-guidelines/naming.html
>>
>>>
>>>> +    #[pin]
>>>> +    subsystem: Opaque<bindings::configfs_subsystem>,
>>>> +    #[pin]
>>>> +    data: Data,
>>>> +}
>>>> +
>>>> +// SAFETY: We do not provide any operations on `Subsystem`.
>>>> +unsafe impl<Data> Sync for Subsystem<Data> {}
>>>> +
>>>> +// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
>>>> +unsafe impl<Data> Send for Subsystem<Data> {}
>>>> +
>>>> +impl<Data> Subsystem<Data> {
>>>> +    /// Create an initializer for a [`Subsystem`].
>>>> +    ///
>>>> +    /// The subsystem will appear in configfs as a directory name given by
>>>> +    /// `name`. The attributes available in directory are specified by
>>>> +    /// `item_type`.
>>>> +    pub fn new(
>>>> +        name: &'static CStr,
>>>> +        item_type: &'static ItemType<Subsystem<Data>, Data>,
>>>> +        data: impl PinInit<Data, Error>,
>>>> +    ) -> impl PinInit<Self, Error> {
>>>> +        try_pin_init!(Self {
>>>> +            subsystem <- kernel::init::zeroed().chain(
>>>> +                |place: &mut Opaque<bindings::configfs_subsystem>| {
>>>> +                    // SAFETY: All of `place` is valid for write.
>>>> +                    unsafe {
>>>> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_name )
>>>> +                            .write(name.as_ptr().cast_mut().cast())
>>>> +                    };
>>>> +                    // SAFETY: All of `place` is valid for write.
>>>> +                    unsafe {
>>>> +                        addr_of_mut!((*place.get()).su_group.cg_item.ci_type)
>>>> +                            .write(item_type.as_ptr())
>>>> +                    };
>>>> +                    // SAFETY: We initialized the required fields of `place.group` above.
>>>> +                    unsafe { bindings::config_group_init(&mut (*placeget()).su_group) };
>>>> +                    // SAFETY: `place.su_mutex` is valid for use as a mutex.
>>>> +                    unsafe { bindings::__mutex_init(
>>>> +                        &mut (*place.get()).su_mutex,
>>>> +                        kernel::optional_name!().as_char_ptr(),
>>>> +                        kernel::static_lock_class!().as_ptr())
>>>
>>> Formatting for this code is weird.
>>>
>>> (since this is inside of the `try_pin_init!` macro, rustfmt doesn't
>>> format it, since `<-` isn't part of rust syntax, so it doesn't know what
>>> to do. I usually fix this by replacing all `<-` with `:`, format and
>>> then change things back)
>>
>> Such is the perils of macros. I'll try to go over it again. Perhaps we
>> could make `rustfmt` understand `<-`?
>
> There have been several discussions about teaching rustfmt custom macro
> rules, but I don't think that such a feature exists.
>
>>> Also, is there no function in C that does all of this initialization for
>>> you?
>>
>> I might be able to do a little better. There is a C function that takes
>> care of initialization of `ci_name` and `ci_type` as well. I can't
>> recall if there was a particular reason for not using it, but I'll
>> check.
>
> Just checking that we don't miss an initialization function, since that
> makes it easier to maintain the code.

Yea, I think I can use the other version that takes name and type.

[...]

>>>> +                unsafe {
>>>> +                    $crate::macros::paste!( [< $data:upper _ATTRS >])
>>>> +                        .add::<N, $attr, _>(
>>>> +                            & $crate::macros::paste!( [< $data:upper _ $name:upper _ATTR >])
>>>> +                        )
>>>> +                };
>>>> +            }),
>>>
>>> You can merge the two `paste!` invocations into one:
>>
>> Is that better?
>
> I feel it is. You trade one indentation level for having less characters
> in the body. To me that is worth it, because then I don't have to ignore
> the `$crate::macros::paste!` characters.

I'll give it a shot.

[...]

>>>> +                    // SAFETY: We are expanding `configfs_attrs`.
>>>> +                    static [< $data:upper _ $name:upper _ATTR >]:
>>>> +                      $crate::configfs::Attribute<$attr, $data, $data> =
>>>> +                        unsafe {
>>>> +                            $crate::configfs::Attribute::new(c_str!(::core::stringify!($name)))
>>>> +                        };
>>>> +                }
>>>> +            )*
>>>> +
>>>> +
>>>> +            const N: usize = $cnt + 1usize;
>>>
>>> Why do we need an additional copy? To have a zero entry at the end for C
>>> to know it's the end of the list? If so, a comment here would be very
>>> helpful.
>>
>> Yes, we need space for a null terminator. I'll add a comment.
>>
>> We actually have a static check to make sure that we not missing this.
>
> Where is this static check?

In `Attribute::add`:

        if I >= N - 1 {
            kernel::build_error!("Invalid attribute index");
        }



Best regards,
Andreas Hindborg

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Benno Lossin]

On 17.02.25 13:20, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>> On 17.02.25 12:08, Andreas Hindborg wrote:
>>>>> +
>>>>> +/// A `configfs` subsystem.
>>>>> +///
>>>>> +/// This is the top level entrypoint for a `configfs` hierarchy. To register
>>>>> +/// with configfs, embed a field of this type into your kernel module struct.
>>>>> +#[pin_data(PinnedDrop)]
>>>>> +pub struct Subsystem<Data> {
>>>>
>>>> Usually, we don't have multi-character generics, any specific reason
>>>> that you chose `Data` here over `T` or `D`?
>>>
>>> Yes, I find it more descriptive. The patch set went through quite a bit
>>> of evolution, and the generics got a bit complicated in earlier
>>> iterations, which necessitated more descriptive generic type parameter
>>> names. It's not so bad in this version after I restricted the pointer
>>> type to just `Arc`, but I still think that using a word rather a single
>>> letter makes the code easier to comprehend at first pass.
>>
>> Makes sense. I'm not opposed to it, but I am a bit cautious, because one
>> disadvantage with using multi-character names for generics is that one
>> cannot easily see if a type is a generic or not. Maybe that is not as
>> important as I think it could be, but to me it seems useful.
> 
> If you use an editor with semantic highlighting, you can style the
> generic identifiers. I am currently trying out Helix, and that is
> unfortunately on of the features it is missing. Can't have it all I
> guess.

That is true, but there are a lot of places where Rust code is put that
aren't my editor (git diffs/commit messages, mails, lore.kernel.org,
github) and there it'll become more difficult to read (also people might
not have their editor configured to highlight them).

So I think we should at least consider it more.

>>>>> +                    // SAFETY: We are expanding `configfs_attrs`.
>>>>> +                    static [< $data:upper _ $name:upper _ATTR >]:
>>>>> +                      $crate::configfs::Attribute<$attr, $data, $data> =
>>>>> +                        unsafe {
>>>>> +                            $crate::configfs::Attribute::new(c_str!(::core::stringify!($name)))
>>>>> +                        };
>>>>> +                }
>>>>> +            )*
>>>>> +
>>>>> +
>>>>> +            const N: usize = $cnt + 1usize;
>>>>
>>>> Why do we need an additional copy? To have a zero entry at the end for C
>>>> to know it's the end of the list? If so, a comment here would be very
>>>> helpful.
>>>
>>> Yes, we need space for a null terminator. I'll add a comment.
>>>
>>> We actually have a static check to make sure that we not missing this.
>>
>> Where is this static check?
> 
> In `Attribute::add`:
> 
>         if I >= N - 1 {
>             kernel::build_error!("Invalid attribute index");
>         }

Ahh I see, would be also nice to have a comment there explaining why the
check is `>= N - 1`.

---
Cheers,
Benno

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 17.02.25 13:20, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>> On 17.02.25 12:08, Andreas Hindborg wrote:
>>>>>> +
>>>>>> +/// A `configfs` subsystem.
>>>>>> +///
>>>>>> +/// This is the top level entrypoint for a `configfs` hierarchy. To register
>>>>>> +/// with configfs, embed a field of this type into your kernel module struct.
>>>>>> +#[pin_data(PinnedDrop)]
>>>>>> +pub struct Subsystem<Data> {
>>>>>
>>>>> Usually, we don't have multi-character generics, any specific reason
>>>>> that you chose `Data` here over `T` or `D`?
>>>>
>>>> Yes, I find it more descriptive. The patch set went through quite a bit
>>>> of evolution, and the generics got a bit complicated in earlier
>>>> iterations, which necessitated more descriptive generic type parameter
>>>> names. It's not so bad in this version after I restricted the pointer
>>>> type to just `Arc`, but I still think that using a word rather a single
>>>> letter makes the code easier to comprehend at first pass.
>>>
>>> Makes sense. I'm not opposed to it, but I am a bit cautious, because one
>>> disadvantage with using multi-character names for generics is that one
>>> cannot easily see if a type is a generic or not. Maybe that is not as
>>> important as I think it could be, but to me it seems useful.
>>
>> If you use an editor with semantic highlighting, you can style the
>> generic identifiers. I am currently trying out Helix, and that is
>> unfortunately on of the features it is missing. Can't have it all I
>> guess.
>
> That is true, but there are a lot of places where Rust code is put that
> aren't my editor (git diffs/commit messages, mails, lore.kernel.org,
> github) and there it'll become more difficult to read (also people might
> not have their editor configured to highlight them).
>
> So I think we should at least consider it more.

There is a trade-off to be made for sure.

>
>>>>>> +                    // SAFETY: We are expanding `configfs_attrs`.
>>>>>> +                    static [< $data:upper _ $name:upper _ATTR >]:
>>>>>> +                      $crate::configfs::Attribute<$attr, $data, $data> =
>>>>>> +                        unsafe {
>>>>>> +                            $crate::configfs::Attribute::new(c_str!(::core::stringify!($name)))
>>>>>> +                        };
>>>>>> +                }
>>>>>> +            )*
>>>>>> +
>>>>>> +
>>>>>> +            const N: usize = $cnt + 1usize;
>>>>>
>>>>> Why do we need an additional copy? To have a zero entry at the end for C
>>>>> to know it's the end of the list? If so, a comment here would be very
>>>>> helpful.
>>>>
>>>> Yes, we need space for a null terminator. I'll add a comment.
>>>>
>>>> We actually have a static check to make sure that we not missing this.
>>>
>>> Where is this static check?
>>
>> In `Attribute::add`:
>>
>>         if I >= N - 1 {
>>             kernel::build_error!("Invalid attribute index");
>>         }
>
> Ahh I see, would be also nice to have a comment there explaining why the
> check is `>= N - 1`.

I'll add a comment 👍


Best regards,
Andreas Hindborg

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> "Benno Lossin" <benno.lossin@proton.me> writes:
>
>> On 17.02.25 12:08, Andreas Hindborg wrote:
>
> [...]
>
>>>>
>>>>> +//!
>>>>> +//! See the [rust_configfs.rs] sample for a full example use of this module.
>>>>
>>>> It could also be useful to just put the example directly here into the
>>>> docs instead of/additionally to having it as a sample.
>>>
>>> I don't think we should duplicate the code. As long as the link works, I
>>> think having it separately is fine.
>>
>> When I'm coding in my editor and read some docs directly in it, it often
>> is annoying to find a link, because then I have to open the docs in my
>> web-browser.
>> I understand that you don't want to duplicate the code (and it also is a
>> bit too much for a short example), so how about having a simpler
>> example? Maybe with only a single operation that has no associated data
>> (use `()`)?
>
> Sure, we can do that.

Actually, I am having some problems making the inline example compile.
`configfs_attrs!` references `THIS_MODULE`, which is not present for
doctests.

I'll add the example but mark it `ignore` ?


Best regards,
Andreas Hindborg

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Benno Lossin]

On 18.02.25 13:17, Andreas Hindborg wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
> 
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>
>>> On 17.02.25 12:08, Andreas Hindborg wrote:
>>
>> [...]
>>
>>>>>
>>>>>> +//!
>>>>>> +//! See the [rust_configfs.rs] sample for a full example use of this module.
>>>>>
>>>>> It could also be useful to just put the example directly here into the
>>>>> docs instead of/additionally to having it as a sample.
>>>>
>>>> I don't think we should duplicate the code. As long as the link works, I
>>>> think having it separately is fine.
>>>
>>> When I'm coding in my editor and read some docs directly in it, it often
>>> is annoying to find a link, because then I have to open the docs in my
>>> web-browser.
>>> I understand that you don't want to duplicate the code (and it also is a
>>> bit too much for a short example), so how about having a simpler
>>> example? Maybe with only a single operation that has no associated data
>>> (use `()`)?
>>
>> Sure, we can do that.
> 
> Actually, I am having some problems making the inline example compile.
> `configfs_attrs!` references `THIS_MODULE`, which is not present for
> doctests.

I see. Does KUNIT have a THIS_MODULE that we could use? If so, we might
want to make that accessible in the test harness. Does anyone know more
about this?

> I'll add the example but mark it `ignore` ?

Sounds good. If we end up adding a THIS_MODULE to the test harness, we
can remove the ignore later.

---
Cheers,
Benno

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Danilo Krummrich]

On Mon, Feb 17, 2025 at 12:08:22PM +0100, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
> 
> > On 07.02.25 15:41, Andreas Hindborg wrote:
> >> +//!
> >> +//! C header: [`include/linux/configfs.h`](srctree/include/linux/configfs.h)
> >> +//!
> >> +//! [C documentation]: srctree/Documentation/filesystems/configfs.rst
> >> +//! [rust_configfs.rs]: srctree/samples/rust/rust_configfs.rs
> >> +
> >> +use crate::alloc::flags;
> >> +use crate::container_of;
> >> +use crate::page::PAGE_SIZE;
> >> +use crate::prelude::*;
> >> +use crate::str::CString;
> >> +use crate::sync::Arc;
> >> +use crate::types::ForeignOwnable;
> >> +use crate::types::Opaque;
> >> +use core::cell::UnsafeCell;
> >> +use core::marker::PhantomData;
> >> +use core::ptr::addr_of;
> >> +use core::ptr::addr_of_mut;
> >
> > I usually would import this like so:
> >
> >     use crate::{
> >         alloc::flags,
> >         container_of,
> >         page::PAGE_SIZE,
> >         prelude::*,
> >         str::CString,
> >         sync::Arc,
> >         types::{ForeignOwnable, Opaque},
> >     };
> >     use core::{
> >         cell::UnsafeCell,
> >         marker::PhantomData,
> >         ptr::{addr_of, addr_of_mut},
> >     };
> >
> > To me this is more readable.
> 
> I disagree with that. I don't think what you suggest is easier to read,
> and it is much more difficult to work with when rebasing and merging
> things.

I have to agree that it is more difficult to work with. So far I used the style
as proposed by Benno, but it creates unncessary big and difficult to review
diffs when rustfmt moves things from a horizontal list to a vertical one and
vice versa.

> >> +    /// Implementations can use this method to do house keeping before
> >> +    /// `configfs` drops its reference to `Child`.
> >> +    fn drop_item(
> >
> > `drop` doesn't really fit here, I think something like `unlink_item`
> > fits better, since the child isn't actually dropped after this function
> > returns.
> 
> Yea, I know. But the function is called `drop_item` on the C side of
> things. Usually we keep the C names.

I agree C names should be kept as possible.

To me it seems obvious from the context, but maybe it'd still makes sense to add
a brief note that this callback's name is not related to 'drop' in the sense of
Rust?

Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

[Andreas Hindborg]

"Danilo Krummrich" <dakr@kernel.org> writes:

> On Mon, Feb 17, 2025 at 12:08:22PM +0100, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>
>> > On 07.02.25 15:41, Andreas Hindborg wrote:

[...]

>> >> +    /// Implementations can use this method to do house keeping before
>> >> +    /// `configfs` drops its reference to `Child`.
>> >> +    fn drop_item(
>> >
>> > `drop` doesn't really fit here, I think something like `unlink_item`
>> > fits better, since the child isn't actually dropped after this function
>> > returns.
>>
>> Yea, I know. But the function is called `drop_item` on the C side of
>> things. Usually we keep the C names.
>
> I agree C names should be kept as possible.
>
> To me it seems obvious from the context, but maybe it'd still makes sense to add
> a brief note that this callback's name is not related to 'drop' in the sense of
> Rust?

Yes, good idea. I'll get than in for v4.


Best regards,
Andreas Hindborg

[PATCH v2 3/3] MAINTAINERS: add entry for configfs Rust abstractions

[Andreas Hindborg]

Update MAINTAINERS with entry for Rust configfs abstractions.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 896a307fa0654..9b4d5c12eb438 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5870,6 +5870,13 @@ F:	fs/configfs/
 F:	include/linux/configfs.h
 F:	samples/configfs/
 
+CONFIGFS [RUST]
+M:	Andreas Hindborg <a.hindborg@kernel.org>
+L:	rust-for-linux@vger.kernel.org
+S:	Supported
+F:	rust/kernel/configfs.rs
+F:	samples/rust/rust_configfs.rs
+
 CONGATEC BOARD CONTROLLER MFD DRIVER
 M:	Thomas Richard <thomas.richard@bootlin.com>
 S:	Maintained

-- 
2.47.0