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

["Remo Senekowitsch" <remo@buenzli.dev>]

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