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

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

On Mon May 19, 2025 at 6:55 PM CEST, Danilo Krummrich wrote:
> On Mon, May 19, 2025 at 05:43:17PM +0200, Remo Senekowitsch wrote:
>> On Mon May 12, 2025 at 3:36 PM CEST, Danilo Krummrich 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.
>> >
>> > I think you have additional requirements on the fwnode, propname and val
>> > pointers as well as on nval, please document them as well.
>> 
>> What are the additional requirements? The implementation just calls the
>> underlying `fwnode_property_read_*_array` with the exact same arguments,
>> so I don't know what the additional requirements are.
>
> First of all, I don't think you can refer to the safety requirements of the
> `fwnode_property_read_*_array` functions, since they don't have any documented
> safety requirements.
>
> So, I think you have safety requirements regarding pointer validity of fwnode,
> propname and val.
>
> Additionally, there's the requirement that val has to be an array of nval
> length.
>
> Also, the PropertyInt trait itself has to be unsafe, given that it contains
> unsafe functions.

I don't think a trait necessarily has to be marked unsafe just because
it has unsafe methods. Marking a trait as unsafe means that implementors
of the trait must uphold some invariants. This is not the case here
IIUC. Here's a good explanation of my understanding: [1]

But I should anyway seal the two traits. They're not supposed to be
implemented outside the kernel crate.

[1] https://users.rust-lang.org/t/safe-trait-with-an-unsafe-method/67993/3

> I also pinged Benno about it, he usually knows best how to cover such things
> properly. :)
>
>> >> +    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;
>> >> +}