[PATCH v4 0/4] rust: configfs abstractions

[PATCH v4 0/4] 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 v4:
- Fix a build issue by depending on v18 of "rust: types: add `ForeignOwnable::PointedTo`"
- Link to v3: https://lore.kernel.org/r/20250218-configfs-v3-0-0e40c0778187@kernel.org

Changes in v3:
- Allow trailing commas in invocation of `configfs_attrs!`.
- Use a more suitable C initialization function when initializing `Subsystem`.
- Split sample into separate patch.
- Add an inline example.

The remaining changes in this version are style fixes, documentation
improvements and typo fixes. They are enumerated below:
- Consolidate `paste` macro calls.
- Do not hard code page size in example.
- Remove prefix of `c_str!` in sample.
- Use a more descriptive variable name in `into_foreign`.
- Improve code formatting in macros invocations.
- Add comment related to null terminator in `configfs_attrs!`
- Move attributes below docstrings.
- Remove a rogue todo.
- Remove trait bound from struct definition `GroupOperationsVTable`.
- Remove `as _` casts.
- Remove `GroupOprations::Parent` associated type.
- General documentation improvements.
- Explicitly use `ArcBorrow` for `drop_item` parameter type.
- Add a comment describing expansion to a call to `Attribute::add`.
- Add a comment explaining bound check in `Attribute::add`.
- Link to v2: https://lore.kernel.org/r/20250207-configfs-v2-0-f7a60b24d38e@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 (4):
      rust: sync: change `<Arc<T> as ForeignOwnable>::PointedTo` to `T`
      rust: configfs: introduce rust support for configfs
      rust: configfs: add a sample demonstrating configfs usage
      MAINTAINERS: add entry for configfs Rust abstractions

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

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

[PATCH v4 1/4] 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 dfe4abf82c25..3d77a31e116f 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 this = 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!((*this).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 v4 1/4] rust: sync: change `<Arc<T> as ForeignOwnable>::PointedTo` to `T`

[Daniel Almeida]

Hi Andreas,

