Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Medium] The `# Safety` section of `Node::from_raw` fails to enforce a positive reference count, and the function is missing the mandatory `// INVARIANT:` comment. - [Medium] The `Node` type lacks explicit `Send` and `Sync` trait implementations. - [Low] Missing `#[inline]` annotations on small forwarding abstraction functions. - [Low] Violation of the kernel vertical import style. - [Low] `Node` is incorrectly documented as wrapping a pointer. --
--- Patch [1]: [PATCH 1/5] rust: of: add Node type --- commit 61d64a4023f5c5b35bf62557893fca7718f72090 Author: Albert Esteve <[email protected]> rust: of: add Node type This commit introduces a Rust abstraction for the device tree node type. It wraps the device_node struct and implements the AlwaysRefCounted trait, allowing safe ownership and reference counting of device tree nodes in Rust. It also adds C helpers to access the reference counting functions. > Node wraps device_node type pointer and implements > AlwaysRefCounted, allowing owned references to device > tree nodes to be held. [Severity: Low] This isn't a bug, but could this description cause confusion? The commit message and the invariants section state that Node wraps a pointer, but the struct actually wraps Opaque<bindings::device_node>. This represents the struct itself rather than a pointer to it, which might confuse whether it is meant to be passed by value or strictly by reference. > diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs > index 58b20c367f993..e75ab81cfe1f7 100644 > --- a/rust/kernel/of.rs > +++ b/rust/kernel/of.rs > @@ -6,7 +6,10 @@ > bindings, > device_id::{RawDeviceId, RawDeviceIdIndex}, > prelude::*, > + sync::aref::AlwaysRefCounted, > + types::Opaque, > }; > +use core::ptr::NonNull; [Severity: Low] This isn't a bug, but does this import ordering follow the kernel vertical import style? Standard library imports like core::ptr::NonNull are typically placed before the crate imports and separated by a blank line. > + > +/// A device tree node (struct device_node). > +/// > +/// # Invariants > +/// > +/// The inner pointer is always a valid, non-null pointer to a struct > device_node > +/// with a positive reference count. > +#[repr(transparent)] > +pub struct Node(Opaque<bindings::device_node>); [Severity: Medium] Are we missing explicit Send and Sync trait implementations for Node? Since Node wraps Opaque<bindings::device_node>, it defaults to not implementing Send and Sync. Device Tree nodes use thread-safe atomic reference counting and are safe to share across threads. Without explicit Send and Sync implementations, ARef<Node> cannot be passed across thread boundaries or used in concurrent driver structures. > +impl Node { > + /// Creates a reference from a raw pointer. > + /// > + /// # Safety > + /// > + /// ptr must be a valid, non-null struct device_node pointer that remains > + /// valid for the lifetime 'a. > + pub unsafe fn from_raw<'a>(ptr: *const bindings::device_node) -> &'a > Self { [Severity: Medium] Should the safety documentation for from_raw require that the pointer has a positive reference count? The type invariant for Node strictly requires a positive reference count, but the safety preconditions here omit this requirement. A caller could pass a valid but zero-refcount pointer, complying with the safety contract but violating the type invariant. If this is later converted to an owned ARef, inc_ref will call of_node_get on a node with a zero refcount. [Severity: Low] This isn't a bug, but are we missing the #[inline] annotation on this small forwarding abstraction function? > + // SAFETY: Caller guarantees ptr is valid and lives for 'a. > + unsafe { &*ptr.cast() } > + } [Severity: Medium] Are we missing the mandatory // INVARIANT: comment here when casting to the struct, as required by the Rust subsystem guidelines? > + > + /// Returns the raw pointer to the underlying struct device_node. > + pub fn as_raw(&self) -> *const bindings::device_node { > + self.0.get() as _ > + } > +} [Severity: Low] This isn't a bug, but should this small getter function be annotated with #[inline]? > +// SAFETY: By the type invariants, this type is always refcounted. > +unsafe impl AlwaysRefCounted for Node { > + fn inc_ref(&self) { [Severity: Low] This isn't a bug, but should this forwarding abstraction method be annotated with #[inline]? > + // SAFETY: The type invariant guarantees the pointer is valid. > + unsafe { bindings::of_node_get(self.as_raw().cast_mut()) }; > + } > + > + unsafe fn dec_ref(obj: NonNull<Self>) { [Severity: Low] This isn't a bug, but should this forwarding abstraction method be annotated with #[inline]? > + // SAFETY: The safety requirements guarantee that the refcount is > non-zero. > + unsafe { bindings::of_node_put(obj.cast().as_ptr()) }; > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
