| Message ID | 20250326171411.590681-5-remo@buenzli.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | More Rust bindings for device property reads | expand |
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
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
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.