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

["Remo Senekowitsch" <remo@buenzli.dev>]

On Tue May 20, 2025 at 9:37 AM CEST, Benno Lossin wrote:
> 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.

What is the concrete problem with that? Or is it just "not pretty"?
(I'm fine with making it prettier, just making sure I understand
correctly.)

>> +}
>> +// 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`.

It's used in three places. Taking Rust references as input reduces the
safety requirements the callers must uphold. But I guess it's a small
benefit, I can remove it.

> 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`...)

Yeah, I saw that too. The original implementation is from Rob. I assume
the recommendation is for users, maybe for clarity and readability of
the code. These Rust abstractions are pretty low level, so I thought
it's probably fine.

> 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.

I think I tried to make the macro as small as possible and use regular
generics on top, because I was concerned people would be put off by big
complicated macros. I'm fine with moving the actual trait implementation
into the macro body, seems reasonable.

But still, I'm not sure why we're trying to make the PropertyInt trait's
interface pretty or rusty. It's not supposed to be used, implemented or
in any way interacted with outside this module. It's just a low-level
building block to make some functions generic, giving users type
inference.

Best regards,
Remo