> On 24 Feb 2025, at 10:21, 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`.
> 
> 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 dfe4abf82c25..3d77a31e116f 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 this = 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!((*this).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
> 
> 


Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

[PATCH v4 2/4] 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         | 938 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   2 +
 4 files changed, 946 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55354e4dec14..d115a770306f 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 06575553eda5..3e9b910a88e9 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 000000000000..fd19ddd209b1
--- /dev/null
+++ b/rust/kernel/configfs.rs
@@ -0,0 +1,938 @@
+// 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)
+//!
+//! # Example
+//!
+//! ```ignore
+//! use kernel::alloc::flags;
+//! use kernel::c_str;
+//! use kernel::configfs_attrs;
+//! use kernel::configfs;
+//! use kernel::new_mutex;
+//! use kernel::page::PAGE_SIZE;
+//! use kernel::sync::Mutex;
+//! use kernel::ThisModule;
+//!
+//! #[pin_data]
+//! struct RustConfigfs {
+//!     #[pin]
+//!     config: configfs::Subsystem<Configuration>,
+//! }
+//!
+//! 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,
+//!             attributes: [
+//!                 message: 0,
+//!                 bar: 1,
+//!             ],
+//!         };
+//!
+//!         try_pin_init!(Self {
+//!             config <- configfs::Subsystem::new(
+//!                 c_str!("rust_configfs"), item_type, Configuration::new()
+//!             ),
+//!         })
+//!     }
+//! }
+//!
+//! #[pin_data]
+//! struct Configuration {
+//!     message: &'static CStr,
+//!     #[pin]
+//!     bar: Mutex<(KBox<[u8; PAGE_SIZE]>, 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; PAGE_SIZE], flags::GFP_KERNEL)?, 0)),
+//!         })
+//!     }
+//! }
+//!
+//! #[vtable]
+//! impl configfs::AttributeOperations<0> for Configuration {
+//!     type Data = Configuration;
+//!
+//!     fn show(container: &Configuration, page: &mut [u8; PAGE_SIZE]) -> 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; PAGE_SIZE]) -> 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(())
+//!     }
+//! }
+//! ```
+//!
+//! [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::sync::ArcBorrow;
+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: We initialized the required fields of `place.group` above.
+                    unsafe {
+                        bindings::config_group_init_type_name(
+                            &mut (*place.get()).su_group,
+                            name.as_ptr(),
+                            item_type.as_ptr(),
+                        )
+                    };
+
+                    // 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.cast(), 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 {
+    // 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)>);
+
+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<Child>`.
+        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 child data object type.
+    ///
+    /// This group will create subgroups (subdirectories) backed by this kind of
+    /// object.
+    type Child: 'static;
+
+    /// Creates a new subgroup.
+    ///
+    /// 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
+    /// return an initializer of a `Group<Self::Child>`. To prevent creation,
+    /// return a suitable error.
+    fn make_group(&self, name: &CStr) -> Result<impl PinInit<Group<Self::Child>, Error>>;
+
+    /// Prepares the group for removal from configfs.
+    ///
+    /// 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(&self, _child: ArcBorrow<'_, Group<Self::Child>>) {
+        kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
+    }
+}
+
+/// A `configfs` attribute.
+///
+/// An attribute appears 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;
+
+    /// Renders the value of an attribute.
+    ///
+    /// 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>;
+
+    /// Stores the value of an attribute.
+    ///
+    /// 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.
+    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> {
+    /// # Safety
+    ///
+    /// This function can only be called by the `configfs_attrs`
+    /// macro.
+    #[doc(hidden)]
+    pub const unsafe fn new() -> Self {
+        Self(UnsafeCell::new([core::ptr::null_mut(); N]), PhantomData)
+    }
+
+    /// # Safety
+    ///
+    /// This function can only be called by the `configfs_attrs`
+    /// macro.
+    #[doc(hidden)]
+    pub const unsafe fn add<
+        const I: usize,
+        const ID: u64,
+        O: AttributeOperations<ID, Data = Data>,
+    >(
+        &'static self,
+        attribute: &'static Attribute<ID, O, Data>,
+    ) {
+        // We need a space at the end of our list for a null terminator.
+        if I >= N - 1 {
+            kernel::build_error!("Invalid attribute index");
+        }
+
+        // SAFETY: This function is only called through the `configfs_attrs`
+        // macro. 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;
+                // The following macro text expands to a call to `Attribute::add`.
+                // SAFETY: We are expanding `configfs_attrs`.
+                unsafe {
+                    $crate::macros::paste!(
+                        [< $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!{
+            {
+                $(
+                    // 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)))
+                            };
+                )*
+
+
+                // We need space for a null terminator.
+                const N: usize = $cnt + 1usize;
+
+                // SAFETY: We are expanding `configfs_attrs`.
+                static [< $data:upper _ATTRS >]:
+                $crate::configfs::AttributeList<N, $data> =
+                    unsafe { $crate::configfs::AttributeList::new() };
+
+                $($assign)*
+
+                $(
+                    const [<$no_child:upper>]: bool = true;
+
+                    static [< $data:upper _TPE >] : $crate::configfs::ItemType<$container, $data>  =
+                        $crate::configfs::ItemType::<$container, $data>::new::<N>(
+                            &THIS_MODULE, &[<$ data:upper _ATTRS >]
+                        );
+                )?
+
+                $(
+                    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 >]
+                            );
+                )?
+
+                & [< $data:upper _TPE >]
+            }
+        }
+    };
+
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..ec84653ab8c7 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;

-- 
2.47.0

Re: [PATCH v4 2/4] rust: configfs: introduce rust support for configfs

[Daniel Almeida]

Hi Andreas,

> On 24 Feb 2025, at 10:21, Andreas Hindborg <a.hindborg@kernel.org> 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

Where?

> 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         | 938 ++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs              |   2 +
> 4 files changed, 946 insertions(+)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 55354e4dec14..d115a770306f 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 06575553eda5..3e9b910a88e9 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 000000000000..fd19ddd209b1
> --- /dev/null
> +++ b/rust/kernel/configfs.rs
> @@ -0,0 +1,938 @@
> +// 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)
> +//!
> +//! # Example
> +//!
> +//! ```ignore
> +//! use kernel::alloc::flags;
> +//! use kernel::c_str;
> +//! use kernel::configfs_attrs;
> +//! use kernel::configfs;
> +//! use kernel::new_mutex;
> +//! use kernel::page::PAGE_SIZE;
> +//! use kernel::sync::Mutex;
> +//! use kernel::ThisModule;
> +//!
> +//! #[pin_data]
> +//! struct RustConfigfs {
> +//!     #[pin]
> +//!     config: configfs::Subsystem<Configuration>,
> +//! }
> +//!
> +//! 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,
> +//!             attributes: [
> +//!                 message: 0,
> +//!                 bar: 1,
> +//!             ],
> +//!         };
> +//!
> +//!         try_pin_init!(Self {
> +//!             config <- configfs::Subsystem::new(
> +//!                 c_str!("rust_configfs"), item_type, Configuration::new()
> +//!             ),
> +//!         })
> +//!     }
> +//! }
> +//!
> +//! #[pin_data]
> +//! struct Configuration {
> +//!     message: &'static CStr,
> +//!     #[pin]
> +//!     bar: Mutex<(KBox<[u8; PAGE_SIZE]>, 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; PAGE_SIZE], flags::GFP_KERNEL)?, 0)),
> +//!         })
> +//!     }
> +//! }
> +//!
> +//! #[vtable]
> +//! impl configfs::AttributeOperations<0> for Configuration {
> +//!     type Data = Configuration;
> +//!
> +//!     fn show(container: &Configuration, page: &mut [u8; PAGE_SIZE]) -> 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; PAGE_SIZE]) -> 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(())
> +//!     }
> +//! }
> +//! ```
> +//!
> +//! [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::sync::ArcBorrow;
> +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: We initialized the required fields of `place.group` above.
> +                    unsafe {
> +                        bindings::config_group_init_type_name(
> +                            &mut (*place.get()).su_group,
> +                            name.as_ptr(),
> +                            item_type.as_ptr(),
> +                        )
> +                    };
> +
> +                    // 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,

Is it me or this can be simply &CStr? `config_item_set_name` either copies into an internal
buffer or allocates, so I see no reason to pass an owned type here.

> +        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.cast(), 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 {
> +    // 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>`.

This phrase is confusing.

> +        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)>);
> +
> +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) };

nit: container_of is already in scope

> +        // SAFETY: By function safety requirements, `c_child_group_ptr` is
> +        // embedded within a `Group<Child>`.
> +        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 child data object type.
> +    ///
> +    /// This group will create subgroups (subdirectories) backed by this kind of
> +    /// object.
> +    type Child: 'static;
> +
> +    /// Creates a new subgroup.
> +    ///
> +    /// 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
> +    /// return an initializer of a `Group<Self::Child>`. To prevent creation,
> +    /// return a suitable error.
> +    fn make_group(&self, name: &CStr) -> Result<impl PinInit<Group<Self::Child>, Error>>;
> +
> +    /// Prepares the group for removal from configfs.
> +    ///
> +    /// 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(&self, _child: ArcBorrow<'_, Group<Self::Child>>) {
> +        kernel::build_error!(kernel::error::VTABLE_DEFAULT_ERROR)
> +    }
> +}
> +
> +/// A `configfs` attribute.
> +///
> +/// An attribute appears 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> {

The first thing I thought here is “what is O?!"

