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

[Rob Herring <robh@kernel.org>]

On Tue, Oct 29, 2024 at 9:16 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Fri, Oct 25, 2024 at 11:06 PM Rob Herring (Arm) <robh@kernel.org> wrote:
> >
> > The device property API is a firmware agnostic API for reading
> > properties from firmware (DT/ACPI) devices nodes and swnodes.
> >
> > While the C API takes a pointer to a caller allocated variable/buffer,
> > the rust API is designed to return a value and can be used in struct
> > initialization. Rust generics are also utilized to support different
> > sizes of properties (e.g. u8, u16, u32).
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > Not sure if we need the KVec variant, but I kept it as that was my first
> > pass attempt. Most callers are filling in some value in a driver data
> > struct. Sometimes the number of elements is not known, so the caller
> > calls to get the array size, allocs the correct size buffer, and then
> > reads the property again to fill in the buffer.
> >
> > I have not implemented a wrapper for device_property_read_string(_array)
> > because that API is problematic for dynamic DT nodes. The API just
> > returns pointer(s) into the raw DT data. We probably need to return a
> > copy of the string(s) instead for rust.
> >
> > After property accessors, next up is child node accessors/iterators.
> > ---
> >  rust/bindings/bindings_helper.h |   1 +
> >  rust/kernel/device.rs           | 145 +++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 145 insertions(+), 1 deletion(-)
> >
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 217c776615b9..65717cc20a23 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -19,6 +19,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/phy.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/property.h>
> >  #include <linux/refcount.h>
> >  #include <linux/sched.h>
> >  #include <linux/slab.h>
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 0c28b1e6b004..bb66a28df890 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -5,10 +5,14 @@
> >  //! C header: [`include/linux/device.h`](srctree/include/linux/device.h)
> >
> >  use crate::{
> > +    alloc::KVec,
> >      bindings,
> > +    error::{to_result, Result},
> > +    prelude::*,
> > +    str::CStr,
> >      types::{ARef, Opaque},
> >  };
> > -use core::{fmt, ptr};
> > +use core::{fmt, mem::size_of, ptr};
> >
> >  #[cfg(CONFIG_PRINTK)]
> >  use crate::c_str;
> > @@ -189,6 +193,145 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> >              )
> >          };
> >      }
> > +
> > +    /// Returns if a firmware property `name` is true or false
> > +    pub fn property_read_bool(&self, name: &CStr) -> bool {
> > +        unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const i8) }
> > +    }
> > +
> > +    /// Returns if a firmware string property `name` has match for `match_str`
> > +    pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> > +        let ret = unsafe {
> > +            bindings::device_property_match_string(
> > +                self.as_raw(),
> > +                name.as_ptr() as *const i8,
> > +                match_str.as_ptr() as *const i8,
> > +            )
> > +        };
> > +        to_result(ret)?;
> > +        Ok(ret as usize)
> > +    }
> > +
> > +    /// Returns firmware property `name` scalar value
> > +    ///
> > +    /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > +    pub fn property_read<T: Copy>(&self, name: &CStr) -> Result<T> {
> > +        let mut val: [T; 1] = unsafe { core::mem::zeroed() };
> > +
> > +        Self::_property_read_array(&self, name, &mut val)?;
> > +        Ok(val[0])
> > +    }
> > +
> > +    /// Returns firmware property `name` array values
> > +    ///
> > +    /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
> > +    pub fn property_read_array<T, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
> > +        let mut val: [T; N] = unsafe { core::mem::zeroed() };
> > +
> > +        Self::_property_read_array(self, name, &mut val)?;
> > +        Ok(val)
> > +    }
> > +
> > +    fn _property_read_array<T>(&self, name: &CStr, val: &mut [T]) -> Result {
> > +        match size_of::<T>() {
> > +            1 => to_result(unsafe {
> > +                bindings::device_property_read_u8_array(
> > +                    self.as_raw(),
> > +                    name.as_ptr() as *const i8,
> > +                    val.as_ptr() as *mut u8,
> > +                    val.len(),
> > +                )
> > +            })?,
> > +            2 => to_result(unsafe {
> > +                bindings::device_property_read_u16_array(
> > +                    self.as_raw(),
> > +                    name.as_ptr() as *const i8,
> > +                    val.as_ptr() as *mut u16,
> > +                    val.len(),
> > +                )
> > +            })?,
> > +            4 => to_result(unsafe {
> > +                bindings::device_property_read_u32_array(
> > +                    self.as_raw(),
> > +                    name.as_ptr() as *const i8,
> > +                    val.as_ptr() as *mut u32,
> > +                    val.len(),
> > +                )
> > +            })?,
> > +            8 => to_result(unsafe {
> > +                bindings::device_property_read_u64_array(
> > +                    self.as_raw(),
> > +                    name.as_ptr() as *const i8,
> > +                    val.as_ptr() as *mut u64,
> > +                    val.len(),
> > +                )
> > +            })?,
> > +            _ => return Err(EINVAL),
> > +        }
> > +        Ok(())
> > +    }
> > +
> > +    pub fn property_read_array_vec<T>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> > +        let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
> > +
> > +        // SAFETY: len always matches capacity
> > +        unsafe { val.set_len(len) }
> > +        Self::_property_read_array::<T>(&self, name, val.as_mut_slice())?;
> > +        Ok(val)
> > +    }
> > +
> > +    /// 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?


> > +        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?

Rob