[PATCH v4 0/9] More Rust bindings for device property reads

[PATCH v4 0/9] More Rust bindings for device property reads

[Remo Senekowitsch]

changes in v4:
* Avoid code duplication in `FwNode::display_path`.
* Add missing safety comment.
* Replace `PropertyGuard::required` with `PropertyGuard::required_by`
  to associate logs with the proper device.
* Split commit moving property_present into three separate ones:
  1. Create FwNode abstraction.
  2. Access FwNode via Device.
  3. Move property_present from Device to FwNode.

Best regards,
Remo

Remo Senekowitsch (9):
  rust: device: Create FwNode abstraction for accessing device
    properties
  rust: device: Enable accessing the FwNode of a Device
  rust: device: Move property_present() to FwNode
  rust: device: Enable printing fwnode name and path
  rust: device: Introduce PropertyGuard
  rust: device: Add bindings for reading device properties
  rust: device: Add child accessor and iterator
  rust: device: Add property_get_reference_args
  samples: rust: platform: Add property read examples

 MAINTAINERS                                  |   2 +-
 drivers/of/unittest-data/tests-platform.dtsi |   3 +
 rust/helpers/helpers.c                       |   1 +
 rust/helpers/property.c                      |   8 +
 rust/kernel/{device.rs => device/mod.rs}     |  20 +-
 rust/kernel/device/property.rs               | 557 +++++++++++++++++++
 samples/rust/rust_driver_platform.rs         |  71 ++-
 7 files changed, 654 insertions(+), 8 deletions(-)
 create mode 100644 rust/helpers/property.c
 rename rust/kernel/{device.rs => device/mod.rs} (95%)
 create mode 100644 rust/kernel/device/property.rs

-- 
2.49.0

[PATCH v4 1/9] rust: device: Create FwNode abstraction for accessing device properties

[Remo Senekowitsch]

Not all property-related APIs can be exposed directly on a device.
For example, iterating over child nodes of a device will yield
fwnode_handle. Thus, in order to access properties on these child nodes,
the property access methods must be implemented on the abstraction over
fwnode_handle.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 MAINTAINERS                              |  2 +-
 rust/helpers/helpers.c                   |  1 +
 rust/helpers/property.c                  |  8 ++++
 rust/kernel/{device.rs => device/mod.rs} |  2 +
 rust/kernel/device/property.rs           | 47 ++++++++++++++++++++++++
 5 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 rust/helpers/property.c
 rename rust/kernel/{device.rs => device/mod.rs} (99%)
 create mode 100644 rust/kernel/device/property.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 3cbf9ac0d83f6..8a47c0e10e58d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7247,7 +7247,7 @@ F:	include/linux/kobj*
 F:	include/linux/property.h
 F:	include/linux/sysfs.h
 F:	lib/kobj*
-F:	rust/kernel/device.rs
+F:	rust/kernel/device/
 F:	rust/kernel/device_id.rs
 F:	rust/kernel/devres.rs
 F:	rust/kernel/driver.rs
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 1e7c84df72521..6aac840417648 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -25,6 +25,7 @@
 #include "platform.c"
 #include "pci.c"
 #include "pid_namespace.c"
+#include "property.c"
 #include "rbtree.c"
 #include "rcu.c"
 #include "refcount.c"
diff --git a/rust/helpers/property.c b/rust/helpers/property.c
new file mode 100644
index 0000000000000..08f68e2dac4a9
--- /dev/null
+++ b/rust/helpers/property.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/property.h>
+
+void rust_helper_fwnode_handle_put(struct fwnode_handle *fwnode)
+{
+	fwnode_handle_put(fwnode);
+}
diff --git a/rust/kernel/device.rs b/rust/kernel/device/mod.rs
similarity index 99%
rename from rust/kernel/device.rs
rename to rust/kernel/device/mod.rs
index 0353c5552769c..d8619d4485fb4 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device/mod.rs
@@ -14,6 +14,8 @@
 #[cfg(CONFIG_PRINTK)]
 use crate::c_str;
 
+pub mod property;
+
 /// A reference-counted device.
 ///
 /// This structure represents the Rust abstraction for a C `struct device`. This implementation
diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
new file mode 100644
index 0000000000000..e75d55f5856cf
--- /dev/null
+++ b/rust/kernel/device/property.rs
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Unified device property interface.
+//!
+//! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
+
+use core::ptr;
+
+use crate::{bindings, types::Opaque};
+
+/// A reference-counted fwnode_handle.
+///
+/// This structure represents the Rust abstraction for a
+/// C `struct fwnode_handle`. This implementation abstracts the usage of an
+/// already existing C `struct fwnode_handle` within Rust code that we get
+/// passed from the C side.
+///
+/// # Invariants
+///
+/// A `FwNode` instance represents a valid `struct fwnode_handle` created by the
+/// C portion of the kernel.
+///
+/// Instances of this type are always reference-counted, that is, a call to
+/// `fwnode_handle_get` ensures that the allocation remains valid at least until
+/// the matching call to `fwnode_handle_put`.
+#[repr(transparent)]
+pub struct FwNode(Opaque<bindings::fwnode_handle>);
+
+impl FwNode {
+    /// Obtain the raw `struct fwnode_handle *`.
+    pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
+        self.0.get()
+    }
+}
+
+// SAFETY: Instances of `FwNode` are always reference-counted.
+unsafe impl crate::types::AlwaysRefCounted for FwNode {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
+        unsafe { bindings::fwnode_handle_get(self.as_raw()) };
+    }
+
+    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
+    }
+}
-- 
2.49.0

[PATCH v4 2/9] rust: device: Enable accessing the FwNode of a Device

[Remo Senekowitsch]

The FwNode is an abstraction over the C struct fwnode_handle. It will be
used to access all device properties.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/mod.rs | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/rust/kernel/device/mod.rs b/rust/kernel/device/mod.rs
index d8619d4485fb4..b4b7056eb80f8 100644
--- a/rust/kernel/device/mod.rs
+++ b/rust/kernel/device/mod.rs
@@ -186,6 +186,21 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
         };
     }
 
+    /// Obtain the [`FwNode`](property::FwNode) corresponding to the device.
+    pub fn fwnode(&self) -> Option<&property::FwNode> {
+        // SAFETY: `self` is valid.
+        let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
+        if fwnode_handle.is_null() {
+            return None;
+        }
+        // SAFETY: `fwnode_handle` is valid. Its lifetime is tied to `&self`. We
+        // return a reference instead of an `ARef<FwNode>` because `dev_fwnode()`
+        // doesn't increment the refcount. It is safe to cast from a
+        // `struct fwnode_handle*` to a `*const FwNode` because `FwNode` is
+        // defined as a `#[repr(transparent)]` wrapper around `fwnode_handle`.
+        Some(unsafe { &*fwnode_handle.cast() })
+    }
+
     /// Checks if property is present or not.
     pub fn property_present(&self, name: &CStr) -> bool {
         // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
-- 
2.49.0

[PATCH v4 3/9] rust: device: Move property_present() to FwNode

[Remo Senekowitsch]

The new FwNode abstraction will be used for accessing all device
properties, so it must have the property_present method.

It's possible to duplicate the methods on the device itself, but since
some of the methods on Device would have different type sigatures as the
ones on FwNode, this would only lead to inconsistency and confusion.
Hence, remove the method from Device.

There aren't any users to update yet.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/mod.rs      | 7 -------
 rust/kernel/device/property.rs | 8 +++++++-
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/device/mod.rs b/rust/kernel/device/mod.rs
index b4b7056eb80f8..15d89cd45e871 100644
--- a/rust/kernel/device/mod.rs
+++ b/rust/kernel/device/mod.rs
@@ -6,7 +6,6 @@
 
 use crate::{
     bindings,
-    str::CStr,
     types::{ARef, Opaque},
 };
 use core::{fmt, marker::PhantomData, ptr};
@@ -200,12 +199,6 @@ pub fn fwnode(&self) -> Option<&property::FwNode> {
         // defined as a `#[repr(transparent)]` wrapper around `fwnode_handle`.
         Some(unsafe { &*fwnode_handle.cast() })
     }
-
-    /// Checks if property is present or not.
-    pub fn property_present(&self, name: &CStr) -> bool {
-        // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
-        unsafe { bindings::device_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
-    }
 }
 
 // SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic
diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index e75d55f5856cf..70593343bd811 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -6,7 +6,7 @@
 
 use core::ptr;
 
-use crate::{bindings, types::Opaque};
+use crate::{bindings, str::CStr, types::Opaque};
 
 /// A reference-counted fwnode_handle.
 ///
@@ -31,6 +31,12 @@ impl FwNode {
     pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
         self.0.get()
     }
+
+    /// Checks if property is present or not.
+    pub fn property_present(&self, name: &CStr) -> bool {
+        // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
+        unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
-- 
2.49.0

[PATCH v4 4/9] rust: device: Enable printing fwnode name and path

[Remo Senekowitsch]

Add two new public methods `display_name` and `display_path` to
`FwNode`. They can be used by driver authors for logging purposes. In
addition, they will be used by core property abstractions for automatic
logging, for example when a driver attempts to read a required but
missing property.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/property.rs | 72 ++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index 70593343bd811..6ccc7947f9c31 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -32,6 +32,78 @@ pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
         self.0.get()
     }
 
+    /// Returns an object that implements [`Display`](core::fmt::Display) for
+    /// printing the name of a node.
+    pub fn display_name(&self) -> impl core::fmt::Display + '_ {
+        struct FwNodeDisplayName<'a>(&'a FwNode);
+
+        impl core::fmt::Display for FwNodeDisplayName<'_> {
+            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+                // SAFETY: self is valid by its type invariant
+                let name = unsafe { bindings::fwnode_get_name(self.0.as_raw()) };
+                if name.is_null() {
+                    return Ok(());
+                }
+                // SAFETY: fwnode_get_name returns null or a valid C string and
+                // name is not null
+                let name = unsafe { CStr::from_char_ptr(name) };
+                write!(f, "{name}")
+            }
+        }
+
+        FwNodeDisplayName(self)
+    }
+
+    /// Returns an object that implements [`Display`](core::fmt::Display) for
+    /// printing the full path of a node.
+    pub fn display_path(&self) -> impl core::fmt::Display + '_ {
+        struct FwNodeDisplayPath<'a>(&'a FwNode);
+
+        impl core::fmt::Display for FwNodeDisplayPath<'_> {
+            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+                // The logic here is the same as the one in lib/vsprintf.c
+                // (fwnode_full_name_string).
+
+                // SAFETY: `self.0.as_raw()` is valid by its type invariant
+                let num_parents = unsafe { bindings::fwnode_count_parents(self.0.as_raw()) };
+
+                for depth in (0..=num_parents).rev() {
+                    let fwnode = if depth == 0 {
+                        self.0.as_raw()
+                    } else {
+                        // SAFETY: `self.0.as_raw()` is valid
+                        unsafe { bindings::fwnode_get_nth_parent(self.0.as_raw(), depth) }
+                    };
+
+                    // SAFETY: fwnode is valid, it is either `self.0.as_raw()` or
+                    // the return value of `bindings::fwnode_get_nth_parent` which
+                    // returns a valid pointer to a fwnode_handle if the provided
+                    // depth is within the valid range, which we know to be true.
+                    let prefix = unsafe { bindings::fwnode_get_name_prefix(fwnode) };
+                    if !prefix.is_null() {
+                        // SAFETY: fwnode_get_name_prefix returns null or a
+                        // valid C string
+                        let prefix = unsafe { CStr::from_char_ptr(prefix) };
+                        write!(f, "{prefix}")?;
+                    }
+                    write!(f, "{}", self.0.display_name())?;
+
+                    if depth != 0 {
+                        // SAFETY: `fwnode` is valid, because `depth` is
+                        // a valid depth of a parent of `self.0.as_raw()`.
+                        // `fwnode_get_nth_parent` increments the refcount and
+                        // we are responsible to decrement it.
+                        unsafe { bindings::fwnode_handle_put(fwnode) }
+                    }
+                }
+
+                Ok(())
+            }
+        }
+
+        FwNodeDisplayPath(self)
+    }
+
     /// Checks if property is present or not.
     pub fn property_present(&self, name: &CStr) -> bool {
         // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
-- 
2.49.0

[PATCH v4 5/9] rust: device: Introduce PropertyGuard

[Remo Senekowitsch]

This abstraction is a way to force users to specify whether a property
is supposed to be required or not. This allows us to move error
logging of missing required properties into core, preventing a lot of
boilerplate in drivers.

It will be used by upcoming methods for reading device properties.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index 6ccc7947f9c31..59c61e2493831 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
         unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
     }
 }