> +    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>,

I recommend renaming “O" into something that denotes this bound better.

It can be terse as appropriate, but simply “O” is a bit *too terse* .

> +{
> +    /// # 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,

Shouldn’t `ca_mode` be somehow taken as an input? Also, can we get rid of the literal here?

> +                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> {

I assume that this ID parameter is to allow for multiple implementations, like we currently do
for the Workqueue? If so, can you mention this in the docs?

> +    /// The type of the object that contains the field that is backing the
> +    /// attribute for this operation.
> +    type Data;
> +
> +    /// Renders the value of an attribute.
> +    ///
> +    /// 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>;
> +
> +    /// Stores the value of an attribute.
> +    ///
> +    /// 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.
> +    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]>,

For the sake of maintainability, can you provide some docs on this type? 

For example, what is the c_void here?

> +    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> {
> +    /// # Safety
> +    ///
> +    /// This function can only be called by the `configfs_attrs`
> +    /// macro.

Well, a pub function can be called from anywhere in the crate. Maybe `should` would be more
appropriate? I assume you want to tell people to not call this directly. Otherwise I’m left wondering
whether there is some magic going on that I’m unaware of to make what you said possible.

> +    #[doc(hidden)]
> +    pub const unsafe fn new() -> Self {
> +        Self(UnsafeCell::new([core::ptr::null_mut(); N]), PhantomData)
> +    }
> +
> +    /// # Safety
> +    ///
> +    /// This function can only be called by the `configfs_attrs`
> +    /// macro.
> +    #[doc(hidden)]
> +    pub const unsafe fn add<
> +        const I: usize,
> +        const ID: u64,
> +        O: AttributeOperations<ID, Data = Data>,
> +    >(
> +        &'static self,
> +        attribute: &'static Attribute<ID, O, Data>,
> +    ) {

Can you convert this into a where clause? IMHO it will be much easier to read, given how the
function args got formatted here.


> +        // We need a space at the end of our list for a null terminator.
> +        if I >= N - 1 {
> +            kernel::build_error!("Invalid attribute index");
> +        }
> +
> +        // SAFETY: This function is only called through the `configfs_attrs`
> +        // macro. 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;
> +                // The following macro text expands to a call to `Attribute::add`.
> +                // SAFETY: We are expanding `configfs_attrs`.
> +                unsafe {
> +                    $crate::macros::paste!(
> +                        [< $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!{
> +            {
> +                $(
> +                    // 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)))
> +                            };
> +                )*
> +
> +
> +                // We need space for a null terminator.
> +                const N: usize = $cnt + 1usize;
> +
> +                // SAFETY: We are expanding `configfs_attrs`.
> +                static [< $data:upper _ATTRS >]:
> +                $crate::configfs::AttributeList<N, $data> =
> +                    unsafe { $crate::configfs::AttributeList::new() };
> +
> +                $($assign)*
> +
> +                $(
> +                    const [<$no_child:upper>]: bool = true;
> +
> +                    static [< $data:upper _TPE >] : $crate::configfs::ItemType<$container, $data>  =
> +                        $crate::configfs::ItemType::<$container, $data>::new::<N>(
> +                            &THIS_MODULE, &[<$ data:upper _ATTRS >]
> +                        );
> +                )?
> +
> +                $(
> +                    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 >]
> +                            );
> +                )?
> +
> +                & [< $data:upper _TPE >]
> +            }
> +        }
> +    };
> +
> +}

Andreas, just a suggestion for the sake of maintainability, can you add some docs to this macro?

I think you’ll agree that there is a *lot* going on here.

In fact, this patch is a bit complex, so kudos on making it work in a very simple way for the callers.

> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911..ec84653ab8c7 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;
> 
> -- 
> 2.47.0
> 
> 

I’ll probably do another pass here later, there’s a lot to unpack.

— Daniel

Re: [PATCH v4 2/4] rust: configfs: introduce rust support for configfs

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Hi Andreas,
>
>> On 24 Feb 2025, at 10:21, Andreas Hindborg <a.hindborg@kernel.org> 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
>
> Where?

Not any more! It was merged with `IdArray`, and apparently it did not
make a conflict when I rebased. I'll drop this paragraph.

[...]

>> +
>> +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,
>
> Is it me or this can be simply &CStr? `config_item_set_name` either copies into an internal
> buffer or allocates, so I see no reason to pass an owned type here.

This function returns an initializer that would be bound by the lifetime
of the reference you pass in. That in turn has a very negative effect on
the ergonomics of the function, as it would limit the places you can
call it and still satisfy lifetime requirements.

We could pass in a borrow and create an owned instance from the borrow,
then move the owned value into the initializer. But I think taking an
owned parameter in the first place is better.

[...]

>> +
>> +/// # 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 {
>> +    // 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>`.
>
> This phrase is confusing.

I am not sure how to rephrase it to be less confusing. The pointer is
the thing returned from `make_group`. `make_group` would only return a
pointer into a `Group<Parent>`.


[...]

>> +    /// # 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) };
>
> nit: container_of is already in scope

Thanks!

[...]

>> +
>> +/// A `configfs` attribute.
>> +///
>> +/// An attribute appears 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> {
>
> The first thing I thought here is “what is O?!"

Would you prefer a rename to "Operations"? I was told to not add trait
bounds on the struct, because it is not necessary.

>
>> +    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>,
>
> I recommend renaming “O" into something that denotes this bound better.
>
> It can be terse as appropriate, but simply “O” is a bit *too terse* .

Right, I agree. However, other reviewers have argued negatively about
using 4 letters for the "Data" bound, since generic type parameters are
often just single capital letters.

I will draw attention to the the Rust API guidelines[1] which allow
`UpperCamelCase` names .

@Benno, you had a different opinion, can you weigh in?

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

[...]

>> +    /// 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 _,

Let's get rid of this `as _` cast.

>> +                ca_owner: core::ptr::null_mut(),
>> +                ca_mode: 0o660,
>
> Shouldn’t `ca_mode` be somehow taken as an input? Also, can we get rid of the literal here?

That would be a nice addition for a follow up series.

>
>> +                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> {
>
> I assume that this ID parameter is to allow for multiple implementations, like we currently do
> for the Workqueue? If so, can you mention this in the docs?

Absolutely.

>
>> +    /// The type of the object that contains the field that is backing the
>> +    /// attribute for this operation.
>> +    type Data;
>> +
>> +    /// Renders the value of an attribute.
>> +    ///
>> +    /// 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>;
>> +
>> +    /// Stores the value of an attribute.
>> +    ///
>> +    /// 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.
>> +    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]>,
>
> For the sake of maintainability, can you provide some docs on this type?
>
> For example, what is the c_void here?

As per the docstring above, is a list of `Attribute`. I can expand it a bit:

 /// 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.
+///
+/// # Note
+///
+/// This type is constructed statically at compile time and is by the
+/// [`kernel::configfs_attrs`] macro.
 #[repr(transparent)]
 pub struct AttributeList<const N: usize, Data>(
+    /// Null terminated Array of pointers to `Attribute`. The type is `c_void`
+    /// to conform to the C API.
     UnsafeCell<[*mut kernel::ffi::c_void; N]>,
     PhantomData<Data>,
 );


Does that work?

>
>> +    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> {
>> +    /// # Safety
>> +    ///
>> +    /// This function can only be called by the `configfs_attrs`
>> +    /// macro.
>
> Well, a pub function can be called from anywhere in the crate. Maybe `should` would be more
> appropriate? I assume you want to tell people to not call this
> directly.

Yes, it is in the safety requirements section of an unsafe function. But
you are right "can" is not the right word. I think "must" is appropriate.

> Otherwise I’m left wondering
> whether there is some magic going on that I’m unaware of to make what you said possible.

No magic, it is a prerequisite for calling, something you would have to
justify in the safety comment at the call site.

>
>> +    #[doc(hidden)]
>> +    pub const unsafe fn new() -> Self {
>> +        Self(UnsafeCell::new([core::ptr::null_mut(); N]), PhantomData)
>> +    }
>> +
>> +    /// # Safety
>> +    ///
>> +    /// This function can only be called by the `configfs_attrs`
>> +    /// macro.
>> +    #[doc(hidden)]
>> +    pub const unsafe fn add<
>> +        const I: usize,
>> +        const ID: u64,
>> +        O: AttributeOperations<ID, Data = Data>,
>> +    >(
>> +        &'static self,
>> +        attribute: &'static Attribute<ID, O, Data>,
>> +    ) {
>
> Can you convert this into a where clause? IMHO it will be much easier to read, given how the
> function args got formatted here.

Sure. Const types cannot go in where clause, but formatting is better
with the bound on "O" moved.

[...]

>> +                $(
>> +                    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 >]
>> +                            );
>> +                )?
>> +
>> +                & [< $data:upper _TPE >]
>> +            }
>> +        }
>> +    };
>> +
>> +}
>
> Andreas, just a suggestion for the sake of maintainability, can you add some docs to this macro?
>
> I think you’ll agree that there is a *lot* going on here.
>
> In fact, this patch is a bit complex, so kudos on making it work in a very simple way for the callers.

