Message-ID: <e2dbf92f-5024-4dd2-848f-3d9e2f85698c@de.bosch.com>
Date: Wed, 30 Apr 2025 08:26:50 +0200
From: Dirk Behme <dirk.behme@...bosch.com>
To: Remo Senekowitsch <remo@...nzli.dev>, 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>, Danilo Krummrich <dakr@...nel.org>, "Greg
 Kroah-Hartman" <gregkh@...uxfoundation.org>, "Rafael J. Wysocki"
	<rafael@...nel.org>
CC: <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3 5/7] rust: property: Add child accessor and iterator

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@...nzli.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

Powered by blists - more mailing lists