+
+/// A helper for reading device properties.
+///
+/// Use [`Self::required_by`] if a missing property is considered a bug and
+/// [`Self::optional`] otherwise.
+///
+/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
+pub struct PropertyGuard<'fwnode, 'name, T> {
+    /// The result of reading the property.
+    inner: Result<T>,
+    /// The fwnode of the property, used for logging in the "required" case.
+    fwnode: &'fwnode FwNode,
+    /// The name of the property, used for logging in the "required" case.
+    name: &'name CStr,
+}
+
+impl<T> PropertyGuard<'_, '_, T> {
+    /// Access the property, indicating it is required.
+    ///
+    /// If the property is not present, the error is automatically logged. If a
+    /// missing property is not an error, use [`Self::optional`] instead. The
+    /// device is required to associate the log with it.
+    pub fn required_by(self, dev: &super::Device) -> Result<T> {
+        if self.inner.is_err() {
+            dev_err!(
+                dev,
+                "{}: property '{}' is missing\n",
+                self.fwnode.display_path(),
+                self.name
+            );
+        }
+        self.inner
+    }
+
+    /// Access the property, indicating it is optional.
+    ///
+    /// In contrast to [`Self::required_by`], no error message is logged if
+    /// the property is not present.
+    pub fn optional(self) -> Option<T> {
+        self.inner.ok()
+    }
+
+    /// Access the property or the specified default value.
+    ///
+    /// Do not pass a sentinel value as default to detect a missing property.
+    /// Use [`Self::required_by`] or [`Self::optional`] instead.
+    pub fn or(self, default: T) -> T {
+        self.inner.unwrap_or(default)
+    }
+}
+
+impl<T: Default> PropertyGuard<'_, '_, T> {
+    /// Access the property or a default value.
+    ///
+    /// Use [`Self::or`] to specify a custom default value.
+    pub fn or_default(self) -> T {
+        self.inner.unwrap_or_default()
+    }
+}
-- 
2.49.0

[PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Remo Senekowitsch]

The device property API is a firmware agnostic API for reading
properties from firmware (DT/ACPI) devices nodes and swnodes.

While the C API takes a pointer to a caller allocated variable/buffer,
the rust API is designed to return a value and can be used in struct
initialization. Rust generics are also utilized to support different
types of properties where appropriate.

Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/property.rs | 232 ++++++++++++++++++++++++++++++++-
 1 file changed, 230 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index 59c61e2493831..413166e2d082e 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -4,9 +4,16 @@
 //!
 //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
 
-use core::ptr;
+use core::{mem::MaybeUninit, ptr};
 
-use crate::{bindings, str::CStr, types::Opaque};
+use crate::{
+    alloc::KVec,
+    bindings,
+    error::{to_result, Result},
+    prelude::*,
+    str::{CStr, CString},
+    types::Opaque,
+};
 
 /// A reference-counted fwnode_handle.
 ///
@@ -109,6 +116,105 @@ pub fn property_present(&self, name: &CStr) -> bool {
         // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
         unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
     }
+
+    /// Returns firmware property `name` boolean value
+    pub fn property_read_bool(&self, name: &CStr) -> bool {
+        // SAFETY: `name` is non-null and null-terminated. `self.as_raw()` is valid
+        // because `self` is valid.
+        unsafe { bindings::fwnode_property_read_bool(self.as_raw(), name.as_char_ptr()) }
+    }
+
+    /// Returns the index of matching string `match_str` for firmware string property `name`
+    pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
+        // SAFETY: `name` and `match_str` are non-null and null-terminated. `self.as_raw` is
+        // valid because `self` is valid.
+        let ret = unsafe {
+            bindings::fwnode_property_match_string(
+                self.as_raw(),
+                name.as_char_ptr(),
+                match_str.as_char_ptr(),
+            )
+        };
+        to_result(ret)?;
+        Ok(ret as usize)
+    }
+
+    /// Returns firmware property `name` integer array values in a KVec
+    pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
+        &'fwnode self,
+        name: &'name CStr,
+        len: usize,
+    ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
+        let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
+
+        // SAFETY: `val.as_mut_ptr()` is valid because `KVec::with_capacity`
+        // didn't return an error and it has at least space for `len` number
+        // of elements.
+        let err = unsafe { read_array_out_param::<T>(self, name, val.as_mut_ptr(), len) };
+        let res = if err < 0 {
+            Err(Error::from_errno(err))
+        } else {
+            // SAFETY: fwnode_property_read_int_array() writes exactly `len`
+            // entries on success
+            unsafe { val.set_len(len) }
+            Ok(val)
+        };
+        Ok(PropertyGuard {
+            inner: res,
+            fwnode: self,
+            name,
+        })
+    }
+
+    /// Returns integer array length for firmware property `name`
+    pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
+        // SAFETY: `out_param` is allowed to be null because `len` is zero.
+        let ret = unsafe { read_array_out_param::<T>(self, name, ptr::null_mut(), 0) };
+        to_result(ret)?;
+        Ok(ret as usize)
+    }
+
+    /// Returns the value of firmware property `name`.
+    ///
+    /// This method is generic over the type of value to read. Informally,
+    /// the types that can be read are booleans, strings, unsigned integers and
+    /// arrays of unsigned integers.
+    ///
+    /// Reading a `KVec` of integers is done with the separate
+    /// method [`Self::property_read_array_vec`], because it takes an
+    /// additional `len` argument.
+    ///
+    /// When reading a boolean, this method never fails. A missing property
+    /// is interpreted as `false`, whereas a present property is interpreted
+    /// as `true`.
+    ///
+    /// For more precise documentation about what types can be read, see
+    /// the [implementors of Property][Property#implementors] and [its
+    /// implementations on foreign types][Property#foreign-impls].
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::{c_str, device::{Device, property::FwNode}, str::CString};
+    /// fn examples(dev: &Device) -> Result {
+    ///     let fwnode = dev.fwnode().ok_or(ENOENT)?;
+    ///     let b: u32 = fwnode.property_read(c_str!("some-number")).required_by(dev)?;
+    ///     if let Some(s) = fwnode.property_read::<CString>(c_str!("some-str")).optional() {
+    ///         // ...
+    ///     }
+    ///     Ok(())
+    /// }
+    /// ```
+    pub fn property_read<'fwnode, 'name, T: Property>(
+        &'fwnode self,
+        name: &'name CStr,
+    ) -> PropertyGuard<'fwnode, 'name, T> {
+        PropertyGuard {
+            inner: T::read_from_fwnode_property(self, name),
+            fwnode: self,
+            name,
+        }
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
@@ -124,6 +230,128 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
     }
 }
 
+/// Implemented for several types that can be read as properties.
+///
+/// Informally, this is implemented for strings, integers and arrays of
+/// integers. It's used to make [`FwNode::property_read`] generic over the
+/// type of property being read. There are also two dedicated methods to read
+/// other types, because they require more specialized function signatures:
+/// - [`property_read_bool`](Device::property_read_bool)
+/// - [`property_read_array_vec`](Device::property_read_array_vec)
+pub trait Property: Sized {
+    /// Used to make [`FwNode::property_read`] generic.
+    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self>;
+}
+
+impl Property for CString {
+    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        let mut str: *mut u8 = ptr::null_mut();
+        let pstr: *mut _ = &mut str;
+
+        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
+        // valid because `fwnode` is valid.
+        let ret = unsafe {
+            bindings::fwnode_property_read_string(fwnode.as_raw(), name.as_char_ptr(), pstr.cast())
+        };
+        to_result(ret)?;
+
+        // SAFETY: `pstr` contains a non-null ptr on success
+        let str = unsafe { CStr::from_char_ptr(*pstr) };
+        Ok(str.try_into()?)
+    }
+}
+/// Implemented for all integers that can be read as properties.
+///
+/// This helper trait is needed on top of the existing [`Property`]
+/// trait to associate the integer types of various sizes with their
+/// corresponding `fwnode_property_read_*_array` functions.
+pub trait PropertyInt: Copy {
+    /// # Safety
+    ///
+    /// Callers must uphold the same safety invariants as for the various
+    /// `fwnode_property_read_*_array` functions.
+    unsafe fn read_array_from_fwnode_property(
+        fwnode: *const bindings::fwnode_handle,
+        propname: *const ffi::c_char,
+        val: *mut Self,
+        nval: usize,
+    ) -> ffi::c_int;
+}
+// This macro generates implementations of the traits `Property` and
+// `PropertyInt` for integers of various sizes. Its input is a list
+// of pairs separated by commas. The first element of the pair is the
+// type of the integer, the second one is the name of its corresponding
+// `fwnode_property_read_*_array` function.
+macro_rules! impl_property_for_int {
+    ($($int:ty: $f:ident),* $(,)?) => { $(
+        impl PropertyInt for $int {
+            unsafe fn read_array_from_fwnode_property(
+                fwnode: *const bindings::fwnode_handle,
+                propname: *const ffi::c_char,
+                val: *mut Self,
+                nval: usize,
+            ) -> ffi::c_int {
+                // SAFETY: The safety invariants on the trait require
+                // callers to uphold the invariants of the functions
+                // this macro is called with.
+                unsafe {
+                    bindings::$f(fwnode, propname, val.cast(), nval)
+                }
+            }
+        }
+    )* };
+}
+impl_property_for_int! {
+    u8: fwnode_property_read_u8_array,
+    u16: fwnode_property_read_u16_array,
+    u32: fwnode_property_read_u32_array,
+    u64: fwnode_property_read_u64_array,
+    i8: fwnode_property_read_u8_array,
+    i16: fwnode_property_read_u16_array,
+    i32: fwnode_property_read_u32_array,
+    i64: fwnode_property_read_u64_array,
+}
+/// # Safety
+///
+/// Callers must ensure that if `len` is non-zero, `out_param` must be
+/// valid and point to memory that has enough space to hold at least
+/// `len` number of elements.
+unsafe fn read_array_out_param<T: PropertyInt>(
+    fwnode: &FwNode,
+    name: &CStr,
+    out_param: *mut T,
+    len: usize,
+) -> ffi::c_int {
+    // SAFETY: `name` is non-null and null-terminated.
+    // `fwnode.as_raw` is valid because `fwnode` is valid.
+    // `out_param` is valid and has enough space for at least
+    // `len` number of elements as per the safety requirement.
+    unsafe {
+        T::read_array_from_fwnode_property(fwnode.as_raw(), name.as_char_ptr(), out_param, len)
+    }
+}
+impl<T: PropertyInt, const N: usize> Property for [T; N] {
+    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
+
+        // SAFETY: `val.as_mut_ptr()` is valid and points to enough space for
+        // `N` elements. Casting from `*mut MaybeUninit<T>` to `*mut T` is safe
+        // because `MaybeUninit<T>` has the same memory layout as `T`.
+        let ret = unsafe { read_array_out_param::<T>(fwnode, name, val.as_mut_ptr().cast(), N) };
+        to_result(ret)?;
+
+        // SAFETY: `val` is always initialized when
+        // fwnode_property_read_<T>_array is successful.
+        Ok(val.map(|v| unsafe { v.assume_init() }))
+    }
+}
+impl<T: PropertyInt> Property for T {
+    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        let val: [_; 1] = <[T; 1] as Property>::read_from_fwnode_property(fwnode, name)?;
+        Ok(val[0])
+    }
+}
+
 /// A helper for reading device properties.
 ///
 /// Use [`Self::required_by`] if a missing property is considered a bug and
-- 
2.49.0

[PATCH v4 7/9] rust: device: Add child accessor and iterator

[Remo Senekowitsch]

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 413166e2d082e..10061156f79a8 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -12,7 +12,7 @@
     error::{to_result, Result},
     prelude::*,
     str::{CStr, CString},
-    types::Opaque,
+    types::{ARef, Opaque},
 };
 
 /// A reference-counted fwnode_handle.
@@ -34,6 +34,27 @@
 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.
+        // 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()
@@ -215,6 +236,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 used anymore.
+            let next = unsafe { bindings::fwnode_get_next_child_node(self.as_raw(), prev_ptr) };
+            if next.is_null() {
+                return None;
+            }
+            // SAFETY:
+            // - `next` is valid because `fwnode_get_next_child_node` returns a
+            //   pointer with its refcount incremented.
+            // - That increment is relinquished, i.e. the underlying object
+            //   won't be used anymore, except via the newly created
+            //   `ARef<Self>`.
+            let next = unsafe { FwNode::from_raw(next) };
+            prev = Some(next.clone());
+            Some(next)
+        })
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
-- 
2.49.0