I could write a small wall of text and do some step by step expansion of
the macro. But I would rather not, since we are soon (TM) going to have
`syn` and `quote`, and all this horror will go away.

One way to help parsing this mess, is using the "expand macro" feature
of `rust-analyzer` in an editor and looking at the expanded code.

>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 496ed32b0911..ec84653ab8c7 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;
>>
>> --
>> 2.47.0
>>
>>
>
> I’ll probably do another pass here later, there’s a lot to unpack.

Appreciate it! Thanks for the comments!


Best regards,
Andreas Hindborg

Re: [PATCH v4 2/4] rust: configfs: introduce rust support for configfs

[Daniel Almeida]

> On 25 Feb 2025, at 07:11, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> "Daniel Almeida" <daniel.almeida@collabora.com> writes:
> 
>> Hi Andreas,
>> 
>>> On 24 Feb 2025, at 10:21, Andreas Hindborg <a.hindborg@kernel.org> 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
>> 
>> Where?
> 
> Not any more! It was merged with `IdArray`, and apparently it did not
> make a conflict when I rebased. I'll drop this paragraph.
> 
> [...]
> 
>>> +
>>> +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,
>> 
>> Is it me or this can be simply &CStr? `config_item_set_name` either copies into an internal
>> buffer or allocates, so I see no reason to pass an owned type here.
> 
> This function returns an initializer that would be bound by the lifetime
> of the reference you pass in. That in turn has a very negative effect on
> the ergonomics of the function, as it would limit the places you can
> call it and still satisfy lifetime requirements.
> 
> We could pass in a borrow and create an owned instance from the borrow,
> then move the owned value into the initializer. But I think taking an
> owned parameter in the first place is better.


Do you mean that the CString is used to guarantee that the string is alive when the initializer
actually runs?

If so, I did not consider that. Please disregard this comment then.

> 
> [...]
> 
>>> +
>>> +/// # 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 {
>>> +    // 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>`.
>> 
>> This phrase is confusing.
> 
> I am not sure how to rephrase it to be less confusing. The pointer is
> the thing returned from `make_group`. `make_group` would only return a
> pointer into a `Group<Parent>`.

The problem is this: "that we passed as a return value in from”, to pass something as a return value
is already hard to parse, and when you reach the “in from” part, it becomes even harder.

Just say a variation of what you said above, that is perfectly understandable.

