Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

["Benno Lossin" <lossin@kernel.org>]

On Sun May 4, 2025 at 7:31 PM CEST, Remo Senekowitsch wrote:
> +/// Implemented for all integers that can be read as properties.
> +///
> +/// This helper trait is needed on top of the existing [`Property`]
> +/// trait to associate the integer types of various sizes with their
> +/// corresponding `fwnode_property_read_*_array` functions.
> +pub trait PropertyInt: Copy {
> +    /// # Safety
> +    ///
> +    /// Callers must uphold the same safety invariants as for the various
> +    /// `fwnode_property_read_*_array` functions.
> +    unsafe fn read_array_from_fwnode_property(
> +        fwnode: *const bindings::fwnode_handle,
> +        propname: *const ffi::c_char,
> +        val: *mut Self,
> +        nval: usize,
> +    ) -> ffi::c_int;

I really, really dislike that this trait has to have an unsafe function
with all those raw pointer inputs.

> +}
> +// 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 PropertyInt for $int {
> +            unsafe fn read_array_from_fwnode_property(
> +                fwnode: *const bindings::fwnode_handle,
> +                propname: *const ffi::c_char,
> +                val: *mut Self,
> +                nval: usize,
> +            ) -> ffi::c_int {
> +                // SAFETY: The safety invariants on the trait require
> +                // callers to uphold the invariants of the functions
> +                // this macro is called with.
> +                unsafe {
> +                    bindings::$f(fwnode, propname, val.cast(), nval)
> +                }
> +            }
> +        }
> +    )* };
> +}
> +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,
> +}
> +/// # 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<T: PropertyInt>(
> +    fwnode: &FwNode,
> +    name: &CStr,
> +    out_param: *mut T,
> +    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 {
> +        T::read_array_from_fwnode_property(fwnode.as_raw(), name.as_char_ptr(), out_param, len)
> +    }
> +}

Why does this function exist? It doesn't do anything and just delegates
the call to `T::read_array_from_fwnode_property`.

This feels like you're dragging the C interface through the lower layers
of your Rust abstractions, which I wouldn't do. I also looked a bit at
the C code and saw this comment in `driver/base/property.c:324`:

     * It's recommended to call fwnode_property_count_u8() instead of calling
     * this function with @val equals %NULL and @nval equals 0.

That probably holds also for the other functions, so maybe we should do
that instead? (although `fwnode_property_count_u8` just delegates and
calls with `fwnode_property_read_u8_array`...)

How about we do it like this:

    pub trait PropertyInt: Copy + Sized + private::Sealed {
        /// ...
        ///
        /// Returns a reference to `out` containing all written elements.
        fn read<'a>(
            fwnode: &FwNode,
            name: &CStr,
            out: &'a mut [MaybeUninit<Self>],
        ) -> Result<&'a mut [Self]>;
    
        fn length(fwnode: &FwNode, name: &CStr) -> Result<usize>;
    }

And then have a macro to implement it on all the integers.

Hope this helps!

---
Cheers,
Benno