[PATCH v4 8/9] rust: device: Add property_get_reference_args

[Remo Senekowitsch]

Allow Rust code to read reference args from device properties. The
wrapper type `FwNodeReferenceArgs` allows callers to access the buffer
of read args safely.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/property.rs | 68 ++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index 10061156f79a8..1cd3886b8f552 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -292,6 +292,65 @@ pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
             Some(next)
         })
     }
+
+    /// Finds a reference with arguments.
+    pub fn property_get_reference_args(
+        &self,
+        prop: &CStr,
+        nargs: NArgs<'_>,
+        index: u32,
+    ) -> Result<FwNodeReferenceArgs> {
+        let mut out_args = FwNodeReferenceArgs::default();
+
+        let (nargs_prop, nargs) = match nargs {
+            NArgs::Prop(nargs_prop) => (nargs_prop.as_char_ptr(), 0),
+            NArgs::N(nargs) => (ptr::null(), nargs),
+        };
+
+        // SAFETY: `self.0.get()` is valid. `prop.as_char_ptr()` is valid and
+        // zero-terminated. `nargs_prop` is valid and zero-terminated if `nargs`
+        // is zero, otherwise it is allowed to be a null-pointer.
+        let ret = unsafe {
+            bindings::fwnode_property_get_reference_args(
+                self.0.get(),
+                prop.as_char_ptr(),
+                nargs_prop,
+                nargs,
+                index,
+                &mut out_args.0,
+            )
+        };
+        to_result(ret)?;
+
+        Ok(out_args)
+    }
+}
+
+/// The return value of `property_get_reference_args`.
+///
+/// - [`Device::property_get_reference_args`]
+/// - [`FwNode::property_get_reference_args`]
+#[repr(transparent)]
+#[derive(Copy, Clone, Default)]
+pub struct FwNodeReferenceArgs(bindings::fwnode_reference_args);
+
+impl FwNodeReferenceArgs {
+    /// Returns the slice of reference arguments.
+    pub fn as_slice(&self) -> &[u64] {
+        // SAFETY: As per the safety invariant of FwNodeReferenceArgs, `nargs`
+        // is the number of elements in `args` that is valid.
+        unsafe { core::slice::from_raw_parts(self.0.args.as_ptr(), self.0.nargs as usize) }
+    }
+
+    /// Returns the number of reference arguments.
+    pub fn len(&self) -> usize {
+        self.0.nargs as usize
+    }
+
+    /// Returns `true` if there are no reference arguments.
+    pub fn is_empty(&self) -> bool {
+        self.0.nargs == 0
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
@@ -429,6 +488,15 @@ fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
     }
 }
 
+/// The number of arguments of a reference.
+pub enum NArgs<'a> {
+    /// The name of the property of the reference indicating the number of
+    /// arguments.
+    Prop(&'a CStr),
+    /// The known number of arguments.
+    N(u32),
+}
+
 /// A helper for reading device properties.
 ///
 /// Use [`Self::required_by`] if a missing property is considered a bug and
-- 
2.49.0

[PATCH v4 9/9] samples: rust: platform: Add property read examples

[Remo Senekowitsch]

Add some example usage of the device property read methods for
DT/ACPI/swnode properties.

Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 drivers/of/unittest-data/tests-platform.dtsi |  3 +
 samples/rust/rust_driver_platform.rs         | 71 +++++++++++++++++++-
 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
index 4171f43cf01cc..50a51f38afb60 100644
--- a/drivers/of/unittest-data/tests-platform.dtsi
+++ b/drivers/of/unittest-data/tests-platform.dtsi
@@ -37,6 +37,9 @@ dev@100 {
 			test-device@2 {
 				compatible = "test,rust-device";
 				reg = <0x2>;
+
+				test,u32-prop = <0xdeadbeef>;
+				test,i16-array = /bits/ 16 <1 2 (-3) (-4)>;
 			};
 		};
 
diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
index 8b42b3cfb363a..a04ff4afb1325 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -2,7 +2,7 @@
 
 //! Rust Platform driver sample.
 
-use kernel::{c_str, device::Core, of, platform, prelude::*, types::ARef};
+use kernel::{c_str, device::Core, of, platform, prelude::*, str::CString, types::ARef};
 
 struct SampleDriver {
     pdev: ARef<platform::Device>,
@@ -25,18 +25,85 @@ fn probe(
         pdev: &platform::Device<Core>,
         info: Option<&Self::IdInfo>,
     ) -> Result<Pin<KBox<Self>>> {
+        let dev = pdev.as_ref();
+
         dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n");
 
         if let Some(info) = info {
-            dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
+            dev_info!(dev, "Probed with info: '{}'.\n", info.0);
         }
 
+        Self::properties_parse(dev)?;
+
         let drvdata = KBox::new(Self { pdev: pdev.into() }, GFP_KERNEL)?;
 
         Ok(drvdata.into())
     }
 }
 
+impl SampleDriver {
+    fn properties_parse(dev: &kernel::device::Device) -> Result<()> {
+        let fwnode = dev.fwnode().ok_or(ENOENT)?;
+
+        if let Ok(idx) =
+            fwnode.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
+        {
+            dev_info!(dev, "matched compatible string idx = {}\n", idx);
+        }
+
+        if let Ok(str) = fwnode
+            .property_read::<CString>(c_str!("compatible"))
+            .required_by(dev)
+        {
+            dev_info!(dev, "compatible string = {:?}\n", str);
+        }
+
+        let prop = fwnode.property_read_bool(c_str!("test,bool-prop"));
+        dev_info!(dev, "bool prop is {}\n", prop);
+
+        if fwnode.property_present(c_str!("test,u32-prop")) {
+            dev_info!(dev, "'test,u32-prop' is present\n");
+        }
+
+        let prop = fwnode
+            .property_read::<u32>(c_str!("test,u32-optional-prop"))
+            .or(0x12);
+        dev_info!(
+            dev,
+            "'test,u32-optional-prop' is {:#x} (default = {:#x})\n",
+            prop,
+            0x12
+        );
+
+        // Missing property without a default will print an error
+        let _ = fwnode
+            .property_read::<u32>(c_str!("test,u32-required-prop"))
+            .required_by(dev);
+
+        let prop: u32 = fwnode
+            .property_read(c_str!("test,u32-prop"))
+            .required_by(dev)?;
+        dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
+
+        let prop: [i16; 4] = fwnode
+            .property_read(c_str!("test,i16-array"))
+            .required_by(dev)?;
+        dev_info!(dev, "'test,i16-array' is {:?}\n", prop);
+        dev_info!(
+            dev,
+            "'test,i16-array' length is {}\n",
+            fwnode.property_count_elem::<u16>(c_str!("test,i16-array"))?,
+        );
+
+        let prop: KVec<i16> = fwnode
+            .property_read_array_vec(c_str!("test,i16-array"), 4)?
+            .required_by(dev)?;
+        dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop);
+
+        Ok(())
+    }
+}
+
 impl Drop for SampleDriver {
     fn drop(&mut self) {
         dev_dbg!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n");
-- 
2.49.0

Re: [PATCH v4 5/9] rust: device: Introduce PropertyGuard

[Dirk Behme]

On 04/05/2025 19:31, Remo Senekowitsch wrote:
> This abstraction is a way to force users to specify whether a property
> is supposed to be required or not. This allows us to move error
> logging of missing required properties into core, preventing a lot of
> boilerplate in drivers.
> 
> It will be used by upcoming methods for reading device properties.
> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index 6ccc7947f9c31..59c61e2493831 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>      }
>  }
> +
> +/// A helper for reading device properties.
> +///
> +/// Use [`Self::required_by`] if a missing property is considered a bug and
> +/// [`Self::optional`] otherwise.
> +///
> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> +pub struct PropertyGuard<'fwnode, 'name, T> {
> +    /// The result of reading the property.
> +    inner: Result<T>,
> +    /// The fwnode of the property, used for logging in the "required" case.
> +    fwnode: &'fwnode FwNode,
> +    /// The name of the property, used for logging in the "required" case.
> +    name: &'name CStr,
> +}
> +
> +impl<T> PropertyGuard<'_, '_, T> {
> +    /// Access the property, indicating it is required.
> +    ///
> +    /// If the property is not present, the error is automatically logged. If a
> +    /// missing property is not an error, use [`Self::optional`] instead. The
> +    /// device is required to associate the log with it.
> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
> +        if self.inner.is_err() {
> +            dev_err!(
> +                dev,
> +                "{}: property '{}' is missing\n",
> +                self.fwnode.display_path(),
> +                self.name
> +            );
> +        }
> +        self.inner
> +    }

Thinking about the .required_by(dev) I wonder if there will be cases
where we do *not* have a device? I.e. where we really have a fwnode,
only. And therefore can't pass a device. If we have such cases do we
need to be able to pass e.g. Option(dev) and switch back to pr_err() in
case of None?

From the beginning of our discussion I think to remember that the C API
has both the fwnode_property_*() and device_property_*() because there
are use cases for the fwnode_property_*() API where is no device?

Thanks

Dirk

Re: [PATCH v4 5/9] rust: device: Introduce PropertyGuard

[Remo Senekowitsch]

On Mon May 5, 2025 at 7:14 AM CEST, Dirk Behme wrote:
> On 04/05/2025 19:31, Remo Senekowitsch wrote:
>> This abstraction is a way to force users to specify whether a property
>> is supposed to be required or not. This allows us to move error
>> logging of missing required properties into core, preventing a lot of
>> boilerplate in drivers.
>> 
>> It will be used by upcoming methods for reading device properties.
>> 
>> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>> ---
>>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>> 
>> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
>> index 6ccc7947f9c31..59c61e2493831 100644
>> --- a/rust/kernel/device/property.rs
>> +++ b/rust/kernel/device/property.rs
>> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>>      }
>>  }
>> +
>> +/// A helper for reading device properties.
>> +///
>> +/// Use [`Self::required_by`] if a missing property is considered a bug and
>> +/// [`Self::optional`] otherwise.
>> +///
>> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
>> +pub struct PropertyGuard<'fwnode, 'name, T> {
>> +    /// The result of reading the property.
>> +    inner: Result<T>,
>> +    /// The fwnode of the property, used for logging in the "required" case.
>> +    fwnode: &'fwnode FwNode,
>> +    /// The name of the property, used for logging in the "required" case.
>> +    name: &'name CStr,
>> +}
>> +
>> +impl<T> PropertyGuard<'_, '_, T> {
>> +    /// Access the property, indicating it is required.
>> +    ///
>> +    /// If the property is not present, the error is automatically logged. If a
>> +    /// missing property is not an error, use [`Self::optional`] instead. The
>> +    /// device is required to associate the log with it.
>> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
>> +        if self.inner.is_err() {
>> +            dev_err!(
>> +                dev,
>> +                "{}: property '{}' is missing\n",
>> +                self.fwnode.display_path(),
>> +                self.name
>> +            );
>> +        }
>> +        self.inner
>> +    }
>
> Thinking about the .required_by(dev) I wonder if there will be cases
> where we do *not* have a device? I.e. where we really have a fwnode,
> only. And therefore can't pass a device. If we have such cases do we
> need to be able to pass e.g. Option(dev) and switch back to pr_err() in
> case of None?

In that case, bringing back the previous .required() method seems
reasonable to me. But only if we definitely know such cases exist.

> From the beginning of our discussion I think to remember that the C API
> has both the fwnode_property_*() and device_property_*() because there
> are use cases for the fwnode_property_*() API where is no device?

I'm not sure what you're referring to, the closest thing I can think of
is this comment by Rob [1] where he mentions the device_property_*()
functions only exist in C for a minimal convenience gain and we may not
want to keep that in Rust.