What about:

```

`this` is a pointer to a `bindings::config_group` that was returned from a call to `make_group`. The pointer is known
to be embedded within a `Group<Parent>`.

```


> 
> 
> [...]
> 
>>> +    /// # 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) };
>> 
>> nit: container_of is already in scope
> 
> Thanks!
> 
> [...]
> 
>>> +
>>> +/// A `configfs` attribute.
>>> +///
>>> +/// An attribute appears 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> {
>> 
>> The first thing I thought here is “what is O?!"
> 
> Would you prefer a rename to "Operations"? I was told to not add trait
> bounds on the struct, because it is not necessary.

I’d prefer Operations, yes.

> 
>> 
>>> +    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>,
>> 
>> I recommend renaming “O" into something that denotes this bound better.
>> 
>> It can be terse as appropriate, but simply “O” is a bit *too terse* .
> 
> Right, I agree. However, other reviewers have argued negatively about
> using 4 letters for the "Data" bound, since generic type parameters are
> often just single capital letters.

This is a convention, that is all. As a person trying to make sense of this code, Data was *much* 
more informative than T or U, or etc. Same for `Parent`. 

If you see things like:

```
impl<Data> Subsystem<Data>
```

You already know it’s a type parameter. If you click on Data, assuming LSP support, it will also tell
you that.

This code is already a bit hard to follow as is, let’s make sure that the types actually aid in its
comprehension and not the other way around, please.

> 
> I will draw attention to the the Rust API guidelines[1] which allow
> `UpperCamelCase` names .
> 
> @Benno, you had a different opinion, can you weigh in?
> 
> [1] https://rust-lang.github.io/api-guidelines/naming.html
> 
> [...]
> 
>>> +    /// 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 _,
> 
> Let's get rid of this `as _` cast.
> 
>>> +                ca_owner: core::ptr::null_mut(),
>>> +                ca_mode: 0o660,
>> 
>> Shouldn’t `ca_mode` be somehow taken as an input? Also, can we get rid of the literal here?
> 
> That would be a nice addition for a follow up series.
> 
>> 
>>> +                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> {
>> 
>> I assume that this ID parameter is to allow for multiple implementations, like we currently do
>> for the Workqueue? If so, can you mention this in the docs?
> 
> Absolutely.
> 
>> 
>>> +    /// The type of the object that contains the field that is backing the
>>> +    /// attribute for this operation.
>>> +    type Data;
>>> +
>>> +    /// Renders the value of an attribute.
>>> +    ///
>>> +    /// 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>;
>>> +
>>> +    /// Stores the value of an attribute.
>>> +    ///
>>> +    /// 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.
>>> +    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]>,
>> 
>> For the sake of maintainability, can you provide some docs on this type?
>> 
>> For example, what is the c_void here?
> 
> As per the docstring above, is a list of `Attribute`. I can expand it a bit:
> 
> /// 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.
> +///
> +/// # Note
> +///
> +/// This type is constructed statically at compile time and is by the
> +/// [`kernel::configfs_attrs`] macro.
> #[repr(transparent)]
> pub struct AttributeList<const N: usize, Data>(
> +    /// Null terminated Array of pointers to `Attribute`. The type is `c_void`
> +    /// to conform to the C API.

Yes this is much better :)

>     UnsafeCell<[*mut kernel::ffi::c_void; N]>,
>     PhantomData<Data>,
> );
> 
> 
> Does that work?
> 
>> 
>>> +    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> {
>>> +    /// # Safety
>>> +    ///
>>> +    /// This function can only be called by the `configfs_attrs`
>>> +    /// macro.
>> 
>> Well, a pub function can be called from anywhere in the crate. Maybe `should` would be more
>> appropriate? I assume you want to tell people to not call this
>> directly.
> 
> Yes, it is in the safety requirements section of an unsafe function. But
> you are right "can" is not the right word. I think "must" is appropriate.

Ack.

> 
>> Otherwise I’m left wondering
>> whether there is some magic going on that I’m unaware of to make what you said possible.
> 
> No magic, it is a prerequisite for calling, something you would have to
> justify in the safety comment at the call site.

Ack.

> 
>> 
>>> +    #[doc(hidden)]
>>> +    pub const unsafe fn new() -> Self {
>>> +        Self(UnsafeCell::new([core::ptr::null_mut(); N]), PhantomData)
>>> +    }
>>> +
>>> +    /// # Safety
>>> +    ///
>>> +    /// This function can only be called by the `configfs_attrs`
>>> +    /// macro.
>>> +    #[doc(hidden)]
>>> +    pub const unsafe fn add<
>>> +        const I: usize,
>>> +        const ID: u64,
>>> +        O: AttributeOperations<ID, Data = Data>,
>>> +    >(
>>> +        &'static self,
>>> +        attribute: &'static Attribute<ID, O, Data>,
>>> +    ) {
>> 
>> Can you convert this into a where clause? IMHO it will be much easier to read, given how the
>> function args got formatted here.
> 
> Sure. Const types cannot go in where clause, but formatting is better
> with the bound on "O" moved.

Oh, I see. My main issue here is that the formatting makes it hard to see that this is a function,
and that the things enclosed in parenthesis are the actual arguments. Our eyes really have to
scan for the `fn` token here.


> 
> [...]
> 
>>> +                $(
>>> +                    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 >]
>>> +                            );
>>> +                )?
>>> +
>>> +                & [< $data:upper _TPE >]
>>> +            }
>>> +        }
>>> +    };
>>> +
>>> +}
>> 
>> Andreas, just a suggestion for the sake of maintainability, can you add some docs to this macro?
>> 
>> I think you’ll agree that there is a *lot* going on here.
>> 
>> In fact, this patch is a bit complex, so kudos on making it work in a very simple way for the callers.
> 
> I could write a small wall of text and do some step by step expansion of
> the macro. But I would rather not, since we are soon (TM) going to have
> `syn` and `quote`, and all this horror will go away.

