Message-Id: <DA0XS7JK5B71.3D2CS7QVKOM52@kernel.org>
Date: Tue, 20 May 2025 13:04:01 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Remo Senekowitsch" <remo@...nzli.dev>, "Rob Herring" <robh@...nel.org>,
 "Saravana Kannan" <saravanak@...gle.com>, "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>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 "Dirk Behme" <dirk.behme@...bosch.com>
Cc: <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v4 6/9] rust: device: Add bindings for reading device
 properties

On Tue May 20, 2025 at 12:32 PM CEST, Remo Senekowitsch wrote:
> 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.)

With the design I explained below, this becomes not necessary. Less
unsafe code always is better, since there are fewer places that can go
wrong.

Also hiding all the unsafe code inside of your module as private code or
implementation detail is also much better, as you prevent people from
accidentally using it.

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

That's true, but why not make the trait take references if all users
will use references anyways :)

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

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

Big macro bodies are rarely the issue. Lots of parsing in declarative
macros is where they become unreadable.

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

Sure, but if we can make it safe, we should. I could also very much
imagine that someone has a statically allocated buffer they would want
to write stuff to and this function would then allow that with much
easier `unsafe` code than the current approach.

---
Cheers,
Benno

Powered by blists - more mailing lists