Re: [PATCH 09/10] rust: property: Add PropertyGuard

[Rob Herring <robh@kernel.org>]

On Wed, Mar 26, 2025 at 06:13:48PM +0100, Remo Senekowitsch wrote:
> Forcing users to specify whether a property is supposed to be required
> or not allows us to move error logging of missing required properties
> into core, preventing a lot of boilerplate in drivers.

We'll want to squash this one too.

> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/property.rs | 146 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 127 insertions(+), 19 deletions(-)
> 
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index f1d0a33ba..e4f0e5f97 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -42,7 +42,11 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
>      }
>  
>      /// 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>> {
> +    pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +        len: usize,
> +    ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
>          self.fwnode().property_read_array_vec(name, len)
>      }
>  
> @@ -52,12 +56,18 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
>      }
>  
>      /// Returns firmware property `name` integer scalar value
> -    pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
> +    pub fn property_read<'fwnode, 'name, T: Property>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, T> {
>          self.fwnode().property_read(name)
>      }
>  
>      /// Returns first matching named child node handle.
> -    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
> +    pub fn get_child_by_name<'fwnode, 'name>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, ARef<FwNode>> {
>          self.fwnode().get_child_by_name(name)
>      }

While child nodes can be required or optional, it's a bit less common to 
be optional. There error message on missing required child nodes is 
going to be a bit misleading saying 'property'. So maybe don't do the 
PropertyGuard on child node handling?

>  
> @@ -132,12 +142,16 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
>      }
>  
>      /// 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>> {
> +    pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +        len: usize,
> +    ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
>          let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
>  
>          // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
>          // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
> -        to_result(unsafe {
> +        let err = unsafe {
>              bindings::fwnode_property_read_int_array(
>                  self.as_raw(),
>                  name.as_char_ptr(),
> @@ -145,11 +159,19 @@ pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Re
>                  val.as_ptr() as *mut c_void,
>                  len,
>              )
> -        })?;
> -
> -        // SAFETY: fwnode_property_read_int_array() writes exactly `len` entries on success
> -        unsafe { val.set_len(len) }
> -        Ok(val)
> +        };
> +        let res = if err < 0 {
> +            Err(Error::from_errno(err))
> +        } else {
> +            // SAFETY: fwnode_property_read_int_array() writes exactly `len` entries on success
> +            unsafe { val.set_len(len) }
> +            Ok(val)
> +        };
> +        Ok(PropertyGuard {
> +            inner: res,
> +            fwnode: self,
> +            name,
> +        })
>      }
>  
>      /// Returns integer array length for firmware property `name`
> @@ -194,24 +216,42 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
>      /// # use crate::{device::Device, types::CString};
>      /// fn examples(dev: &Device) -> Result {
>      ///     let fwnode = dev.fwnode();
> -    ///     let b: bool = fwnode.property_read("some-bool")?;
> -    ///     let s: CString = fwnode.property_read("some-str")?;
> +    ///     let b: bool = fwnode.property_read("some-bool").required()?;
> +    ///     if let Some(s) = fwnode.property_read::<CString>("some-str").optional() {
> +    ///         // ...
> +    ///     }
>      /// }
>      /// ```
> -    pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
> -        T::read(&self, name)
> +    pub fn property_read<'fwnode, 'name, T: Property>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, T> {
> +        PropertyGuard {
> +            inner: T::read(&self, name),
> +            fwnode: self,
> +            name,
> +        }
>      }
>  
>      /// Returns first matching named child node handle.
> -    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<Self>> {
> +    pub fn get_child_by_name<'fwnode, 'name>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, ARef<Self>> {
>          // SAFETY: `self` and `name` are valid.
>          let child =
>              unsafe { bindings::fwnode_get_named_child_node(self.as_raw(), name.as_char_ptr()) };
> -        if child.is_null() {
> -            return None;
> +        let res = if child.is_null() {
> +            Err(ENOENT)
> +        } else {
> +            // SAFETY: `fwnode_get_named_child_node` returns a pointer with refcount incremented.
> +            Ok(unsafe { Self::from_raw(child) })
> +        };
> +        PropertyGuard {
> +            inner: res,
> +            fwnode: self,
> +            name,
>          }
> -        // SAFETY: `fwnode_get_named_child_node` returns a pointer with refcount incremented.
> -        Some(unsafe { Self::from_raw(child) })
>      }
>  
>      /// Returns an iterator over a node's children.
> @@ -365,3 +405,71 @@ pub enum NArgs<'a> {
>      /// The known number of arguments.
>      N(u32),
>  }
> +
> +/// A helper for reading device properties.
> +///
> +/// Use [Self::required] if a missing property is considered a bug and
> +/// [Self::optional] otherwise.
> +///
> +/// For convenience, [Self::or] and [Self::or_default] are provided.
> +pub struct PropertyGuard<'fwnode, 'name, T> {
> +    /// The result of reading the property.
> +    inner: Result<T>,
> +    /// The fwnode of the property, used for logging in the "required" case.
> +    fwnode: &'fwnode FwNode,
> +    /// The name of the property, used for logging in the "required" case.
> +    name: &'name CStr,
> +}
> +
> +impl<T> PropertyGuard<'_, '_, T> {
> +    /// Access the property, indicating it is required.
> +    ///
> +    /// If the property is not present, the error is automatically logged. If a
> +    /// missing property is not an error, use [Self::optional] instead.
> +    pub fn required(self) -> Result<T> {
> +        if self.inner.is_err() {
> +            // Get the device associated with the fwnode for device-associated
> +            // logging.
> +            // TODO: Are we allowed to do this? The field `fwnode_handle.dev`
> +            // has a somewhat vague comment, which could mean we're not
> +            // supposed to access it:
> +            // https://elixir.bootlin.com/linux/v6.13.6/source/include/linux/fwnode.h#L51
> +            // SAFETY: According to the invariant of FwNode, it is valid.
> +            let dev = unsafe { (*self.fwnode.as_raw()).dev };
> +
> +            if dev.is_null() {
> +                pr_err!("property '{}' is missing\n", self.name);

We should print the node path in this case (and perhaps both cases).

We have "%pfw" printf format specifier for C. We're going to need the 
same for rust.

> +            } else {
> +                // SAFETY: If dev is not null, it points to a valid device.
> +                let dev: &Device = unsafe { &*dev.cast() };
> +                dev_err!(dev, "property '{}' is missing\n", self.name);
> +            };
> +        }
> +        self.inner
> +    }
> +
> +    /// Access the property, indicating it is optional.
> +    ///
> +    /// In contrast to [Self::required], no error message is logged if the
> +    /// property is not present.
> +    pub fn optional(self) -> Option<T> {
> +        self.inner.ok()
> +    }
> +
> +    /// Access the property or the specified default value.
> +    ///
> +    /// Do not pass a sentinel value as default to detect a missing property.
> +    /// Use [Self::required] or [Self::optional] instead.
> +    pub fn or(self, default: T) -> T {
> +        self.inner.unwrap_or(default)
> +    }
> +}
> +
> +impl<T: Default> PropertyGuard<'_, '_, T> {
> +    /// Access the property or a default value.
> +    ///
> +    /// Use [Self::or] to specify a custom default value.
> +    pub fn or_default(self) -> T {
> +        self.inner.unwrap_or_default()
> +    }
> +}
> -- 
> 2.49.0
>