Ok, your call.

> 
> One way to help parsing this mess, is using the "expand macro" feature
> of `rust-analyzer` in an editor and looking at the expanded code.

I wonder if that can’t be pasted inline with the docs for a trivial use of the macro?

I will take the verbosity *any day* over trying to figure out what is going on
manually here.

Or you can wait for syn and quote, as you said, that’s OK.

By the way, with the somewhat limited support for rust-analyzer in the kernel,
I wonder whether that is in fact possible. Things are much more restricted for
non-Cargo projects.


> 
>> 
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index 496ed32b0911..ec84653ab8c7 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;
>>> 
>>> --
>>> 2.47.0
>>> 
>>> 
>> 
>> I’ll probably do another pass here later, there’s a lot to unpack.
> 
> Appreciate it! Thanks for the comments!
> 
> 
> Best regards,
> Andreas Hindborg

— Daniel

Re: [PATCH v4 2/4] rust: configfs: introduce rust support for configfs

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

[...]

>>>> +
>>>> +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,
>>>
>>> Is it me or this can be simply &CStr? `config_item_set_name` either copies into an internal
>>> buffer or allocates, so I see no reason to pass an owned type here.
>>
>> This function returns an initializer that would be bound by the lifetime
>> of the reference you pass in. That in turn has a very negative effect on
>> the ergonomics of the function, as it would limit the places you can
>> call it and still satisfy lifetime requirements.
>>
>> We could pass in a borrow and create an owned instance from the borrow,
>> then move the owned value into the initializer. But I think taking an
>> owned parameter in the first place is better.
>
>
> Do you mean that the CString is used to guarantee that the string is alive when the initializer
> actually runs?

Exactly.

>
> If so, I did not consider that. Please disregard this comment then.

OK.

>
>>
>> [...]
>>
>>>> +
>>>> +/// # 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 {
>>>> +    // 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>`.
>>>
>>> This phrase is confusing.
>>
>> I am not sure how to rephrase it to be less confusing. The pointer is
>> the thing returned from `make_group`. `make_group` would only return a
>> pointer into a `Group<Parent>`.
>
> The problem is this: "that we passed as a return value in from”, to pass something as a return value
> is already hard to parse, and when you reach the “in from” part, it becomes even harder.
>
> Just say a variation of what you said above, that is perfectly understandable.
>
> What about:
>
> ```
>
> `this` is a pointer to a `bindings::config_group` that was returned from a call to `make_group`. The pointer is known
> to be embedded within a `Group<Parent>`.
>
> ```

How is this:

        // SAFETY: By C API contact,`this` was returned from a call to
        // `make_group`. The pointer is known to be embedded within a
        // `Group<Parent>`.

[...]

>>>> +
>>>> +/// A `configfs` attribute.
>>>> +///
>>>> +/// An attribute appears 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> {
>>>
>>> The first thing I thought here is “what is O?!"
>>
>> Would you prefer a rename to "Operations"? I was told to not add trait
>> bounds on the struct, because it is not necessary.
>
> I’d prefer Operations, yes.
>
>>
>>>
>>>> +    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>,
>>>
>>> I recommend renaming “O" into something that denotes this bound better.
>>>
>>> It can be terse as appropriate, but simply “O” is a bit *too terse* .
>>
>> Right, I agree. However, other reviewers have argued negatively about
>> using 4 letters for the "Data" bound, since generic type parameters are
>> often just single capital letters.
>
> This is a convention, that is all. As a person trying to make sense of this code, Data was *much*
> more informative than T or U, or etc. Same for `Parent`.
>
> If you see things like:
>
> ```
> impl<Data> Subsystem<Data>
> ```
>
> You already know it’s a type parameter. If you click on Data, assuming LSP support, it will also tell
> you that.
>
> This code is already a bit hard to follow as is, let’s make sure that the types actually aid in its
> comprehension and not the other way around, please.

I would agree. I believe Benno was arguing that it is difficult to see
what identifiers are generic type parameters when they are words rather
than letters.

[...]

>>>> +/// 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]>,
>>>
>>> For the sake of maintainability, can you provide some docs on this type?
>>>
>>> For example, what is the c_void here?
>>
>> As per the docstring above, is a list of `Attribute`. I can expand it a bit:
>>
>> /// 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.
>> +///
>> +/// # Note
>> +///
>> +/// This type is constructed statically at compile time and is by the
>> +/// [`kernel::configfs_attrs`] macro.
>> #[repr(transparent)]
>> pub struct AttributeList<const N: usize, Data>(
>> +    /// Null terminated Array of pointers to `Attribute`. The type is `c_void`
>> +    /// to conform to the C API.
>
> Yes this is much better :)

Let's go with that then.

[...]

>>>> +                $(
>>>> +                    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 >]
>>>> +                            );
>>>> +                )?
>>>> +
>>>> +                & [< $data:upper _TPE >]
>>>> +            }
>>>> +        }
>>>> +    };
>>>> +
>>>> +}
>>>
>>> Andreas, just a suggestion for the sake of maintainability, can you add some docs to this macro?
>>>
>>> I think you’ll agree that there is a *lot* going on here.
>>>
>>> In fact, this patch is a bit complex, so kudos on making it work in a very simple way for the callers.
>>
>> I could write a small wall of text and do some step by step expansion of
>> the macro. But I would rather not, since we are soon (TM) going to have
>> `syn` and `quote`, and all this horror will go away.
>
> Ok, your call.
>
>>
>> One way to help parsing this mess, is using the "expand macro" feature
>> of `rust-analyzer` in an editor and looking at the expanded code.
>
> I wonder if that can’t be pasted inline with the docs for a trivial use of the macro?
>
> I will take the verbosity *any day* over trying to figure out what is going on
> manually here.
>
> Or you can wait for syn and quote, as you said, that’s OK.
>
> By the way, with the somewhat limited support for rust-analyzer in the kernel,
> I wonder whether that is in fact possible. Things are much more restricted for
> non-Cargo projects.

