Re: [PATCH v6 1/3] rust: configfs: introduce rust support for configfs

[Miguel Ojeda]

On Thu, May 1, 2025 at 12:15 PM Andreas Hindborg <a.hindborg@xxxxxxxxxx> wrote:
>
> Signed-off-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
>
> ---
>

Spurious newlines.

> This patch is a direct dependency for `rnull`, the rust null block driver.
> ---

By the way, you don't need this `---`.

> +//! `configfs` interface.

I don't know if configfs is supposed to be written in code spans, but
I appreciate you are trying to be throughout in your Markdown use ;)
It may be easier to read to not have it in code spans, since we have
many already and it is not code anyway.

By the way, you may want to mention somehow the title they use in
their docs: "Userspace-driven Kernel Object Configuration".

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

Files are, though, like the C header below, so: [`rust_configfs.rs`]

> +/// with configfs, embed a field of this type into your kernel module struct.

Either with or without a code span, i.e. being consistent is best.

> +    /// Return the address of the `bindings::config_group` embedded in `Self`.

I think you may be able to use intra-doc links for [`Self`].

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

It doesn't work to put the safety comment on top? (We had issues
similar to this in the past, so if it is intentional, that is fine).

> +/// This type is constructed statically at compile time and is by the
> +/// [`kernel::configfs_attrs`] macro.

Sentence is missing something. Also, we never used `# Note` yet, but I
guess it is fine.

> +    /// Null terminated Array of pointers to `Attribute`. The type is `c_void`

Intar-doc link(s)?

> +        // We need a space at the end of our list for a null terminator.
> +        if I >= N - 1 {
> +            kernel::build_error!("Invalid attribute index");
> +        }

Would the following work instead?

    const { assert!(I < N - 1, "Invalid attribute index") };

(Please double-check it actually catches the cases you need)

Thanks!

Cheers,
Miguel