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

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

[Remo Senekowitsch]

More Rust bindings for device property reads

This adds more Rust bindings for reading device properties, based on
Rob Herring's work. I'm working on a driver[1] that uses these, but the
driver has more dependencies than this.

Rob Herring and Dirk Behme did several review iterations over on Zulip
already[1], I'm thankful for their great input.

This is my first time posting to the mailing list, please let me know if
I did anything wrong.

Remo

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

[PATCH 01/10] rust: Move property_present to property.rs

[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 APIs has to be duplicated on a fwnode as they are in C.

A related discussion can be found on the R4L Zulip[1].

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

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/helpers/helpers.c  |  1 +
 rust/helpers/property.c | 13 ++++++++
 rust/kernel/device.rs   |  7 ----
 rust/kernel/lib.rs      |  1 +
 rust/kernel/property.rs | 73 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 88 insertions(+), 7 deletions(-)
 create mode 100644 rust/helpers/property.c
 create mode 100644 rust/kernel/property.rs

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0640b7e11..b4eec5bf2 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -23,6 +23,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 000000000..c37c74488
--- /dev/null
+++ b/rust/helpers/property.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/property.h>
+
+struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
+{
+	return dev_fwnode(dev);
+}
+
+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.rs
index db2d9658b..d5e6a19ff 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,7 +6,6 @@
 
 use crate::{
     bindings,
-    str::CStr,
     types::{ARef, Opaque},
 };
 use core::{fmt, ptr};
@@ -181,12 +180,6 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
             )
         };
     }
-
-    /// 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: Instances of `Device` are always reference-counted.
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0..ca233fd20 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -67,6 +67,7 @@
 pub mod platform;
 pub mod prelude;
 pub mod print;
+pub mod property;
 pub mod rbtree;
 pub mod revocable;
 pub mod security;
diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
new file mode 100644
index 000000000..b0a4bb63a
--- /dev/null
+++ b/rust/kernel/property.rs
@@ -0,0 +1,73 @@
+// 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, device::Device, str::CStr, types::Opaque};
+
+impl Device {
+    /// Obtain the fwnode corresponding to the device.
+    fn fwnode(&self) -> &FwNode {
+        // SAFETY: `self` is valid.
+        let fwnode_handle = unsafe { bindings::dev_fwnode(self.as_raw()) };
+        if fwnode_handle.is_null() {
+            panic!("fwnode_handle cannot be null");
+        }
+        // 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.
+        unsafe { &*fwnode_handle.cast() }
+    }
+
+    /// Checks if property is present or not.
+    pub fn property_present(&self, name: &CStr) -> bool {
+        self.fwnode().property_present(name)
+    }
+}
+
+/// 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()
+    }
+
+    /// 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.
+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 02/10] rust: Add an Integer trait

[Remo Senekowitsch]

From: "Rob Herring (Arm)" <robh@kernel.org>

Add an "Integer" trait similar to crate::num::Integer. This is useful
for implementing generic methods which operate on different sizes of
integers. One example is reading DT/ACPI firmware properties.

This was originally proposed by Alice Ryhl[1].

[1] https://lore.kernel.org/rust-for-linux/CAH5fLgiXPZqKpWSSNdx-Ww-E9h2tOLcF3_8Y4C_JQ0eU8EMwFw@mail.gmail.com/

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 rust/kernel/types.rs | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 2bbaab83b..21647b7ba 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -3,10 +3,11 @@
 //! Kernel types.
 
 use crate::init::{self, PinInit};
+use crate::transmute::{AsBytes, FromBytes};
 use core::{
     cell::UnsafeCell,
     marker::{PhantomData, PhantomPinned},
-    mem::{ManuallyDrop, MaybeUninit},
+    mem::{size_of, ManuallyDrop, MaybeUninit},
     ops::{Deref, DerefMut},
     ptr::NonNull,
 };
@@ -553,6 +554,25 @@ pub enum Either<L, R> {
     Right(R),
 }
 
+/// Trait defined for all integer types similar to `crate::num::Integer`
+pub trait Integer: FromBytes + AsBytes + Copy {
+    /// Size of the integer in bytes
+    const SIZE: usize;
+}
+
+macro_rules! impl_int {
+    ($($typ:ty),* $(,)?) => {$(
+        impl Integer for $typ {
+            const SIZE: usize = size_of::<Self>();
+        }
+    )*};
+}
+
+impl_int! {
+    u8, u16, u32, u64, usize,
+    i8, i16, i32, i64, isize,
+}
+
 /// Zero-sized type to mark types not [`Send`].
 ///
 /// Add this type as a field to your struct if your type should not be sent to a different task.
-- 
2.49.0

[PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Remo Senekowitsch]

The rust bindings for reading device properties has a single
implementation supporting differing sizes of integers. The fwnode C API
already has a similar interface, but it is not exposed with the
fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.

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/base/property.c  | 9 +++++----
 include/linux/property.h | 2 ++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c1392743d..64d5b7055 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -292,10 +292,10 @@ int device_property_match_string(const struct device *dev, const char *propname,
 }
 EXPORT_SYMBOL_GPL(device_property_match_string);
 
-static int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
-					  const char *propname,
-					  unsigned int elem_size, void *val,
-					  size_t nval)
+int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
+				   const char *propname,
+				   unsigned int elem_size, void *val,
+				   size_t nval)
 {
 	int ret;
 
@@ -310,6 +310,7 @@ static int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
 	return fwnode_call_int_op(fwnode->secondary, property_read_int_array, propname,
 				  elem_size, val, nval);
 }
+EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
 
 /**
  * fwnode_property_read_u8_array - return a u8 array property of firmware node
diff --git a/include/linux/property.h b/include/linux/property.h
index e214ecd24..441a1ad76 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -57,6 +57,8 @@ bool fwnode_property_present(const struct fwnode_handle *fwnode,
 			     const char *propname);
 bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
 			     const char *propname);
+int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
+				   unsigned int elem_size, void *val, size_t nval);
 int fwnode_property_read_u8_array(const struct fwnode_handle *fwnode,
 				  const char *propname, u8 *val,
 				  size_t nval);
-- 
2.49.0

[PATCH 04/10] rust: 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
sizes of properties (e.g. u8, u16, u32).

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/property.rs | 153 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 151 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index b0a4bb63a..4756ea766 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -4,9 +4,17 @@
 //!
 //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
 
-use core::ptr;
+use core::{ffi::c_void, mem::MaybeUninit, ptr};
 
-use crate::{bindings, device::Device, str::CStr, types::Opaque};
+use crate::{
+    alloc::KVec,
+    bindings,
+    device::Device,
+    error::{to_result, Result},
+    prelude::*,
+    str::{CStr, CString},
+    types::{Integer, Opaque},
+};
 
 impl Device {
     /// Obtain the fwnode corresponding to the device.
@@ -26,6 +34,41 @@ fn fwnode(&self) -> &FwNode {
     pub fn property_present(&self, name: &CStr) -> bool {
         self.fwnode().property_present(name)
     }
+
+    /// Returns if a firmware property `name` is true or false
+    pub fn property_read_bool(&self, name: &CStr) -> bool {
+        self.fwnode().property_read_bool(name)
+    }
+
+    /// Returns the index of matching string `match_str` for firmware string property `name`
+    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
+        self.fwnode().property_read_string(name)
+    }
+
+    /// 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> {
+        self.fwnode().property_match_string(name, match_str)
+    }
+
+    /// Returns firmware property `name` integer scalar value
+    pub fn property_read<T: Integer>(&self, name: &CStr) -> Result<T> {
+        self.fwnode().property_read(name)
+    }
+
+    /// Returns firmware property `name` integer array values
+    pub fn property_read_array<T: Integer, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
+        self.fwnode().property_read_array(name)
+    }
+
+    /// Returns firmware property `name` integer array values in a KVec
+    pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
+        self.fwnode().property_read_array_vec(name, len)
+    }
+
+    /// Returns integer array length for firmware property `name`
+    pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
+        self.fwnode().property_count_elem::<T>(name)
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -57,6 +100,112 @@ 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 if a firmware property `name` is true or false
+    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_read_string(&self, name: &CStr) -> Result<CString> {
+        let mut str = core::ptr::null_mut();
+        let pstr: *mut _ = &mut str;
+
+        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is
+        // valid because `self` is valid.
+        let ret = unsafe {
+            bindings::fwnode_property_read_string(self.as_raw(), name.as_char_ptr(), pstr as _)
+        };
+        to_result(ret)?;
+
+        // SAFETY: `pstr` contains a non-null ptr on success
+        let str = unsafe { CStr::from_char_ptr(*pstr) };
+        Ok(str.try_into()?)
+    }
+
+    /// 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 scalar value
+    pub fn property_read<T: Integer>(&self, name: &CStr) -> Result<T> {
+        let val: [_; 1] = Self::property_read_array(self, name)?;
+        Ok(val[0])
+    }
+
+    /// Returns firmware property `name` integer array values
+    pub fn property_read_array<T: Integer, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
+        let val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
+
+        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
+        // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
+        let ret = unsafe {
+            bindings::fwnode_property_read_int_array(
+                self.as_raw(),
+                name.as_char_ptr(),
+                T::SIZE as u32,
+                val.as_ptr() as *mut c_void,
+                val.len(),
+            )
+        };
+        to_result(ret)?;
+
+        // SAFETY: `val` is always initialized when
+        // fwnode_property_read_int_array is successful.
+        Ok(val.map(|v| unsafe { v.assume_init() }))
+    }
+
+    /// Returns firmware property `name` integer array values in a KVec
+    pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
+        let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
+
+        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
+        // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
+        to_result(unsafe {
+            bindings::fwnode_property_read_int_array(
+                self.as_raw(),
+                name.as_char_ptr(),
+                T::SIZE as u32,
+                val.as_ptr() as *mut c_void,
+                len,
+            )
+        })?;
+
+        // SAFETY: fwnode_property_read_int_array() writes exactly `len` entries on success
+        unsafe { val.set_len(len) }
+        Ok(val)
+    }
+
+    /// Returns integer array length for firmware property `name`
+    pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
+        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
+        // because `self` is valid. Passing null pointer buffer is valid to obtain
+        // the number of elements in the property array.
+        let ret = unsafe {
+            bindings::fwnode_property_read_int_array(
+                self.as_raw(),
+                name.as_char_ptr(),
+                T::SIZE as u32,
+                ptr::null_mut(),
+                0,
+            )
+        };
+        to_result(ret)?;
+        Ok(ret as usize)
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
-- 
2.49.0

[PATCH 05/10] rust: Read properties via single generic method

[Remo Senekowitsch]

This can probably be squashed into its parent. I haven't done that
yet in case there is something wrong with the generic approach and the
previous one is preferred.

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

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index 4756ea766..4a03008ce 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -35,31 +35,11 @@ pub fn property_present(&self, name: &CStr) -> bool {
         self.fwnode().property_present(name)
     }
 
-    /// Returns if a firmware property `name` is true or false
-    pub fn property_read_bool(&self, name: &CStr) -> bool {
-        self.fwnode().property_read_bool(name)
-    }
-
-    /// Returns the index of matching string `match_str` for firmware string property `name`
-    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
-        self.fwnode().property_read_string(name)
-    }
-
     /// 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> {
         self.fwnode().property_match_string(name, match_str)
     }
 
-    /// Returns firmware property `name` integer scalar value
-    pub fn property_read<T: Integer>(&self, name: &CStr) -> Result<T> {
-        self.fwnode().property_read(name)
-    }
-
-    /// Returns firmware property `name` integer array values
-    pub fn property_read_array<T: Integer, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
-        self.fwnode().property_read_array(name)
-    }
-
     /// Returns firmware property `name` integer array values in a KVec
     pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
         self.fwnode().property_read_array_vec(name, len)
@@ -69,6 +49,11 @@ pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Re
     pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
         self.fwnode().property_count_elem::<T>(name)
     }
+
+    /// Returns firmware property `name` integer scalar value
+    pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
+        self.fwnode().property_read(name)
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -101,30 +86,6 @@ pub fn property_present(&self, name: &CStr) -> bool {
         unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
     }
 
-    /// Returns if a firmware property `name` is true or false
-    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_read_string(&self, name: &CStr) -> Result<CString> {
-        let mut str = core::ptr::null_mut();
-        let pstr: *mut _ = &mut str;
-
-        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is
-        // valid because `self` is valid.
-        let ret = unsafe {
-            bindings::fwnode_property_read_string(self.as_raw(), name.as_char_ptr(), pstr as _)
-        };
-        to_result(ret)?;
-
-        // SAFETY: `pstr` contains a non-null ptr on success
-        let str = unsafe { CStr::from_char_ptr(*pstr) };
-        Ok(str.try_into()?)
-    }
-
     /// 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
@@ -140,34 +101,6 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
         Ok(ret as usize)
     }
 
-    /// Returns firmware property `name` integer scalar value
-    pub fn property_read<T: Integer>(&self, name: &CStr) -> Result<T> {
-        let val: [_; 1] = Self::property_read_array(self, name)?;
-        Ok(val[0])
-    }
-
-    /// Returns firmware property `name` integer array values
-    pub fn property_read_array<T: Integer, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
-        let val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
-
-        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
-        // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
-        let ret = unsafe {
-            bindings::fwnode_property_read_int_array(
-                self.as_raw(),
-                name.as_char_ptr(),
-                T::SIZE as u32,
-                val.as_ptr() as *mut c_void,
-                val.len(),
-            )
-        };
-        to_result(ret)?;
-
-        // SAFETY: `val` is always initialized when
-        // fwnode_property_read_int_array is successful.
-        Ok(val.map(|v| unsafe { v.assume_init() }))
-    }
-
     /// Returns firmware property `name` integer array values in a KVec
     pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
         let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
@@ -206,6 +139,38 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
         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, integers and arrays
+    /// of 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 crate::{device::Device, types::CString};
+    /// fn examples(dev: &Device) -> Result {
+    ///     let fwnode = dev.fwnode();
+    ///     let b: bool = fwnode.property_read("some-bool")?;
+    ///     let s: CString = fwnode.property_read("some-str")?;
+    /// }
+    /// ```
+    pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
+        T::read(&self, name)
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
@@ -220,3 +185,65 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
         unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
     }
 }
+
+/// Implemented for all types that can be read as properties.
+///
+/// This is used to make [FwNode::property_read] generic over the type of
+/// property being read.
+pub trait Property: Sized {
+    /// Used to make [FwNode::property_read] generic.
+    fn read(fwnode: &FwNode, name: &CStr) -> Result<Self>;
+}
+
+impl Property for bool {
+    fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is valid
+        // because `fwnode` is valid.
+        Ok(unsafe { bindings::fwnode_property_read_bool(fwnode.as_raw(), name.as_char_ptr()) })
+    }
+}
+impl Property for CString {
+    fn read(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()?)
+    }
+}
+impl<T: Integer, const N: usize> Property for [T; N] {
+    fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
+
+        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is valid
+        // because `fwnode` is valid. `val.as_ptr` is valid because `val` is valid.
+        let ret = unsafe {
+            bindings::fwnode_property_read_int_array(
+                fwnode.as_raw(),
+                name.as_char_ptr(),
+                T::SIZE as u32,
+                val.as_mut_ptr().cast(),
+                val.len(),
+            )
+        };
+        to_result(ret)?;
+
+        // SAFETY: `val` is always initialized when
+        // fwnode_property_read_int_array is successful.
+        Ok(val.map(|v| unsafe { v.assume_init() }))
+    }
+}
+impl<T: Integer> Property for T {
+    fn read(fwnode: &FwNode, name: &CStr) -> Result<T> {
+        let val: [_; 1] = <[T; 1] as Property>::read(fwnode, name)?;
+        Ok(val[0])
+    }
+}
-- 
2.49.0

[PATCH 06/10] rust: property: Add child accessor and iterator

[Remo Senekowitsch]

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

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index 4a03008ce..dc927ad93 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -13,7 +13,7 @@
     error::{to_result, Result},
     prelude::*,
     str::{CStr, CString},
-    types::{Integer, Opaque},
+    types::{ARef, Integer, Opaque},
 };
 
 impl Device {
@@ -54,6 +54,16 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
     pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
         self.fwnode().property_read(name)
     }
+
+    /// Returns first matching named child node handle.
+    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
+        self.fwnode().get_child_by_name(name)
+    }
+
+    /// Returns an iterator over a node's children.
+    pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
+        self.fwnode().children()
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -75,6 +85,11 @@ pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
 pub struct FwNode(Opaque<bindings::fwnode_handle>);
 
 impl FwNode {
+    // SAFETY: `raw` must have its refcount incremented.
+    unsafe fn from_raw(raw: *mut bindings::fwnode_handle) -> ARef<Self> {
+        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()
@@ -171,6 +186,46 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
     pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
         T::read(&self, 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.
+        let child =
+            unsafe { bindings::fwnode_get_named_child_node(self.as_raw(), name.as_char_ptr()) };
+        if child.is_null() {
+            return None;
+        }
+        // SAFETY: `fwnode_get_named_child_node` returns a pointer with refcount incremented.
+        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()
+                }
+            };
+            let next = unsafe { bindings::fwnode_get_next_child_node(self.as_raw(), prev_ptr) };
+            if next.is_null() {
+                return None;
+            }
+            // SAFETY: `fwnode_get_next_child_node` returns a pointer with
+            // refcount incremented.
+            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 07/10] rust: Add arrayvec

[Remo Senekowitsch]

This patch is basically a proof of concept intendend to gather feedback
about how to do this properly. Normally I would want to use the crate
from crates.io[1], but that's not an option in the kernel. We could also
vendor the entire source code of arrayvec. I'm not sure if people will
be happy with that.

[1] https://crates.io/crates/arrayvec

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/arrayvec.rs | 81 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs      |  1 +
 2 files changed, 82 insertions(+)
 create mode 100644 rust/kernel/arrayvec.rs

diff --git a/rust/kernel/arrayvec.rs b/rust/kernel/arrayvec.rs
new file mode 100644
index 000000000..041e7dcce
--- /dev/null
+++ b/rust/kernel/arrayvec.rs
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Provides [ArrayVec], a stack-allocated vector with static capacity.
+
+use core::mem::MaybeUninit;
+
+/// A stack-allocated vector with statically fixed capacity.
+///
+/// This can be useful to avoid heap allocation and still ensure safety where a
+/// small but dynamic number of elements is needed.
+///
+/// For example, consider a function that returns a variable number of values,
+/// but no more than 8. In C, one might achieve this by passing a pointer to
+/// a stack-allocated array as an out-parameter and making the function return
+/// the actual number of elements. This is not safe, because nothing prevents
+/// the caller from reading elements from the array that weren't actually
+/// initialized by the function. `ArrayVec` solves this problem, users are
+/// prevented from accessing uninitialized elements.
+///
+/// This basically exists already (in a much more mature form) on crates.io:
+/// <https://crates.io/crates/arrayvec>
+#[derive(Debug)]
+pub struct ArrayVec<const N: usize, T> {
+    array: [core::mem::MaybeUninit<T>; N],
+    len: usize,
+}
+
+impl<const N: usize, T> ArrayVec<N, T> {
+    /// Adds a new element to the end of the vector.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the vector is already full.
+    pub fn push(&mut self, elem: T) {
+        if self.len == N {
+            panic!("OOM")
+        }
+        self.array[self.len] = MaybeUninit::new(elem);
+        self.len += 1;
+    }
+
+    /// Returns the length of the vector.
+    pub fn len(&self) -> usize {
+        self.len
+    }
+}
+
+impl<const N: usize, T> Default for ArrayVec<N, T> {
+    fn default() -> Self {
+        Self {
+            array: [const { MaybeUninit::uninit() }; N],
+            len: 0,
+        }
+    }
+}
+
+impl<const N: usize, T> AsRef<[T]> for ArrayVec<N, T> {
+    fn as_ref(&self) -> &[T] {
+        // SAFETY: As per the type invariant, all elements at index < self.len
+        // are initialized.
+        unsafe { core::mem::transmute(&self.array[..self.len]) }
+    }
+}
+
+impl<const N: usize, T> AsMut<[T]> for ArrayVec<N, T> {
+    fn as_mut(&mut self) -> &mut [T] {
+        // SAFETY: As per the type invariant, all elements at index < self.len
+        // are initialized.
+        unsafe { core::mem::transmute(&mut self.array[..self.len]) }
+    }
+}
+
+impl<const N: usize, T> Drop for ArrayVec<N, T> {
+    fn drop(&mut self) {
+        unsafe {
+            let slice: &mut [T] =
+                core::slice::from_raw_parts_mut(self.array.as_mut_ptr().cast(), self.len);
+            core::ptr::drop_in_place(slice);
+        }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ca233fd20..0777f7a42 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -36,6 +36,7 @@
 pub use ffi;
 
 pub mod alloc;
+pub mod arrayvec;
 #[cfg(CONFIG_BLOCK)]
 pub mod block;
 #[doc(hidden)]
-- 
2.49.0

[PATCH 08/10] rust: property: Add property_get_reference_args

[Remo Senekowitsch]

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

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index dc927ad93..f1d0a33ba 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -8,6 +8,7 @@
 
 use crate::{
     alloc::KVec,
+    arrayvec::ArrayVec,
     bindings,
     device::Device,
     error::{to_result, Result},
@@ -64,6 +65,20 @@ pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
     pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
         self.fwnode().children()
     }
+
+    /// Finds a reference with arguments.
+    pub fn property_get_reference_args(
+        &self,
+        prop: &CStr,
+        nargs: NArgs<'_>,
+        index: u32,
+    ) -> Result<(
+        ARef<FwNode>,
+        ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>,
+    )> {
+        self.fwnode()
+            .property_get_reference_args(prop, nargs, index)
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -226,6 +241,45 @@ 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<(
+        ARef<Self>,
+        ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>,
+    )> {
+        let mut out_args = bindings::fwnode_reference_args::default();
+
+        let (nargs_prop, nargs) = match nargs {
+            NArgs::Prop(nargs_prop) => (nargs_prop.as_char_ptr(), 0),
+            NArgs::N(nargs) => (ptr::null(), nargs),
+        };
+
+        let ret = unsafe {
+            bindings::fwnode_property_get_reference_args(
+                self.0.get(),
+                prop.as_char_ptr(),
+                nargs_prop,
+                nargs,
+                index,
+                &mut out_args,
+            )
+        };
+        to_result(ret)?;
+
+        let node = unsafe { FwNode::from_raw(out_args.fwnode) };
+        let mut args = ArrayVec::default();
+
+        for i in 0..out_args.nargs {
+            args.push(out_args.args[i as usize]);
+        }
+
+        Ok((node, args))
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
@@ -302,3 +356,12 @@ fn read(fwnode: &FwNode, name: &CStr) -> Result<T> {
         Ok(val[0])
     }
 }
+
+/// 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),
+}
-- 
2.49.0

[PATCH 09/10] rust: property: Add PropertyGuard

[Remo Senekowitsch]

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

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

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index f1d0a33ba..e4f0e5f97 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -42,7 +42,11 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
     }
 
     /// Returns firmware property `name` integer array values in a KVec
-    pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
+    pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
+        &'fwnode self,
+        name: &'name CStr,
+        len: usize,
+    ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
         self.fwnode().property_read_array_vec(name, len)
     }
 
@@ -52,12 +56,18 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
     }
 
     /// Returns firmware property `name` integer scalar value
-    pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
+    pub fn property_read<'fwnode, 'name, T: Property>(
+        &'fwnode self,
+        name: &'name CStr,
+    ) -> PropertyGuard<'fwnode, 'name, T> {
         self.fwnode().property_read(name)
     }
 
     /// Returns first matching named child node handle.
-    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
+    pub fn get_child_by_name<'fwnode, 'name>(
+        &'fwnode self,
+        name: &'name CStr,
+    ) -> PropertyGuard<'fwnode, 'name, ARef<FwNode>> {
         self.fwnode().get_child_by_name(name)
     }
 
@@ -132,12 +142,16 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
     }
 
     /// Returns firmware property `name` integer array values in a KVec
-    pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
+    pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
+        &'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: `name` is non-null and null-terminated. `self.as_raw` is valid
         // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
-        to_result(unsafe {
+        let err = unsafe {
             bindings::fwnode_property_read_int_array(
                 self.as_raw(),
                 name.as_char_ptr(),
@@ -145,11 +159,19 @@ pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Re
                 val.as_ptr() as *mut c_void,
                 len,
             )
-        })?;
-
-        // SAFETY: fwnode_property_read_int_array() writes exactly `len` entries on success
-        unsafe { val.set_len(len) }
-        Ok(val)
+        };
+        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`
@@ -194,24 +216,42 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
     /// # use crate::{device::Device, types::CString};
     /// fn examples(dev: &Device) -> Result {
     ///     let fwnode = dev.fwnode();
-    ///     let b: bool = fwnode.property_read("some-bool")?;
-    ///     let s: CString = fwnode.property_read("some-str")?;
+    ///     let b: bool = fwnode.property_read("some-bool").required()?;
+    ///     if let Some(s) = fwnode.property_read::<CString>("some-str").optional() {
+    ///         // ...
+    ///     }
     /// }
     /// ```
-    pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
-        T::read(&self, name)
+    pub fn property_read<'fwnode, 'name, T: Property>(
+        &'fwnode self,
+        name: &'name CStr,
+    ) -> PropertyGuard<'fwnode, 'name, T> {
+        PropertyGuard {
+            inner: T::read(&self, name),
+            fwnode: self,
+            name,
+        }
     }
 
     /// Returns first matching named child node handle.
-    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<Self>> {
+    pub fn get_child_by_name<'fwnode, 'name>(
+        &'fwnode self,
+        name: &'name CStr,
+    ) -> PropertyGuard<'fwnode, 'name, ARef<Self>> {
         // SAFETY: `self` and `name` are valid.
         let child =
             unsafe { bindings::fwnode_get_named_child_node(self.as_raw(), name.as_char_ptr()) };
-        if child.is_null() {
-            return None;
+        let res = if child.is_null() {
+            Err(ENOENT)
+        } else {
+            // SAFETY: `fwnode_get_named_child_node` returns a pointer with refcount incremented.
+            Ok(unsafe { Self::from_raw(child) })
+        };
+        PropertyGuard {
+            inner: res,
+            fwnode: self,
+            name,
         }
-        // SAFETY: `fwnode_get_named_child_node` returns a pointer with refcount incremented.
-        Some(unsafe { Self::from_raw(child) })
     }
 
     /// Returns an iterator over a node's children.
@@ -365,3 +405,71 @@ pub enum NArgs<'a> {
     /// The known number of arguments.
     N(u32),
 }
+
+/// A helper for reading device properties.
+///
+/// Use [Self::required] 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.
+    pub fn required(self) -> Result<T> {
+        if self.inner.is_err() {
+            // Get the device associated with the fwnode for device-associated
+            // logging.
+            // TODO: Are we allowed to do this? The field `fwnode_handle.dev`
+            // has a somewhat vague comment, which could mean we're not
+            // supposed to access it:
+            // https://elixir.bootlin.com/linux/v6.13.6/source/include/linux/fwnode.h#L51
+            // SAFETY: According to the invariant of FwNode, it is valid.
+            let dev = unsafe { (*self.fwnode.as_raw()).dev };
+
+            if dev.is_null() {
+                pr_err!("property '{}' is missing\n", self.name);
+            } else {
+                // SAFETY: If dev is not null, it points to a valid device.
+                let dev: &Device = unsafe { &*dev.cast() };
+                dev_err!(dev, "property '{}' is missing\n", self.name);
+            };
+        }
+        self.inner
+    }
+
+    /// Access the property, indicating it is optional.
+    ///
+    /// In contrast to [Self::required], 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] 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 10/10] 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         | 56 +++++++++++++++++++-
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
index 4171f43cf..50a51f38a 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 8120609e2..ed25a3781 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, of, platform, prelude::*};
+use kernel::{c_str, of, platform, prelude::*, str::CString};
 
 struct SampleDriver {
     pdev: platform::Device,
@@ -28,6 +28,60 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin
             dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
         }
 
+        let dev = pdev.as_ref();
+        if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
+        {
+            dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx);
+        }
+
+        if let Ok(str) = dev
+            .property_read::<CString>(c_str!("compatible"))
+            .required()
+        {
+            dev_info!(pdev.as_ref(), "compatible string = {:?}\n", str);
+        }
+
+        let prop = dev
+            .property_read::<bool>(c_str!("test,bool-prop"))
+            .required()?;
+        dev_info!(dev, "bool prop is {}\n", prop);
+
+        if dev.property_present(c_str!("test,u32-prop")) {
+            dev_info!(dev, "'test,u32-prop' is present\n");
+        }
+
+        let prop = dev
+            .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 _ = dev
+            .property_read::<u32>(c_str!("test,u32-required-prop"))
+            .required()?;
+
+        let prop: u32 = dev.property_read(c_str!("test,u32-prop")).required()?;
+        dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
+
+        let prop: [i16; 4] = dev.property_read(c_str!("test,i16-array")).required()?;
+        dev_info!(dev, "'test,i16-array' is {:?}\n", prop);
+        dev_info!(
+            dev,
+            "'test,i16-array' length is {}\n",
+            dev.property_count_elem::<u16>(c_str!("test,i16-array"))
+                .unwrap()
+        );
+
+        let prop: KVec<i16> = dev
+            .property_read_array_vec(c_str!("test,i16-array"), 4)?
+            .required()?;
+        dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop);
+
         let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
 
         Ok(drvdata.into())
-- 
2.49.0

Re: [PATCH 02/10] rust: Add an Integer trait

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:41PM +0100, Remo Senekowitsch wrote:
> From: "Rob Herring (Arm)" <robh@kernel.org>
> 
> Add an "Integer" trait similar to crate::num::Integer. This is useful
> for implementing generic methods which operate on different sizes of
> integers. One example is reading DT/ACPI firmware properties.
> 
> This was originally proposed by Alice Ryhl[1].
> 
> [1] https://lore.kernel.org/rust-for-linux/CAH5fLgiXPZqKpWSSNdx-Ww-E9h2tOLcF3_8Y4C_JQ0eU8EMwFw@mail.gmail.com/
> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

This needs your Signed-off-by too because you are sending it.

> ---
>  rust/kernel/types.rs | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 2bbaab83b..21647b7ba 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -3,10 +3,11 @@
>  //! Kernel types.
>  
>  use crate::init::{self, PinInit};
> +use crate::transmute::{AsBytes, FromBytes};
>  use core::{
>      cell::UnsafeCell,
>      marker::{PhantomData, PhantomPinned},
> -    mem::{ManuallyDrop, MaybeUninit},
> +    mem::{size_of, ManuallyDrop, MaybeUninit},
>      ops::{Deref, DerefMut},
>      ptr::NonNull,
>  };
> @@ -553,6 +554,25 @@ pub enum Either<L, R> {
>      Right(R),
>  }
>  
> +/// Trait defined for all integer types similar to `crate::num::Integer`
> +pub trait Integer: FromBytes + AsBytes + Copy {
> +    /// Size of the integer in bytes
> +    const SIZE: usize;
> +}
> +
> +macro_rules! impl_int {
> +    ($($typ:ty),* $(,)?) => {$(
> +        impl Integer for $typ {
> +            const SIZE: usize = size_of::<Self>();
> +        }
> +    )*};
> +}
> +
> +impl_int! {
> +    u8, u16, u32, u64, usize,
> +    i8, i16, i32, i64, isize,
> +}
> +
>  /// Zero-sized type to mark types not [`Send`].
>  ///
>  /// Add this type as a field to your struct if your type should not be sent to a different task.
> -- 
> 2.49.0
> 

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:40PM +0100, 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 APIs has to be duplicated on a fwnode as they are in C.

s/has/have/

> 
> A related discussion can be found on the R4L Zulip[1].
> 
> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697

Useful below the '---', but I don't think we want to keep this link 
forever. And who knows how long it will be valid? The commit msg needs 
to stand on its own, and I think it does.

> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/helpers/helpers.c  |  1 +
>  rust/helpers/property.c | 13 ++++++++
>  rust/kernel/device.rs   |  7 ----
>  rust/kernel/lib.rs      |  1 +
>  rust/kernel/property.rs | 73 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 88 insertions(+), 7 deletions(-)
>  create mode 100644 rust/helpers/property.c
>  create mode 100644 rust/kernel/property.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 0640b7e11..b4eec5bf2 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -23,6 +23,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 000000000..c37c74488
> --- /dev/null
> +++ b/rust/helpers/property.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/property.h>
> +
> +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> +{
> +	return dev_fwnode(dev);
> +}
> +
> +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.rs
> index db2d9658b..d5e6a19ff 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -6,7 +6,6 @@
>  
>  use crate::{
>      bindings,
> -    str::CStr,
>      types::{ARef, Opaque},
>  };
>  use core::{fmt, ptr};
> @@ -181,12 +180,6 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
>              )
>          };
>      }
> -
> -    /// 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: Instances of `Device` are always reference-counted.
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0..ca233fd20 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -67,6 +67,7 @@
>  pub mod platform;
>  pub mod prelude;
>  pub mod print;
> +pub mod property;
>  pub mod rbtree;
>  pub mod revocable;
>  pub mod security;
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> new file mode 100644
> index 000000000..b0a4bb63a
> --- /dev/null
> +++ b/rust/kernel/property.rs
> @@ -0,0 +1,73 @@
> +// 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, device::Device, str::CStr, types::Opaque};
> +
> +impl Device {
> +    /// Obtain the fwnode corresponding to the device.
> +    fn fwnode(&self) -> &FwNode {
> +        // SAFETY: `self` is valid.
> +        let fwnode_handle = unsafe { bindings::dev_fwnode(self.as_raw()) };
> +        if fwnode_handle.is_null() {
> +            panic!("fwnode_handle cannot be null");

It's usually not a good idea to panic the kernel especially with 
something a driver calls as that's probably recoverable.

Users/drivers testing fwnode_handle/of_node for NULL is pretty common. 
Though often that's a legacy code path, so maybe not allowing NULL is 
fine for now.

> +        }
> +        // 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.
> +        unsafe { &*fwnode_handle.cast() }
> +    }
> +
> +    /// Checks if property is present or not.
> +    pub fn property_present(&self, name: &CStr) -> bool {
> +        self.fwnode().property_present(name)
> +    }
> +}

The C developer in me wants to put this after the FwNode stuff since 
this uses it.

> +
> +/// 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()
> +    }
> +
> +    /// 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.
> +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
> 

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

[Andrew Ballance]

Hi thanks for sending these in.

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> This is my first time posting to the mailing list, please let me know if
> I did anything wrong.

you probably will have to resubmit in the patch canonical format[1]

I would recomend reading the docs but in summary:
 - all of your patches should have the subsystem in the subject line.
 - patch 6 is missing a commit message
 - commit messages should be written in the imperitive tone
 - a couple of your commit messages give a reason for the changes but
   do not have a summary of the changes
 - for your v2 you should add a diffstat to your cover letter running
   `git format-patch --base=auto --cover-letter` does this automatically
   for you

Andrew


[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Andrew Ballance]

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> +        // 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.
> +        unsafe { &*fwnode_handle.cast() }


this safety comment should say why it is safe to cast from a
struct fwnode_handle* to a *const FwNode

Andrew

Re: [PATCH 06/10] rust: property: Add child accessor and iterator

[Andrew Ballance]

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
>  impl FwNode {
> +    // SAFETY: `raw` must have its refcount incremented.
> +    unsafe fn from_raw(raw: *mut bindings::fwnode_handle) -> ARef<Self> {
> +        unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(raw.cast())) }
> +    }

this safety comment should say why it is ok to cast from a
struct fwnode_handle* to a *const FwNode

> +                    // 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()
> +                }
> +            };
> +            let next = unsafe { bindings::fwnode_get_next_child_node(self.as_raw(), prev_ptr) };

this is missing a safety comment

> +            if next.is_null() {
> +                return None;
> +            }

Andrew

Re: [PATCH 07/10] rust: Add arrayvec

[Andrew Ballance]

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> +pub struct ArrayVec<const N: usize, T> {
> +    array: [core::mem::MaybeUninit<T>; N],
> +    len: usize,
> +}

I would reverse the order of the generic arguments so T is first so
that it matches the declaration of an array: [T; N] 

> +    /// Adds a new element to the end of the vector.
> +    ///
> +    /// # Panics
> +    ///
> +    /// Panics if the vector is already full.
> +    pub fn push(&mut self, elem: T) {
> +        if self.len == N {
> +            panic!("OOM")
> +        }
> +        self.array[self.len] = MaybeUninit::new(elem);
> +        self.len += 1;
> +    }

This function should not panic. I would rewrite it so that its signature is
pub fn push(&mut self, elem: T) -> Result<(), T>

> +impl<const N: usize, T> AsRef<[T]> for ArrayVec<N, T> {
> +    fn as_ref(&self) -> &[T] {
> +        // SAFETY: As per the type invariant, all elements at index < self.len
> +        // are initialized.
> +        unsafe { core::mem::transmute(&self.array[..self.len]) }
> +    }
> +}
> +
> +impl<const N: usize, T> AsMut<[T]> for ArrayVec<N, T> {
> +    fn as_mut(&mut self) -> &mut [T] {
> +        // SAFETY: As per the type invariant, all elements at index < self.len
> +        // are initialized.
> +        unsafe { core::mem::transmute(&mut self.array[..self.len]) }
> +    }
> +}

I would replace both uses of transmute here with slice::from_raw_parts
like you did in drop. 

also it's not needed but you could also impl Deref and DerefMut for ArrayVec
because you have already impl'ed AsRef and AsMut so it should be simple

> +impl<const N: usize, T> Drop for ArrayVec<N, T> {
> +    fn drop(&mut self) {
> +        unsafe {

this is missing a safety comment

> +            let slice: &mut [T] =
> +                core::slice::from_raw_parts_mut(self.array.as_mut_ptr().cast(), self.len);
> +            core::ptr::drop_in_place(slice);
> +        }
> +    }
> +}

Andrew

Re: [PATCH 08/10] rust: property: Add property_get_reference_args

[Andrew Ballance]

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> +    pub fn property_get_reference_args(
> +        &self,
> +        prop: &CStr,
> +        nargs: NArgs<'_>,
> +        index: u32,
> +    ) -> Result<(
> +        ARef<Self>,
> +        ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>,
> +    )> {
> +        let mut out_args = bindings::fwnode_reference_args::default();
> +
> +        let (nargs_prop, nargs) = match nargs {
> +            NArgs::Prop(nargs_prop) => (nargs_prop.as_char_ptr(), 0),
> +            NArgs::N(nargs) => (ptr::null(), nargs),
> +        };
> +
> +        let ret = unsafe {
> +            bindings::fwnode_property_get_reference_args(
> +                self.0.get(),
> +                prop.as_char_ptr(),
> +                nargs_prop,
> +                nargs,
> +                index,
> +                &mut out_args,
> +            )
> +        };
> +        to_result(ret)?;
> +
> +        let node = unsafe { FwNode::from_raw(out_args.fwnode) };
> +        let mut args = ArrayVec::default();
> +
> +        for i in 0..out_args.nargs {
> +            args.push(out_args.args[i as usize]);
> +        }
> +
> +        Ok((node, args))
> +    }

This function is missing safety comments.

Andrew

Re: [PATCH 09/10] rust: property: Add PropertyGuard

[Andrew Ballance]

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> +/// A helper for reading device properties.
> +///
> +/// Use [Self::required] 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> {

nit: when linking items in rustdoc comments you should suround them like [`this`]

Andrew

Re: [PATCH 08/10] rust: property: Add property_get_reference_args

[Miguel Ojeda]

On Wed, Mar 26, 2025 at 10:07 PM Andrew Ballance
<andrewjballance@gmail.com> wrote:
>
> This function is missing safety comments.

Yeah -- that should be spotted by Clippy.

Remo: did you build with `CLIPPY=1`?

Thanks!

Cheers,
Miguel

Re: [PATCH 04/10] rust: Add bindings for reading device properties

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:43PM +0100, 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
> sizes of properties (e.g. u8, u16, u32).
> 
> 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/property.rs | 153 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 151 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index b0a4bb63a..4756ea766 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -4,9 +4,17 @@
>  //!
>  //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
>  
> -use core::ptr;
> +use core::{ffi::c_void, mem::MaybeUninit, ptr};
>  
> -use crate::{bindings, device::Device, str::CStr, types::Opaque};
> +use crate::{
> +    alloc::KVec,
> +    bindings,
> +    device::Device,
> +    error::{to_result, Result},
> +    prelude::*,
> +    str::{CStr, CString},
> +    types::{Integer, Opaque},
> +};
>  
>  impl Device {
>      /// Obtain the fwnode corresponding to the device.
> @@ -26,6 +34,41 @@ fn fwnode(&self) -> &FwNode {
>      pub fn property_present(&self, name: &CStr) -> bool {
>          self.fwnode().property_present(name)
>      }
> +
> +    /// Returns if a firmware property `name` is true or false
> +    pub fn property_read_bool(&self, name: &CStr) -> bool {
> +        self.fwnode().property_read_bool(name)
> +    }
> +
> +    /// Returns the index of matching string `match_str` for firmware string property `name`

Comment doesn't match the function.

> +    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
> +        self.fwnode().property_read_string(name)
> +    }
> +
> +    /// 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> {
> +        self.fwnode().property_match_string(name, match_str)
> +    }
> +
> +    /// Returns firmware property `name` integer scalar value
> +    pub fn property_read<T: Integer>(&self, name: &CStr) -> Result<T> {
> +        self.fwnode().property_read(name)
> +    }
> +
> +    /// Returns firmware property `name` integer array values
> +    pub fn property_read_array<T: Integer, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
> +        self.fwnode().property_read_array(name)
> +    }
> +
> +    /// Returns firmware property `name` integer array values in a KVec
> +    pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> +        self.fwnode().property_read_array_vec(name, len)
> +    }
> +
> +    /// Returns integer array length for firmware property `name`
> +    pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
> +        self.fwnode().property_count_elem::<T>(name)
> +    }
>  }
>  
>  /// A reference-counted fwnode_handle.
> @@ -57,6 +100,112 @@ 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 if a firmware property `name` is true or false
> +    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`

Same comment copy-n-paste mismatch.

> +    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
> +        let mut str = core::ptr::null_mut();
> +        let pstr: *mut _ = &mut str;
> +
> +        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is
> +        // valid because `self` is valid.
> +        let ret = unsafe {
> +            bindings::fwnode_property_read_string(self.as_raw(), name.as_char_ptr(), pstr as _)
> +        };
> +        to_result(ret)?;
> +
> +        // SAFETY: `pstr` contains a non-null ptr on success
> +        let str = unsafe { CStr::from_char_ptr(*pstr) };
> +        Ok(str.try_into()?)
> +    }

There's a problem with the C version of this function that I'd like to 
not repeat in Rust especially since ownership is clear. 

The issue is that we never know when the returned string is no longer 
needed. For DT overlays, we need to be able free the string when/if an 
overlay is removed. Though overlays are somewhat orthogonal to this. 
It's really just when the property's node refcount goes to 0 that the 
property value could be freed.

So this function should probably return a copy of the string that the 
caller owns.

Rob

Re: [PATCH 05/10] rust: Read properties via single generic method

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:44PM +0100, Remo Senekowitsch wrote:
> This can probably be squashed into its parent. I haven't done that
> yet in case there is something wrong with the generic approach and the
> previous one is preferred.

Looks good to me other than the returning of strings issue. And yes it 
should be squashed. (Really, it should have been in this series so 
reviewers aren't reviewing code you delete in the next patch).

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

Re: [PATCH 06/10] rust: property: Add child accessor and iterator

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:45PM +0100, Remo Senekowitsch wrote:
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>

Needs a commit msg.

> ---
>  rust/kernel/property.rs | 57 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)

Re: [PATCH 08/10] rust: property: Add property_get_reference_args

[Remo Senekowitsch]

On Wed Mar 26, 2025 at 10:25 PM CET, Miguel Ojeda wrote:
>
> Remo: did you build with `CLIPPY=1`?

I didn't! Usually I have rust-analyzer configured with clippy, but that doesn't seem to work here.
I added it to my build scripts and can see the warnings now, future patches should be free of clippy warnings.

Thanks for the tip!

Remo

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

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:49PM +0100, 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         | 56 +++++++++++++++++++-
>  2 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
> index 4171f43cf..50a51f38a 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 8120609e2..ed25a3781 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, of, platform, prelude::*};
> +use kernel::{c_str, of, platform, prelude::*, str::CString};
>  
>  struct SampleDriver {
>      pdev: platform::Device,
> @@ -28,6 +28,60 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin
>              dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
>          }
>  
> +        let dev = pdev.as_ref();

We should move this to the top and replace all the 'pdev.as_ref()' with 
'dev'.

> +        if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
> +        {
> +            dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx);

Like here. (Looks like this is my fault.)

> +        }
> +
> +        if let Ok(str) = dev
> +            .property_read::<CString>(c_str!("compatible"))
> +            .required()
> +        {
> +            dev_info!(pdev.as_ref(), "compatible string = {:?}\n", str);
> +        }
> +
> +        let prop = dev
> +            .property_read::<bool>(c_str!("test,bool-prop"))
> +            .required()?;

The 'required' is kind of odd for boolean properties. They are never 
required as not present is the only way to to get false.

> +        dev_info!(dev, "bool prop is {}\n", prop);
> +
> +        if dev.property_present(c_str!("test,u32-prop")) {
> +            dev_info!(dev, "'test,u32-prop' is present\n");
> +        }
> +
> +        let prop = dev
> +            .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 _ = dev
> +            .property_read::<u32>(c_str!("test,u32-required-prop"))
> +            .required()?;
> +
> +        let prop: u32 = dev.property_read(c_str!("test,u32-prop")).required()?;
> +        dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
> +
> +        let prop: [i16; 4] = dev.property_read(c_str!("test,i16-array")).required()?;
> +        dev_info!(dev, "'test,i16-array' is {:?}\n", prop);
> +        dev_info!(
> +            dev,
> +            "'test,i16-array' length is {}\n",
> +            dev.property_count_elem::<u16>(c_str!("test,i16-array"))
> +                .unwrap()
> +        );
> +
> +        let prop: KVec<i16> = dev
> +            .property_read_array_vec(c_str!("test,i16-array"), 4)?
> +            .required()?;
> +        dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop);
> +
>          let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
>  
>          Ok(drvdata.into())
> -- 
> 2.49.0
> 

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

[Remo Senekowitsch]

On Wed Mar 26, 2025 at 11:01 PM CET, Rob Herring wrote:
>>
>> +        let prop = dev
>> +            .property_read::<bool>(c_str!("test,bool-prop"))
>> +            .required()?;
>
> The 'required' is kind of odd for boolean properties. They are never 
> required as not present is the only way to to get false.

Agreed. I can think of a few alternatives:

* Make the trait `Property` more flexible to allow each implementor to specify
  what its output type for the `read` function is, via an associated type.
  I really don't like this idea, because overly generic APIs can mess with type
  inference and become less ergonomic because of it.

* Use `propert_present` instead. That doesn't perfectly express the intention,
  because it doesn't warn if the property is present but has a type other than
  bool.

* Add an additional inherent method `property_read_bool`, which returns a plain
  `bool` instead of `PropertyGuard<bool>`. Then there will be three slightly
  different ways to read a bool: `property_present`, `property_read_bool` and
  `property_read::<bool>`. Maybe that's confusing.

* Add `property_read_bool` and remove `impl Property for bool`. That would avoid
  confusion between `property_read_bool` and `property_read::<bool>`, only the
  former would work.

Re: [PATCH 09/10] rust: property: Add PropertyGuard

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:48PM +0100, Remo Senekowitsch wrote:
> Forcing users to specify whether a property is supposed to be required
> or not allows us to move error logging of missing required properties
> into core, preventing a lot of boilerplate in drivers.

We'll want to squash this one too.

> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/property.rs | 146 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 127 insertions(+), 19 deletions(-)
> 
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index f1d0a33ba..e4f0e5f97 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -42,7 +42,11 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
>      }
>  
>      /// Returns firmware property `name` integer array values in a KVec
> -    pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> +    pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +        len: usize,
> +    ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
>          self.fwnode().property_read_array_vec(name, len)
>      }
>  
> @@ -52,12 +56,18 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
>      }
>  
>      /// Returns firmware property `name` integer scalar value
> -    pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
> +    pub fn property_read<'fwnode, 'name, T: Property>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, T> {
>          self.fwnode().property_read(name)
>      }
>  
>      /// Returns first matching named child node handle.
> -    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
> +    pub fn get_child_by_name<'fwnode, 'name>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, ARef<FwNode>> {
>          self.fwnode().get_child_by_name(name)
>      }

While child nodes can be required or optional, it's a bit less common to 
be optional. There error message on missing required child nodes is 
going to be a bit misleading saying 'property'. So maybe don't do the 
PropertyGuard on child node handling?

>  
> @@ -132,12 +142,16 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
>      }
>  
>      /// Returns firmware property `name` integer array values in a KVec
> -    pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> +    pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
> +        &'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: `name` is non-null and null-terminated. `self.as_raw` is valid
>          // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
> -        to_result(unsafe {
> +        let err = unsafe {
>              bindings::fwnode_property_read_int_array(
>                  self.as_raw(),
>                  name.as_char_ptr(),
> @@ -145,11 +159,19 @@ pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Re
>                  val.as_ptr() as *mut c_void,
>                  len,
>              )
> -        })?;
> -
> -        // SAFETY: fwnode_property_read_int_array() writes exactly `len` entries on success
> -        unsafe { val.set_len(len) }
> -        Ok(val)
> +        };
> +        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`
> @@ -194,24 +216,42 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
>      /// # use crate::{device::Device, types::CString};
>      /// fn examples(dev: &Device) -> Result {
>      ///     let fwnode = dev.fwnode();
> -    ///     let b: bool = fwnode.property_read("some-bool")?;
> -    ///     let s: CString = fwnode.property_read("some-str")?;
> +    ///     let b: bool = fwnode.property_read("some-bool").required()?;
> +    ///     if let Some(s) = fwnode.property_read::<CString>("some-str").optional() {
> +    ///         // ...
> +    ///     }
>      /// }
>      /// ```
> -    pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
> -        T::read(&self, name)
> +    pub fn property_read<'fwnode, 'name, T: Property>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, T> {
> +        PropertyGuard {
> +            inner: T::read(&self, name),
> +            fwnode: self,
> +            name,
> +        }
>      }
>  
>      /// Returns first matching named child node handle.
> -    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<Self>> {
> +    pub fn get_child_by_name<'fwnode, 'name>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, ARef<Self>> {
>          // SAFETY: `self` and `name` are valid.
>          let child =
>              unsafe { bindings::fwnode_get_named_child_node(self.as_raw(), name.as_char_ptr()) };
> -        if child.is_null() {
> -            return None;
> +        let res = if child.is_null() {
> +            Err(ENOENT)
> +        } else {
> +            // SAFETY: `fwnode_get_named_child_node` returns a pointer with refcount incremented.
> +            Ok(unsafe { Self::from_raw(child) })
> +        };
> +        PropertyGuard {
> +            inner: res,
> +            fwnode: self,
> +            name,
>          }
> -        // SAFETY: `fwnode_get_named_child_node` returns a pointer with refcount incremented.
> -        Some(unsafe { Self::from_raw(child) })
>      }
>  
>      /// Returns an iterator over a node's children.
> @@ -365,3 +405,71 @@ pub enum NArgs<'a> {
>      /// The known number of arguments.
>      N(u32),
>  }
> +
> +/// A helper for reading device properties.
> +///
> +/// Use [Self::required] 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.
> +    pub fn required(self) -> Result<T> {
> +        if self.inner.is_err() {
> +            // Get the device associated with the fwnode for device-associated
> +            // logging.
> +            // TODO: Are we allowed to do this? The field `fwnode_handle.dev`
> +            // has a somewhat vague comment, which could mean we're not
> +            // supposed to access it:
> +            // https://elixir.bootlin.com/linux/v6.13.6/source/include/linux/fwnode.h#L51
> +            // SAFETY: According to the invariant of FwNode, it is valid.
> +            let dev = unsafe { (*self.fwnode.as_raw()).dev };
> +
> +            if dev.is_null() {
> +                pr_err!("property '{}' is missing\n", self.name);

We should print the node path in this case (and perhaps both cases).

We have "%pfw" printf format specifier for C. We're going to need the 
same for rust.

> +            } else {
> +                // SAFETY: If dev is not null, it points to a valid device.
> +                let dev: &Device = unsafe { &*dev.cast() };
> +                dev_err!(dev, "property '{}' is missing\n", self.name);
> +            };
> +        }
> +        self.inner
> +    }
> +
> +    /// Access the property, indicating it is optional.
> +    ///
> +    /// In contrast to [Self::required], 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] 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
> 

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Rob Herring]

On Wed, Mar 26, 2025 at 03:51:06PM -0500, Rob Herring wrote:
> On Wed, Mar 26, 2025 at 06:13:40PM +0100, 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 APIs has to be duplicated on a fwnode as they are in C.
> 
> s/has/have/
> 
> > 
> > A related discussion can be found on the R4L Zulip[1].
> > 
> > [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697
> 
> Useful below the '---', but I don't think we want to keep this link 
> forever. And who knows how long it will be valid? The commit msg needs 
> to stand on its own, and I think it does.
> 
> > 
> > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> > ---
> >  rust/helpers/helpers.c  |  1 +
> >  rust/helpers/property.c | 13 ++++++++
> >  rust/kernel/device.rs   |  7 ----
> >  rust/kernel/lib.rs      |  1 +
> >  rust/kernel/property.rs | 73 +++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 88 insertions(+), 7 deletions(-)
> >  create mode 100644 rust/helpers/property.c
> >  create mode 100644 rust/kernel/property.rs

Also, property.rs needs to be added to MAINTAINERS. I guess it goes 
under driver core with Greg.

Rob

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

[Rob Herring]

On Wed, Mar 26, 2025 at 5:24 PM Remo Senekowitsch <remo@buenzli.dev> wrote:
>
> On Wed Mar 26, 2025 at 11:01 PM CET, Rob Herring wrote:
> >>
> >> +        let prop = dev
> >> +            .property_read::<bool>(c_str!("test,bool-prop"))
> >> +            .required()?;
> >
> > The 'required' is kind of odd for boolean properties. They are never
> > required as not present is the only way to to get false.
>
> Agreed. I can think of a few alternatives:
>
> * Make the trait `Property` more flexible to allow each implementor to specify
>   what its output type for the `read` function is, via an associated type.
>   I really don't like this idea, because overly generic APIs can mess with type
>   inference and become less ergonomic because of it.
>
> * Use `propert_present` instead. That doesn't perfectly express the intention,
>   because it doesn't warn if the property is present but has a type other than
>   bool.

Right. I've been cleaning up the tree to use of_property_read_bool()
on bools and of_property_present() on non-bools, so don't want to go
back to 1 function. The C code now warns on a mismatch.

> * Add an additional inherent method `property_read_bool`, which returns a plain
>   `bool` instead of `PropertyGuard<bool>`. Then there will be three slightly
>   different ways to read a bool: `property_present`, `property_read_bool` and
>   `property_read::<bool>`. Maybe that's confusing.
>
> * Add `property_read_bool` and remove `impl Property for bool`. That would avoid
>   confusion between `property_read_bool` and `property_read::<bool>`, only the
>   former would work.

I think I would go with this option. Easier to add another way later
than remove one.

Rob

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Andy Shevchenko]

On Wed, Mar 26, 2025 at 06:13:40PM +0100, 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 APIs has to be duplicated on a fwnode as they are in C.
> 
> A related discussion can be found on the R4L Zulip[1].
> 
> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697

You can make the above to be a Link tag like

Link: ... [1]

> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>

...

> +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> +{
> +	return dev_fwnode(dev);
> +}

Why not const? For most of the property retrieval APIs the parameter is const.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> The rust bindings for reading device properties has a single
> implementation supporting differing sizes of integers. The fwnode C API
> already has a similar interface, but it is not exposed with the
> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.

...

> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);

I'm not sure about this. We have a lot of assumptions in the code that the
arrays beneath are only represented by the selected number of integer types.
This opens a Pandora's box, e.g., reading in u24, which is not supported by
the upper layers..

> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
> +				   unsigned int elem_size, void *val, size_t nval);

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Mar 26, 2025 at 03:54:09PM -0500, Andrew Ballance wrote:
> Hi thanks for sending these in.
> 
> On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> > This is my first time posting to the mailing list, please let me know if
> > I did anything wrong.
> 
> you probably will have to resubmit in the patch canonical format[1]
> 
> I would recomend reading the docs but in summary:
>  - all of your patches should have the subsystem in the subject line.
>  - patch 6 is missing a commit message
>  - commit messages should be written in the imperitive tone
>  - a couple of your commit messages give a reason for the changes but
>    do not have a summary of the changes

>  - for your v2 you should add a diffstat to your cover letter running
>    `git format-patch --base=auto --cover-letter` does this automatically
>    for you

Also -v<X> gives the proper versioning.

> [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

-- 
With Best Regards,
Andy Shevchenko

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

[Danilo Krummrich]

On Wed, Mar 26, 2025 at 06:13:49PM +0100, Remo Senekowitsch wrote:
> --- 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, of, platform, prelude::*};
> +use kernel::{c_str, of, platform, prelude::*, str::CString};
>  
>  struct SampleDriver {
>      pdev: platform::Device,
> @@ -28,6 +28,60 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin
>              dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
>          }
>  
> +        let dev = pdev.as_ref();
> +        if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
> +        {
> +            dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx);
> +        }
> +
> +        if let Ok(str) = dev
> +            .property_read::<CString>(c_str!("compatible"))
> +            .required()
> +        {
> +            dev_info!(pdev.as_ref(), "compatible string = {:?}\n", str);
> +        }
> +
> +        let prop = dev
> +            .property_read::<bool>(c_str!("test,bool-prop"))
> +            .required()?;
> +        dev_info!(dev, "bool prop is {}\n", prop);
> +
> +        if dev.property_present(c_str!("test,u32-prop")) {
> +            dev_info!(dev, "'test,u32-prop' is present\n");
> +        }
> +
> +        let prop = dev
> +            .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
> +        );

Printing the default value looks more like a test, rather than a sample.

> +
> +        // Missing property without a default will print an error
> +        let _ = dev
> +            .property_read::<u32>(c_str!("test,u32-required-prop"))
> +            .required()?;
> +
> +        let prop: u32 = dev.property_read(c_str!("test,u32-prop")).required()?;
> +        dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
> +
> +        let prop: [i16; 4] = dev.property_read(c_str!("test,i16-array")).required()?;
> +        dev_info!(dev, "'test,i16-array' is {:?}\n", prop);
> +        dev_info!(
> +            dev,
> +            "'test,i16-array' length is {}\n",
> +            dev.property_count_elem::<u16>(c_str!("test,i16-array"))
> +                .unwrap()

Please no unwrap() in the kernel, it may panic.

> +        );
> +
> +        let prop: KVec<i16> = dev
> +            .property_read_array_vec(c_str!("test,i16-array"), 4)?
> +            .required()?;
> +        dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop);
> +

Please move this code to a new function, e.g.

	impl SampleDriver {
	   fn properties_parse(dev: &device::Device)) -> Result;
	}

in order to keep probe() as clean as possible. If we put too much stuff in
there, it might become too confusing to serve as a simple example showing how to
implement a platform driver in Rust.

Besides that, are we sure we really want to print everything?

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Rob Herring]

On Thu, Mar 27, 2025 at 3:37 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Mar 26, 2025 at 06:13:40PM +0100, 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 APIs has to be duplicated on a fwnode as they are in C.
> >
> > A related discussion can be found on the R4L Zulip[1].
> >
> > [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697
>
> You can make the above to be a Link tag like
>
> Link: ... [1]
>
> > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>
> ...
>
> > +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> > +{
> > +     return dev_fwnode(dev);
> > +}
>
> Why not const? For most of the property retrieval APIs the parameter is const.

Because you might need to modify the refcount.

Though the rust code should probably use __dev_fwnode() and/or
__dev_fwnode_const() directly and avoid the need for the helper here.

Rob

Re: [PATCH 08/10] rust: property: Add property_get_reference_args

[Danilo Krummrich]

On Wed, Mar 26, 2025 at 06:13:47PM +0100, Remo Senekowitsch wrote:
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/property.rs | 63 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index dc927ad93..f1d0a33ba 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -8,6 +8,7 @@
>  
>  use crate::{
>      alloc::KVec,
> +    arrayvec::ArrayVec,
>      bindings,
>      device::Device,
>      error::{to_result, Result},
> @@ -64,6 +65,20 @@ pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
>      pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
>          self.fwnode().children()
>      }
> +
> +    /// Finds a reference with arguments.
> +    pub fn property_get_reference_args(
> +        &self,
> +        prop: &CStr,
> +        nargs: NArgs<'_>,
> +        index: u32,
> +    ) -> Result<(
> +        ARef<FwNode>,
> +        ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>,
> +    )> {
> +        self.fwnode()
> +            .property_get_reference_args(prop, nargs, index)
> +    }
>  }
>  
>  /// A reference-counted fwnode_handle.
> @@ -226,6 +241,45 @@ 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<(
> +        ARef<Self>,
> +        ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>,
> +    )> {
> +        let mut out_args = bindings::fwnode_reference_args::default();
> +
> +        let (nargs_prop, nargs) = match nargs {
> +            NArgs::Prop(nargs_prop) => (nargs_prop.as_char_ptr(), 0),
> +            NArgs::N(nargs) => (ptr::null(), nargs),
> +        };
> +
> +        let ret = unsafe {
> +            bindings::fwnode_property_get_reference_args(
> +                self.0.get(),
> +                prop.as_char_ptr(),
> +                nargs_prop,
> +                nargs,
> +                index,
> +                &mut out_args,
> +            )
> +        };
> +        to_result(ret)?;
> +
> +        let node = unsafe { FwNode::from_raw(out_args.fwnode) };
> +        let mut args = ArrayVec::default();
> +
> +        for i in 0..out_args.nargs {
> +            args.push(out_args.args[i as usize]);
> +        }

Why the copy? Can't you just write an abstraction for struct
fwnode_reference_args and just return an instance of that?

Re: [PATCH 07/10] rust: Add arrayvec

[Danilo Krummrich]

On Wed, Mar 26, 2025 at 06:13:46PM +0100, Remo Senekowitsch wrote:
> This patch is basically a proof of concept intendend to gather feedback
> about how to do this properly. Normally I would want to use the crate
> from crates.io[1], but that's not an option in the kernel. We could also
> vendor the entire source code of arrayvec. I'm not sure if people will
> be happy with that.

Do we really need this? The only user in this series I could spot was
property_get_reference_args(). And I think that with a proper abstraction of
struct fwnode_reference_args we could avoid to copy memory at all.

If it turns out we actually need something like this, I'd prefer to move it to
rust/kernel/alloc/ and see if it's worth to derive a common trait that maybe can
share a few things between ArrayVec and Vec.

> 
> [1] https://crates.io/crates/arrayvec
> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/arrayvec.rs | 81 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs      |  1 +
>  2 files changed, 82 insertions(+)
>  create mode 100644 rust/kernel/arrayvec.rs
> 
> diff --git a/rust/kernel/arrayvec.rs b/rust/kernel/arrayvec.rs
> new file mode 100644
> index 000000000..041e7dcce
> --- /dev/null
> +++ b/rust/kernel/arrayvec.rs
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Provides [ArrayVec], a stack-allocated vector with static capacity.
> +
> +use core::mem::MaybeUninit;
> +
> +/// A stack-allocated vector with statically fixed capacity.
> +///
> +/// This can be useful to avoid heap allocation and still ensure safety where a
> +/// small but dynamic number of elements is needed.
> +///
> +/// For example, consider a function that returns a variable number of values,
> +/// but no more than 8. In C, one might achieve this by passing a pointer to
> +/// a stack-allocated array as an out-parameter and making the function return
> +/// the actual number of elements. This is not safe, because nothing prevents
> +/// the caller from reading elements from the array that weren't actually
> +/// initialized by the function. `ArrayVec` solves this problem, users are
> +/// prevented from accessing uninitialized elements.
> +///
> +/// This basically exists already (in a much more mature form) on crates.io:
> +/// <https://crates.io/crates/arrayvec>
> +#[derive(Debug)]
> +pub struct ArrayVec<const N: usize, T> {
> +    array: [core::mem::MaybeUninit<T>; N],
> +    len: usize,
> +}
> +
> +impl<const N: usize, T> ArrayVec<N, T> {
> +    /// Adds a new element to the end of the vector.
> +    ///
> +    /// # Panics
> +    ///
> +    /// Panics if the vector is already full.
> +    pub fn push(&mut self, elem: T) {
> +        if self.len == N {
> +            panic!("OOM")

Please do not panic, this should return a Result instead.

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Andy Shevchenko]

On Thu, Mar 27, 2025 at 08:55:42AM -0500, Rob Herring wrote:
> On Thu, Mar 27, 2025 at 3:37 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote:

...

> > > +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> > > +{
> > > +     return dev_fwnode(dev);
> > > +}
> >
> > Why not const? For most of the property retrieval APIs the parameter is const.
> 
> Because you might need to modify the refcount.
> 
> Though the rust code should probably use __dev_fwnode() and/or
> __dev_fwnode_const() directly and avoid the need for the helper here.

Indeed, that would help at least to understand the intention.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Remo Senekowitsch]

On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
> On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
>> The rust bindings for reading device properties has a single
>> implementation supporting differing sizes of integers. The fwnode C API
>> already has a similar interface, but it is not exposed with the
>> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>
> ...
>
>> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
>
> I'm not sure about this. We have a lot of assumptions in the code that the
> arrays beneath are only represented by the selected number of integer types.
> This opens a Pandora's box, e.g., reading in u24, which is not supported by
> the upper layers..
>
>> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
>> +				   unsigned int elem_size, void *val, size_t nval);

Here's an alternative approach using a macro to map each integer type explicitly
to its corresponding read function. There are some additional changes that will
be necessary to make the rest work, but this is the gist of it.

+macro_rules! impl_property_for_int {
+    ($($int:ty: $f:ident),* $(,)?) => {
+        $(
+            impl<const N: usize> Property for [$int; N] {
+                fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+                    let mut val: [MaybeUninit<$int>; N] = [const { MaybeUninit::uninit() }; N];
+
+                    // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is valid
+                    // because `fwnode` is valid. `val.as_ptr` is valid because `val` is valid.
+                    let ret = unsafe {
+                        bindings::$f(
+                            fwnode.as_raw(),
+                            name.as_char_ptr(),
+                            val.as_mut_ptr().cast(),
+                            val.len(),
+                        )
+                    };
+                    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 Property for $int {
+                fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+                    let val: [_; 1] = <[$int; 1] as Property>::read(fwnode, name)?;
+                    Ok(val[0])
+                }
+            }
+        )*
+    };
+}
+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 04/10] rust: Add bindings for reading device properties

[Remo Senekowitsch]

On Wed Mar 26, 2025 at 10:27 PM CET, Rob Herring wrote:
> On Wed, Mar 26, 2025 at 06:13:43PM +0100, Remo Senekowitsch wrote:
>> +    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
>> +        let mut str = core::ptr::null_mut();
>> +        let pstr: *mut _ = &mut str;
>> +
>> +        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is
>> +        // valid because `self` is valid.
>> +        let ret = unsafe {
>> +            bindings::fwnode_property_read_string(self.as_raw(), name.as_char_ptr(), pstr as _)
>> +        };
>> +        to_result(ret)?;
>> +
>> +        // SAFETY: `pstr` contains a non-null ptr on success
>> +        let str = unsafe { CStr::from_char_ptr(*pstr) };
>> +        Ok(str.try_into()?)
>> +    }
>
> There's a problem with the C version of this function that I'd like to 
> not repeat in Rust especially since ownership is clear. 
>
> The issue is that we never know when the returned string is no longer 
> needed. For DT overlays, we need to be able free the string when/if an 
> overlay is removed. Though overlays are somewhat orthogonal to this. 
> It's really just when the property's node refcount goes to 0 that the 
> property value could be freed.
>
> So this function should probably return a copy of the string that the 
> caller owns.

I think the function already does that. The variable `str` is of type `&CStr`,
so the expression `str.try_into()?` on the last line calls the implementation
of `impl TryFrom<&CStr> for CString`, which is documented here:
https://rust.docs.kernel.org/kernel/str/struct.CString.html#impl-TryFrom%3C%26CStr%3E-for-CString

And looking at the source, it shows that the `CString` is created with a new
owned memory allocation:
https://rust.docs.kernel.org/src/kernel/str.rs.html#878-890

Remo

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
> On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
> > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> >> The rust bindings for reading device properties has a single
> >> implementation supporting differing sizes of integers. The fwnode C API
> >> already has a similar interface, but it is not exposed with the
> >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.

...

> >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> >
> > I'm not sure about this. We have a lot of assumptions in the code that the
> > arrays beneath are only represented by the selected number of integer types.
> > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > the upper layers..
> >
> >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
> >> +				   unsigned int elem_size, void *val, size_t nval);
> 
> Here's an alternative approach using a macro to map each integer type explicitly
> to its corresponding read function. There are some additional changes that will
> be necessary to make the rest work, but this is the gist of it.

I don;'t know Rust to tell anything about this, but at least it feels much
better approach.

> +macro_rules! impl_property_for_int {
> +    ($($int:ty: $f:ident),* $(,)?) => {
> +        $(
> +            impl<const N: usize> Property for [$int; N] {
> +                fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> +                    let mut val: [MaybeUninit<$int>; N] = [const { MaybeUninit::uninit() }; N];
> +
> +                    // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is valid
> +                    // because `fwnode` is valid. `val.as_ptr` is valid because `val` is valid.
> +                    let ret = unsafe {
> +                        bindings::$f(
> +                            fwnode.as_raw(),
> +                            name.as_char_ptr(),
> +                            val.as_mut_ptr().cast(),
> +                            val.len(),
> +                        )
> +                    };
> +                    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 Property for $int {
> +                fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> +                    let val: [_; 1] = <[$int; 1] as Property>::read(fwnode, name)?;
> +                    Ok(val[0])
> +                }
> +            }
> +        )*
> +    };
> +}
> +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,

Dunno what these mean. Are they signed integers? Do you really need this kind
of types to be supported? Properties usually unsigned and I never saw (but it
doesn't mean they are not exist) the negative values there.  Does DT schema
even allow this? Does DT validator complain or not on the negative values
supplied in the DTS?

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Thu, Mar 27, 2025 at 3:41 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > The rust bindings for reading device properties has a single
> > implementation supporting differing sizes of integers. The fwnode C API
> > already has a similar interface, but it is not exposed with the
> > fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>
> ...
>
> > +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
>
> I'm not sure about this. We have a lot of assumptions in the code that the
> arrays beneath are only represented by the selected number of integer types.
> This opens a Pandora's box, e.g., reading in u24, which is not supported by
> the upper layers..

We can probably drop the export if it is just that which you object to.

Without this function, the implementation is just multiple levels of
switch statements to work-around what's needed for the nicest rust
API.

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Thu, Apr 3, 2025 at 8:29 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
> > On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > >> The rust bindings for reading device properties has a single
> > >> implementation supporting differing sizes of integers. The fwnode C API
> > >> already has a similar interface, but it is not exposed with the
> > >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>
> ...
>
> > >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> > >
> > > I'm not sure about this. We have a lot of assumptions in the code that the
> > > arrays beneath are only represented by the selected number of integer types.
> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > > the upper layers..
> > >
> > >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
> > >> +                             unsigned int elem_size, void *val, size_t nval);
> >
> > Here's an alternative approach using a macro to map each integer type explicitly
> > to its corresponding read function. There are some additional changes that will
> > be necessary to make the rest work, but this is the gist of it.
>
> I don;'t know Rust to tell anything about this, but at least it feels much
> better approach.

I know a little Rust and it is much worse. It is implementing the same
code 8 times instead of 1 time just to work-around the C API.

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 11:04:32AM -0500, Rob Herring wrote:
> On Thu, Mar 27, 2025 at 3:41 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > > The rust bindings for reading device properties has a single
> > > implementation supporting differing sizes of integers. The fwnode C API
> > > already has a similar interface, but it is not exposed with the
> > > fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.

...

> > > +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> >
> > I'm not sure about this. We have a lot of assumptions in the code that the
> > arrays beneath are only represented by the selected number of integer types.
> > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > the upper layers..
> 
> We can probably drop the export if it is just that which you object to.

Yes, this is main point, but dropping it does not prevent from still using in
the complied-in code. Is it possible to hide it better?

> Without this function, the implementation is just multiple levels of
> switch statements to work-around what's needed for the nicest rust
> API.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Remo Senekowitsch]

On Thu Apr 3, 2025 at 6:08 PM CEST, Rob Herring wrote:
> On Thu, Apr 3, 2025 at 8:29 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
>> > On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
>> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
>> > >> The rust bindings for reading device properties has a single
>> > >> implementation supporting differing sizes of integers. The fwnode C API
>> > >> already has a similar interface, but it is not exposed with the
>> > >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>>
>> ...
>>
>> > >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
>> > >
>> > > I'm not sure about this. We have a lot of assumptions in the code that the
>> > > arrays beneath are only represented by the selected number of integer types.
>> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
>> > > the upper layers..
>> > >
>> > >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
>> > >> +                             unsigned int elem_size, void *val, size_t nval);
>> >
>> > Here's an alternative approach using a macro to map each integer type explicitly
>> > to its corresponding read function. There are some additional changes that will
>> > be necessary to make the rest work, but this is the gist of it.
>>
>> I don;'t know Rust to tell anything about this, but at least it feels much
>> better approach.
>
> I know a little Rust and it is much worse. It is implementing the same
> code 8 times instead of 1 time just to work-around the C API.

You mean it's worse because it will generate too much code during compile time?

Remo

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Thu, Apr 3, 2025 at 11:15 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Apr 03, 2025 at 11:04:32AM -0500, Rob Herring wrote:
> > On Thu, Mar 27, 2025 at 3:41 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > > > The rust bindings for reading device properties has a single
> > > > implementation supporting differing sizes of integers. The fwnode C API
> > > > already has a similar interface, but it is not exposed with the
> > > > fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>
> ...
>
> > > > +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> > >
> > > I'm not sure about this. We have a lot of assumptions in the code that the
> > > arrays beneath are only represented by the selected number of integer types.
> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > > the upper layers..
> >
> > We can probably drop the export if it is just that which you object to.
>
> Yes, this is main point, but dropping it does not prevent from still using in
> the complied-in code. Is it possible to hide it better?

Don't put any declaration in the header and declare it in the rust
code? But lack of declaration generates warnings.

Also, all the backends will reject an arbitrary size. So your worry
about u24 or other odd sizes isn't really valid. But if you want to be
doubly paranoid for when we add a new firmware backend (shoot me now),
you could move this from the swnode implementation to the fwnode
implementation:

        if (!is_power_of_2(elem_size) || elem_size > sizeof(u64))
                return -ENXIO;

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Remo Senekowitsch]

On Thu Apr 3, 2025 at 6:08 PM CEST, Rob Herring wrote:
> On Thu, Apr 3, 2025 at 8:29 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
>> > On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
>> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
>> > >> The rust bindings for reading device properties has a single
>> > >> implementation supporting differing sizes of integers. The fwnode C API
>> > >> already has a similar interface, but it is not exposed with the
>> > >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>>
>> ...
>>
>> > >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
>> > >
>> > > I'm not sure about this. We have a lot of assumptions in the code that the
>> > > arrays beneath are only represented by the selected number of integer types.
>> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
>> > > the upper layers..
>> > >
>> > >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
>> > >> +                             unsigned int elem_size, void *val, size_t nval);
>> >
>> > Here's an alternative approach using a macro to map each integer type explicitly
>> > to its corresponding read function. There are some additional changes that will
>> > be necessary to make the rest work, but this is the gist of it.
>>
>> I don;'t know Rust to tell anything about this, but at least it feels much
>> better approach.
>
> I know a little Rust and it is much worse. It is implementing the same
> code 8 times instead of 1 time just to work-around the C API.

I prepared a functioning version of the macro-based approach. I'll post
the patch for reference and discussion. We don't have to go with it.

Remo

---
 rust/kernel/property.rs | 124 +++++++++++++++++++++++++---------------
 1 file changed, 77 insertions(+), 47 deletions(-)

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index ceedd1a82..78dcc189e 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -4,7 +4,7 @@
 //!
 //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
 
-use core::{ffi::c_void, mem::MaybeUninit, ptr};
+use core::{mem::MaybeUninit, ptr};
 
 use crate::{
     alloc::KVec,
@@ -13,7 +13,7 @@
     error::{to_result, Result},
     prelude::*,
     str::{CStr, CString},
-    types::{ARef, Integer, Opaque},
+    types::{ARef, Opaque},
 };
 
 impl Device {
@@ -43,7 +43,7 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
     }
 
     /// Returns firmware property `name` integer array values in a KVec
-    pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
+    pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
         &'fwnode self,
         name: &'name CStr,
         len: usize,
@@ -52,7 +52,7 @@ pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
     }
 
     /// Returns integer array length for firmware property `name`
-    pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
+    pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
         self.fwnode().property_count_elem::<T>(name)
     }
 
@@ -148,24 +148,17 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
     }
 
     /// Returns firmware property `name` integer array values in a KVec
-    pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
+    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: `name` is non-null and null-terminated. `self.as_raw` is valid
-        // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
-        let err = unsafe {
-            bindings::fwnode_property_read_int_array(
-                self.as_raw(),
-                name.as_char_ptr(),
-                T::SIZE as u32,
-                val.as_ptr() as *mut c_void,
-                len,
-            )
-        };
+        // 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 { T::read_array_out_param(self, name, val.as_mut_ptr(), len) };
         let res = if err < 0 {
             Err(Error::from_errno(err))
         } else {
@@ -181,19 +174,11 @@ pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
     }
 
     /// Returns integer array length for firmware property `name`
-    pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
+    pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
         // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
         // because `self` is valid. Passing null pointer buffer is valid to obtain
         // the number of elements in the property array.
-        let ret = unsafe {
-            bindings::fwnode_property_read_int_array(
-                self.as_raw(),
-                name.as_char_ptr(),
-                T::SIZE as u32,
-                ptr::null_mut(),
-                0,
-            )
-        };
+        let ret = unsafe { T::read_array_out_param(self, name, ptr::null_mut(), 0) };
         to_result(ret)?;
         Ok(ret as usize)
     }
@@ -201,8 +186,8 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<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, integers and arrays
-    /// of integers.
+    /// 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
@@ -300,6 +285,9 @@ pub fn property_get_reference_args(
             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(),
@@ -388,34 +376,76 @@ fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
         Ok(str.try_into()?)
     }
 }
-impl<T: Integer, const N: usize> Property for [T; N] {
+/// Implemented for all integers that can be read as properties.
+///
+/// This helper trait is needed to associate the integer types of various sizes
+/// with their corresponding `fwnode_property_read_*_array` functions.
+pub trait PropertyInt: Property {
+    /// # 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(
+        fwnode: &FwNode,
+        name: &CStr,
+        out_param: *mut Self,
+        len: 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 Property for $int {
+            fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+                let val: [_; 1] = <[$int; 1] as Property>::read(fwnode, name)?;
+                Ok(val[0])
+            }
+        }
+        impl PropertyInt for $int {
+            unsafe fn read_array_out_param(
+                fwnode: &FwNode,
+                name: &CStr,
+                out_param: *mut Self,
+                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 {
+                    bindings::$f(fwnode.as_raw(), name.as_char_ptr(), out_param, len)
+                }
+            }
+        }
+    )* };
+}
+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,
+}
+impl<T: PropertyInt, const N: usize> Property for [T; N] {
     fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
         let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
 
-        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is valid
-        // because `fwnode` is valid. `val.as_ptr` is valid because `val` is valid.
-        let ret = unsafe {
-            bindings::fwnode_property_read_int_array(
-                fwnode.as_raw(),
-                name.as_char_ptr(),
-                T::SIZE as u32,
-                val.as_mut_ptr().cast(),
-                val.len(),
-            )
-        };
+        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
+        // valid because `fwnode` is valid. `val.as_ptr` is valid because `val`
+        // is valid. Casting from `*mut MaybeUninit<T>` to `*mut T` is safe
+        // because `MaybeUninit<T>` has the same memory layout as `T`.
+        let ret = unsafe { T::read_array_out_param(fwnode, name, val.as_mut_ptr().cast(), N) };
         to_result(ret)?;
 
         // SAFETY: `val` is always initialized when
-        // fwnode_property_read_int_array is successful.
+        // fwnode_property_read_$t_array is successful.
         Ok(val.map(|v| unsafe { v.assume_init() }))
     }
 }
-impl<T: Integer> Property for T {
-    fn read(fwnode: &FwNode, name: &CStr) -> Result<T> {
-        let val: [_; 1] = <[T; 1] as Property>::read(fwnode, name)?;
-        Ok(val[0])
-    }
-}
 
 /// The number of arguments of a reference.
 pub enum NArgs<'a> {
-- 
2.49.0

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Thu, Apr 3, 2025 at 12:04 PM Remo Senekowitsch <remo@buenzli.dev> wrote:
>
> On Thu Apr 3, 2025 at 6:08 PM CEST, Rob Herring wrote:
> > On Thu, Apr 3, 2025 at 8:29 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> >>
> >> On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
> >> > On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
> >> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> >> > >> The rust bindings for reading device properties has a single
> >> > >> implementation supporting differing sizes of integers. The fwnode C API
> >> > >> already has a similar interface, but it is not exposed with the
> >> > >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
> >>
> >> ...
> >>
> >> > >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> >> > >
> >> > > I'm not sure about this. We have a lot of assumptions in the code that the
> >> > > arrays beneath are only represented by the selected number of integer types.
> >> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> >> > > the upper layers..
> >> > >
> >> > >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
> >> > >> +                             unsigned int elem_size, void *val, size_t nval);
> >> >
> >> > Here's an alternative approach using a macro to map each integer type explicitly
> >> > to its corresponding read function. There are some additional changes that will
> >> > be necessary to make the rest work, but this is the gist of it.
> >>
> >> I don;'t know Rust to tell anything about this, but at least it feels much
> >> better approach.
> >
> > I know a little Rust and it is much worse. It is implementing the same
> > code 8 times instead of 1 time just to work-around the C API.
>
> I prepared a functioning version of the macro-based approach. I'll post
> the patch for reference and discussion. We don't have to go with it.

Please wait for the discussion to finish.

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 11:36:38AM -0500, Rob Herring wrote:
> On Thu, Apr 3, 2025 at 11:15 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Apr 03, 2025 at 11:04:32AM -0500, Rob Herring wrote:
> > > On Thu, Mar 27, 2025 at 3:41 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > > > > The rust bindings for reading device properties has a single
> > > > > implementation supporting differing sizes of integers. The fwnode C API
> > > > > already has a similar interface, but it is not exposed with the
> > > > > fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.

...

> > > > > +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> > > >
> > > > I'm not sure about this. We have a lot of assumptions in the code that the
> > > > arrays beneath are only represented by the selected number of integer types.
> > > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > > > the upper layers..
> > >
> > > We can probably drop the export if it is just that which you object to.
> >
> > Yes, this is main point, but dropping it does not prevent from still using in
> > the complied-in code. Is it possible to hide it better?
> 
> Don't put any declaration in the header and declare it in the rust
> code? But lack of declaration generates warnings.

Exactly. And I believe we have the typed versions of int_array for a reason.
Otherwise what's the point in having them to begin with?
(The first what comes to my mind is a compile time type checking, so we don't
 try to load u8 with u32 data or any other dirty tricks.)

Maybe it deserves a header that can be included explicitly in the rust stuff
and being checked at compile time to avoid people using that? Can we achieve
something like C preprocessor

#ifndef FOO
#error This header must not be included directly
#endif

> Also, all the backends will reject an arbitrary size. So your worry
> about u24 or other odd sizes isn't really valid. But if you want to be
> doubly paranoid for when we add a new firmware backend (shoot me now),
> you could move this from the swnode implementation to the fwnode
> implementation:
> 
>         if (!is_power_of_2(elem_size) || elem_size > sizeof(u64))
>                 return -ENXIO;

That might work. But still an interface of int_array seems lower by
level than typed ones.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Thu, Apr 3, 2025 at 12:55 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Apr 03, 2025 at 11:36:38AM -0500, Rob Herring wrote:
> > On Thu, Apr 3, 2025 at 11:15 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Apr 03, 2025 at 11:04:32AM -0500, Rob Herring wrote:
> > > > On Thu, Mar 27, 2025 at 3:41 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > > > > > The rust bindings for reading device properties has a single
> > > > > > implementation supporting differing sizes of integers. The fwnode C API
> > > > > > already has a similar interface, but it is not exposed with the
> > > > > > fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>
> ...
>
> > > > > > +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> > > > >
> > > > > I'm not sure about this. We have a lot of assumptions in the code that the
> > > > > arrays beneath are only represented by the selected number of integer types.
> > > > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > > > > the upper layers..
> > > >
> > > > We can probably drop the export if it is just that which you object to.
> > >
> > > Yes, this is main point, but dropping it does not prevent from still using in
> > > the complied-in code. Is it possible to hide it better?
> >
> > Don't put any declaration in the header and declare it in the rust
> > code? But lack of declaration generates warnings.
>
> Exactly. And I believe we have the typed versions of int_array for a reason.
> Otherwise what's the point in having them to begin with?
> (The first what comes to my mind is a compile time type checking, so we don't
>  try to load u8 with u32 data or any other dirty tricks.)
>
> Maybe it deserves a header that can be included explicitly in the rust stuff
> and being checked at compile time to avoid people using that? Can we achieve
> something like C preprocessor
>
> #ifndef FOO
> #error This header must not be included directly
> #endif

Looks there are RUST_BINDGEN or __BINDGEN__ define which could be used
here. Don't need a header, just wrap the declaration. No idea which
one would be preferred as there's exactly 1 use of each. Miguel?

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Miguel Ojeda]

On Thu, Apr 3, 2025 at 8:48 PM Rob Herring <robh@kernel.org> wrote:
>
> Looks there are RUST_BINDGEN or __BINDGEN__ define which could be used
> here. Don't need a header, just wrap the declaration. No idea which
> one would be preferred as there's exactly 1 use of each. Miguel?

If you mean bcachefs' `RUST_BINDGEN`, then I think that one comes from
bcachefs-tools, i.e. we don't define it.

Cheers,
Miguel

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 10:36:40PM +0200, Miguel Ojeda wrote:
> On Thu, Apr 3, 2025 at 8:48 PM Rob Herring <robh@kernel.org> wrote:
> >
> > Looks there are RUST_BINDGEN or __BINDGEN__ define which could be used
> > here. Don't need a header, just wrap the declaration. No idea which
> > one would be preferred as there's exactly 1 use of each. Miguel?
> 
> If you mean bcachefs' `RUST_BINDGEN`, then I think that one comes from
> bcachefs-tools, i.e. we don't define it.

But is there a way to have a function that is not really exported to be used in
Rust only code?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Remo Senekowitsch]

On Thu Apr 3, 2025 at 6:08 PM CEST, Rob Herring wrote:
> On Thu, Apr 3, 2025 at 8:29 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
>> > On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
>> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
>> > >> The rust bindings for reading device properties has a single
>> > >> implementation supporting differing sizes of integers. The fwnode C API
>> > >> already has a similar interface, but it is not exposed with the
>> > >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>>
>> ...
>>
>> > >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
>> > >
>> > > I'm not sure about this. We have a lot of assumptions in the code that the
>> > > arrays beneath are only represented by the selected number of integer types.
>> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
>> > > the upper layers..
>> > >
>> > >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
>> > >> +                             unsigned int elem_size, void *val, size_t nval);
>> >
>> > Here's an alternative approach using a macro to map each integer type explicitly
>> > to its corresponding read function. There are some additional changes that will
>> > be necessary to make the rest work, but this is the gist of it.
>>
>> I don;'t know Rust to tell anything about this, but at least it feels much
>> better approach.
>
> I know a little Rust and it is much worse. It is implementing the same
> code 8 times instead of 1 time just to work-around the C API.

Can you please elaborate on this concern?

The previous approach uses generics, which means the function is also
"copy-pasted" for each concrete type it is used with at compile time,
just like with a macro. So using a macro wouldn't be worse than generics
if binary size is the concern.

If the concern is about readability, I have managed to move all logic
outside the macro body, all that remains is the association between type
and `*_read_array` function.

Remo

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Remo Senekowitsch]

On Wed Mar 26, 2025 at 9:51 PM CET, Rob Herring wrote:
> On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote:
>>
>> +impl Device {
>> +    /// Obtain the fwnode corresponding to the device.
>> +    fn fwnode(&self) -> &FwNode {
>> +        // SAFETY: `self` is valid.
>> +        let fwnode_handle = unsafe { bindings::dev_fwnode(self.as_raw()) };
>> +        if fwnode_handle.is_null() {
>> +            panic!("fwnode_handle cannot be null");
>
> It's usually not a good idea to panic the kernel especially with 
> something a driver calls as that's probably recoverable.
>
> Users/drivers testing fwnode_handle/of_node for NULL is pretty common. 
> Though often that's a legacy code path, so maybe not allowing NULL is 
> fine for now.

Just to be clear on this, should I keep this as is, or return a result?
In the latter case, all the duplicated methods on `Device` that just
call `self.fwnode().same_method()` would have a result in their function
signatur as well. That includes `property_present`, `read_property`
and `children`.

>> +        }
>> +        // 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.
>> +        unsafe { &*fwnode_handle.cast() }
>> +    }
>> +
>> +    /// Checks if property is present or not.
>> +    pub fn property_present(&self, name: &CStr) -> bool {
>> +        self.fwnode().property_present(name)
>> +    }
>> +}
>
> The C developer in me wants to put this after the FwNode stuff since 
> this uses it.

Is that just a comment or a call to action? :-)

Remo

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Fri, Apr 4, 2025 at 6:00 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Apr 03, 2025 at 10:36:40PM +0200, Miguel Ojeda wrote:
> > On Thu, Apr 3, 2025 at 8:48 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > Looks there are RUST_BINDGEN or __BINDGEN__ define which could be used
> > > here. Don't need a header, just wrap the declaration. No idea which
> > > one would be preferred as there's exactly 1 use of each. Miguel?
> >
> > If you mean bcachefs' `RUST_BINDGEN`, then I think that one comes from
> > bcachefs-tools, i.e. we don't define it.
>
> But is there a way to have a function that is not really exported to be used in
> Rust only code?

#ifdef __BINDGEN__
int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
const char *propname,
                                unsigned int elem_size, void *val, size_t nval);
#endif

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Fri, Apr 04, 2025 at 09:12:57AM -0500, Rob Herring wrote:
> On Fri, Apr 4, 2025 at 6:00 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Apr 03, 2025 at 10:36:40PM +0200, Miguel Ojeda wrote:
> > > On Thu, Apr 3, 2025 at 8:48 PM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > Looks there are RUST_BINDGEN or __BINDGEN__ define which could be used
> > > > here. Don't need a header, just wrap the declaration. No idea which
> > > > one would be preferred as there's exactly 1 use of each. Miguel?
> > >
> > > If you mean bcachefs' `RUST_BINDGEN`, then I think that one comes from
> > > bcachefs-tools, i.e. we don't define it.
> >
> > But is there a way to have a function that is not really exported to be used in
> > Rust only code?
> 
> #ifdef __BINDGEN__
> int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
> const char *propname,
>                                 unsigned int elem_size, void *val, size_t nval);
> #endif

Okay. This can be working compromise. Meanwhile I leave the discussion go on
how to proceed the best on the Rust code side. It seems to me if the macro
approach is indeed looks nice (let's say comparatively nice as this piece in C
header) I would prefer that we do not export it at all.

-- 
With Best Regards,
Andy Shevchenko