Message-Id: <118A2077-91B6-485F-AA5F-03D54AC5771C@collabora.com>
Date: Tue, 25 Feb 2025 07:53:19 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
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



> On 25 Feb 2025, at 07:11, Andreas Hindborg <a.hindborg@...nel.org> wrote:
> 
> "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.


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

Powered by blists - more mailing lists