On Fri, Nov 22, 2024 at 9:13 AM Dirk Behme <dirk.behme@xxxxxxxxxxxx> wrote: > > From: "Rob Herring (Arm)" <robh@xxxxxxxxxx> > > 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, nit: "caller-allocated" or "to a variable/buffer allocated by the caller" > 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). > > To build and run the Examples as `rustdoc` tests the kernel Kconfig > options `CONFIG_OF` and `CONFIG_OF_UNITTEST` need to be enabled > additionally. Besides the default `rustdoc` test options > `CONFIG_KUNIT` and `CONFIG_RUST_KERNEL_DOCTESTS`. This even works > on non-ARM architectures as a test device tree is built into the > kernel, then. > > The Integer trait is proposed by Alic Ryhl [1]. typo ;) > > Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiXPZqKpWSSNdx-Ww-E9h2tOLcF3_8Y4C_JQ0eU8EMwFw@xxxxxxxxxxxxxx/ [1] > Co-developed-by: Dirk Behme <dirk.behme@xxxxxxxxxxxx> > Signed-off-by: Dirk Behme <dirk.behme@xxxxxxxxxxxx> > Signed-off-by: Rob Herring (Arm) <robh@xxxxxxxxxx> > --- > > This is an update of Rob's initial patch > > https://lore.kernel.org/rust-for-linux/20241025-rust-platform-dev-v1-0-0df8dcf7c20b@xxxxxxxxxx/ > > condensed in one patch (see below). Rob's initial cover > letter still stands, esp. the part regarding the dependency > on Danilo's PCI and platform device series [2]. > > Changes in v2: > > * Move the major parts to property.rs > * Use the Integer Trait proposed by Alice > * Use MaybeUninit proposed by Alex > * Use Option<> to distinguish between optional and mandatory properties > proposed by Rob > * Introduce a FwProperty trait. The prefix 'Fw' reads as 'Firmware'. > Just 'Property' seems to be conflicting with existing. > * Add some rustdoc documentation and Examples (based on Danilo's > platform sample module). With that I squashed the test device tree > changes into this patch as we don't need to change Danilo's platform > sample any more. That change is trivial. Please let me know if you > rather like a separate patch for it. > * Some more I most probably missed to mention ;) > > It would be nice to get some improvement hints for the FIXMEs :) See below :) > Trying to use the assert_eq!() fails with > > error[E0425]: cannot find value `__DOCTEST_ANCHOR` in this scope > --> rust/doctests_kernel_generated.rs:4252:102 > | > 4252 | kernel::kunit_assert_eq!("rust_doctest_kernel_property_rs_0", "rust/kernel/property.rs", __DOCTEST_ANCHOR - 150, $left, $right); > | ^^^^^^^^^^^^^^^^ not found in this scope > ... > 4369 | assert_eq!(idx, Ok(0)); // FIXME: How to build this? > | ---------------------- in this macro invocation > | > = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info) > > CC drivers/base/firmware_loader/main.o > CC kernel/module/main.o > error: aborting due to 1 previous error > > [2] https://lore.kernel.org/all/20241022213221.2383-1-dakr@xxxxxxxxxx/ I don't know about this one. > +/// A trait for integer types. > +/// > +/// This trait limits [`FromBytes`] and [`AsBytes`] to integer types only. Excluding the > +/// array types. The integer types are `u8`, `u16`, `u32`, `u64`, `usize`, `i8`, `i16`, > +/// `i32`, `i64` and `isize`. Additionally, the size of these types is encoded in the > +/// `IntSize` enum. > +trait Integer: FromBytes + AsBytes + Copy { > + /// The size of the integer type. > + const SIZE: IntSize; > +} This trait should be unsafe with a safety requirement saying that SIZE must be correct. Is it intentional that Integer is a private trait? I guess you're using it as an implementation detail of FwProperty? It would be nice to mention that 128-bit ints are intentionally not included here. > +/// The sizes of the integer types. Either 8, 16, 32 or 64 bits. > +enum IntSize { > + /// 8 bits > + S8, > + /// 16 bits > + S16, > + /// 32 bits > + S32, > + /// 64 bits > + S64, > +} > + > +macro_rules! impl_int { > + ($($typ:ty),* $(,)?) => {$( > + impl Integer for $typ { > + const SIZE: IntSize = match size_of::<Self>() { > + 1 => IntSize::S8, > + 2 => IntSize::S16, > + 4 => IntSize::S32, > + 8 => IntSize::S64, > + _ => panic!("invalid size"), > + }; > + } > + )*}; > +} > + > +impl_int! { > + u8, u16, u32, u64, usize, > + i8, i16, i32, i64, isize, > +} > + > +/// Reads an array of integers from the device firmware. > +fn read_array<T: Integer>( > + device: &Device, > + name: &CStr, > + val: Option<&mut [MaybeUninit<T>]>, > +) -> Result<usize> { > + let (ptr, len) = match val { > + // The read array data case. > + Some(val) => (val.as_mut_ptr(), val.len()), > + // The read count case. > + None => (core::ptr::null_mut(), 0_usize), > + }; > + let ret = match T::SIZE { > + // SAFETY: `device_property_read_u8_array` is called with a valid device pointer and name. > + IntSize::S8 => unsafe { > + bindings::device_property_read_u8_array( > + device.as_raw(), > + name.as_ptr() as *const i8, > + ptr as *mut u8, > + len, Instead of using the i8 type for the name, you should be using crate::ffi::c_char. Actually, in this case, you can also use `name.as_char_ptr()` instead, which requires no cast. > + }, > + // SAFETY: `device_property_read_u16_array` is called with a valid device pointer and name. > + IntSize::S16 => unsafe { > + bindings::device_property_read_u16_array( > + device.as_raw(), > + name.as_ptr() as *const i8, > + ptr as *mut u16, > + len, > + ) > + }, > + // SAFETY: `device_property_read_u32_array` is called with a valid device pointer and name. > + IntSize::S32 => unsafe { > + bindings::device_property_read_u32_array( > + device.as_raw(), > + name.as_ptr() as *const i8, > + ptr as *mut u32, > + len, > + ) > + }, > + // SAFETY: `device_property_read_u64_array` is called with a valid device pointer and name. > + IntSize::S64 => unsafe { > + bindings::device_property_read_u64_array( > + device.as_raw(), > + name.as_ptr() as *const i8, > + ptr as *mut u64, > + len, > + ) > + }, > + }; > + to_result(ret)?; > + Ok(ret.try_into()?) > +} [...] > +/// *Note*: To build and run below examples as `rustdoc` tests the additional kernel > +/// Kconfig options `CONFIG_OF` and `CONFIG_OF_UNITTEST` need to be enabled. This > +/// even works on non-ARM architectures as a test device tree is built into the > +/// kernel, then. Incomplete sentence? Does this mean that running kunit without those options enabled results in an error? > +/// The above examples intentionally don't print any error messages (e.g. with `dev_err!()`). > +/// The called abstractions already print error messages if needed what is considered to be > +/// sufficient. The goal is to be less verbose regarding error messages. typo: "if needed, which is" > +pub trait FwProperty: Sized { > + /// Reads a property from the device. > + fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self>; > + > + /// Gets the properties element count. > + fn count_elem(device: &Device, name: &CStr) -> Result<usize>; > + > + /// Returns if a firmware string property `name` has match for `match_str`. > + fn match_string(device: &Device, name: &CStr, match_str: &CStr) -> Result<usize> { As far as I can tell, you never override this implementation anywhere. In that case, it shouldn't be a trait method. You can just put its implementation directly in property_match_string. > + // SAFETY: `device_property_match_string` is called with a valid device pointer and name. > + let ret = unsafe { > + bindings::device_property_match_string( > + device.as_raw(), > + name.as_ptr() as *const i8, > + match_str.as_ptr() as *const i8, > + ) Ditto here about i8. > + /// Gets the properties element count. > + // FIXME: Could this be made to be a build time error? > + fn count_elem(device: &Device, _name: &CStr) -> Result<usize> { Yes, you should create two traits: pub trait FwPropertyRead: Sized { fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self>; } pub trait FwPropertyCount: Sized { fn count_elem(device: &Device, name: &CStr) -> Result<usize>; } Then don't implement FwPropertyCount for types that can't be counted. I wonder if it also makes more sense to split FwPropertyRead into two traits: FwPropertyReadMandatory FwPropertyReadOptional to handle the boolean case? > +impl<T: Integer + Copy> FwProperty for T { > + fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self> { > + let mut val: [MaybeUninit<T>; 1] = [const { MaybeUninit::uninit() }; 1]; > + match read_array(device, name, Some(&mut val)) { > + // SAFETY: `read_array` returns with valid data > + Ok(_) => Ok(unsafe { mem::transmute_copy(&val[0]) }), This can be simplified: fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self> { let mut val: MaybeUninit<T> = MaybeUninit::uninit(); match read_array(device, name, Some(core::array::from_mut(&mut val))) { // SAFETY: `read_array` returns with valid data Ok(()) => Ok(val.assume_init()), > +impl<T: Integer, const N: usize> FwProperty for [T; N] { > + fn read_property(device: &Device, name: &CStr, default: Option<Self>) -> Result<Self> { > + let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N]; > + match read_array(device, name, Some(&mut val)) { > + // SAFETY: `read_array` returns with valid data > + Ok(_) => Ok(unsafe { mem::transmute_copy(&val) }), I don't think this one can avoid transmute_copy, though. Alice