[1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697

Re: [PATCH v4 5/9] rust: device: Introduce PropertyGuard

[Rob Herring]

On Mon, May 5, 2025 at 8:02 AM Remo Senekowitsch <remo@buenzli.dev> wrote:
>
> On Mon May 5, 2025 at 7:14 AM CEST, Dirk Behme wrote:
> > On 04/05/2025 19:31, Remo Senekowitsch wrote:
> >> This abstraction is a way to force users to specify whether a property
> >> is supposed to be required or not. This allows us to move error
> >> logging of missing required properties into core, preventing a lot of
> >> boilerplate in drivers.
> >>
> >> It will be used by upcoming methods for reading device properties.
> >>
> >> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> >> ---
> >>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
> >>  1 file changed, 59 insertions(+)
> >>
> >> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> >> index 6ccc7947f9c31..59c61e2493831 100644
> >> --- a/rust/kernel/device/property.rs
> >> +++ b/rust/kernel/device/property.rs
> >> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> >>      }
> >>  }
> >> +
> >> +/// A helper for reading device properties.
> >> +///
> >> +/// Use [`Self::required_by`] if a missing property is considered a bug and
> >> +/// [`Self::optional`] otherwise.
> >> +///
> >> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> >> +pub struct PropertyGuard<'fwnode, 'name, T> {
> >> +    /// The result of reading the property.
> >> +    inner: Result<T>,
> >> +    /// The fwnode of the property, used for logging in the "required" case.
> >> +    fwnode: &'fwnode FwNode,
> >> +    /// The name of the property, used for logging in the "required" case.
> >> +    name: &'name CStr,
> >> +}
> >> +
> >> +impl<T> PropertyGuard<'_, '_, T> {
> >> +    /// Access the property, indicating it is required.
> >> +    ///
> >> +    /// If the property is not present, the error is automatically logged. If a
> >> +    /// missing property is not an error, use [`Self::optional`] instead. The
> >> +    /// device is required to associate the log with it.
> >> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
> >> +        if self.inner.is_err() {
> >> +            dev_err!(
> >> +                dev,
> >> +                "{}: property '{}' is missing\n",
> >> +                self.fwnode.display_path(),
> >> +                self.name
> >> +            );
> >> +        }
> >> +        self.inner
> >> +    }
> >
> > Thinking about the .required_by(dev) I wonder if there will be cases
> > where we do *not* have a device? I.e. where we really have a fwnode,
> > only. And therefore can't pass a device. If we have such cases do we
> > need to be able to pass e.g. Option(dev) and switch back to pr_err() in
> > case of None?
>
> In that case, bringing back the previous .required() method seems
> reasonable to me. But only if we definitely know such cases exist.

They definitely exist. Any property in a child node of the device's
node when the child itself is not another device for example.

> > From the beginning of our discussion I think to remember that the C API
> > has both the fwnode_property_*() and device_property_*() because there
> > are use cases for the fwnode_property_*() API where is no device?

Correct.

> I'm not sure what you're referring to, the closest thing I can think of
> is this comment by Rob [1] where he mentions the device_property_*()
> functions only exist in C for a minimal convenience gain and we may not
> want to keep that in Rust.

The point there was if there's not the same convenience with Rust,
then we shouldn't keep the API.

I think this came up previously, but I don't think it matters whether
we print the device name or fwnode path. If you have either one, you
can figure out both the driver and node. Arguably the fwnode path is
more useful because that's likely where the error is.

Rob

Re: [PATCH v4 5/9] rust: device: Introduce PropertyGuard

[Remo Senekowitsch]

On Mon May 5, 2025 at 5:37 PM CEST, Rob Herring wrote:
> On Mon, May 5, 2025 at 8:02 AM Remo Senekowitsch <remo@buenzli.dev> wrote:
>>
>> On Mon May 5, 2025 at 7:14 AM CEST, Dirk Behme wrote:
>> > On 04/05/2025 19:31, Remo Senekowitsch wrote:
>> >> This abstraction is a way to force users to specify whether a property
>> >> is supposed to be required or not. This allows us to move error
>> >> logging of missing required properties into core, preventing a lot of
>> >> boilerplate in drivers.
>> >>
>> >> It will be used by upcoming methods for reading device properties.
>> >>
>> >> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>> >> ---
>> >>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 59 insertions(+)
>> >>
>> >> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
>> >> index 6ccc7947f9c31..59c61e2493831 100644
>> >> --- a/rust/kernel/device/property.rs
>> >> +++ b/rust/kernel/device/property.rs
>> >> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>> >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>> >>      }
>> >>  }
>> >> +
>> >> +/// A helper for reading device properties.
>> >> +///
>> >> +/// Use [`Self::required_by`] if a missing property is considered a bug and
>> >> +/// [`Self::optional`] otherwise.
>> >> +///
>> >> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
>> >> +pub struct PropertyGuard<'fwnode, 'name, T> {
>> >> +    /// The result of reading the property.
>> >> +    inner: Result<T>,
>> >> +    /// The fwnode of the property, used for logging in the "required" case.
>> >> +    fwnode: &'fwnode FwNode,
>> >> +    /// The name of the property, used for logging in the "required" case.
>> >> +    name: &'name CStr,
>> >> +}
>> >> +
>> >> +impl<T> PropertyGuard<'_, '_, T> {
>> >> +    /// Access the property, indicating it is required.
>> >> +    ///
>> >> +    /// If the property is not present, the error is automatically logged. If a
>> >> +    /// missing property is not an error, use [`Self::optional`] instead. The
>> >> +    /// device is required to associate the log with it.
>> >> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
>> >> +        if self.inner.is_err() {
>> >> +            dev_err!(
>> >> +                dev,
>> >> +                "{}: property '{}' is missing\n",
>> >> +                self.fwnode.display_path(),
>> >> +                self.name
>> >> +            );
>> >> +        }
>> >> +        self.inner
>> >> +    }
>> >
>> > Thinking about the .required_by(dev) I wonder if there will be cases
>> > where we do *not* have a device? I.e. where we really have a fwnode,
>> > only. And therefore can't pass a device. If we have such cases do we
>> > need to be able to pass e.g. Option(dev) and switch back to pr_err() in
>> > case of None?
>>
>> In that case, bringing back the previous .required() method seems
>> reasonable to me. But only if we definitely know such cases exist.
>
> They definitely exist. Any property in a child node of the device's
> node when the child itself is not another device for example.

I don't think that counts, because you do have a device in that
situation. The log should be assicated with that. So callers are
responsible to propagate a reference to the device to wherever the call
to .required_by(dev) is happening.

>> > From the beginning of our discussion I think to remember that the C API
>> > has both the fwnode_property_*() and device_property_*() because there
>> > are use cases for the fwnode_property_*() API where is no device?
>
> Correct.
>
>> I'm not sure what you're referring to, the closest thing I can think of
>> is this comment by Rob [1] where he mentions the device_property_*()
>> functions only exist in C for a minimal convenience gain and we may not
>> want to keep that in Rust.
>
> The point there was if there's not the same convenience with Rust,
> then we shouldn't keep the API.
>
> I think this came up previously, but I don't think it matters whether
> we print the device name or fwnode path. If you have either one, you
> can figure out both the driver and node. Arguably the fwnode path is
> more useful because that's likely where the error is.
>
> Rob

Re: [PATCH v4 5/9] rust: device: Introduce PropertyGuard

[Danilo Krummrich]

On Mon, May 05, 2025 at 05:53:33PM +0200, Remo Senekowitsch wrote:
> On Mon May 5, 2025 at 5:37 PM CEST, Rob Herring wrote:
> > They definitely exist. Any property in a child node of the device's
> > node when the child itself is not another device for example.
> 
> I don't think that counts, because you do have a device in that
> situation. The log should be assicated with that. So callers are
> responsible to propagate a reference to the device to wherever the call
> to .required_by(dev) is happening.

Exactly, let's keep things as they are for now. We can still add further methods
when we run into a real use-case. Currently, I can't see one coming in any time
soon.

- Danilo

Re: [PATCH v4 5/9] rust: device: Introduce PropertyGuard

[Rob Herring]

On Mon, May 05, 2025 at 05:53:33PM +0200, Remo Senekowitsch wrote:
> On Mon May 5, 2025 at 5:37 PM CEST, Rob Herring wrote:
> > On Mon, May 5, 2025 at 8:02 AM Remo Senekowitsch <remo@buenzli.dev> wrote:
> >>
> >> On Mon May 5, 2025 at 7:14 AM CEST, Dirk Behme wrote:
> >> > On 04/05/2025 19:31, Remo Senekowitsch wrote:
> >> >> This abstraction is a way to force users to specify whether a property
> >> >> is supposed to be required or not. This allows us to move error
> >> >> logging of missing required properties into core, preventing a lot of
> >> >> boilerplate in drivers.
> >> >>
> >> >> It will be used by upcoming methods for reading device properties.
> >> >>
> >> >> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> >> >> ---
> >> >>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
> >> >>  1 file changed, 59 insertions(+)
> >> >>
> >> >> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> >> >> index 6ccc7947f9c31..59c61e2493831 100644
> >> >> --- a/rust/kernel/device/property.rs
> >> >> +++ b/rust/kernel/device/property.rs
> >> >> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >> >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> >> >>      }
> >> >>  }
> >> >> +
> >> >> +/// A helper for reading device properties.
> >> >> +///
> >> >> +/// Use [`Self::required_by`] if a missing property is considered a bug and
> >> >> +/// [`Self::optional`] otherwise.
> >> >> +///
> >> >> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> >> >> +pub struct PropertyGuard<'fwnode, 'name, T> {
> >> >> +    /// The result of reading the property.
> >> >> +    inner: Result<T>,
> >> >> +    /// The fwnode of the property, used for logging in the "required" case.
> >> >> +    fwnode: &'fwnode FwNode,
> >> >> +    /// The name of the property, used for logging in the "required" case.
> >> >> +    name: &'name CStr,
> >> >> +}
> >> >> +
> >> >> +impl<T> PropertyGuard<'_, '_, T> {
> >> >> +    /// Access the property, indicating it is required.
> >> >> +    ///
> >> >> +    /// If the property is not present, the error is automatically logged. If a
> >> >> +    /// missing property is not an error, use [`Self::optional`] instead. The
> >> >> +    /// device is required to associate the log with it.
> >> >> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
> >> >> +        if self.inner.is_err() {
> >> >> +            dev_err!(
> >> >> +                dev,
> >> >> +                "{}: property '{}' is missing\n",
> >> >> +                self.fwnode.display_path(),
> >> >> +                self.name
> >> >> +            );
> >> >> +        }
> >> >> +        self.inner
> >> >> +    }
> >> >
> >> > Thinking about the .required_by(dev) I wonder if there will be cases
> >> > where we do *not* have a device? I.e. where we really have a fwnode,
> >> > only. And therefore can't pass a device. If we have such cases do we
> >> > need to be able to pass e.g. Option(dev) and switch back to pr_err() in
> >> > case of None?
> >>
> >> In that case, bringing back the previous .required() method seems
> >> reasonable to me. But only if we definitely know such cases exist.
> >
> > They definitely exist. Any property in a child node of the device's
> > node when the child itself is not another device for example.
> 
> I don't think that counts, because you do have a device in that
> situation. The log should be assicated with that. So callers are
> responsible to propagate a reference to the device to wherever the call
> to .required_by(dev) is happening.

Ah, right. So it would just be cases that aren't a driver at all. That's 
limited to the OF_DECLARE cases. I agree we can worry about those later.

Rob

Re: [PATCH v4 0/9] More Rust bindings for device property reads

[Remo Senekowitsch]

I haven't gotten any actionable feedback on this version.
What's the next step?

Best regards,
Remo

Re: [PATCH v4 0/9] More Rust bindings for device property reads

[Danilo Krummrich]

On Mon, May 12, 2025 at 01:49:27PM +0200, Remo Senekowitsch wrote:
> I haven't gotten any actionable feedback on this version.
> What's the next step?

I wasn't around for the last days, but I'm catching up now. Will go through this
version soon.

- Danilo

Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Danilo Krummrich]

Please change the subject to

	"rust: device: implement accessors for firmware properties"

We don't really add bindings and the accessors are for FwNode.

On Sun, May 04, 2025 at 07:31:51PM +0200, Remo Senekowitsch wrote:
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
> 
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> types of properties where appropriate.

Please use imperative mood.

