Message-ID: <CAH5fLghrgnr_ok5deSAi0JnbWFoWG-4de2K_Hg8qHytyEp_y7w@mail.gmail.com>
Date: Fri, 22 Nov 2024 09:47:09 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Dirk Behme <dirk.behme@...bosch.com>
Cc: Saravana Kannan <saravanak@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J . Wysocki" <rafael@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Rob Herring <robh@...nel.org>, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH RFC v2 1/1] rust: Add bindings for device properties

On Fri, Nov 22, 2024 at 9:13 AM Dirk Behme <dirk.behme@...bosch.com> wrote:
>
> From: "Rob Herring (Arm)" <robh@...nel.org>
>
> 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@mail.gmail.com/ [1]
> Co-developed-by: Dirk Behme <dirk.behme@...bosch.com>
> Signed-off-by: Dirk Behme <dirk.behme@...bosch.com>
> Signed-off-by: Rob Herring (Arm) <robh@...nel.org>
> ---
>
> This is an update of Rob's initial patch
>
> https://lore.kernel.org/rust-for-linux/20241025-rust-platform-dev-v1-0-0df8dcf7c20b@kernel.org/
>
> 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@kernel.org/

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

Powered by blists - more mailing lists