Re: [PATCH RFC 2/3] rust: Add bindings for device properties

[Alice Ryhl]

On Tue, Oct 29, 2024 at 6:58 PM Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Tue, Oct 29, 2024 at 9:16 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
> >
> > On Fri, Oct 25, 2024 at 11:06 PM Rob Herring (Arm) <robh@xxxxxxxxxx> wrote:
> > > +
> > > +    /// Returns array length for firmware property `name`
> > > +    ///
> > > +    /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > > +    pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
> >
> > This always returns usize? I'm a bit confused ...
>
> The C version returned an int so we could return an errno or positive
> count. With Result, we don't need negative values and isn't usize
> generally used for counts of things like size_t in C?

Ok, I think I misunderstood what this does. usize is fine.

> > > +        match size_of::<T>() {
> > > +            1 => {
> > > +                ret = unsafe {
> > > +                    bindings::device_property_read_u8_array(
> > > +                        self.as_raw(),
> > > +                        name.as_ptr() as *const i8,
> > > +                        ptr::null_mut(),
> > > +                        0,
> > > +                    )
> > > +                }
> > > +            }
> > > +            2 => {
> > > +                ret = unsafe {
> > > +                    bindings::device_property_read_u16_array(
> > > +                        self.as_raw(),
> > > +                        name.as_ptr() as *const i8,
> > > +                        ptr::null_mut(),
> > > +                        0,
> > > +                    )
> > > +                }
> > > +            }
> > > +            4 => {
> > > +                ret = unsafe {
> > > +                    bindings::device_property_read_u32_array(
> > > +                        self.as_raw(),
> > > +                        name.as_ptr() as *const i8,
> > > +                        ptr::null_mut(),
> > > +                        0,
> > > +                    )
> > > +                }
> > > +            }
> > > +            8 => {
> > > +                ret = unsafe {
> > > +                    bindings::device_property_read_u64_array(
> > > +                        self.as_raw(),
> > > +                        name.as_ptr() as *const i8,
> > > +                        ptr::null_mut(),
> > > +                        0,
> > > +                    )
> > > +                }
> > > +            }
> > > +            _ => return Err(EINVAL),
> >
> > You can use `kernel::build_error!` here to trigger a build failure if
> > the size is wrong.
>
> I really want a build error if the type is wrong, then the _ case
> would be unreachable. No way to do that?

One option is to define a trait for integers:

trait Integer: FromBytes + AsBytes + Copy {
    const SIZE: IntSize;
}

enum IntSize {
    S8,
    S16,
    S32,
    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,
}

Using the above trait, you can match on the IntSize.

pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
match T::SIZE {
    IntSize::S8 => ...,
    IntSize::S16 => ...,
    IntSize::S32 => ...,
    IntSize::S64 => ...,
}

this leaves no catch-all case and calling it with non-integer types
will not compile.

Alice