It is working for me at least. I build the rust-project.json with `make
rust-analyzer`. I build out of tree, so I have to move the file to the
source tree manually. No additional steps required.


Best regards,
Andreas Hindborg

Re: [PATCH v4 2/4] rust: configfs: introduce rust support for configfs

[Daniel Almeida]

> 
>> 
>>> 
>>> [...]
>>> 
>>>>> +
>>>>> +/// # 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 {
>>>>> +    // 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>`.
>>>> 
>>>> This phrase is confusing.
>>> 
>>> I am not sure how to rephrase it to be less confusing. The pointer is
>>> the thing returned from `make_group`. `make_group` would only return a
>>> pointer into a `Group<Parent>`.
>> 
>> The problem is this: "that we passed as a return value in from”, to pass something as a return value
>> is already hard to parse, and when you reach the “in from” part, it becomes even harder.
>> 
>> Just say a variation of what you said above, that is perfectly understandable.
>> 
>> What about:
>> 
>> ```
>> 
>> `this` is a pointer to a `bindings::config_group` that was returned from a call to `make_group`. The pointer is known
>> to be embedded within a `Group<Parent>`.
>> 
>> ```
> 
> How is this:
> 
>        // SAFETY: By C API contact,`this` was returned from a call to
>        // `make_group`. The pointer is known to be embedded within a
>        // `Group<Parent>`.
> 

This is good.

— Daniel

[PATCH v4 3/4] rust: configfs: add a sample demonstrating configfs usage

[Andreas Hindborg]

Add a sample to the samples folder, demonstrating the intended use of the
rust configfs API.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 samples/rust/Kconfig          |  11 +++
 samples/rust/Makefile         |   1 +
 samples/rust/rust_configfs.rs | 179 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 191 insertions(+)

diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
index 918dbead2c0b..2f97bf9a7b4c 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 5a8ab0df0567..72122f010caf 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 000000000000..36a2c848a979
--- /dev/null
+++ b/samples/rust/rust_configfs.rs
@@ -0,0 +1,179 @@
+// 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::page::PAGE_SIZE;
+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; PAGE_SIZE]>, 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; PAGE_SIZE], 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(
+                c_str!("rust_configfs"), item_type, Configuration::new()
+            ),
+        })
+    }
+}
+
+#[vtable]
+impl configfs::GroupOperations for Configuration {
+    type Child = Child;
+
+    fn make_group(&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; PAGE_SIZE]) -> 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; PAGE_SIZE]) -> 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 Child = GrandChild;
+
+    fn make_group(&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; PAGE_SIZE]) -> 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; PAGE_SIZE]) -> 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 v4 3/4] rust: configfs: add a sample demonstrating configfs usage

[Daniel Almeida]

Hi Andreas,

> On 24 Feb 2025, at 10:21, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> Add a sample to the samples folder, demonstrating the intended use of the
> rust configfs API.

nit: this is not the first time I see Rust not capitalized in this series.

> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
> samples/rust/Kconfig          |  11 +++
> samples/rust/Makefile         |   1 +
> samples/rust/rust_configfs.rs | 179 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 191 insertions(+)
> 
> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
> index 918dbead2c0b..2f97bf9a7b4c 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 5a8ab0df0567..72122f010caf 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 000000000000..36a2c848a979
> --- /dev/null
> +++ b/samples/rust/rust_configfs.rs
> @@ -0,0 +1,179 @@
> +// 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::page::PAGE_SIZE;
> +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; PAGE_SIZE]>, 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; PAGE_SIZE], 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,
> +            ],
> +        };

As I said in the previous patch, this macro is a bit complex. Is there anything more you can do with it?

If so, it better be in this driver, because I hardly think anybody will go through the source code itself
to figure out. My 2c.


> +
> +        try_pin_init!(Self {
> +            config <- configfs::Subsystem::new(
> +                c_str!("rust_configfs"), item_type, Configuration::new()
> +            ),
> +        })
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::GroupOperations for Configuration {
> +    type Child = Child;
> +
> +    fn make_group(&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; PAGE_SIZE]) -> 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; PAGE_SIZE]) -> 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 {}

nit: you don’t need the braces here

> +
> +impl Child {
> +    fn new() -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {})
> +    }
> +}
> +
> +#[vtable]
> +impl configfs::GroupOperations for Child {
> +    type Child = GrandChild;
> +
> +    fn make_group(&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; PAGE_SIZE]) -> 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 {}

…nor here

> +
> +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; PAGE_SIZE]) -> Result<usize> {
> +        pr_info!("Show baz\n");

As I said in the cover letter, perhaps this one slip through :)

> +        let data = c"Hello GC\n".to_bytes();
> +        page[0..data.len()].copy_from_slice(data);
> +        Ok(data.len())
> +    }
> +}
> 
> -- 
> 2.47.0
> 
> 

I’m OK with this patch. It works, and it does what it’s supposed to do, i.e.: showcase the API.

With the “Show baz” nit fixed:

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v4 3/4] rust: configfs: add a sample demonstrating configfs usage

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Hi Andreas,
>
>> On 24 Feb 2025, at 10:21, Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Add a sample to the samples folder, demonstrating the intended use of the
>> rust configfs API.
>
> nit: this is not the first time I see Rust not capitalized in this series.

Will fix 👍

[...]

