Re: [PATCH v3 5/7] rust: property: Add child accessor and iterator

Dirk Behme
2025-04-30T06:27:04Z

On 25/04/2025 17:01, Remo Senekowitsch wrote:
> Allow Rust drivers to access children of a fwnode either by name or by
> iterating over all of them.
>
> In C, there is the function `fwnode_get_next_child_node` for iteration
> and the macro `fwnode_for_each_child_node` that helps with handling the
> pointers. Instead of a macro, a native iterator is used in Rust such
> that regular for-loops can be used.
>
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
> rust/kernel/device/property.rs | 79 +++++++++++++++++++++++++++++++++-
> 1 file changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index 9505cc35d..0a0cb0c02 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -13,7 +13,7 @@
> error::{to_result, Result},
> prelude::*,
> str::{CStr, CString},
> - types::Opaque,
> + types::{ARef, Opaque},
> };
>
> impl Device {
> @@ -52,6 +52,27 @@ pub fn fwnode(&self) -> Option<&FwNode> {
> pub struct FwNode(Opaque<bindings::fwnode_handle>);
>
> impl FwNode {
> + /// # Safety
> + ///
> + /// Callers must ensure that:
> + /// - The reference count was incremented at least once.
> + /// - They relinquish that 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<FwNode>`.
> + unsafe fn from_raw(raw: *mut bindings::fwnode_handle) -> ARef<Self> {
> + // SAFETY: As per the safety requirements of this function:
> + // - `NonNull::new_unchecked`:
> + // - `raw` is not null
> + // - `ARef::from_raw`:
> + // - `raw` has an incremented refcount
> + // - that increment is relinquished, i.e. it won't be decremented
> + // elsewhere.

Quite minor: There is some inconsistency on using the '.' above. The two
`raw` sentences don't have it while the last 'that increment ...' has it.

> + // CAST: 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`.
> + 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()
> @@ -238,6 +259,62 @@ pub fn property_read<'fwnode, 'name, T: Property>(
> name,
> }
> }
> +
> + /// 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 by their type invariants.
> + 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 its refcount
> + // incremented.
> + // - That increment is relinquished, i.e. the underlying object is not
> + // used anymore except via the newly created `ARef`.
> + 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 by its type invariant.
> + // - `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.
> + // - The increment of `prev_ptr` is relinquished, i.e. the
> + // underlying object won't be unsed anymore.

Typo: unsed -> used (?)

Dirk