> 
> Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/device/property.rs | 232 ++++++++++++++++++++++++++++++++-
>  1 file changed, 230 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index 59c61e2493831..413166e2d082e 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -4,9 +4,16 @@
>  //!
>  //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
>  
> -use core::ptr;
> +use core::{mem::MaybeUninit, ptr};
>  
> -use crate::{bindings, str::CStr, types::Opaque};
> +use crate::{
> +    alloc::KVec,
> +    bindings,
> +    error::{to_result, Result},
> +    prelude::*,
> +    str::{CStr, CString},
> +    types::Opaque,
> +};
>  
>  /// A reference-counted fwnode_handle.
>  ///
> @@ -109,6 +116,105 @@ pub fn property_present(&self, name: &CStr) -> bool {
>          // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
>          unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
>      }
> +
> +    /// Returns firmware property `name` boolean value
> +    pub fn property_read_bool(&self, name: &CStr) -> bool {
> +        // SAFETY: `name` is non-null and null-terminated. `self.as_raw()` is valid
> +        // because `self` is valid.
> +        unsafe { bindings::fwnode_property_read_bool(self.as_raw(), name.as_char_ptr()) }
> +    }
> +
> +    /// Returns the index of matching string `match_str` for firmware string property `name`
> +    pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> +        // SAFETY: `name` and `match_str` are non-null and null-terminated. `self.as_raw` is
> +        // valid because `self` is valid.
> +        let ret = unsafe {
> +            bindings::fwnode_property_match_string(
> +                self.as_raw(),
> +                name.as_char_ptr(),
> +                match_str.as_char_ptr(),
> +            )
> +        };
> +        to_result(ret)?;
> +        Ok(ret as usize)
> +    }
> +
> +    /// Returns firmware property `name` integer array values in a KVec
> +    pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +        len: usize,
> +    ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
> +        let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
> +
> +        // SAFETY: `val.as_mut_ptr()` is valid because `KVec::with_capacity`
> +        // didn't return an error and it has at least space for `len` number
> +        // of elements.
> +        let err = unsafe { read_array_out_param::<T>(self, name, val.as_mut_ptr(), len) };
> +        let res = if err < 0 {
> +            Err(Error::from_errno(err))
> +        } else {
> +            // SAFETY: fwnode_property_read_int_array() writes exactly `len`
> +            // entries on success
> +            unsafe { val.set_len(len) }

Note: This conflicts with [1] from the alloc-next tree.

[1] https://github.com/Rust-for-Linux/linux/commit/88d5d6a38d5161228fbfe017eb94d777d5e8a0e4

> +            Ok(val)
> +        };
> +        Ok(PropertyGuard {
> +            inner: res,
> +            fwnode: self,
> +            name,
> +        })
> +    }
> +
> +    /// Returns integer array length for firmware property `name`
> +    pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
> +        // SAFETY: `out_param` is allowed to be null because `len` is zero.
> +        let ret = unsafe { read_array_out_param::<T>(self, name, ptr::null_mut(), 0) };
> +        to_result(ret)?;
> +        Ok(ret as usize)
> +    }
> +
> +    /// Returns the value of firmware property `name`.
> +    ///
> +    /// This method is generic over the type of value to read. Informally,
> +    /// the types that can be read are booleans, strings, unsigned integers and
> +    /// arrays of unsigned integers.
> +    ///
> +    /// Reading a `KVec` of integers is done with the separate

Nit: [`KVec`]

> +    /// method [`Self::property_read_array_vec`], because it takes an
> +    /// additional `len` argument.
> +    ///
> +    /// When reading a boolean, this method never fails. A missing property
> +    /// is interpreted as `false`, whereas a present property is interpreted
> +    /// as `true`.
> +    ///
> +    /// For more precise documentation about what types can be read, see
> +    /// the [implementors of Property][Property#implementors] and [its
> +    /// implementations on foreign types][Property#foreign-impls].
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::{c_str, device::{Device, property::FwNode}, str::CString};
> +    /// fn examples(dev: &Device) -> Result {
> +    ///     let fwnode = dev.fwnode().ok_or(ENOENT)?;
> +    ///     let b: u32 = fwnode.property_read(c_str!("some-number")).required_by(dev)?;
> +    ///     if let Some(s) = fwnode.property_read::<CString>(c_str!("some-str")).optional() {
> +    ///         // ...
> +    ///     }
> +    ///     Ok(())
> +    /// }
> +    /// ```
> +    pub fn property_read<'fwnode, 'name, T: Property>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, T> {
> +        PropertyGuard {
> +            inner: T::read_from_fwnode_property(self, name),
> +            fwnode: self,
> +            name,
> +        }
> +    }
>  }
>  
>  // SAFETY: Instances of `FwNode` are always reference-counted.
> @@ -124,6 +230,128 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>      }
>  }
>  
> +/// Implemented for several types that can be read as properties.

I'd drop "several".

> +///
> +/// Informally, this is implemented for strings, integers and arrays of

Why "informally"?

> +/// integers. It's used to make [`FwNode::property_read`] generic over the
> +/// type of property being read. There are also two dedicated methods to read
> +/// other types, because they require more specialized function signatures:
> +/// - [`property_read_bool`](Device::property_read_bool)
> +/// - [`property_read_array_vec`](Device::property_read_array_vec)

I think you dropped the Device variants.

> +pub trait Property: Sized {
> +    /// Used to make [`FwNode::property_read`] generic.
> +    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self>;
> +}
> +
> +impl Property for CString {
> +    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> +        let mut str: *mut u8 = ptr::null_mut();
> +        let pstr: *mut _ = &mut str;
> +
> +        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
> +        // valid because `fwnode` is valid.
> +        let ret = unsafe {
> +            bindings::fwnode_property_read_string(fwnode.as_raw(), name.as_char_ptr(), pstr.cast())
> +        };
> +        to_result(ret)?;
> +
> +        // SAFETY: `pstr` contains a non-null ptr on success
> +        let str = unsafe { CStr::from_char_ptr(*pstr) };

Formally, this safety comment does not satisfy the requirement of
CStr::from_char_ptr():

	`ptr` must be a valid pointer to a `NUL`-terminated C string, and it must
	last at least `'a`. When `CStr` is alive, the memory pointed by `ptr`
	must not be mutated.

> +        Ok(str.try_into()?)
> +    }
> +}
> +/// Implemented for all integers that can be read as properties.
> +///
> +/// This helper trait is needed on top of the existing [`Property`]
> +/// trait to associate the integer types of various sizes with their
> +/// corresponding `fwnode_property_read_*_array` functions.
> +pub trait PropertyInt: Copy {
> +    /// # Safety
> +    ///
> +    /// Callers must uphold the same safety invariants as for the various
> +    /// `fwnode_property_read_*_array` functions.

I think you have additional requirements on the fwnode, propname and val
pointers as well as on nval, please document them as well.

> +    unsafe fn read_array_from_fwnode_property(
> +        fwnode: *const bindings::fwnode_handle,
> +        propname: *const ffi::c_char,
> +        val: *mut Self,
> +        nval: usize,
> +    ) -> ffi::c_int;
> +}
> +// This macro generates implementations of the traits `Property` and
> +// `PropertyInt` for integers of various sizes. Its input is a list
> +// of pairs separated by commas. The first element of the pair is the
> +// type of the integer, the second one is the name of its corresponding
> +// `fwnode_property_read_*_array` function.
> +macro_rules! impl_property_for_int {
> +    ($($int:ty: $f:ident),* $(,)?) => { $(
> +        impl PropertyInt for $int {
> +            unsafe fn read_array_from_fwnode_property(
> +                fwnode: *const bindings::fwnode_handle,
> +                propname: *const ffi::c_char,
> +                val: *mut Self,
> +                nval: usize,
> +            ) -> ffi::c_int {
> +                // SAFETY: The safety invariants on the trait require
> +                // callers to uphold the invariants of the functions
> +                // this macro is called with.
> +                unsafe {
> +                    bindings::$f(fwnode, propname, val.cast(), nval)
> +                }
> +            }
> +        }
> +    )* };
> +}
> +impl_property_for_int! {
> +    u8: fwnode_property_read_u8_array,
> +    u16: fwnode_property_read_u16_array,
> +    u32: fwnode_property_read_u32_array,
> +    u64: fwnode_property_read_u64_array,
> +    i8: fwnode_property_read_u8_array,
> +    i16: fwnode_property_read_u16_array,
> +    i32: fwnode_property_read_u32_array,
> +    i64: fwnode_property_read_u64_array,
> +}
> +/// # Safety
> +///
> +/// Callers must ensure that if `len` is non-zero, `out_param` must be
> +/// valid and point to memory that has enough space to hold at least
> +/// `len` number of elements.
> +unsafe fn read_array_out_param<T: PropertyInt>(
> +    fwnode: &FwNode,
> +    name: &CStr,
> +    out_param: *mut T,
> +    len: usize,
> +) -> ffi::c_int {
> +    // SAFETY: `name` is non-null and null-terminated.
> +    // `fwnode.as_raw` is valid because `fwnode` is valid.
> +    // `out_param` is valid and has enough space for at least
> +    // `len` number of elements as per the safety requirement.
> +    unsafe {
> +        T::read_array_from_fwnode_property(fwnode.as_raw(), name.as_char_ptr(), out_param, len)
> +    }
> +}
> +impl<T: PropertyInt, const N: usize> Property for [T; N] {
> +    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> +        let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
> +
> +        // SAFETY: `val.as_mut_ptr()` is valid and points to enough space for
> +        // `N` elements. Casting from `*mut MaybeUninit<T>` to `*mut T` is safe
> +        // because `MaybeUninit<T>` has the same memory layout as `T`.

Please use `CAST:` for this.

> +        let ret = unsafe { read_array_out_param::<T>(fwnode, name, val.as_mut_ptr().cast(), N) };
> +        to_result(ret)?;
> +
> +        // SAFETY: `val` is always initialized when
> +        // fwnode_property_read_<T>_array is successful.
> +        Ok(val.map(|v| unsafe { v.assume_init() }))
> +    }
> +}
> +impl<T: PropertyInt> Property for T {
> +    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> +        let val: [_; 1] = <[T; 1] as Property>::read_from_fwnode_property(fwnode, name)?;
> +        Ok(val[0])
> +    }
> +}
> +
>  /// A helper for reading device properties.
>  ///
>  /// Use [`Self::required_by`] if a missing property is considered a bug and
> -- 
> 2.49.0
> 

Re: [PATCH v4 9/9] samples: rust: platform: Add property read examples

[Danilo Krummrich]

