Messages in this thread |  | | Date | Wed, 18 Mar 2026 14:55:17 -0400 | | Subject | Re: [PATCH v13 1/1] rust: interop: Add list module for C linked list interface | | From | 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@nvidia.com> >> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com> >> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com> >> Acked-by: Alexandre Courbot <acourbot@nvidia.com> >> Acked-by: Gary Guo <gary@garyguo.net> >> Acked-by: Miguel Ojeda <ojeda@kernel.org> >> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com> > > I have a few nits below. But overall I think this looks ok: > > Reviewed-by: Alice Ryhl <aliceryhl@google.com> > > 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
|  |