Message-ID: <20250326212753.GA2844851-robh@kernel.org>
Date: Wed, 26 Mar 2025 16:27:53 -0500
From: Rob Herring <robh@...nel.org>
To: Remo Senekowitsch <remo@...nzli.dev>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Daniel Scally <djrscally@...il.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Dirk Behme <dirk.behme@...bosch.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...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>,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
	devicetree@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 04/10] rust: Add bindings for reading device properties

On Wed, Mar 26, 2025 at 06:13:43PM +0100, Remo Senekowitsch 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).
> 
> Co-developed-by: Rob Herring (Arm) <robh@...nel.org>
> Signed-off-by: Rob Herring (Arm) <robh@...nel.org>
> Signed-off-by: Remo Senekowitsch <remo@...nzli.dev>
> ---
>  rust/kernel/property.rs | 153 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 151 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index b0a4bb63a..4756ea766 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -4,9 +4,17 @@
>  //!
>  //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
>  
> -use core::ptr;
> +use core::{ffi::c_void, mem::MaybeUninit, ptr};
>  
> -use crate::{bindings, device::Device, str::CStr, types::Opaque};
> +use crate::{
> +    alloc::KVec,
> +    bindings,
> +    device::Device,
> +    error::{to_result, Result},
> +    prelude::*,
> +    str::{CStr, CString},
> +    types::{Integer, Opaque},
> +};
>  
>  impl Device {
>      /// Obtain the fwnode corresponding to the device.
> @@ -26,6 +34,41 @@ fn fwnode(&self) -> &FwNode {
>      pub fn property_present(&self, name: &CStr) -> bool {
>          self.fwnode().property_present(name)
>      }
> +
> +    /// Returns if a firmware property `name` is true or false
> +    pub fn property_read_bool(&self, name: &CStr) -> bool {
> +        self.fwnode().property_read_bool(name)
> +    }
> +
> +    /// Returns the index of matching string `match_str` for firmware string property `name`

Comment doesn't match the function.

> +    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
> +        self.fwnode().property_read_string(name)
> +    }
> +
> +    /// Returns the index of matching string `match_str` for firmware string property `name`
> +    pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> +        self.fwnode().property_match_string(name, match_str)
> +    }
> +
> +    /// Returns firmware property `name` integer scalar value
> +    pub fn property_read<T: Integer>(&self, name: &CStr) -> Result<T> {
> +        self.fwnode().property_read(name)
> +    }
> +
> +    /// Returns firmware property `name` integer array values
> +    pub fn property_read_array<T: Integer, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
> +        self.fwnode().property_read_array(name)
> +    }
> +
> +    /// Returns firmware property `name` integer array values in a KVec
> +    pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
> +        self.fwnode().property_read_array_vec(name, len)
> +    }
> +
> +    /// Returns integer array length for firmware property `name`
> +    pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
> +        self.fwnode().property_count_elem::<T>(name)
> +    }
>  }
>  
>  /// A reference-counted fwnode_handle.
> @@ -57,6 +100,112 @@ pub fn property_present(&self, name: &CStr) -> bool {
>          // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
>          unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
>      }
> +
> +    /// Returns if a firmware property `name` is true or false
> +    pub fn property_read_bool(&self, name: &CStr) -> bool {
> +        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
> +        // because `self` is valid.
> +        unsafe { bindings::fwnode_property_read_bool(self.as_raw(), name.as_char_ptr()) }
> +    }
> +
> +    /// Returns the index of matching string `match_str` for firmware string property `name`

Same comment copy-n-paste mismatch.

> +    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
> +        let mut str = core::ptr::null_mut();
> +        let pstr: *mut _ = &mut str;
> +
> +        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is
> +        // valid because `self` is valid.
> +        let ret = unsafe {
> +            bindings::fwnode_property_read_string(self.as_raw(), name.as_char_ptr(), pstr as _)
> +        };
> +        to_result(ret)?;
> +
> +        // SAFETY: `pstr` contains a non-null ptr on success
> +        let str = unsafe { CStr::from_char_ptr(*pstr) };
> +        Ok(str.try_into()?)
> +    }

There's a problem with the C version of this function that I'd like to 
not repeat in Rust especially since ownership is clear. 

The issue is that we never know when the returned string is no longer 
needed. For DT overlays, we need to be able free the string when/if an 
overlay is removed. Though overlays are somewhat orthogonal to this. 
It's really just when the property's node refcount goes to 0 that the 
property value could be freed.

So this function should probably return a copy of the string that the 
caller owns.

Rob

Powered by blists - more mailing lists