On Sun, May 04, 2025 at 07:31:54PM +0200, Remo Senekowitsch wrote:
> Add some example usage of the device property read methods for
> DT/ACPI/swnode properties.
> 
> Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  drivers/of/unittest-data/tests-platform.dtsi |  3 +
>  samples/rust/rust_driver_platform.rs         | 71 +++++++++++++++++++-
>  2 files changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
> index 4171f43cf01cc..50a51f38afb60 100644
> --- a/drivers/of/unittest-data/tests-platform.dtsi
> +++ b/drivers/of/unittest-data/tests-platform.dtsi
> @@ -37,6 +37,9 @@ dev@100 {
>  			test-device@2 {
>  				compatible = "test,rust-device";
>  				reg = <0x2>;
> +
> +				test,u32-prop = <0xdeadbeef>;
> +				test,i16-array = /bits/ 16 <1 2 (-3) (-4)>;
>  			};
>  		};
>  
> diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
> index 8b42b3cfb363a..a04ff4afb1325 100644
> --- a/samples/rust/rust_driver_platform.rs
> +++ b/samples/rust/rust_driver_platform.rs
> @@ -2,7 +2,7 @@
>  
>  //! Rust Platform driver sample.
>  
> -use kernel::{c_str, device::Core, of, platform, prelude::*, types::ARef};
> +use kernel::{c_str, device::Core, of, platform, prelude::*, str::CString, types::ARef};
>  
>  struct SampleDriver {
>      pdev: ARef<platform::Device>,
> @@ -25,18 +25,85 @@ fn probe(
>          pdev: &platform::Device<Core>,
>          info: Option<&Self::IdInfo>,
>      ) -> Result<Pin<KBox<Self>>> {
> +        let dev = pdev.as_ref();
> +
>          dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n");
>  
>          if let Some(info) = info {
> -            dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
> +            dev_info!(dev, "Probed with info: '{}'.\n", info.0);

You switch to use dev here, but not for dev_dbg() above.

>          }
>  
> +        Self::properties_parse(dev)?;

Let's just use pdev.as_ref() here too.

> +
>          let drvdata = KBox::new(Self { pdev: pdev.into() }, GFP_KERNEL)?;
>  
>          Ok(drvdata.into())
>      }
>  }
>  
> +impl SampleDriver {
> +    fn properties_parse(dev: &kernel::device::Device) -> Result<()> {

Please refer to this as &device::Device, i.e. import kernel::device. You should
also be able to just use Result, without the generic.

> +        let fwnode = dev.fwnode().ok_or(ENOENT)?;
> +
> +        if let Ok(idx) =
> +            fwnode.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
> +        {
> +            dev_info!(dev, "matched compatible string idx = {}\n", idx);
> +        }
> +
> +        if let Ok(str) = fwnode
> +            .property_read::<CString>(c_str!("compatible"))
> +            .required_by(dev)
> +        {
> +            dev_info!(dev, "compatible string = {:?}\n", str);
> +        }

And else? Why do you ignore a potential error?

> +
> +        let prop = fwnode.property_read_bool(c_str!("test,bool-prop"));
> +        dev_info!(dev, "bool prop is {}\n", prop);

Let's use a consistent style for all those prints, e.g. '$name'='$value'. For
instance:

	let name = c_str!("test,bool-prop");
	let prop = fwnode.property_read_bool(name);
	dev_info!(dev, "'{}'='{}'\n", name, prop);

> +        if fwnode.property_present(c_str!("test,u32-prop")) {
> +            dev_info!(dev, "'test,u32-prop' is present\n");

Given the above, I'd keep this one as it is.

> +        }
> +
> +        let prop = fwnode
> +            .property_read::<u32>(c_str!("test,u32-optional-prop"))
> +            .or(0x12);
> +        dev_info!(
> +            dev,
> +            "'test,u32-optional-prop' is {:#x} (default = {:#x})\n",
> +            prop,
> +            0x12
> +        );
> +
> +        // Missing property without a default will print an error

Maybe additionally add that you discard the Result intentionally in order to not
make properties_parse() fail in this case.

> +        let _ = fwnode
> +            .property_read::<u32>(c_str!("test,u32-required-prop"))
> +            .required_by(dev);
> +
> +        let prop: u32 = fwnode
> +            .property_read(c_str!("test,u32-prop"))
> +            .required_by(dev)?;
> +        dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
> +
> +        let prop: [i16; 4] = fwnode
> +            .property_read(c_str!("test,i16-array"))
> +            .required_by(dev)?;
> +        dev_info!(dev, "'test,i16-array' is {:?}\n", prop);
> +        dev_info!(
> +            dev,
> +            "'test,i16-array' length is {}\n",
> +            fwnode.property_count_elem::<u16>(c_str!("test,i16-array"))?,
> +        );
> +
> +        let prop: KVec<i16> = fwnode
> +            .property_read_array_vec(c_str!("test,i16-array"), 4)?
> +            .required_by(dev)?;
> +        dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop);
> +
> +        Ok(())
> +    }
> +}
> +
>  impl Drop for SampleDriver {
>      fn drop(&mut self) {
>          dev_dbg!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n");
> -- 
> 2.49.0
> 

Re: [PATCH v4 1/9] rust: device: Create FwNode abstraction for accessing device properties

[Danilo Krummrich]

On Sun, May 04, 2025 at 07:31:46PM +0200, Remo Senekowitsch wrote:
> Not all property-related APIs can be exposed directly on a device.
> For example, iterating over child nodes of a device will yield
> fwnode_handle. Thus, in order to access properties on these child nodes,
> the property access methods must be implemented on the abstraction over
> fwnode_handle.

This reads as if it is a snippet from a larger text.

Please start with a brief motivation of the patch (even if it is trivial) and,
using imperative mood, what's changed by the patch, then you can add why you do
things a certain way (and not another).

The same applies (more or less) for patch 2.

Re: [PATCH v4 1/9] rust: device: Create FwNode abstraction for accessing device properties

[Danilo Krummrich]

On Sun, May 04, 2025 at 07:31:46PM +0200, Remo Senekowitsch wrote:
>  rust/kernel/{device.rs => device/mod.rs} |  2 +
>  rust/kernel/device/property.rs           | 47 ++++++++++++++++++++++++

This one is my bad, I think I suggested rust/kernel/device/ and
rust/kernel/device/mod.rs.

Can you please change it to rust/kernel/device/ and rust/kernel/device.rs, like
we do everywhere else?

Thanks,
Danilo

Re: [PATCH v4 5/9] rust: device: Introduce PropertyGuard

[Rob Herring]

On Sun, May 04, 2025 at 07:31:50PM +0200, Remo Senekowitsch wrote:
> This abstraction is a way to force users to specify whether a property
> is supposed to be required or not. This allows us to move error
> logging of missing required properties into core, preventing a lot of
> boilerplate in drivers.
> 
> It will be used by upcoming methods for reading device properties.
> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/device/property.rs | 59 ++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index 6ccc7947f9c31..59c61e2493831 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -123,3 +123,62 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>      }
>  }
> +
> +/// A helper for reading device properties.
> +///
> +/// Use [`Self::required_by`] if a missing property is considered a bug and
> +/// [`Self::optional`] otherwise.
> +///
> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> +pub struct PropertyGuard<'fwnode, 'name, T> {
> +    /// The result of reading the property.
> +    inner: Result<T>,
> +    /// The fwnode of the property, used for logging in the "required" case.
> +    fwnode: &'fwnode FwNode,
> +    /// The name of the property, used for logging in the "required" case.
> +    name: &'name CStr,
> +}
> +
> +impl<T> PropertyGuard<'_, '_, T> {
> +    /// Access the property, indicating it is required.
> +    ///
> +    /// If the property is not present, the error is automatically logged. If a
> +    /// missing property is not an error, use [`Self::optional`] instead. The
> +    /// device is required to associate the log with it.
> +    pub fn required_by(self, dev: &super::Device) -> Result<T> {
> +        if self.inner.is_err() {
> +            dev_err!(
> +                dev,
> +                "{}: property '{}' is missing\n",
> +                self.fwnode.display_path(),

Is it possible to do "{self.fwnode}: property ..."?

Then if we want to modify the default, we could do something like 
"{self.fwnode:pn}"? It doesn't look like that's something which would 
work OOTB and is not something we need to solve now. So just the default 
formatter is good for now.

Rob

Re: [PATCH v4 3/9] rust: device: Move property_present() to FwNode

[Rob Herring]

On Sun, May 04, 2025 at 07:31:48PM +0200, Remo Senekowitsch wrote:
> The new FwNode abstraction will be used for accessing all device
> properties, so it must have the property_present method.
> 
> It's possible to duplicate the methods on the device itself, but since
> some of the methods on Device would have different type sigatures as the
> ones on FwNode, this would only lead to inconsistency and confusion.
> Hence, remove the method from Device.
> 
> There aren't any users to update yet.

But there is one going into 6.16 most likely with the cpufreq stuff[1] 
which complicates merging.

Rob

[1] https://lore.kernel.org/all/f7e96b7da77ac217be5ccb09b9309da28fd96c90.1745218976.git.viresh.kumar@linaro.org/

> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/device/mod.rs      | 7 -------
>  rust/kernel/device/property.rs | 8 +++++++-
>  2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/rust/kernel/device/mod.rs b/rust/kernel/device/mod.rs
> index b4b7056eb80f8..15d89cd45e871 100644
> --- a/rust/kernel/device/mod.rs
> +++ b/rust/kernel/device/mod.rs
> @@ -6,7 +6,6 @@
>  
>  use crate::{
>      bindings,
> -    str::CStr,
>      types::{ARef, Opaque},
>  };
>  use core::{fmt, marker::PhantomData, ptr};
> @@ -200,12 +199,6 @@ pub fn fwnode(&self) -> Option<&property::FwNode> {
>          // defined as a `#[repr(transparent)]` wrapper around `fwnode_handle`.
>          Some(unsafe { &*fwnode_handle.cast() })
>      }
> -
> -    /// Checks if property is present or not.
> -    pub fn property_present(&self, name: &CStr) -> bool {
> -        // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
> -        unsafe { bindings::device_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
> -    }
>  }
>  
>  // SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index e75d55f5856cf..70593343bd811 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -6,7 +6,7 @@
>  
>  use core::ptr;
>  
> -use crate::{bindings, types::Opaque};
> +use crate::{bindings, str::CStr, types::Opaque};
>  
>  /// A reference-counted fwnode_handle.
>  ///
> @@ -31,6 +31,12 @@ impl FwNode {
>      pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
>          self.0.get()
>      }
> +
> +    /// Checks if property is present or not.
> +    pub fn property_present(&self, name: &CStr) -> bool {
> +        // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
> +        unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
> +    }
>  }
>  
>  // SAFETY: Instances of `FwNode` are always reference-counted.
> -- 
> 2.49.0
> 

Re: [PATCH v4 3/9] rust: device: Move property_present() to FwNode

[Danilo Krummrich]

On Mon, May 12, 2025 at 12:29:57PM -0500, Rob Herring wrote:
> On Sun, May 04, 2025 at 07:31:48PM +0200, Remo Senekowitsch wrote:
> > The new FwNode abstraction will be used for accessing all device
> > properties, so it must have the property_present method.
> > 
> > It's possible to duplicate the methods on the device itself, but since
> > some of the methods on Device would have different type sigatures as the
> > ones on FwNode, this would only lead to inconsistency and confusion.
> > Hence, remove the method from Device.
> > 
> > There aren't any users to update yet.
> 
> But there is one going into 6.16 most likely with the cpufreq stuff[1] 
> which complicates merging.

Should we target the upcoming merge window, I suggest to just add
FwNode::property_present() and remove Device::property_present() with a
subsequent patch for v6.17.

Should we not make it (which given that there are a few open points and -rc7 is
rather close is not that unlikely), we can just rebase and merge early.

> 
> Rob
> 
> [1] https://lore.kernel.org/all/f7e96b7da77ac217be5ccb09b9309da28fd96c90.1745218976.git.viresh.kumar@linaro.org/

Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Remo Senekowitsch]

On Mon May 12, 2025 at 3:36 PM CEST, Danilo Krummrich wrote:
>> +/// Implemented for all integers that can be read as properties.
>> +///
>> +/// This helper trait is needed on top of the existing [`Property`]
>> +/// trait to associate the integer types of various sizes with their
>> +/// corresponding `fwnode_property_read_*_array` functions.
>> +pub trait PropertyInt: Copy {
>> +    /// # Safety
>> +    ///
>> +    /// Callers must uphold the same safety invariants as for the various
>> +    /// `fwnode_property_read_*_array` functions.
>
> I think you have additional requirements on the fwnode, propname and val
> pointers as well as on nval, please document them as well.

What are the additional requirements? The implementation just calls the
underlying `fwnode_property_read_*_array` with the exact same arguments,
so I don't know what the additional requirements are.

>> +    unsafe fn read_array_from_fwnode_property(
>> +        fwnode: *const bindings::fwnode_handle,
>> +        propname: *const ffi::c_char,
>> +        val: *mut Self,
>> +        nval: usize,
>> +    ) -> ffi::c_int;
>> +}
>> +// This macro generates implementations of the traits `Property` and
>> +// `PropertyInt` for integers of various sizes. Its input is a list
>> +// of pairs separated by commas. The first element of the pair is the
>> +// type of the integer, the second one is the name of its corresponding
>> +// `fwnode_property_read_*_array` function.
>> +macro_rules! impl_property_for_int {
>> +    ($($int:ty: $f:ident),* $(,)?) => { $(
>> +        impl PropertyInt for $int {
>> +            unsafe fn read_array_from_fwnode_property(
>> +                fwnode: *const bindings::fwnode_handle,
>> +                propname: *const ffi::c_char,
>> +                val: *mut Self,
>> +                nval: usize,
>> +            ) -> ffi::c_int {
>> +                // SAFETY: The safety invariants on the trait require
>> +                // callers to uphold the invariants of the functions
>> +                // this macro is called with.
>> +                unsafe {
>> +                    bindings::$f(fwnode, propname, val.cast(), nval)
>> +                }
>> +            }
>> +        }
>> +    )* };
>> +}
>> +impl_property_for_int! {
>> +    u8: fwnode_property_read_u8_array,
>> +    u16: fwnode_property_read_u16_array,
>> +    u32: fwnode_property_read_u32_array,
>> +    u64: fwnode_property_read_u64_array,
>> +    i8: fwnode_property_read_u8_array,
>> +    i16: fwnode_property_read_u16_array,
>> +    i32: fwnode_property_read_u32_array,
>> +    i64: fwnode_property_read_u64_array,
>> +}

Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Danilo Krummrich]

On Mon, May 19, 2025 at 05:43:17PM +0200, Remo Senekowitsch wrote:
> On Mon May 12, 2025 at 3:36 PM CEST, Danilo Krummrich wrote:
> >> +/// Implemented for all integers that can be read as properties.
> >> +///
> >> +/// This helper trait is needed on top of the existing [`Property`]
> >> +/// trait to associate the integer types of various sizes with their
> >> +/// corresponding `fwnode_property_read_*_array` functions.
> >> +pub trait PropertyInt: Copy {
> >> +    /// # Safety
> >> +    ///
> >> +    /// Callers must uphold the same safety invariants as for the various
> >> +    /// `fwnode_property_read_*_array` functions.
> >
> > I think you have additional requirements on the fwnode, propname and val
> > pointers as well as on nval, please document them as well.
> 
> What are the additional requirements? The implementation just calls the
> underlying `fwnode_property_read_*_array` with the exact same arguments,
> so I don't know what the additional requirements are.

First of all, I don't think you can refer to the safety requirements of the
`fwnode_property_read_*_array` functions, since they don't have any documented
safety requirements.

So, I think you have safety requirements regarding pointer validity of fwnode,
propname and val.

Additionally, there's the requirement that val has to be an array of nval
length.

Also, the PropertyInt trait itself has to be unsafe, given that it contains
unsafe functions.

I also pinged Benno about it, he usually knows best how to cover such things
properly. :)

> >> +    unsafe fn read_array_from_fwnode_property(
> >> +        fwnode: *const bindings::fwnode_handle,
> >> +        propname: *const ffi::c_char,
> >> +        val: *mut Self,
> >> +        nval: usize,
> >> +    ) -> ffi::c_int;
> >> +}

Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Remo Senekowitsch]

