Re: [PATCH v13 1/1] rust: interop: Add list module for C linked list interface

[Joel Fernandes]

On 3/18/2026 5:10 AM, Alice Ryhl wrote:
> On Tue, Mar 17, 2026 at 04:17:10PM -0400, Joel Fernandes wrote:
>> Add a new module `kernel::interop::list` for working with C's doubly
>> circular linked lists. Provide low-level iteration over list nodes.
>>
>> Typed iteration over actual items is provided with a `clist_create`
>> macro to assist in creation of the `CList` type.
>>
>> Cc: Nikola Djukic <ndjukic@xxxxxxxxxx>
>> Reviewed-by: Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx>
>> Reviewed-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> Acked-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> Acked-by: Gary Guo <gary@xxxxxxxxxxx>
>> Acked-by: Miguel Ojeda <ojeda@xxxxxxxxxx>
>> Signed-off-by: Joel Fernandes <joelagnelf@xxxxxxxxxx>
> 
> I have a few nits below. But overall I think this looks ok:
> 
> Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> 
> Please do consider my mod.rs suggestion too, though.

sure.

> 
>> +//! ```
>> +//! use kernel::{
>> +//!     bindings,
>> +//!     clist_create,
> 
> IMO the automatic re-exports of macros at the root shouldn't be used.
> Import it from kernel::interop::list::clist_create instead.
> 
> Note that you need to put a re-export below macro definition to do this.
> 
> 	macro_rules! clist_create {
> 	    (unsafe { $head:ident, $rust_type:ty, $c_type:ty, $($field:tt).+ }) => {{
> 	        // Compile-time check that field path is a `list_head`.
> 	        // SAFETY: `p` is a valid pointer to `$c_type`.
> 	        let _: fn(*const $c_type) -> *const $crate::bindings::list_head =
> 	            |p| unsafe { &raw const (*p).$($field).+ };
> 	
> 	        // Calculate offset and create `CList`.
> 	        const OFFSET: usize = ::core::mem::offset_of!($c_type, $($field).+);
> 	        // SAFETY: The caller of this macro is responsible for ensuring safety.
> 	        unsafe { $crate::interop::list::CList::<$rust_type, OFFSET>::from_raw($head) }
> 	    }};
> 	}
> 	pub use clist_create; // <-- you need this
> 
> See tracepoint.rs or any of the other macros for an example.

Ok.

> 
>> +//! // Create typed [`CList`] from sentinel head.
>> +//! // SAFETY: `head` is valid and initialized, items are `SampleItemC` with
>> +//! // embedded `link` field, and `Item` is `#[repr(transparent)]` over `SampleItemC`.
>> +//! let list = clist_create!(unsafe { head, Item, SampleItemC, link });
> 
> Did you try using this in your real use-case? You require `head` to be
> an :ident, but I think for any 'struct list_head' not stored on the
> stack, accepting an :expr would be easier to use so that you can just
> pass `&raw mut my_c_struct.the_list_head` directly to the macro. Right
> now you have to put the raw pointer in a local variable first.
For buddy.rs usecase, currently we do put it in a local variable so it
works fine. Not doing :ident was causing clippy errors.


warning: this macro expands metavariables in an unsafe block
   --> rust/kernel/interop/list.rs:341:9
    |
341 |         unsafe { $crate::interop::list::CList::<$rust_type,
OFFSET>::from_raw($head) }
    |
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: this allows the user of the macro to write unsafe code outside
of an unsafe block
    = help: consider expanding any metavariables outside of this block,
e.g. by storing them in a variable
    = help: ... or also expand referenced metavariables in a safe context
to require an unsafe block at callsite
    = help: for further information visit
https://rust-lang.github.io/rust-clippy/master/index.html#macro_metavars_in_unsafe
    = note: `-W clippy::macro-metavars-in-unsafe` implied by `-W clippy::all`
    = help: to override `-W clippy::all` add
`#[allow(clippy::macro_metavars_in_unsafe)]`


thanks,

--
Joel Fernandes