>> +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,
>> +            ],
>> +        };
>
> As I said in the previous patch, this macro is a bit complex. Is there anything more you can do with it?
>
> If so, it better be in this driver, because I hardly think anybody will go through the source code itself
> to figure out. My 2c.

I can add some inline comments on the usage here. Is that what you are
thinking of?

>
>
>> +
>> +        try_pin_init!(Self {
>> +            config <- configfs::Subsystem::new(
>> +                c_str!("rust_configfs"), item_type, Configuration::new()
>> +            ),
>> +        })
>> +    }
>> +}
>> +
>> +#[vtable]
>> +impl configfs::GroupOperations for Configuration {
>> +    type Child = Child;
>> +
>> +    fn make_group(&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; PAGE_SIZE]) -> 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; PAGE_SIZE]) -> 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 {}
>
> nit: you don’t need the braces here

Can't do that. The `pin_data` macro won't eat it. I'll add a comment.

>
>> +
>> +impl Child {
>> +    fn new() -> impl PinInit<Self, Error> {
>> +        try_pin_init!(Self {})
>> +    }
>> +}
>> +
>> +#[vtable]
>> +impl configfs::GroupOperations for Child {
>> +    type Child = GrandChild;
>> +
>> +    fn make_group(&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; PAGE_SIZE]) -> 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 {}
>
> …nor here

>
>> +
>> +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; PAGE_SIZE]) -> Result<usize> {
>> +        pr_info!("Show baz\n");
>
> As I said in the cover letter, perhaps this one slip through :)

Yes, thank you.

>
>> +        let data = c"Hello GC\n".to_bytes();
>> +        page[0..data.len()].copy_from_slice(data);
>> +        Ok(data.len())
>> +    }
>> +}
>>
>> --
>> 2.47.0
>>
>>
>
> I’m OK with this patch. It works, and it does what it’s supposed to do, i.e.: showcase the API.
>
> With the “Show baz” nit fixed:
>
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Thanks!


Best regards,
Andreas Hindborg

Re: [PATCH v4 3/4] rust: configfs: add a sample demonstrating configfs usage

[Daniel Almeida]

>> 
>> As I said in the previous patch, this macro is a bit complex. Is there anything more you can do with it?
>> 
>> If so, it better be in this driver, because I hardly think anybody will go through the source code itself
>> to figure out. My 2c.
> 
> I can add some inline comments on the usage here. Is that what you are
> thinking of?
> 

That can’t hurt, but my point is, if you can think of any other example that is substantially different from
what you have, include it, specially if it involves some different syntax somehow.

Otherwise this is fine as is.

— Daniel

[PATCH v4 4/4] 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 896a307fa065..9b4d5c12eb43 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

Re: [PATCH v4 4/4] MAINTAINERS: add entry for configfs Rust abstractions

[Daniel Almeida]

Hi Andreas,

> On 24 Feb 2025, at 10:21, Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> 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 896a307fa065..9b4d5c12eb43 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
> 
> 

Acked-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v4 0/4] rust: configfs abstractions

[Daniel Almeida]

Hi Andreas, FYI:

Tested-by: Daniel Almeida <daniel.almeida@collabora.com>

i.e.:

```
sudo cat /mnt/rust_configfs/message 
[  234.971000] rust_configfs: Show message
Hello World

sudo cat /mnt/rust_configfs/bar
[  335.542585] rust_configfs: Show bar

sudo sh -c "echo new_bar > /mnt/rust_configfs/bar"
[  382.114901] rust_configfs: Store bar

sudo cat /mnt/rust_configfs/bar
[  401.348487] rust_configfs: Show bar
new_bar

sudo mkdir /mnt/rust_configfs/child

sudo ls /mnt/rust_configfs/
bar      child/   message

sudo ls /mnt/rust_configfs/child/
baz

sudo cat /mnt/rust_configfs/child/baz 
[  600.651618] rust_configfs: Show baz
Hello Baz

sudo mkdir /mnt/rust_configfs/child/grandchild

sudo ls /mnt/rust_configfs/child/grandchild/
gc

sudo cat /mnt/rust_configfs/child/grandchild/gc
[  670.093647] rust_configfs: Show baz
Hello GC
```

Is that last one (for the grandchild) really supposed to print “Show baz” ?

Please let me know if there’s anything else needed to test this fully. A review will soon follow.

— Daniel

Re: [PATCH v4 0/4] rust: configfs abstractions

[Andreas Hindborg]

"Daniel Almeida" <daniel.almeida@collabora.com> writes:

> Hi Andreas, FYI:
>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>
> i.e.:
>
> ```
> sudo cat /mnt/rust_configfs/message
> [  234.971000] rust_configfs: Show message
> Hello World
>
> sudo cat /mnt/rust_configfs/bar
> [  335.542585] rust_configfs: Show bar
>
> sudo sh -c "echo new_bar > /mnt/rust_configfs/bar"
> [  382.114901] rust_configfs: Store bar
>
> sudo cat /mnt/rust_configfs/bar
> [  401.348487] rust_configfs: Show bar
> new_bar
>
> sudo mkdir /mnt/rust_configfs/child
>
> sudo ls /mnt/rust_configfs/
> bar      child/   message
>
> sudo ls /mnt/rust_configfs/child/
> baz
>
> sudo cat /mnt/rust_configfs/child/baz
> [  600.651618] rust_configfs: Show baz
> Hello Baz
>
> sudo mkdir /mnt/rust_configfs/child/grandchild
>
> sudo ls /mnt/rust_configfs/child/grandchild/
> gc
>
> sudo cat /mnt/rust_configfs/child/grandchild/gc
> [  670.093647] rust_configfs: Show baz
> Hello GC
> ```
>
> Is that last one (for the grandchild) really supposed to print “Show baz” ?

Thanks for testing! Good catch of copy pasta on the last one - I will
fix that.


Best regards,
Andreas Hindborg