Message-ID: <87plj6wcz3.fsf@kernel.org>
Date: Tue, 25 Feb 2025 11:11:28 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "Danilo Krummrich" <dakr@...nel.org>,  "Miguel Ojeda"
 <ojeda@...nel.org>,  "Alex Gaynor" <alex.gaynor@...il.com>,  "Boqun Feng"
 <boqun.feng@...il.com>,  "Gary Guo" <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Benno Lossin" <benno.lossin@...ton.me>,
  "Alice Ryhl" <aliceryhl@...gle.com>,  "Trevor Gross" <tmgross@...ch.edu>,
  "Joel Becker" <jlbec@...lplan.org>,  "Christoph Hellwig" <hch@....de>,
  "Peter Zijlstra" <peterz@...radead.org>,  "Ingo Molnar"
 <mingo@...hat.com>,  "Will Deacon" <will@...nel.org>,  "Waiman Long"
 <longman@...hat.com>,  "Fiona Behrens" <me@...enk.dev>,  "Charalampos
 Mitrodimas" <charmitro@...teo.net>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/4] rust: configfs: introduce rust support for configfs

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

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

Powered by blists - more mailing lists