On Mon May 19, 2025 at 6:55 PM CEST, Danilo Krummrich wrote:
> On Mon, May 19, 2025 at 05:43:17PM +0200, Remo Senekowitsch wrote:
>> On Mon May 12, 2025 at 3:36 PM CEST, Danilo Krummrich wrote:
>> >> +/// Implemented for all integers that can be read as properties.
>> >> +///
>> >> +/// This helper trait is needed on top of the existing [`Property`]
>> >> +/// trait to associate the integer types of various sizes with their
>> >> +/// corresponding `fwnode_property_read_*_array` functions.
>> >> +pub trait PropertyInt: Copy {
>> >> +    /// # Safety
>> >> +    ///
>> >> +    /// Callers must uphold the same safety invariants as for the various
>> >> +    /// `fwnode_property_read_*_array` functions.
>> >
>> > I think you have additional requirements on the fwnode, propname and val
>> > pointers as well as on nval, please document them as well.
>> 
>> What are the additional requirements? The implementation just calls the
>> underlying `fwnode_property_read_*_array` with the exact same arguments,
>> so I don't know what the additional requirements are.
>
> First of all, I don't think you can refer to the safety requirements of the
> `fwnode_property_read_*_array` functions, since they don't have any documented
> safety requirements.
>
> So, I think you have safety requirements regarding pointer validity of fwnode,
> propname and val.
>
> Additionally, there's the requirement that val has to be an array of nval
> length.
>
> Also, the PropertyInt trait itself has to be unsafe, given that it contains
> unsafe functions.

I don't think a trait necessarily has to be marked unsafe just because
it has unsafe methods. Marking a trait as unsafe means that implementors
of the trait must uphold some invariants. This is not the case here
IIUC. Here's a good explanation of my understanding: [1]

But I should anyway seal the two traits. They're not supposed to be
implemented outside the kernel crate.

[1] https://users.rust-lang.org/t/safe-trait-with-an-unsafe-method/67993/3

> I also pinged Benno about it, he usually knows best how to cover such things
> properly. :)
>
>> >> +    unsafe fn read_array_from_fwnode_property(
>> >> +        fwnode: *const bindings::fwnode_handle,
>> >> +        propname: *const ffi::c_char,
>> >> +        val: *mut Self,
>> >> +        nval: usize,
>> >> +    ) -> ffi::c_int;
>> >> +}

Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Benno Lossin]

On Mon May 19, 2025 at 9:51 PM CEST, Remo Senekowitsch wrote:
> On Mon May 19, 2025 at 6:55 PM CEST, Danilo Krummrich wrote:
>> On Mon, May 19, 2025 at 05:43:17PM +0200, Remo Senekowitsch wrote:
>>> On Mon May 12, 2025 at 3:36 PM CEST, Danilo Krummrich wrote:
>>> >> +/// Implemented for all integers that can be read as properties.
>>> >> +///
>>> >> +/// This helper trait is needed on top of the existing [`Property`]
>>> >> +/// trait to associate the integer types of various sizes with their
>>> >> +/// corresponding `fwnode_property_read_*_array` functions.
>>> >> +pub trait PropertyInt: Copy {
>>> >> +    /// # Safety
>>> >> +    ///
>>> >> +    /// Callers must uphold the same safety invariants as for the various
>>> >> +    /// `fwnode_property_read_*_array` functions.
>>> >
>>> > I think you have additional requirements on the fwnode, propname and val
>>> > pointers as well as on nval, please document them as well.
>>> 
>>> What are the additional requirements? The implementation just calls the
>>> underlying `fwnode_property_read_*_array` with the exact same arguments,
>>> so I don't know what the additional requirements are.
>>
>> First of all, I don't think you can refer to the safety requirements of the
>> `fwnode_property_read_*_array` functions, since they don't have any documented
>> safety requirements.

Yes. We do sometimes link to other function for safety requirements if
they are very repetitive, but in that case we use a rustdoc link (so
[`my_other_function`]).

In this case, one doesn't even have a name to search for, only the
`fwnode_property_read_` prefix (and who is going to bother doing that?).
Additionally, the functions that you are referring to are from the
`bindings`! Those functions do not have safety documentation!

Another thing, functions do not have safety invariants, they only have
safety requirements and guarantees.

>> So, I think you have safety requirements regarding pointer validity of fwnode,
>> propname and val.
>>
>> Additionally, there's the requirement that val has to be an array of nval
>> length.

Yes these two are probably required, but to be sure one would have to
dig through the C code.

>> Also, the PropertyInt trait itself has to be unsafe, given that it contains
>> unsafe functions.
>
> I don't think a trait necessarily has to be marked unsafe just because
> it has unsafe methods. Marking a trait as unsafe means that implementors
> of the trait must uphold some invariants. This is not the case here
> IIUC. Here's a good explanation of my understanding: [1]

Yes this is correct, I don't think that the trait itself should be
unsafe.

> But I should anyway seal the two traits. They're not supposed to be
> implemented outside the kernel crate.

Yes that sounds like a good idea.

I'll send a separate email for some more comments on the design.

---
Cheers,
Benno

> [1] https://users.rust-lang.org/t/safe-trait-with-an-unsafe-method/67993/3
>
>> I also pinged Benno about it, he usually knows best how to cover such things
>> properly. :)
>>
>>> >> +    unsafe fn read_array_from_fwnode_property(
>>> >> +        fwnode: *const bindings::fwnode_handle,
>>> >> +        propname: *const ffi::c_char,
>>> >> +        val: *mut Self,
>>> >> +        nval: usize,
>>> >> +    ) -> ffi::c_int;
>>> >> +}

Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Benno Lossin]

On Sun May 4, 2025 at 7:31 PM CEST, Remo Senekowitsch wrote:
> +/// Implemented for all integers that can be read as properties.
> +///
> +/// This helper trait is needed on top of the existing [`Property`]
> +/// trait to associate the integer types of various sizes with their
> +/// corresponding `fwnode_property_read_*_array` functions.
> +pub trait PropertyInt: Copy {
> +    /// # Safety
> +    ///
> +    /// Callers must uphold the same safety invariants as for the various
> +    /// `fwnode_property_read_*_array` functions.
> +    unsafe fn read_array_from_fwnode_property(
> +        fwnode: *const bindings::fwnode_handle,
> +        propname: *const ffi::c_char,
> +        val: *mut Self,
> +        nval: usize,
> +    ) -> ffi::c_int;

I really, really dislike that this trait has to have an unsafe function
with all those raw pointer inputs.

> +}
> +// This macro generates implementations of the traits `Property` and
> +// `PropertyInt` for integers of various sizes. Its input is a list
> +// of pairs separated by commas. The first element of the pair is the
> +// type of the integer, the second one is the name of its corresponding
> +// `fwnode_property_read_*_array` function.
> +macro_rules! impl_property_for_int {
> +    ($($int:ty: $f:ident),* $(,)?) => { $(
> +        impl PropertyInt for $int {
> +            unsafe fn read_array_from_fwnode_property(
> +                fwnode: *const bindings::fwnode_handle,
> +                propname: *const ffi::c_char,
> +                val: *mut Self,
> +                nval: usize,
> +            ) -> ffi::c_int {
> +                // SAFETY: The safety invariants on the trait require
> +                // callers to uphold the invariants of the functions
> +                // this macro is called with.
> +                unsafe {
> +                    bindings::$f(fwnode, propname, val.cast(), nval)
> +                }
> +            }
> +        }
> +    )* };
> +}
> +impl_property_for_int! {
> +    u8: fwnode_property_read_u8_array,
> +    u16: fwnode_property_read_u16_array,
> +    u32: fwnode_property_read_u32_array,
> +    u64: fwnode_property_read_u64_array,
> +    i8: fwnode_property_read_u8_array,
> +    i16: fwnode_property_read_u16_array,
> +    i32: fwnode_property_read_u32_array,
> +    i64: fwnode_property_read_u64_array,
> +}
> +/// # Safety
> +///
> +/// Callers must ensure that if `len` is non-zero, `out_param` must be
> +/// valid and point to memory that has enough space to hold at least
> +/// `len` number of elements.
> +unsafe fn read_array_out_param<T: PropertyInt>(
> +    fwnode: &FwNode,
> +    name: &CStr,
> +    out_param: *mut T,
> +    len: usize,
> +) -> ffi::c_int {
> +    // SAFETY: `name` is non-null and null-terminated.
> +    // `fwnode.as_raw` is valid because `fwnode` is valid.
> +    // `out_param` is valid and has enough space for at least
> +    // `len` number of elements as per the safety requirement.
> +    unsafe {
> +        T::read_array_from_fwnode_property(fwnode.as_raw(), name.as_char_ptr(), out_param, len)
> +    }
> +}

Why does this function exist? It doesn't do anything and just delegates
the call to `T::read_array_from_fwnode_property`.

This feels like you're dragging the C interface through the lower layers
of your Rust abstractions, which I wouldn't do. I also looked a bit at
the C code and saw this comment in `driver/base/property.c:324`:

     * It's recommended to call fwnode_property_count_u8() instead of calling
     * this function with @val equals %NULL and @nval equals 0.

That probably holds also for the other functions, so maybe we should do
that instead? (although `fwnode_property_count_u8` just delegates and
calls with `fwnode_property_read_u8_array`...)

How about we do it like this:

    pub trait PropertyInt: Copy + Sized + private::Sealed {
        /// ...
        ///
        /// Returns a reference to `out` containing all written elements.
        fn read<'a>(
            fwnode: &FwNode,
            name: &CStr,
            out: &'a mut [MaybeUninit<Self>],
        ) -> Result<&'a mut [Self]>;
    
        fn length(fwnode: &FwNode, name: &CStr) -> Result<usize>;
    }

And then have a macro to implement it on all the integers.

Hope this helps!

---
Cheers,
Benno

Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Benno Lossin]

On Tue May 20, 2025 at 9:21 AM CEST, Benno Lossin wrote:
> On Mon May 19, 2025 at 9:51 PM CEST, Remo Senekowitsch wrote:
>> On Mon May 19, 2025 at 6:55 PM CEST, Danilo Krummrich wrote:
>>> Also, the PropertyInt trait itself has to be unsafe, given that it contains
>>> unsafe functions.
>>
>> I don't think a trait necessarily has to be marked unsafe just because
>> it has unsafe methods. Marking a trait as unsafe means that implementors
>> of the trait must uphold some invariants. This is not the case here
>> IIUC. Here's a good explanation of my understanding: [1]
>
> Yes this is correct, I don't think that the trait itself should be
> unsafe.

Ahh, I understood now why Danilo suggested this: if the trait should
guarantee that `fwnode_property_read_*_array` is called, then the trait
would have to be `unsafe`.

But I don't think that's necessary, we don't have any other unsafe code
that needs to rely on that.

---
Cheers,
Benno

Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Remo Senekowitsch]

