Re: [PATCH -next v9 1/3] rust: clist: Add support to interface with C linked lists

[Joel Fernandes <joelagnelf@nvidia.com>]

On Thu, Feb 19, 2026 at 09:35:31AM +0900, Alexandre Courbot wrote:
> I saw that v10 has been sent, but since the code is mostly identical
> this review should still apply (I was in the middle of writing it before
> going to sleep).

Thanks for the thorough review, Alex! All comments addressed below.

> > + * Helpers for C Circular doubly linked list implementation.
>
> nit: "Circular" doesn't need a capital "C".

Fixed.

> > +//! #     // SAFETY: pointers are to allocated test objects with a list_head field.
> > +//! #     unsafe {
> > +//! #         (*ptr).value = i as i32 * 10;
> > +//! #         // &raw mut computes address of link directly as link is uninitialized.
> > +//! #         bindings::INIT_LIST_HEAD(&raw mut (*ptr).link);
> > +//! #         bindings::list_add_tail(&mut (*ptr).link, head);
>
> Is the `INIT_LIST_HEAD` line needed? `list_add_tail` will immediately
> overwrite the initial values of `ptr.link`.

Good point - list_add_tail does overwrite the link fields. However I'm
keeping the INIT_LIST_HEAD call because the doctest is meant to show
the full pattern of working with list_head from Rust, including proper
initialization. Also, skipping it would mean calling list_add_tail on
an uninitialized list_head, which while it works in practice (since
list_add_tail overwrites both fields), feels wrong to demonstrate in
an example.

> > +//! let list = unsafe { clist_create!(head, Item, SampleItemC, link) };
>
> `head` appears in the documentation for the first time here - it would
> help to make at least its declaration visible, or add a comment
> specifying its type.

The head declaration is in the hidden doctest setup code (marked with
`# `). Since it's setup code that would be done by C in practice,
keeping it hidden seems right. The SAFETY comment on the clist_create!
call now documents what head is.

> > +    pub unsafe fn from_raw<'a>(ptr: *mut bindings::list_head) -> &'a Self {
> > +        // SAFETY:
> > +        // - [`CList`] has same layout as [`CListHead`] due to repr(transparent).
> > +        // - Caller guarantees `ptr` is a valid, sentinel `list_head` object.
> > +        unsafe { &*ptr.cast() }
> > +    }
>
> IIUC you can call `CListHead::from_raw` here instead of repeating its
> code.

CListHead::from_raw returns &CListHead, but we need &CList<T, OFFSET>.
Since CList is repr(transparent) over CListHead, the direct ptr.cast()
is the correct approach - we'd need an additional cast after
CListHead::from_raw anyway, which would be more code, not less.

> > +    fn next(&mut self) -> Option<Self::Item> {
>
> This method is the only one not marked `#[inline]`.

Added #[inline], thanks for catching that.

> > +impl<'a> FusedIterator for CListHeadIter<'a> {}
>
> I asked this a couple of times ([1], [2]) but got no reply, so let me
> try again. :) Given that `list_head` is doubly-linked, can we also
> implement `DoubleEndedIterator`?

Apologies for the missed replies! Yes, DoubleEndedIterator makes sense
for doubly-linked lists. I'll add it as a follow-up patch since it
requires a prev() method and some additional iterator state tracking.

> > +// SAFETY: [`CListHead`] can be sent to any thread.
> > +unsafe impl Send for CListHead {}
>
> That safety comment is circular. I guess you mean to say that
> `list_head` can be sent to any thread?

Fixed. Updated to: "list_head contains no thread-bound state; it only
holds next/prev pointers."

> Since Rust macros let us be more creative, how about something like
> this:
>
>     let list = unsafe { clist_create!(head => Item, SampleItemC.link) };

Interesting idea! For now I've kept the current comma-separated syntax
but with the device.rs unsafe pattern:

    clist_create!(unsafe { head, Item, SampleItemC, link })

This keeps consistency with other kernel macros. The unsafe { } block
is the key improvement from Danilo's review. We can explore syntax
sugar in a follow-up if desired.

Thanks,
Joel