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@xxxxxxxxxxxxxxx> 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
