Re: [PATCH 1/5] rust: of: add Node type

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/20260817-drm_panel_bindings-v1-0-1f974508a31c-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org?part=1