Re: [PATCH v2 3/5] rust: property: Add child accessor and iterator

Danilo Krummrich
2025-04-14T16:09:22Z

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
>