Message-ID: <Z_0zLK7K3G7QK3gT@cassiopeiae>
Date: Mon, 14 Apr 2025 18:09:16 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Remo Senekowitsch <remo@...nzli.dev>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 3/5] rust: property: Add child accessor and iterator

On Mon, Apr 14, 2025 at 05:26:28PM +0200, Remo Senekowitsch wrote:
>  impl Device {
> @@ -68,6 +68,16 @@ pub fn property_read<'fwnode, 'name, T: Property>(
>      ) -> PropertyGuard<'fwnode, 'name, T> {
>          self.fwnode().property_read(name)
>      }
> +
> +    /// Returns first matching named child node handle.
> +    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
> +        self.fwnode().get_child_by_name(name)
> +    }
> +
> +    /// Returns an iterator over a node's children.
> +    pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
> +        self.fwnode().children()
> +    }
>  }

Since those functions are within the impl Device block, please move them to
device.rs.

>  
>  /// A reference-counted fwnode_handle.
> @@ -89,6 +99,22 @@ pub fn property_read<'fwnode, 'name, T: Property>(
>  pub struct FwNode(Opaque<bindings::fwnode_handle>);
>  
>  impl FwNode {
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the reference count was incremented at least
> +    /// once, and that they are properly relinquishing one increment. That is,
> +    /// if there is only one increment, callers must not use the underlying
> +    /// object anymore -- it is only safe to do so via the newly created
> +    /// [`ARef`].

Please compile multiple safety requirements into a list.

> +    unsafe fn from_raw(raw: *mut bindings::fwnode_handle) -> ARef<Self> {
> +        // SAFETY: As per the safety requirement, raw has an incremented
> +        // refcount which won't be decremented elsewhere. It also it not null.

Same here.

> +        // It is safe to cast from a `*mut fwnode_handle` to `*mut FwNode`,
> +        // because `FwNode` is  defined as a `#[repr(transparent)]` wrapper
> +        // around `fwnode_handle`.

This should be CAST: instead.

> +        unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(raw.cast())) }
> +    }
> +
>      /// Obtain the raw `struct fwnode_handle *`.
>      pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
>          self.0.get()
> @@ -243,6 +269,53 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
>  
>          FwNodeDisplayPath(self)
>      }
> +
> +    /// Returns first matching named child node handle.
> +    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<Self>> {
> +        // SAFETY: `self` and `name` are valid.

I'd say that `&self` is valid by its type invariant.

> +        let child =
> +            unsafe { bindings::fwnode_get_named_child_node(self.as_raw(), name.as_char_ptr()) };
> +        if child.is_null() {
> +            return None;
> +        }
> +        // SAFETY: `fwnode_get_named_child_node` returns a pointer with refcount incremented.

Please cover all safety requirements from from_raw().

> +        Some(unsafe { Self::from_raw(child) })
> +    }
> +
> +    /// Returns an iterator over a node's children.
> +    pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
> +        let mut prev: Option<ARef<FwNode>> = None;
> +
> +        core::iter::from_fn(move || {
> +            let prev_ptr = match prev.take() {
> +                None => ptr::null_mut(),
> +                Some(prev) => {
> +                    // We will pass `prev` to `fwnode_get_next_child_node`,
> +                    // which decrements its refcount, so we use
> +                    // `ARef::into_raw` to avoid decrementing the refcount
> +                    // twice.
> +                    let prev = ARef::into_raw(prev);
> +                    prev.as_ptr().cast()
> +                }
> +            };
> +            // SAFETY: `self.as_raw()` is valid. `prev_ptr` may be null,
> +            // which is allowed and corresponds to getting the first child.
> +            // Otherwise, `prev_ptr` is valid, as it is the stored return
> +            // value from the previous invocation. `prev_ptr` has its refount
> +            // incremented and it won't be decremented anymore on the Rust
> +            // side. `fwnode_get_next_child_node` decrements the refcount of
> +            // `prev_ptr`, so the refcount is handled correctly.

Please also compile this into a list.

> +            let next = unsafe { bindings::fwnode_get_next_child_node(self.as_raw(), prev_ptr) };
> +            if next.is_null() {
> +                return None;
> +            }
> +            // SAFETY: `fwnode_get_next_child_node` returns a pointer with
> +            // refcount incremented.

You rather need to justify why the next you pass into from_raw() is valid.

> +            let next = unsafe { FwNode::from_raw(next) };
> +            prev = Some(next.clone());
> +            Some(next)
> +        })
> +    }
>  }
>  
>  // SAFETY: Instances of `FwNode` are always reference-counted.
> -- 
> 2.49.0
> 

Powered by blists - more mailing lists