| Message ID | 20250326171411.590681-4-remo@buenzli.dev |
|---|---|
| State | New |
| Headers | show |
| Series | More Rust bindings for device property reads | expand |
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);
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, +}
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
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
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> {
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.
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);