Message-ID: <20250326171411.590681-10-remo@buenzli.dev>
Date: Wed, 26 Mar 2025 18:13:48 +0100
From: Remo Senekowitsch <remo@...nzli.dev>
To: 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>,
	Rob Herring <robh@...nel.org>
Cc: 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>,
	Remo Senekowitsch <remo@...nzli.dev>,
	linux-kernel@...r.kernel.org,
	linux-acpi@...r.kernel.org,
	devicetree@...r.kernel.org,
	rust-for-linux@...r.kernel.org
Subject: [PATCH 09/10] rust: property: Add PropertyGuard

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.

Signed-off-by: Remo Senekowitsch <remo@...nzli.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)
     }
 
@@ -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);
+            } 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

Powered by blists - more mailing lists