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

From: Rob Herring
Date: Wed Mar 26 2025 - 18:25:40 EST

Next message: Mike Christie: "Re: [PATCH v2 02/10] vhost-scsi: Fix vhost_scsi_send_bad_target()"
Previous message: Remo Senekowitsch: "Re: [PATCH 10/10] samples: rust: platform: Add property read examples"
In reply to: Andrew Ballance: "Re: [PATCH 09/10] rust: property: Add PropertyGuard"
Next in thread: Remo Senekowitsch: "[PATCH 02/10] rust: Add an Integer trait"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
> ---
> 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
>

Next message: Mike Christie: "Re: [PATCH v2 02/10] vhost-scsi: Fix vhost_scsi_send_bad_target()"
Previous message: Remo Senekowitsch: "Re: [PATCH 10/10] samples: rust: platform: Add property read examples"
In reply to: Andrew Ballance: "Re: [PATCH 09/10] rust: property: Add PropertyGuard"
Next in thread: Remo Senekowitsch: "[PATCH 02/10] rust: Add an Integer trait"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]