On Tue May 20, 2025 at 9:37 AM CEST, Benno Lossin wrote:
> On Sun May 4, 2025 at 7:31 PM CEST, Remo Senekowitsch wrote:
>> +/// Implemented for all integers that can be read as properties.
>> +///
>> +/// This helper trait is needed on top of the existing [`Property`]
>> +/// trait to associate the integer types of various sizes with their
>> +/// corresponding `fwnode_property_read_*_array` functions.
>> +pub trait PropertyInt: Copy {
>> +    /// # Safety
>> +    ///
>> +    /// Callers must uphold the same safety invariants as for the various
>> +    /// `fwnode_property_read_*_array` functions.
>> +    unsafe fn read_array_from_fwnode_property(
>> +        fwnode: *const bindings::fwnode_handle,
>> +        propname: *const ffi::c_char,
>> +        val: *mut Self,
>> +        nval: usize,
>> +    ) -> ffi::c_int;
>
> I really, really dislike that this trait has to have an unsafe function
> with all those raw pointer inputs.

What is the concrete problem with that? Or is it just "not pretty"?
(I'm fine with making it prettier, just making sure I understand
correctly.)

>> +}
>> +// This macro generates implementations of the traits `Property` and
>> +// `PropertyInt` for integers of various sizes. Its input is a list
>> +// of pairs separated by commas. The first element of the pair is the
>> +// type of the integer, the second one is the name of its corresponding
>> +// `fwnode_property_read_*_array` function.
>> +macro_rules! impl_property_for_int {
>> +    ($($int:ty: $f:ident),* $(,)?) => { $(
>> +        impl PropertyInt for $int {
>> +            unsafe fn read_array_from_fwnode_property(
>> +                fwnode: *const bindings::fwnode_handle,
>> +                propname: *const ffi::c_char,
>> +                val: *mut Self,
>> +                nval: usize,
>> +            ) -> ffi::c_int {
>> +                // SAFETY: The safety invariants on the trait require
>> +                // callers to uphold the invariants of the functions
>> +                // this macro is called with.
>> +                unsafe {
>> +                    bindings::$f(fwnode, propname, val.cast(), nval)
>> +                }
>> +            }
>> +        }
>> +    )* };
>> +}
>> +impl_property_for_int! {
>> +    u8: fwnode_property_read_u8_array,
>> +    u16: fwnode_property_read_u16_array,
>> +    u32: fwnode_property_read_u32_array,
>> +    u64: fwnode_property_read_u64_array,
>> +    i8: fwnode_property_read_u8_array,
>> +    i16: fwnode_property_read_u16_array,
>> +    i32: fwnode_property_read_u32_array,
>> +    i64: fwnode_property_read_u64_array,
>> +}
>> +/// # Safety
>> +///
>> +/// Callers must ensure that if `len` is non-zero, `out_param` must be
>> +/// valid and point to memory that has enough space to hold at least
>> +/// `len` number of elements.
>> +unsafe fn read_array_out_param<T: PropertyInt>(
>> +    fwnode: &FwNode,
>> +    name: &CStr,
>> +    out_param: *mut T,
>> +    len: usize,
>> +) -> ffi::c_int {
>> +    // SAFETY: `name` is non-null and null-terminated.
>> +    // `fwnode.as_raw` is valid because `fwnode` is valid.
>> +    // `out_param` is valid and has enough space for at least
>> +    // `len` number of elements as per the safety requirement.
>> +    unsafe {
>> +        T::read_array_from_fwnode_property(fwnode.as_raw(), name.as_char_ptr(), out_param, len)
>> +    }
>> +}
>
> Why does this function exist? It doesn't do anything and just delegates
> the call to `T::read_array_from_fwnode_property`.

It's used in three places. Taking Rust references as input reduces the
safety requirements the callers must uphold. But I guess it's a small
benefit, I can remove it.

> This feels like you're dragging the C interface through the lower layers
> of your Rust abstractions, which I wouldn't do. I also looked a bit at
> the C code and saw this comment in `driver/base/property.c:324`:
>
>      * It's recommended to call fwnode_property_count_u8() instead of calling
>      * this function with @val equals %NULL and @nval equals 0.
>
> That probably holds also for the other functions, so maybe we should do
> that instead? (although `fwnode_property_count_u8` just delegates and
> calls with `fwnode_property_read_u8_array`...)

Yeah, I saw that too. The original implementation is from Rob. I assume
the recommendation is for users, maybe for clarity and readability of
the code. These Rust abstractions are pretty low level, so I thought
it's probably fine.

> How about we do it like this:
>
>     pub trait PropertyInt: Copy + Sized + private::Sealed {
>         /// ...
>         ///
>         /// Returns a reference to `out` containing all written elements.
>         fn read<'a>(
>             fwnode: &FwNode,
>             name: &CStr,
>             out: &'a mut [MaybeUninit<Self>],
>         ) -> Result<&'a mut [Self]>;
>
>         fn length(fwnode: &FwNode, name: &CStr) -> Result<usize>;
>     }
>
> And then have a macro to implement it on all the integers.

I think I tried to make the macro as small as possible and use regular
generics on top, because I was concerned people would be put off by big
complicated macros. I'm fine with moving the actual trait implementation
into the macro body, seems reasonable.

But still, I'm not sure why we're trying to make the PropertyInt trait's
interface pretty or rusty. It's not supposed to be used, implemented or
in any way interacted with outside this module. It's just a low-level
building block to make some functions generic, giving users type
inference.

Best regards,
Remo

Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Remo Senekowitsch]

On Tue May 20, 2025 at 9:40 AM CEST, Benno Lossin wrote:
> On Tue May 20, 2025 at 9:21 AM CEST, Benno Lossin wrote:
>> On Mon May 19, 2025 at 9:51 PM CEST, Remo Senekowitsch wrote:
>>> On Mon May 19, 2025 at 6:55 PM CEST, Danilo Krummrich wrote:
>>>> Also, the PropertyInt trait itself has to be unsafe, given that it contains
>>>> unsafe functions.
>>>
>>> I don't think a trait necessarily has to be marked unsafe just because
>>> it has unsafe methods. Marking a trait as unsafe means that implementors
>>> of the trait must uphold some invariants. This is not the case here
>>> IIUC. Here's a good explanation of my understanding: [1]
>>
>> Yes this is correct, I don't think that the trait itself should be
>> unsafe.
>
> Ahh, I understood now why Danilo suggested this: if the trait should
> guarantee that `fwnode_property_read_*_array` is called, then the trait
> would have to be `unsafe`.

Oh yeah, that's true. Callers rely on the fact that if the function
returns zero, the requested number of elements have been initialized.
That's definitely a safety invariant implementors must uphold...
So I guess marking the trait unsafe is correct.

> But I don't think that's necessary, we don't have any other unsafe code
> that needs to rely on that.

Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

[Benno Lossin]

On Tue May 20, 2025 at 12:32 PM CEST, Remo Senekowitsch wrote:
> On Tue May 20, 2025 at 9:37 AM CEST, Benno Lossin wrote:
>> On Sun May 4, 2025 at 7:31 PM CEST, Remo Senekowitsch wrote:
>>> +/// Implemented for all integers that can be read as properties.
>>> +///
>>> +/// This helper trait is needed on top of the existing [`Property`]
>>> +/// trait to associate the integer types of various sizes with their
>>> +/// corresponding `fwnode_property_read_*_array` functions.
>>> +pub trait PropertyInt: Copy {
>>> +    /// # Safety
>>> +    ///
>>> +    /// Callers must uphold the same safety invariants as for the various
>>> +    /// `fwnode_property_read_*_array` functions.
>>> +    unsafe fn read_array_from_fwnode_property(
>>> +        fwnode: *const bindings::fwnode_handle,
>>> +        propname: *const ffi::c_char,
>>> +        val: *mut Self,
>>> +        nval: usize,
>>> +    ) -> ffi::c_int;
>>
>> I really, really dislike that this trait has to have an unsafe function
>> with all those raw pointer inputs.
>
> What is the concrete problem with that? Or is it just "not pretty"?
> (I'm fine with making it prettier, just making sure I understand
> correctly.)

With the design I explained below, this becomes not necessary. Less
unsafe code always is better, since there are fewer places that can go
wrong.

Also hiding all the unsafe code inside of your module as private code or
implementation detail is also much better, as you prevent people from
accidentally using it.

>>> +}
>>> +// This macro generates implementations of the traits `Property` and
>>> +// `PropertyInt` for integers of various sizes. Its input is a list
>>> +// of pairs separated by commas. The first element of the pair is the
>>> +// type of the integer, the second one is the name of its corresponding
>>> +// `fwnode_property_read_*_array` function.
>>> +macro_rules! impl_property_for_int {
>>> +    ($($int:ty: $f:ident),* $(,)?) => { $(
>>> +        impl PropertyInt for $int {
>>> +            unsafe fn read_array_from_fwnode_property(
>>> +                fwnode: *const bindings::fwnode_handle,
>>> +                propname: *const ffi::c_char,
>>> +                val: *mut Self,
>>> +                nval: usize,
>>> +            ) -> ffi::c_int {
>>> +                // SAFETY: The safety invariants on the trait require
>>> +                // callers to uphold the invariants of the functions
>>> +                // this macro is called with.
>>> +                unsafe {
>>> +                    bindings::$f(fwnode, propname, val.cast(), nval)
>>> +                }
>>> +            }
>>> +        }
>>> +    )* };
>>> +}
>>> +impl_property_for_int! {
>>> +    u8: fwnode_property_read_u8_array,
>>> +    u16: fwnode_property_read_u16_array,
>>> +    u32: fwnode_property_read_u32_array,
>>> +    u64: fwnode_property_read_u64_array,
>>> +    i8: fwnode_property_read_u8_array,
>>> +    i16: fwnode_property_read_u16_array,
>>> +    i32: fwnode_property_read_u32_array,
>>> +    i64: fwnode_property_read_u64_array,
>>> +}
>>> +/// # Safety
>>> +///
>>> +/// Callers must ensure that if `len` is non-zero, `out_param` must be
>>> +/// valid and point to memory that has enough space to hold at least
>>> +/// `len` number of elements.
>>> +unsafe fn read_array_out_param<T: PropertyInt>(
>>> +    fwnode: &FwNode,
>>> +    name: &CStr,
>>> +    out_param: *mut T,
>>> +    len: usize,
>>> +) -> ffi::c_int {
>>> +    // SAFETY: `name` is non-null and null-terminated.
>>> +    // `fwnode.as_raw` is valid because `fwnode` is valid.
>>> +    // `out_param` is valid and has enough space for at least
>>> +    // `len` number of elements as per the safety requirement.
>>> +    unsafe {
>>> +        T::read_array_from_fwnode_property(fwnode.as_raw(), name.as_char_ptr(), out_param, len)
>>> +    }
>>> +}
>>
>> Why does this function exist? It doesn't do anything and just delegates
>> the call to `T::read_array_from_fwnode_property`.
>
> It's used in three places. Taking Rust references as input reduces the
> safety requirements the callers must uphold. But I guess it's a small
> benefit, I can remove it.

That's true, but why not make the trait take references if all users
will use references anyways :)

>> This feels like you're dragging the C interface through the lower layers
>> of your Rust abstractions, which I wouldn't do. I also looked a bit at
>> the C code and saw this comment in `driver/base/property.c:324`:
>>
>>      * It's recommended to call fwnode_property_count_u8() instead of calling
>>      * this function with @val equals %NULL and @nval equals 0.
>>
>> That probably holds also for the other functions, so maybe we should do
>> that instead? (although `fwnode_property_count_u8` just delegates and
>> calls with `fwnode_property_read_u8_array`...)
>
> Yeah, I saw that too. The original implementation is from Rob. I assume
> the recommendation is for users, maybe for clarity and readability of
> the code. These Rust abstractions are pretty low level, so I thought
> it's probably fine.

Yeah it's probably fine.

>> How about we do it like this:
>>
>>     pub trait PropertyInt: Copy + Sized + private::Sealed {
>>         /// ...
>>         ///
>>         /// Returns a reference to `out` containing all written elements.
>>         fn read<'a>(
>>             fwnode: &FwNode,
>>             name: &CStr,
>>             out: &'a mut [MaybeUninit<Self>],
>>         ) -> Result<&'a mut [Self]>;
>>
>>         fn length(fwnode: &FwNode, name: &CStr) -> Result<usize>;
>>     }
>>
>> And then have a macro to implement it on all the integers.
>
> I think I tried to make the macro as small as possible and use regular
> generics on top, because I was concerned people would be put off by big
> complicated macros. I'm fine with moving the actual trait implementation
> into the macro body, seems reasonable.

Big macro bodies are rarely the issue. Lots of parsing in declarative
macros is where they become unreadable.

> But still, I'm not sure why we're trying to make the PropertyInt trait's
> interface pretty or rusty. It's not supposed to be used, implemented or
> in any way interacted with outside this module. It's just a low-level
> building block to make some functions generic, giving users type
> inference.

Sure, but if we can make it safe, we should. I could also very much
imagine that someone has a statically allocated buffer they would want
to write stuff to and this function would then allow that with much
easier `unsafe` code than the current approach.

---
Cheers,
Benno