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

[Dirk Behme <dirk.behme@de.bosch.com>]

On 25.10.2024 23:05, Rob Herring (Arm) 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>


Building this on rust-next with CLIPPY=1 there are some improvement 
requests:

* Function documentation for property_read_array_vec
* SAFETY comments for all unsafe{} blocks
* Different ret usage for match size_of::<T>()
* Use self instead of &self

See [1] below (without the SAFETY comments).

Best regards

Dirk

> ---
> 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> {
> +        let ret;
> +
> +        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),
> +        }
> +        to_result(ret)?;
> +        Ok(ret.try_into().unwrap())
> +    }
>   }
>   
>   // SAFETY: Instances of `Device` are always reference-counted.


[1]

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index bb66a28df890..3c461b1602d5 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -218,7 +218,7 @@ pub fn property_match_string(&self, name: &CStr, 
match_str: &CStr) -> Result<usi
      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)?;
+        Self::_property_read_array(self, name, &mut val)?;
          Ok(val[0])
      }

@@ -271,12 +271,13 @@ fn _property_read_array<T>(&self, name: &CStr, 
val: &mut [T]) -> Result {
          Ok(())
      }

+    /// ToDo
      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())?;
+        Self::_property_read_array::<T>(self, name, val.as_mut_slice())?;
          Ok(val)
      }

@@ -284,11 +285,10 @@ pub fn property_read_array_vec<T>(&self, name: 
&CStr, len: usize) -> Result<KVec
      ///
      /// Valid types are i8, u8, i16, u16, i32, u32, i64, u64
      pub fn property_count_elem<T>(&self, name: &CStr) -> Result<usize> {
-        let ret;

-        match size_of::<T>() {
+        let ret = match size_of::<T>() {
              1 => {
-                ret = unsafe {
+                unsafe {
                      bindings::device_property_read_u8_array(
                          self.as_raw(),
                          name.as_ptr() as *const i8,
@@ -298,7 +298,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) -> 
Result<usize> {
                  }
              }
              2 => {
-                ret = unsafe {
+                unsafe {
                      bindings::device_property_read_u16_array(
                          self.as_raw(),
                          name.as_ptr() as *const i8,
@@ -308,7 +308,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) -> 
Result<usize> {
                  }
              }
              4 => {
-                ret = unsafe {
+                unsafe {
                      bindings::device_property_read_u32_array(
                          self.as_raw(),
                          name.as_ptr() as *const i8,
@@ -318,7 +318,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) -> 
Result<usize> {
                  }
              }
              8 => {
-                ret = unsafe {
+                unsafe {
                      bindings::device_property_read_u64_array(
                          self.as_raw(),
                          name.as_ptr() as *const i8,
@@ -328,7 +328,7 @@ pub fn property_count_elem<T>(&self, name: &CStr) -> 
Result<usize> {
                  }
              }
              _ => return Err(EINVAL),
-        }
+        };
          to_result(ret)?;
          Ok(ret.try_into().unwrap())
      }