[PATCH v3 0/7] More Rust bindings for device property reads

[PATCH v3 0/7] More Rust bindings for device property reads

[Remo Senekowitsch]

This adds more Rust bindings for reading device properties, based on
Rob Herring's work. I'm working on a driver[1] that uses these, but the
driver has more dependencies than this.

Best regards,
Remo

changes in v3:
* (started testing myself with Rust 1.78 and doctests enabled)
* Fix doctest and platform driver sample.
* Move property.rs to device/property.rs (a submodule of device).
* Make `Device::fwnode` fallible, avoiding a panic.
* Remove the duplicated property reading methods on Device. Now that
  `Device::fwnode` is fallible, these methods would have confusingly
  different signatures than the ones on `FwNode`. It will be clearer for
  users to explicitly convert from `&Device` to `&FwNode` first,
  handling that error case separately, and then reading properties on
  `FwNode`.
* Split off separate commits for:
  - printing fwnode name and path
  - adding PropertyGuard
* Do not access `fwnode_handle.dev` in PropertyGuard for
  device-associated logging, fwnode_handle doesn't own a reference to
  the device.
* Rename some extension trait methods to be more descriptive:
  - Property::read => read_from_fwnode_property
  - PropertyInt::read_array => read_array_from_fwnode_property
  These methods are not meant to be used directly and won't be
  accessible unless their traits are in scope. (And there is no reason
  for API users to pull them into scope.) Nevertheless, this reduces the
  risk of confusion caused by non-descriptive methods like "read" being
  attached to primitive types.
* Implement fwnode printing logic in Rust directly instead of calling
  scnprintf.
* Improve some safety comments.

Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/with/507874342 [1]

Remo Senekowitsch (7):
  rust: property: Move property_present to separate file
  rust: property: Enable printing fwnode name and path
  rust: property: Introduce PropertyGuard
  rust: property: Add bindings for reading device properties
  rust: property: Add child accessor and iterator
  rust: property: Add property_get_reference_args
  samples: rust: platform: Add property read examples

 MAINTAINERS                                  |   3 +-
 drivers/of/unittest-data/tests-platform.dtsi |   3 +
 rust/helpers/helpers.c                       |   1 +
 rust/helpers/property.c                      |   8 +
 rust/kernel/{device.rs => device/mod.rs}     |   9 +-
 rust/kernel/device/property.rs               | 578 +++++++++++++++++++
 samples/rust/rust_driver_platform.rs         |  72 ++-
 7 files changed, 663 insertions(+), 11 deletions(-)
 create mode 100644 rust/helpers/property.c
 rename rust/kernel/{device.rs => device/mod.rs} (97%)
 create mode 100644 rust/kernel/device/property.rs

-- 
2.49.0

[PATCH v3 1/7] rust: property: Move property_present to separate file

[Remo Senekowitsch]

Not all property-related APIs can be exposed directly on a device.
For example, iterating over child nodes of a device will yield
fwnode_handle. Thus, in order to access properties on these child nodes,
the property access methods must be implemented on the abstraction over
fwnode_handle.

While it's possible to expose similar methods on `Device` directly for
convenience, those methods would have to get the `FwNode` first, which
is a fallible operation, making the API inconsistent. For this reason,
such duplicated methods are omitted. Users who need to read properties
of a device will have to explictily get the `FwNode` first (handle the
`None` case) and then read properties on that.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 MAINTAINERS                              |  3 +-
 rust/helpers/helpers.c                   |  1 +
 rust/helpers/property.c                  |  8 +++
 rust/kernel/{device.rs => device/mod.rs} |  9 +--
 rust/kernel/device/property.rs           | 70 ++++++++++++++++++++++++
 5 files changed, 83 insertions(+), 8 deletions(-)
 create mode 100644 rust/helpers/property.c
 rename rust/kernel/{device.rs => device/mod.rs} (97%)
 create mode 100644 rust/kernel/device/property.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index c8d9e8187..4585f9e7f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7112,7 +7112,8 @@ F:	include/linux/kobj*
 F:	include/linux/property.h
 F:	include/linux/sysfs.h
 F:	lib/kobj*
-F:	rust/kernel/device.rs
+F:	rust/kernel/device/mod.rs
+F:	rust/kernel/device/property.rs
 F:	rust/kernel/device_id.rs
 F:	rust/kernel/devres.rs
 F:	rust/kernel/driver.rs
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0640b7e11..b4eec5bf2 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -23,6 +23,7 @@
 #include "platform.c"
 #include "pci.c"
 #include "pid_namespace.c"
+#include "property.c"
 #include "rbtree.c"
 #include "rcu.c"
 #include "refcount.c"
diff --git a/rust/helpers/property.c b/rust/helpers/property.c
new file mode 100644
index 000000000..08f68e2da
--- /dev/null
+++ b/rust/helpers/property.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/property.h>
+
+void rust_helper_fwnode_handle_put(struct fwnode_handle *fwnode)
+{
+	fwnode_handle_put(fwnode);
+}
diff --git a/rust/kernel/device.rs b/rust/kernel/device/mod.rs
similarity index 97%
rename from rust/kernel/device.rs
rename to rust/kernel/device/mod.rs
index db2d9658b..e49107452 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device/mod.rs
@@ -6,7 +6,6 @@
 
 use crate::{
     bindings,
-    str::CStr,
     types::{ARef, Opaque},
 };
 use core::{fmt, ptr};
@@ -14,6 +13,8 @@
 #[cfg(CONFIG_PRINTK)]
 use crate::c_str;
 
+pub mod property;
+
 /// A reference-counted device.
 ///
 /// This structure represents the Rust abstraction for a C `struct device`. This implementation
@@ -181,12 +182,6 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
             )
         };
     }
-
-    /// Checks if property is present or not.
-    pub fn property_present(&self, name: &CStr) -> bool {
-        // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
-        unsafe { bindings::device_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
-    }
 }
 
 // SAFETY: Instances of `Device` are always reference-counted.
diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
new file mode 100644
index 000000000..d89715f7d
--- /dev/null
+++ b/rust/kernel/device/property.rs
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Unified device property interface.
+//!
+//! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
+
+use core::ptr;
+
+use crate::{bindings, device::Device, str::CStr, types::Opaque};
+
+impl Device {
+    /// Obtain the fwnode corresponding to the device.
+    pub fn fwnode(&self) -> Option<&FwNode> {
+        // SAFETY: `self` is valid.
+        let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
+        if fwnode_handle.is_null() {
+            return None;
+        }
+        // SAFETY: `fwnode_handle` is valid. Its lifetime is tied to `&self`. We
+        // return a reference instead of an `ARef<FwNode>` because `dev_fwnode()`
+        // doesn't increment the refcount. It is safe to cast from a
+        // `struct fwnode_handle*` to a `*const FwNode` because `FwNode` is
+        // defined as a `#[repr(transparent)]` wrapper around `fwnode_handle`.
+        Some(unsafe { &*fwnode_handle.cast() })
+    }
+}
+
+/// A reference-counted fwnode_handle.
+///
+/// This structure represents the Rust abstraction for a
+/// C `struct fwnode_handle`. This implementation abstracts the usage of an
+/// already existing C `struct fwnode_handle` within Rust code that we get
+/// passed from the C side.
+///
+/// # Invariants
+///
+/// A `FwNode` instance represents a valid `struct fwnode_handle` created by the
+/// C portion of the kernel.
+///
+/// Instances of this type are always reference-counted, that is, a call to
+/// `fwnode_handle_get` ensures that the allocation remains valid at least until
+/// the matching call to `fwnode_handle_put`.
+#[repr(transparent)]
+pub struct FwNode(Opaque<bindings::fwnode_handle>);
+
+impl FwNode {
+    /// Obtain the raw `struct fwnode_handle *`.
+    pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
+        self.0.get()
+    }
+
+    /// Checks if property is present or not.
+    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()) }
+    }
+}
+
+// SAFETY: Instances of `FwNode` are always reference-counted.
+unsafe impl crate::types::AlwaysRefCounted for FwNode {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
+        unsafe { bindings::fwnode_handle_get(self.as_raw()) };
+    }
+
+    unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
+    }
+}
-- 
2.49.0

[PATCH v3 2/7] rust: property: Enable printing fwnode name and path

[Remo Senekowitsch]

Add two new public methods `display_name` and `display_path` to
`FwNode`. They can be used by driver authors for logging purposes. In
addition, they will be used by core property abstractions for automatic
logging, for example when a driver attempts to read a required but
missing property.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/property.rs | 78 ++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index d89715f7d..28850aa3b 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -49,6 +49,84 @@ pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
         self.0.get()
     }
 
+    /// Returns an object that implements [`Display`](core::fmt::Display) for
+    /// printing the name of a node.
+    pub fn display_name(&self) -> impl core::fmt::Display + '_ {
+        struct FwNodeDisplayName<'a>(&'a FwNode);
+
+        impl core::fmt::Display for FwNodeDisplayName<'_> {
+            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+                // SAFETY: self is valid by its type invariant
+                let name = unsafe { bindings::fwnode_get_name(self.0.as_raw()) };
+                if name.is_null() {
+                    return Ok(());
+                }
+                // SAFETY: fwnode_get_name returns null or a valid C string and
+                // name is not null
+                let name = unsafe { CStr::from_char_ptr(name) };
+                write!(f, "{name}")
+            }
+        }
+
+        FwNodeDisplayName(self)
+    }
+
+    /// Returns an object that implements [`Display`](core::fmt::Display) for
+    /// printing the full path of a node.
+    pub fn display_path(&self) -> impl core::fmt::Display + '_ {
+        struct FwNodeDisplayPath<'a>(&'a FwNode);
+
+        impl core::fmt::Display for FwNodeDisplayPath<'_> {
+            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+                // The logic here is the same as the one in lib/vsprintf.c
+                // (fwnode_full_name_string).
+
+                let num_parents = unsafe { bindings::fwnode_count_parents(self.0.as_raw()) };
+
+                for depth in (0..=num_parents).rev() {
+                    let fwnode = if depth == 0 {
+                        self.0.as_raw()
+                    } else {
+                        // SAFETY: `self.0.as_raw()` is valid
+                        unsafe { bindings::fwnode_get_nth_parent(self.0.as_raw(), depth) }
+                    };
+
+                    // SAFETY: fwnode is valid, it is either `self.0.as_raw()` or
+                    // the return value of `bindings::fwnode_get_nth_parent` which
+                    // returns a valid pointer to a fwnode_handle if the provided
+                    // depth is within the valid range, which we know to be true.
+                    let prefix = unsafe { bindings::fwnode_get_name_prefix(fwnode) };
+                    if !prefix.is_null() {
+                        // SAFETY: fwnode_get_name_prefix returns null or a
+                        // valid C string
+                        let prefix = unsafe { CStr::from_char_ptr(prefix) };
+                        write!(f, "{prefix}")?;
+                    }
+                    // SAFETY: fwnode is valid for the reasons stated above
+                    let name = unsafe { bindings::fwnode_get_name(fwnode) };
+                    if !name.is_null() {
+                        // SAFETY: fwnode_get_name returns null or a valid
+                        // C string
+                        let name = unsafe { CStr::from_char_ptr(name) };
+                        write!(f, "{name}")?;
+                    }
+
+                    if depth != 0 {
+                        // SAFETY: `fwnode` is valid, because `depth` is
+                        // a valid depth of a parent of `self.0.as_raw()`.
+                        // `fwnode_get_nth_parent` increments the refcount and
+                        // we are responsible to decrement it.
+                        unsafe { bindings::fwnode_handle_put(fwnode) }
+                    }
+                }
+
+                Ok(())
+            }
+        }
+
+        FwNodeDisplayPath(self)
+    }
+
     /// Checks if property is present or not.
     pub fn property_present(&self, name: &CStr) -> bool {
         // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
-- 
2.49.0

[PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Remo Senekowitsch]

This abstraction is a way to force users to specify whether a property
is supposed to be required or not. This allows us to move error
logging of missing required properties into core, preventing a lot of
boilerplate in drivers.

It will be used by upcoming methods for reading device properties.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/property.rs | 57 ++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index 28850aa3b..de31a1f56 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -146,3 +146,60 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
         unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
     }
 }
+
+/// 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() {
+            pr_err!(
+                "{}: property '{}' is missing\n",
+                self.fwnode.display_path(),
+                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

[PATCH v3 4/7] rust: property: Add bindings for reading device properties

[Remo Senekowitsch]

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
types of properties where appropriate.

Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/property.rs | 232 ++++++++++++++++++++++++++++++++-
 1 file changed, 230 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index de31a1f56..9505cc35d 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -4,9 +4,17 @@
 //!
 //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
 
-use core::ptr;
+use core::{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::Opaque,
+};
 
 impl Device {
     /// Obtain the fwnode corresponding to the device.
@@ -132,6 +140,104 @@ 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 firmware property `name` boolean value
+    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`
+    pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
+        // SAFETY: `name` and `match_str` are non-null and null-terminated. `self.as_raw` is
+        // valid because `self` is valid.
+        let ret = unsafe {
+            bindings::fwnode_property_match_string(
+                self.as_raw(),
+                name.as_char_ptr(),
+                match_str.as_char_ptr(),
+            )
+        };
+        to_result(ret)?;
+        Ok(ret as usize)
+    }
+
+    /// Returns firmware property `name` integer array values in a KVec
+    pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
+        &'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: `val.as_mut_ptr()` is valid because `KVec::with_capacity`
+        // didn't return an error and it has at least space for `len` number
+        // of elements.
+        let err = unsafe { read_array_out_param::<T>(self, name, val.as_mut_ptr(), len) };
+        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`
+    pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
+        // SAFETY: `out_param` is allowed to be null because `len` is zero.
+        let ret = unsafe { read_array_out_param::<T>(self, name, ptr::null_mut(), 0) };
+        to_result(ret)?;
+        Ok(ret as usize)
+    }
+
+    /// Returns the value of firmware property `name`.
+    ///
+    /// This method is generic over the type of value to read. Informally,
+    /// the types that can be read are booleans, strings, unsigned integers and
+    /// arrays of unsigned integers.
+    ///
+    /// Reading a `KVec` of integers is done with the separate
+    /// method [`Self::property_read_array_vec`], because it takes an
+    /// additional `len` argument.
+    ///
+    /// When reading a boolean, this method never fails. A missing property
+    /// is interpreted as `false`, whereas a present property is interpreted
+    /// as `true`.
+    ///
+    /// For more precise documentation about what types can be read, see
+    /// the [implementors of Property][Property#implementors] and [its
+    /// implementations on foreign types][Property#foreign-impls].
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::{c_str, device::property::FwNode, str::CString};
+    /// fn examples(fwnode: &FwNode) -> Result {
+    ///     let b: u32 = fwnode.property_read(c_str!("some-number")).required()?;
+    ///     if let Some(s) = fwnode.property_read::<CString>(c_str!("some-str")).optional() {
+    ///         // ...
+    ///     }
+    ///     Ok(())
+    /// }
+    /// ```
+    pub fn property_read<'fwnode, 'name, T: Property>(
+        &'fwnode self,
+        name: &'name CStr,
+    ) -> PropertyGuard<'fwnode, 'name, T> {
+        PropertyGuard {
+            inner: T::read_from_fwnode_property(self, name),
+            fwnode: self,
+            name,
+        }
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
@@ -147,6 +253,128 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
     }
 }
 
+/// Implemented for several types that can be read as properties.
+///
+/// Informally, this is implemented for strings, integers and arrays of
+/// integers. It's used to make [`FwNode::property_read`] generic over the
+/// type of property being read. There are also two dedicated methods to read
+/// other types, because they require more specialized function signatures:
+/// - [`property_read_bool`](Device::property_read_bool)
+/// - [`property_read_array_vec`](Device::property_read_array_vec)
+pub trait Property: Sized {
+    /// Used to make [`FwNode::property_read`] generic.
+    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self>;
+}
+
+impl Property for CString {
+    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        let mut str: *mut u8 = ptr::null_mut();
+        let pstr: *mut _ = &mut str;
+
+        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
+        // valid because `fwnode` is valid.
+        let ret = unsafe {
+            bindings::fwnode_property_read_string(fwnode.as_raw(), name.as_char_ptr(), pstr.cast())
+        };
+        to_result(ret)?;
+
+        // SAFETY: `pstr` contains a non-null ptr on success
+        let str = unsafe { CStr::from_char_ptr(*pstr) };
+        Ok(str.try_into()?)
+    }
+}
+/// Implemented for all integers that can be read as properties.
+///
+/// This helper trait is needed on top of the existing [`Property`]
+/// trait to associate the integer types of various sizes with their
+/// corresponding `fwnode_property_read_*_array` functions.
+pub trait PropertyInt: Copy {
+    /// # Safety
+    ///
+    /// Callers must uphold the same safety invariants as for the various
+    /// `fwnode_property_read_*_array` functions.
+    unsafe fn read_array_from_fwnode_property(
+        fwnode: *const bindings::fwnode_handle,
+        propname: *const ffi::c_char,
+        val: *mut Self,
+        nval: usize,
+    ) -> ffi::c_int;
+}
+// This macro generates implementations of the traits `Property` and
+// `PropertyInt` for integers of various sizes. Its input is a list
+// of pairs separated by commas. The first element of the pair is the
+// type of the integer, the second one is the name of its corresponding
+// `fwnode_property_read_*_array` function.
+macro_rules! impl_property_for_int {
+    ($($int:ty: $f:ident),* $(,)?) => { $(
+        impl PropertyInt for $int {
+            unsafe fn read_array_from_fwnode_property(
+                fwnode: *const bindings::fwnode_handle,
+                propname: *const ffi::c_char,
+                val: *mut Self,
+                nval: usize,
+            ) -> ffi::c_int {
+                // SAFETY: The safety invariants on the trait require
+                // callers to uphold the invariants of the functions
+                // this macro is called with.
+                unsafe {
+                    bindings::$f(fwnode, propname, val.cast(), nval)
+                }
+            }
+        }
+    )* };
+}
+impl_property_for_int! {
+    u8: fwnode_property_read_u8_array,
+    u16: fwnode_property_read_u16_array,
+    u32: fwnode_property_read_u32_array,
+    u64: fwnode_property_read_u64_array,
+    i8: fwnode_property_read_u8_array,
+    i16: fwnode_property_read_u16_array,
+    i32: fwnode_property_read_u32_array,
+    i64: fwnode_property_read_u64_array,
+}
+/// # Safety
+///
+/// Callers must ensure that if `len` is non-zero, `out_param` must be
+/// valid and point to memory that has enough space to hold at least
+/// `len` number of elements.
+unsafe fn read_array_out_param<T: PropertyInt>(
+    fwnode: &FwNode,
+    name: &CStr,
+    out_param: *mut T,
+    len: usize,
+) -> ffi::c_int {
+    // SAFETY: `name` is non-null and null-terminated.
+    // `fwnode.as_raw` is valid because `fwnode` is valid.
+    // `out_param` is valid and has enough space for at least
+    // `len` number of elements as per the safety requirement.
+    unsafe {
+        T::read_array_from_fwnode_property(fwnode.as_raw(), name.as_char_ptr(), out_param, len)
+    }
+}
+impl<T: PropertyInt, const N: usize> Property for [T; N] {
+    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
+
+        // SAFETY: `val.as_mut_ptr()` is valid and points to enough space for
+        // `N` elements. Casting from `*mut MaybeUninit<T>` to `*mut T` is safe
+        // because `MaybeUninit<T>` has the same memory layout as `T`.
+        let ret = unsafe { read_array_out_param::<T>(fwnode, name, val.as_mut_ptr().cast(), N) };
+        to_result(ret)?;
+
+        // SAFETY: `val` is always initialized when
+        // fwnode_property_read_<T>_array is successful.
+        Ok(val.map(|v| unsafe { v.assume_init() }))
+    }
+}
+impl<T: PropertyInt> Property for T {
+    fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        let val: [_; 1] = <[T; 1] as Property>::read_from_fwnode_property(fwnode, name)?;
+        Ok(val[0])
+    }
+}
+
 /// A helper for reading device properties.
 ///
 /// Use [`Self::required`] if a missing property is considered a bug and
-- 
2.49.0

[PATCH v3 5/7] rust: property: Add child accessor and iterator

[Remo Senekowitsch]

Allow Rust drivers to access children of a fwnode either by name or by
iterating over all of them.

In C, there is the function `fwnode_get_next_child_node` for iteration
and the macro `fwnode_for_each_child_node` that helps with handling the
pointers. Instead of a macro, a native iterator is used in Rust such
that regular for-loops can be used.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/property.rs | 79 +++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index 9505cc35d..0a0cb0c02 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -13,7 +13,7 @@
     error::{to_result, Result},
     prelude::*,
     str::{CStr, CString},
-    types::Opaque,
+    types::{ARef, Opaque},
 };
 
 impl Device {
@@ -52,6 +52,27 @@ pub fn fwnode(&self) -> Option<&FwNode> {
 pub struct FwNode(Opaque<bindings::fwnode_handle>);
 
 impl FwNode {
+    /// # Safety
+    ///
+    /// Callers must ensure that:
+    /// - The reference count was incremented at least once.
+    /// - They relinquish that increment. That is, if there is only one
+    ///   increment, callers must not use the underlying object anymore -- it is
+    ///   only safe to do so via the newly created `ARef<FwNode>`.
+    unsafe fn from_raw(raw: *mut bindings::fwnode_handle) -> ARef<Self> {
+        // SAFETY: As per the safety requirements of this function:
+        // - `NonNull::new_unchecked`:
+        //   - `raw` is not null
+        // - `ARef::from_raw`:
+        //   - `raw` has an incremented refcount
+        //   - that increment is relinquished, i.e. it won't be decremented
+        //     elsewhere.
+        // CAST: It is safe to cast from a `*mut fwnode_handle` to
+        // `*mut FwNode`, because `FwNode` is  defined as a
+        // `#[repr(transparent)]` wrapper around `fwnode_handle`.
+        unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(raw.cast())) }
+    }
+
     /// Obtain the raw `struct fwnode_handle *`.
     pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
         self.0.get()
@@ -238,6 +259,62 @@ pub fn property_read<'fwnode, 'name, T: Property>(
             name,
         }
     }
+
+    /// Returns first matching named child node handle.
+    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<Self>> {
+        // SAFETY: `self` and `name` are valid by their type invariants.
+        let child =
+            unsafe { bindings::fwnode_get_named_child_node(self.as_raw(), name.as_char_ptr()) };
+        if child.is_null() {
+            return None;
+        }
+        // SAFETY:
+        // - `fwnode_get_named_child_node` returns a pointer with its refcount
+        //   incremented.
+        // - That increment is relinquished, i.e. the underlying object is not
+        //   used anymore except via the newly created `ARef`.
+        Some(unsafe { Self::from_raw(child) })
+    }
+
+    /// Returns an iterator over a node's children.
+    pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
+        let mut prev: Option<ARef<FwNode>> = None;
+
+        core::iter::from_fn(move || {
+            let prev_ptr = match prev.take() {
+                None => ptr::null_mut(),
+                Some(prev) => {
+                    // We will pass `prev` to `fwnode_get_next_child_node`,
+                    // which decrements its refcount, so we use
+                    // `ARef::into_raw` to avoid decrementing the refcount
+                    // twice.
+                    let prev = ARef::into_raw(prev);
+                    prev.as_ptr().cast()
+                }
+            };
+            // SAFETY:
+            // - `self.as_raw()` is valid by its type invariant.
+            // - `prev_ptr` may be null, which is allowed and corresponds to
+            //   getting the first child. Otherwise, `prev_ptr` is valid, as it
+            //   is the stored return value from the previous invocation.
+            // - `prev_ptr` has its refount incremented.
+            // - The increment of `prev_ptr` is relinquished, i.e. the
+            //   underlying object won't be unsed anymore.
+            let next = unsafe { bindings::fwnode_get_next_child_node(self.as_raw(), prev_ptr) };
+            if next.is_null() {
+                return None;
+            }
+            // SAFETY:
+            // - `next` is valid because `fwnode_get_next_child_node` returns a
+            //   pointer with its refcount incremented.
+            // - That increment is relinquished, i.e. the underlying object
+            //   won't be used anymore, except via the newly created
+            //   `ARef<Self>`.
+            let next = unsafe { FwNode::from_raw(next) };
+            prev = Some(next.clone());
+            Some(next)
+        })
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
-- 
2.49.0

[PATCH v3 6/7] rust: property: Add property_get_reference_args

[Remo Senekowitsch]

Allow Rust code to read reference args from device properties. The
wrapper type `FwNodeReferenceArgs` allows callers to access the buffer
of read args safely.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/device/property.rs | 68 ++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
index 0a0cb0c02..475b916d0 100644
--- a/rust/kernel/device/property.rs
+++ b/rust/kernel/device/property.rs
@@ -315,6 +315,65 @@ pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
             Some(next)
         })
     }
+
+    /// Finds a reference with arguments.
+    pub fn property_get_reference_args(
+        &self,
+        prop: &CStr,
+        nargs: NArgs<'_>,
+        index: u32,
+    ) -> Result<FwNodeReferenceArgs> {
+        let mut out_args = FwNodeReferenceArgs::default();
+
+        let (nargs_prop, nargs) = match nargs {
+            NArgs::Prop(nargs_prop) => (nargs_prop.as_char_ptr(), 0),
+            NArgs::N(nargs) => (ptr::null(), nargs),
+        };
+
+        // SAFETY: `self.0.get()` is valid. `prop.as_char_ptr()` is valid and
+        // zero-terminated. `nargs_prop` is valid and zero-terminated if `nargs`
+        // is zero, otherwise it is allowed to be a null-pointer.
+        let ret = unsafe {
+            bindings::fwnode_property_get_reference_args(
+                self.0.get(),
+                prop.as_char_ptr(),
+                nargs_prop,
+                nargs,
+                index,
+                &mut out_args.0,
+            )
+        };
+        to_result(ret)?;
+
+        Ok(out_args)
+    }
+}
+
+/// The return value of `property_get_reference_args`.
+///
+/// - [`Device::property_get_reference_args`]
+/// - [`FwNode::property_get_reference_args`]
+#[repr(transparent)]
+#[derive(Copy, Clone, Default)]
+pub struct FwNodeReferenceArgs(bindings::fwnode_reference_args);
+
+impl FwNodeReferenceArgs {
+    /// Returns the slice of reference arguments.
+    pub fn as_slice(&self) -> &[u64] {
+        // SAFETY: As per the safety invariant of FwNodeReferenceArgs, `nargs`
+        // is the number of elements in `args` that is valid.
+        unsafe { core::slice::from_raw_parts(self.0.args.as_ptr(), self.0.nargs as usize) }
+    }
+
+    /// Returns the number of reference arguments.
+    pub fn len(&self) -> usize {
+        self.0.nargs as usize
+    }
+
+    /// Returns `true` if there are no reference arguments.
+    pub fn is_empty(&self) -> bool {
+        self.0.nargs == 0
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
@@ -452,6 +511,15 @@ fn read_from_fwnode_property(fwnode: &FwNode, name: &CStr) -> Result<Self> {
     }
 }
 
+/// The number of arguments of a reference.
+pub enum NArgs<'a> {
+    /// The name of the property of the reference indicating the number of
+    /// arguments.
+    Prop(&'a CStr),
+    /// 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
-- 
2.49.0

[PATCH v3 7/7] samples: rust: platform: Add property read examples

[Remo Senekowitsch]

Add some example usage of the device property read methods for
DT/ACPI/swnode properties.

Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 drivers/of/unittest-data/tests-platform.dtsi |  3 +
 samples/rust/rust_driver_platform.rs         | 72 +++++++++++++++++++-
 2 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi
index 4171f43cf..50a51f38a 100644
--- a/drivers/of/unittest-data/tests-platform.dtsi
+++ b/drivers/of/unittest-data/tests-platform.dtsi
@@ -37,6 +37,9 @@ dev@100 {
 			test-device@2 {
 				compatible = "test,rust-device";
 				reg = <0x2>;
+
+				test,u32-prop = <0xdeadbeef>;
+				test,i16-array = /bits/ 16 <1 2 (-3) (-4)>;
 			};
 		};
 
diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
index 8120609e2..04731baa7 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -2,7 +2,7 @@
 
 //! Rust Platform driver sample.
 
-use kernel::{c_str, of, platform, prelude::*};
+use kernel::{c_str, of, platform, prelude::*, str::CString};
 
 struct SampleDriver {
     pdev: platform::Device,
@@ -22,18 +22,84 @@ impl platform::Driver for SampleDriver {
     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
 
     fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
-        dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n");
+        let dev = pdev.as_ref();
+
+        dev_dbg!(dev, "Probe Rust Platform driver sample.\n");
 
         if let Some(info) = info {
-            dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
+            dev_info!(dev, "Probed with info: '{}'.\n", info.0);
         }
 
+        Self::properties_parse(dev)?;
+
         let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
 
         Ok(drvdata.into())
     }
 }
 
+impl SampleDriver {
+    fn properties_parse(dev: &kernel::device::Device) -> Result<()> {
+        let fwnode = dev.fwnode().ok_or(ENOENT)?;
+
+        if let Ok(idx) =
+            fwnode.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
+        {
+            dev_info!(dev, "matched compatible string idx = {}\n", idx);
+        }
+
+        if let Ok(str) = fwnode
+            .property_read::<CString>(c_str!("compatible"))
+            .required()
+        {
+            dev_info!(dev, "compatible string = {:?}\n", str);
+        }
+
+        let prop = fwnode.property_read_bool(c_str!("test,bool-prop"));
+        dev_info!(dev, "bool prop is {}\n", prop);
+
+        if fwnode.property_present(c_str!("test,u32-prop")) {
+            dev_info!(dev, "'test,u32-prop' is present\n");
+        }
+
+        let prop = fwnode
+            .property_read::<u32>(c_str!("test,u32-optional-prop"))
+            .or(0x12);
+        dev_info!(
+            dev,
+            "'test,u32-optional-prop' is {:#x} (default = {:#x})\n",
+            prop,
+            0x12
+        );
+
+        // Missing property without a default will print an error
+        let _ = fwnode
+            .property_read::<u32>(c_str!("test,u32-required-prop"))
+            .required();
+
+        let prop: u32 = fwnode.property_read(c_str!("test,u32-prop")).required()?;
+        dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
+
+        // TODO: remove or replace with u16? `Property` is not implemented for
+        // unsigned integers, as suggested by Andy Shevchenko.
+
+        let prop: [i16; 4] = fwnode.property_read(c_str!("test,i16-array")).required()?;
+        dev_info!(dev, "'test,i16-array' is {:?}\n", prop);
+        dev_info!(
+            dev,
+            "'test,i16-array' length is {}\n",
+            fwnode.property_count_elem::<u16>(c_str!("test,i16-array"))?,
+        );
+
+        let prop: KVec<i16> = fwnode
+            .property_read_array_vec(c_str!("test,i16-array"), 4)?
+            .required()?;
+        dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop);
+
+        Ok(())
+    }
+}
+
 impl Drop for SampleDriver {
     fn drop(&mut self) {
         dev_dbg!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n");
-- 
2.49.0

Re: [PATCH v3 1/7] rust: property: Move property_present to separate file

[Danilo Krummrich]

On Fri, Apr 25, 2025 at 05:01:24PM +0200, Remo Senekowitsch wrote:
> Not all property-related APIs can be exposed directly on a device.
> For example, iterating over child nodes of a device will yield
> fwnode_handle. Thus, in order to access properties on these child nodes,
> the property access methods must be implemented on the abstraction over
> fwnode_handle.
> 
> While it's possible to expose similar methods on `Device` directly for
> convenience, those methods would have to get the `FwNode` first, which
> is a fallible operation, making the API inconsistent. For this reason,
> such duplicated methods are omitted. Users who need to read properties
> of a device will have to explictily get the `FwNode` first (handle the
> `None` case) and then read properties on that.

I think I mentioned that in v2 [1]; when the commit subject says "rust:
property: Move property_present to separate", the commit shouldn't do anything
beyond this scope.

I can see that you switch from device_property_present() to
fwnode_property_present(), without fixing users, so obviously the former is
unused.

Please make the implementation of the FwNode abstraction and the removal of
device_property_present() separate commits.

[1] https://lore.kernel.org/lkml/Z_0xGRsI74PsAL_E@cassiopeiae/

> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  MAINTAINERS                              |  3 +-
>  rust/helpers/helpers.c                   |  1 +
>  rust/helpers/property.c                  |  8 +++
>  rust/kernel/{device.rs => device/mod.rs} |  9 +--
>  rust/kernel/device/property.rs           | 70 ++++++++++++++++++++++++
>  5 files changed, 83 insertions(+), 8 deletions(-)
>  create mode 100644 rust/helpers/property.c
>  rename rust/kernel/{device.rs => device/mod.rs} (97%)
>  create mode 100644 rust/kernel/device/property.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c8d9e8187..4585f9e7f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7112,7 +7112,8 @@ F:	include/linux/kobj*
>  F:	include/linux/property.h
>  F:	include/linux/sysfs.h
>  F:	lib/kobj*
> -F:	rust/kernel/device.rs
> +F:	rust/kernel/device/mod.rs
> +F:	rust/kernel/device/property.rs

This should just be

	F:	rust/kernel/device/

>  F:	rust/kernel/device_id.rs
>  F:	rust/kernel/devres.rs
>  F:	rust/kernel/driver.rs
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 0640b7e11..b4eec5bf2 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -23,6 +23,7 @@
>  #include "platform.c"
>  #include "pci.c"
>  #include "pid_namespace.c"
> +#include "property.c"
>  #include "rbtree.c"
>  #include "rcu.c"
>  #include "refcount.c"
> diff --git a/rust/helpers/property.c b/rust/helpers/property.c
> new file mode 100644
> index 000000000..08f68e2da
> --- /dev/null
> +++ b/rust/helpers/property.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/property.h>
> +
> +void rust_helper_fwnode_handle_put(struct fwnode_handle *fwnode)
> +{
> +	fwnode_handle_put(fwnode);
> +}
> diff --git a/rust/kernel/device.rs b/rust/kernel/device/mod.rs
> similarity index 97%
> rename from rust/kernel/device.rs
> rename to rust/kernel/device/mod.rs
> index db2d9658b..e49107452 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device/mod.rs
> @@ -6,7 +6,6 @@
>  
>  use crate::{
>      bindings,
> -    str::CStr,
>      types::{ARef, Opaque},
>  };
>  use core::{fmt, ptr};
> @@ -14,6 +13,8 @@
>  #[cfg(CONFIG_PRINTK)]
>  use crate::c_str;
>  
> +pub mod property;
> +
>  /// A reference-counted device.
>  ///
>  /// This structure represents the Rust abstraction for a C `struct device`. This implementation
> @@ -181,12 +182,6 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
>              )
>          };
>      }
> -
> -    /// Checks if property is present or not.
> -    pub fn property_present(&self, name: &CStr) -> bool {
> -        // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
> -        unsafe { bindings::device_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
> -    }
>  }
>  
>  // SAFETY: Instances of `Device` are always reference-counted.
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> new file mode 100644
> index 000000000..d89715f7d
> --- /dev/null
> +++ b/rust/kernel/device/property.rs
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Unified device property interface.
> +//!
> +//! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
> +
> +use core::ptr;
> +
> +use crate::{bindings, device::Device, str::CStr, types::Opaque};
> +
> +impl Device {
> +    /// Obtain the fwnode corresponding to the device.
> +    pub fn fwnode(&self) -> Option<&FwNode> {
> +        // SAFETY: `self` is valid.
> +        let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
> +        if fwnode_handle.is_null() {
> +            return None;
> +        }
> +        // SAFETY: `fwnode_handle` is valid. Its lifetime is tied to `&self`. We
> +        // return a reference instead of an `ARef<FwNode>` because `dev_fwnode()`
> +        // doesn't increment the refcount. It is safe to cast from a
> +        // `struct fwnode_handle*` to a `*const FwNode` because `FwNode` is
> +        // defined as a `#[repr(transparent)]` wrapper around `fwnode_handle`.
> +        Some(unsafe { &*fwnode_handle.cast() })
> +    }
> +}

Given that the cover letter says "Remove the duplicated property reading methods
on Device.", I assume that's the only Device method you introduce? If so, please
keep this one in the impl block of device/mod.rs.

Please also rebase onto driver-core-next and put this method in the following
impl block.

	impl<Ctx: DeviceContext> Device<Ctx>

I assume this is valid to call from any device context.

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Danilo Krummrich]

On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
> This abstraction is a way to force users to specify whether a property
> is supposed to be required or not. This allows us to move error
> logging of missing required properties into core, preventing a lot of
> boilerplate in drivers.
> 
> It will be used by upcoming methods for reading device properties.
> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/device/property.rs | 57 ++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index 28850aa3b..de31a1f56 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -146,3 +146,60 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>      }
>  }
> +
> +/// 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() {
> +            pr_err!(
> +                "{}: property '{}' is missing\n",
> +                self.fwnode.display_path(),
> +                self.name
> +            );

Hm, we can't use the device pointer of the fwnode_handle, since it is not
guaranteed to be valid, hence the pr_*() print...

Anyways, I'm not sure we need to print here at all. If a driver wants to print
that it is unhappy about a missing required property it can do so by itself, I
think.

Re: [PATCH v3 2/7] rust: property: Enable printing fwnode name and path

[Danilo Krummrich]

On Fri, Apr 25, 2025 at 05:01:25PM +0200, Remo Senekowitsch wrote:
> Add two new public methods `display_name` and `display_path` to
> `FwNode`. They can be used by driver authors for logging purposes. In
> addition, they will be used by core property abstractions for automatic
> logging, for example when a driver attempts to read a required but
> missing property.
> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/device/property.rs | 78 ++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index d89715f7d..28850aa3b 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -49,6 +49,84 @@ pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
>          self.0.get()
>      }
>  
> +    /// Returns an object that implements [`Display`](core::fmt::Display) for
> +    /// printing the name of a node.
> +    pub fn display_name(&self) -> impl core::fmt::Display + '_ {
> +        struct FwNodeDisplayName<'a>(&'a FwNode);
> +
> +        impl core::fmt::Display for FwNodeDisplayName<'_> {
> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> +                // SAFETY: self is valid by its type invariant
> +                let name = unsafe { bindings::fwnode_get_name(self.0.as_raw()) };
> +                if name.is_null() {
> +                    return Ok(());
> +                }
> +                // SAFETY: fwnode_get_name returns null or a valid C string and
> +                // name is not null
> +                let name = unsafe { CStr::from_char_ptr(name) };
> +                write!(f, "{name}")
> +            }
> +        }
> +
> +        FwNodeDisplayName(self)
> +    }
> +
> +    /// Returns an object that implements [`Display`](core::fmt::Display) for
> +    /// printing the full path of a node.
> +    pub fn display_path(&self) -> impl core::fmt::Display + '_ {
> +        struct FwNodeDisplayPath<'a>(&'a FwNode);
> +
> +        impl core::fmt::Display for FwNodeDisplayPath<'_> {
> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> +                // The logic here is the same as the one in lib/vsprintf.c
> +                // (fwnode_full_name_string).
> +
> +                let num_parents = unsafe { bindings::fwnode_count_parents(self.0.as_raw()) };
> +
> +                for depth in (0..=num_parents).rev() {
> +                    let fwnode = if depth == 0 {
> +                        self.0.as_raw()
> +                    } else {
> +                        // SAFETY: `self.0.as_raw()` is valid
> +                        unsafe { bindings::fwnode_get_nth_parent(self.0.as_raw(), depth) }
> +                    };
> +
> +                    // SAFETY: fwnode is valid, it is either `self.0.as_raw()` or
> +                    // the return value of `bindings::fwnode_get_nth_parent` which
> +                    // returns a valid pointer to a fwnode_handle if the provided
> +                    // depth is within the valid range, which we know to be true.
> +                    let prefix = unsafe { bindings::fwnode_get_name_prefix(fwnode) };
> +                    if !prefix.is_null() {
> +                        // SAFETY: fwnode_get_name_prefix returns null or a
> +                        // valid C string
> +                        let prefix = unsafe { CStr::from_char_ptr(prefix) };
> +                        write!(f, "{prefix}")?;
> +                    }
> +                    // SAFETY: fwnode is valid for the reasons stated above
> +                    let name = unsafe { bindings::fwnode_get_name(fwnode) };
> +                    if !name.is_null() {
> +                        // SAFETY: fwnode_get_name returns null or a valid
> +                        // C string
> +                        let name = unsafe { CStr::from_char_ptr(name) };
> +                        write!(f, "{name}")?;
> +                    }

I think you should be able to just call

	FwNodeDisplayName(self).fmt(f)?

for this part, which saves you the duplicated code.

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Dirk Behme]

On 25.04.25 17:35, Danilo Krummrich wrote:
> On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
>> This abstraction is a way to force users to specify whether a property
>> is supposed to be required or not. This allows us to move error
>> logging of missing required properties into core, preventing a lot of
>> boilerplate in drivers.
>>
>> It will be used by upcoming methods for reading device properties.
>>
>> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>> ---
>>  rust/kernel/device/property.rs | 57 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 57 insertions(+)
>>
>> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
>> index 28850aa3b..de31a1f56 100644
>> --- a/rust/kernel/device/property.rs
>> +++ b/rust/kernel/device/property.rs
>> @@ -146,3 +146,60 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>>      }
>>  }
>> +
>> +/// 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() {
>> +            pr_err!(
>> +                "{}: property '{}' is missing\n",
>> +                self.fwnode.display_path(),
>> +                self.name
>> +            );
> 
> Hm, we can't use the device pointer of the fwnode_handle, since it is not
> guaranteed to be valid, hence the pr_*() print...
> 
> Anyways, I'm not sure we need to print here at all. If a driver wants to print
> that it is unhappy about a missing required property it can do so by itself, I
> think.

Hmm, the driver said by using 'required' that it *is* required. So a
missing property is definitely an error here. Else it would have used
'optional'. Which doesn't print in case the property is missing.

If I remember correctly having 'required' and 'optional' is the result
of some discussion on Zulip. And one conclusion of that discussion was
to move checking & printing the error out of the individual drivers
into a central place to avoid this error checking & printing in each
and every driver. I think the idea is that the drivers just have to do
...required()?; and that's it, then.

Best regards

Dirk

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
> On 25.04.25 17:35, Danilo Krummrich wrote:
> > On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
> >> This abstraction is a way to force users to specify whether a property
> >> is supposed to be required or not. This allows us to move error
> >> logging of missing required properties into core, preventing a lot of
> >> boilerplate in drivers.
> >>
> >> It will be used by upcoming methods for reading device properties.
> >>
> >> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> >> ---
> >>  rust/kernel/device/property.rs | 57 ++++++++++++++++++++++++++++++++++
> >>  1 file changed, 57 insertions(+)
> >>
> >> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> >> index 28850aa3b..de31a1f56 100644
> >> --- a/rust/kernel/device/property.rs
> >> +++ b/rust/kernel/device/property.rs
> >> @@ -146,3 +146,60 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> >>      }
> >>  }
> >> +
> >> +/// 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() {
> >> +            pr_err!(
> >> +                "{}: property '{}' is missing\n",
> >> +                self.fwnode.display_path(),
> >> +                self.name
> >> +            );
> > 
> > Hm, we can't use the device pointer of the fwnode_handle, since it is not
> > guaranteed to be valid, hence the pr_*() print...
> > 
> > Anyways, I'm not sure we need to print here at all. If a driver wants to print
> > that it is unhappy about a missing required property it can do so by itself, I
> > think.
> 
> Hmm, the driver said by using 'required' that it *is* required. So a
> missing property is definitely an error here. Else it would have used
> 'optional'. Which doesn't print in case the property is missing.
> 
> If I remember correctly having 'required' and 'optional' is the result
> of some discussion on Zulip. And one conclusion of that discussion was
> to move checking & printing the error out of the individual drivers
> into a central place to avoid this error checking & printing in each
> and every driver. I think the idea is that the drivers just have to do
> ...required()?; and that's it, then.

Yes, I get the idea.

If it'd be possible to use dev_err!() instead I wouldn't object in this specific
case. But this code is used by drivers from probe(), hence printing the error
without saying for which device it did occur is a bit pointless.

Drivers can still decide to properly print the error if the returned Result
indicates one.

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Remo Senekowitsch]

On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
>> On 25.04.25 17:35, Danilo Krummrich wrote:
>> > On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
>> >> This abstraction is a way to force users to specify whether a property
>> >> is supposed to be required or not. This allows us to move error
>> >> logging of missing required properties into core, preventing a lot of
>> >> boilerplate in drivers.
>> >>
>> >> It will be used by upcoming methods for reading device properties.
>> >>
>> >> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>> >> ---
>> >>  rust/kernel/device/property.rs | 57 ++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 57 insertions(+)
>> >>
>> >> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
>> >> index 28850aa3b..de31a1f56 100644
>> >> --- a/rust/kernel/device/property.rs
>> >> +++ b/rust/kernel/device/property.rs
>> >> @@ -146,3 +146,60 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>> >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>> >>      }
>> >>  }
>> >> +
>> >> +/// 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() {
>> >> +            pr_err!(
>> >> +                "{}: property '{}' is missing\n",
>> >> +                self.fwnode.display_path(),
>> >> +                self.name
>> >> +            );
>> > 
>> > Hm, we can't use the device pointer of the fwnode_handle, since it is not
>> > guaranteed to be valid, hence the pr_*() print...
>> > 
>> > Anyways, I'm not sure we need to print here at all. If a driver wants to print
>> > that it is unhappy about a missing required property it can do so by itself, I
>> > think.
>> 
>> Hmm, the driver said by using 'required' that it *is* required. So a
>> missing property is definitely an error here. Else it would have used
>> 'optional'. Which doesn't print in case the property is missing.
>> 
>> If I remember correctly having 'required' and 'optional' is the result
>> of some discussion on Zulip. And one conclusion of that discussion was
>> to move checking & printing the error out of the individual drivers
>> into a central place to avoid this error checking & printing in each
>> and every driver. I think the idea is that the drivers just have to do
>> ...required()?; and that's it, then.
>
> Yes, I get the idea.
>
> If it'd be possible to use dev_err!() instead I wouldn't object in this specific
> case. But this code is used by drivers from probe(), hence printing the error
> without saying for which device it did occur is a bit pointless.
>
> Drivers can still decide to properly print the error if the returned Result
> indicates one.

One alternative would be to store a reference count to the device in
`FwNode`. At that point we'd be guaranteed to have a valid reference
whenever we want to log something.

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
> > On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
> >> On 25.04.25 17:35, Danilo Krummrich wrote:
> >> > On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
> >> >> This abstraction is a way to force users to specify whether a property
> >> >> is supposed to be required or not. This allows us to move error
> >> >> logging of missing required properties into core, preventing a lot of
> >> >> boilerplate in drivers.
> >> >>
> >> >> It will be used by upcoming methods for reading device properties.
> >> >>
> >> >> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> >> >> ---
> >> >>  rust/kernel/device/property.rs | 57 ++++++++++++++++++++++++++++++++++
> >> >>  1 file changed, 57 insertions(+)
> >> >>
> >> >> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> >> >> index 28850aa3b..de31a1f56 100644
> >> >> --- a/rust/kernel/device/property.rs
> >> >> +++ b/rust/kernel/device/property.rs
> >> >> @@ -146,3 +146,60 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >> >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> >> >>      }
> >> >>  }
> >> >> +
> >> >> +/// 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() {
> >> >> +            pr_err!(
> >> >> +                "{}: property '{}' is missing\n",
> >> >> +                self.fwnode.display_path(),
> >> >> +                self.name
> >> >> +            );
> >> > 
> >> > Hm, we can't use the device pointer of the fwnode_handle, since it is not
> >> > guaranteed to be valid, hence the pr_*() print...
> >> > 
> >> > Anyways, I'm not sure we need to print here at all. If a driver wants to print
> >> > that it is unhappy about a missing required property it can do so by itself, I
> >> > think.
> >> 
> >> Hmm, the driver said by using 'required' that it *is* required. So a
> >> missing property is definitely an error here. Else it would have used
> >> 'optional'. Which doesn't print in case the property is missing.
> >> 
> >> If I remember correctly having 'required' and 'optional' is the result
> >> of some discussion on Zulip. And one conclusion of that discussion was
> >> to move checking & printing the error out of the individual drivers
> >> into a central place to avoid this error checking & printing in each
> >> and every driver. I think the idea is that the drivers just have to do
> >> ...required()?; and that's it, then.
> >
> > Yes, I get the idea.
> >
> > If it'd be possible to use dev_err!() instead I wouldn't object in this specific
> > case. But this code is used by drivers from probe(), hence printing the error
> > without saying for which device it did occur is a bit pointless.
> >
> > Drivers can still decide to properly print the error if the returned Result
> > indicates one.
> 
> One alternative would be to store a reference count to the device in
> `FwNode`. At that point we'd be guaranteed to have a valid reference
> whenever we want to log something.

Yes, that would work. However, I'm not convinced that it's worth to store an
ARef<Device> (i.e. take a device reference) in each FwNode structure *only* to
be able to force an error print if a required device property isn't available.

Why do you think it is important to force this error print by having it in
PropertyGuard::required() and even take an additional device reference for this
purpose, rather than leaving it to the driver when to print a message for an
error condition that makes it fail to probe()?

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Dirk Behme]

On 26.04.25 16:19, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
>> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
>>> On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
>>>> On 25.04.25 17:35, Danilo Krummrich wrote:
>>>>> On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
>>>>>> This abstraction is a way to force users to specify whether a property
>>>>>> is supposed to be required or not. This allows us to move error
>>>>>> logging of missing required properties into core, preventing a lot of
>>>>>> boilerplate in drivers.
>>>>>>
>>>>>> It will be used by upcoming methods for reading device properties.
>>>>>>
>>>>>> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>>>>>> ---
>>>>>>  rust/kernel/device/property.rs | 57 ++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 57 insertions(+)
>>>>>>
>>>>>> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
>>>>>> index 28850aa3b..de31a1f56 100644
>>>>>> --- a/rust/kernel/device/property.rs
>>>>>> +++ b/rust/kernel/device/property.rs
>>>>>> @@ -146,3 +146,60 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>>>>>>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>>>>>>      }
>>>>>>  }
>>>>>> +
>>>>>> +/// 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() {
>>>>>> +            pr_err!(
>>>>>> +                "{}: property '{}' is missing\n",
>>>>>> +                self.fwnode.display_path(),
>>>>>> +                self.name
>>>>>> +            );
>>>>>
>>>>> Hm, we can't use the device pointer of the fwnode_handle, since it is not
>>>>> guaranteed to be valid, hence the pr_*() print...
>>>>>
>>>>> Anyways, I'm not sure we need to print here at all. If a driver wants to print
>>>>> that it is unhappy about a missing required property it can do so by itself, I
>>>>> think.
>>>>
>>>> Hmm, the driver said by using 'required' that it *is* required. So a
>>>> missing property is definitely an error here. Else it would have used
>>>> 'optional'. Which doesn't print in case the property is missing.
>>>>
>>>> If I remember correctly having 'required' and 'optional' is the result
>>>> of some discussion on Zulip. And one conclusion of that discussion was
>>>> to move checking & printing the error out of the individual drivers
>>>> into a central place to avoid this error checking & printing in each
>>>> and every driver. I think the idea is that the drivers just have to do
>>>> ...required()?; and that's it, then.
>>>
>>> Yes, I get the idea.
>>>
>>> If it'd be possible to use dev_err!() instead I wouldn't object in this specific
>>> case. But this code is used by drivers from probe(), hence printing the error
>>> without saying for which device it did occur is a bit pointless.
>>>
>>> Drivers can still decide to properly print the error if the returned Result
>>> indicates one.
>>
>> One alternative would be to store a reference count to the device in
>> `FwNode`. At that point we'd be guaranteed to have a valid reference
>> whenever we want to log something.
> 
> Yes, that would work. However, I'm not convinced that it's worth to store an
> ARef<Device> (i.e. take a device reference) in each FwNode structure *only* to
> be able to force an error print if a required device property isn't available.
> 
> Why do you think it is important to force this error print by having it in
> PropertyGuard::required() and even take an additional device reference for this
> purpose, rather than leaving it to the driver when to print a message for an
> error condition that makes it fail to probe()?


To my understanding doing the error print in "core" was proposed by
Rob [1]:

"If the property is missing and required, then we may want to print an
error msg (in the core, not every caller)"

Dirk

[1]
https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/496884813

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Danilo Krummrich]

On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
> On 26.04.25 16:19, Danilo Krummrich wrote:
> > On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
> >> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
> >>> If it'd be possible to use dev_err!() instead I wouldn't object in this specific
> >>> case. But this code is used by drivers from probe(), hence printing the error
> >>> without saying for which device it did occur is a bit pointless.
> >>>
> >>> Drivers can still decide to properly print the error if the returned Result
> >>> indicates one.
> >>
> >> One alternative would be to store a reference count to the device in
> >> `FwNode`. At that point we'd be guaranteed to have a valid reference
> >> whenever we want to log something.
> > 
> > Yes, that would work. However, I'm not convinced that it's worth to store an
> > ARef<Device> (i.e. take a device reference) in each FwNode structure *only* to
> > be able to force an error print if a required device property isn't available.
> > 
> > Why do you think it is important to force this error print by having it in
> > PropertyGuard::required() and even take an additional device reference for this
> > purpose, rather than leaving it to the driver when to print a message for an
> > error condition that makes it fail to probe()?
> 
> To my understanding doing the error print in "core" was proposed by
> Rob [1]:

That is fine, though it doesn't answer my question above. :)

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Remo Senekowitsch]

On Sat Apr 26, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
>> On 26.04.25 16:19, Danilo Krummrich wrote:
>> > On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
>> >> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
>> >>> If it'd be possible to use dev_err!() instead I wouldn't object in this specific
>> >>> case. But this code is used by drivers from probe(), hence printing the error
>> >>> without saying for which device it did occur is a bit pointless.
>> >>>
>> >>> Drivers can still decide to properly print the error if the returned Result
>> >>> indicates one.
>> >>
>> >> One alternative would be to store a reference count to the device in
>> >> `FwNode`. At that point we'd be guaranteed to have a valid reference
>> >> whenever we want to log something.
>> > 
>> > Yes, that would work. However, I'm not convinced that it's worth to store an
>> > ARef<Device> (i.e. take a device reference) in each FwNode structure *only* to
>> > be able to force an error print if a required device property isn't available.
>> > 
>> > Why do you think it is important to force this error print by having it in
>> > PropertyGuard::required() and even take an additional device reference for this
>> > purpose, rather than leaving it to the driver when to print a message for an
>> > error condition that makes it fail to probe()?
>> 
>> To my understanding doing the error print in "core" was proposed by
>> Rob [1]:
>
> That is fine, though it doesn't answer my question above. :)

If the question is addressed to me, I don't think it is important.
I don't have a particular preference either way. I'm just trying to
come up with a solution that is satisfactory to everyone. We should
hear from Rob if he's ok with removing the logging entirely given the
limitations.

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Dirk Behme]

On 26.04.25 12:15, Danilo Krummrich wrote:
> On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
>> On 25.04.25 17:35, Danilo Krummrich wrote:
>>> On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
>>>> This abstraction is a way to force users to specify whether a property
>>>> is supposed to be required or not. This allows us to move error
>>>> logging of missing required properties into core, preventing a lot of
>>>> boilerplate in drivers.
>>>>
>>>> It will be used by upcoming methods for reading device properties.
>>>>
>>>> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>>>> ---
>>>>  rust/kernel/device/property.rs | 57 ++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 57 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
>>>> index 28850aa3b..de31a1f56 100644
>>>> --- a/rust/kernel/device/property.rs
>>>> +++ b/rust/kernel/device/property.rs
>>>> @@ -146,3 +146,60 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>>>>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>>>>      }
>>>>  }
>>>> +
>>>> +/// 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() {
>>>> +            pr_err!(
>>>> +                "{}: property '{}' is missing\n",
>>>> +                self.fwnode.display_path(),
>>>> +                self.name
>>>> +            );
>>>
>>> Hm, we can't use the device pointer of the fwnode_handle, since it is not
>>> guaranteed to be valid, hence the pr_*() print...
>>>
>>> Anyways, I'm not sure we need to print here at all. If a driver wants to print
>>> that it is unhappy about a missing required property it can do so by itself, I
>>> think.
>>
>> Hmm, the driver said by using 'required' that it *is* required. So a
>> missing property is definitely an error here. Else it would have used
>> 'optional'. Which doesn't print in case the property is missing.
>>
>> If I remember correctly having 'required' and 'optional' is the result
>> of some discussion on Zulip. And one conclusion of that discussion was
>> to move checking & printing the error out of the individual drivers
>> into a central place to avoid this error checking & printing in each
>> and every driver. I think the idea is that the drivers just have to do
>> ...required()?; and that's it, then.
> 
> Yes, I get the idea.
> 
> If it'd be possible to use dev_err!() instead I wouldn't object in this specific
> case. But this code is used by drivers from probe(), hence printing the error
> without saying for which device it did occur is a bit pointless.

Thinking a little about this, yes, we don't know the device here. But:
Does the device matter here? There is nothing wrong with the (unknown)
device, no? What is wrong here is the firmware (node). It misses
something. And this is exactly what the message tells: "There is an
error due to the missing node 'name' in 'path', please fix it". That
should be sufficient to identify the firmware/device tree description
and fix it.

I can still follow Rob's proposal on doing the printing in 'core' :)

Thanks,

Dirk

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Danilo Krummrich]

On Sun, Apr 27, 2025 at 08:11:58AM +0200, Dirk Behme wrote:
> On 26.04.25 12:15, Danilo Krummrich wrote:
> > On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
> >> On 25.04.25 17:35, Danilo Krummrich wrote:
> >>> On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
> >>>> +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() {
> >>>> +            pr_err!(
> >>>> +                "{}: property '{}' is missing\n",
> >>>> +                self.fwnode.display_path(),
> >>>> +                self.name
> >>>> +            );
> >>>
> >>> Hm, we can't use the device pointer of the fwnode_handle, since it is not
> >>> guaranteed to be valid, hence the pr_*() print...
> >>>
> >>> Anyways, I'm not sure we need to print here at all. If a driver wants to print
> >>> that it is unhappy about a missing required property it can do so by itself, I
> >>> think.
> >>
> >> Hmm, the driver said by using 'required' that it *is* required. So a
> >> missing property is definitely an error here. Else it would have used
> >> 'optional'. Which doesn't print in case the property is missing.
> >>
> >> If I remember correctly having 'required' and 'optional' is the result
> >> of some discussion on Zulip. And one conclusion of that discussion was
> >> to move checking & printing the error out of the individual drivers
> >> into a central place to avoid this error checking & printing in each
> >> and every driver. I think the idea is that the drivers just have to do
> >> ...required()?; and that's it, then.
> > 
> > Yes, I get the idea.
> > 
> > If it'd be possible to use dev_err!() instead I wouldn't object in this specific
> > case. But this code is used by drivers from probe(), hence printing the error
> > without saying for which device it did occur is a bit pointless.
> 
> Thinking a little about this, yes, we don't know the device here. But:
> Does the device matter here?

If the above fails it means that for a (specific) device a driver expects that
a specific property of some firmware node is present. So, yes, I think it does
matter.

> There is nothing wrong with the (unknown)
> device, no? What is wrong here is the firmware (node). It misses
> something.

How do we know the firmware node is wrong? Maybe the driver has wrong
expectations for this device?

> And this is exactly what the message tells: "There is an
> error due to the missing node 'name' in 'path', please fix it". That
> should be sufficient to identify the firmware/device tree description
> and fix it.

I think we can't always fix them, even if they're wrong. How do we fix ACPI
firmware nodes for instance?

(Software nodes provide a solution for that, see also commit 59abd83672f7
("drivers: base: Introducing software nodes to the firmware node framework").)

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[John Hubbard]

On 4/26/25 2:50 PM, Remo Senekowitsch wrote:
> On Sat Apr 26, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
>> On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
>>> On 26.04.25 16:19, Danilo Krummrich wrote:
>>>> On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
>>>>> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
...
>>>> Why do you think it is important to force this error print by having it in
>>>> PropertyGuard::required() and even take an additional device reference for this
>>>> purpose, rather than leaving it to the driver when to print a message for an
>>>> error condition that makes it fail to probe()?
>>>
>>> To my understanding doing the error print in "core" was proposed by
>>> Rob [1]:
>>
>> That is fine, though it doesn't answer my question above. :)
> 
> If the question is addressed to me, I don't think it is important.
> I don't have a particular preference either way. I'm just trying to

Generally, printing in libraries an lower level routines (in this case,
"core") is undesirable. We'll do it anyway, sometimes:

     a) Behind a *_DEBUG configuration, to debug the core itself, or

     b) Desperation: hard to recover from errors, that the upper layers
        for some reason lack context to provide an adequate error
        message for.

The idea is that the lower level you are in the software stack, the
more rare printing should be.


thanks,
-- 
John Hubbard

> come up with a solution that is satisfactory to everyone. We should
> hear from Rob if he's ok with removing the logging entirely given the
> limitations.
> 

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Dirk Behme]

On 27/04/2025 14:23, Danilo Krummrich wrote:
> On Sun, Apr 27, 2025 at 08:11:58AM +0200, Dirk Behme wrote:
>> On 26.04.25 12:15, Danilo Krummrich wrote:
>>> On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
>>>> On 25.04.25 17:35, Danilo Krummrich wrote:
>>>>> On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
>>>>>> +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() {
>>>>>> +            pr_err!(
>>>>>> +                "{}: property '{}' is missing\n",
>>>>>> +                self.fwnode.display_path(),
>>>>>> +                self.name
>>>>>> +            );
>>>>>
>>>>> Hm, we can't use the device pointer of the fwnode_handle, since it is not
>>>>> guaranteed to be valid, hence the pr_*() print...
>>>>>
>>>>> Anyways, I'm not sure we need to print here at all. If a driver wants to print
>>>>> that it is unhappy about a missing required property it can do so by itself, I
>>>>> think.
>>>>
>>>> Hmm, the driver said by using 'required' that it *is* required. So a
>>>> missing property is definitely an error here. Else it would have used
>>>> 'optional'. Which doesn't print in case the property is missing.
>>>>
>>>> If I remember correctly having 'required' and 'optional' is the result
>>>> of some discussion on Zulip. And one conclusion of that discussion was
>>>> to move checking & printing the error out of the individual drivers
>>>> into a central place to avoid this error checking & printing in each
>>>> and every driver. I think the idea is that the drivers just have to do
>>>> ...required()?; and that's it, then.
>>>
>>> Yes, I get the idea.
>>>
>>> If it'd be possible to use dev_err!() instead I wouldn't object in this specific
>>> case. But this code is used by drivers from probe(), hence printing the error
>>> without saying for which device it did occur is a bit pointless.
>>
>> Thinking a little about this, yes, we don't know the device here. But:
>> Does the device matter here?
> 
> If the above fails it means that for a (specific) device a driver expects that
> a specific property of some firmware node is present. So, yes, I think it does
> matter.
> 
>> There is nothing wrong with the (unknown)
>> device, no? What is wrong here is the firmware (node). It misses
>> something.
> 
> How do we know the firmware node is wrong? Maybe the driver has wrong
> expectations for this device?
> 
>> And this is exactly what the message tells: "There is an
>> error due to the missing node 'name' in 'path', please fix it". That
>> should be sufficient to identify the firmware/device tree description
>> and fix it.
> 
> I think we can't always fix them, even if they're wrong. How do we fix ACPI
> firmware nodes for instance?

So the argument here is that the device (driver) is expecting something
to be "required" is wrong and might need to be fixed. Not the firmware.
Yes, ok, that is a valid argument. I have a device tree background and
there in 99% of the cases the device tree needs a fix ;)

But let me ask the other way around, then: What will it hurt or break if
we keep the pr_err() like Remo did? Even knowing that its not perfect?
But knowing that it will give at least a note that something is wrong
with at least a starting point for searching what needs to be fixed. I
mean even if we don't get the device, we will get the affected node we
can search for which device uses it as "required".

Could we somehow agree that in 90% of the cases this should be catched
at device (driver) development time, already? And therefore it should be
beneficial if we don't require each and every driver to be "bloated"
with checking this individually?

Dirk

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Danilo Krummrich]

On Mon, Apr 28, 2025 at 07:03:07AM +0200, Dirk Behme wrote:
> On 27/04/2025 14:23, Danilo Krummrich wrote:
> > On Sun, Apr 27, 2025 at 08:11:58AM +0200, Dirk Behme wrote:
> >> On 26.04.25 12:15, Danilo Krummrich wrote:
> >>> On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
> >>>> On 25.04.25 17:35, Danilo Krummrich wrote:
> >>>>> On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
> >>>>>> +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() {
> >>>>>> +            pr_err!(
> >>>>>> +                "{}: property '{}' is missing\n",
> >>>>>> +                self.fwnode.display_path(),
> >>>>>> +                self.name
> >>>>>> +            );
> >>>>>
> >>>>> Hm, we can't use the device pointer of the fwnode_handle, since it is not
> >>>>> guaranteed to be valid, hence the pr_*() print...
> >>>>>
> >>>>> Anyways, I'm not sure we need to print here at all. If a driver wants to print
> >>>>> that it is unhappy about a missing required property it can do so by itself, I
> >>>>> think.
> >>>>
> >>>> Hmm, the driver said by using 'required' that it *is* required. So a
> >>>> missing property is definitely an error here. Else it would have used
> >>>> 'optional'. Which doesn't print in case the property is missing.
> >>>>
> >>>> If I remember correctly having 'required' and 'optional' is the result
> >>>> of some discussion on Zulip. And one conclusion of that discussion was
> >>>> to move checking & printing the error out of the individual drivers
> >>>> into a central place to avoid this error checking & printing in each
> >>>> and every driver. I think the idea is that the drivers just have to do
> >>>> ...required()?; and that's it, then.
> >>>
> >>> Yes, I get the idea.
> >>>
> >>> If it'd be possible to use dev_err!() instead I wouldn't object in this specific
> >>> case. But this code is used by drivers from probe(), hence printing the error
> >>> without saying for which device it did occur is a bit pointless.
> >>
> >> Thinking a little about this, yes, we don't know the device here. But:
> >> Does the device matter here?
> > 
> > If the above fails it means that for a (specific) device a driver expects that
> > a specific property of some firmware node is present. So, yes, I think it does
> > matter.
> > 
> >> There is nothing wrong with the (unknown)
> >> device, no? What is wrong here is the firmware (node). It misses
> >> something.
> > 
> > How do we know the firmware node is wrong? Maybe the driver has wrong
> > expectations for this device?
> > 
> >> And this is exactly what the message tells: "There is an
> >> error due to the missing node 'name' in 'path', please fix it". That
> >> should be sufficient to identify the firmware/device tree description
> >> and fix it.
> > 
> > I think we can't always fix them, even if they're wrong. How do we fix ACPI
> > firmware nodes for instance?
> 
> So the argument here is that the device (driver) is expecting something
> to be "required" is wrong and might need to be fixed. Not the firmware.
> Yes, ok, that is a valid argument. I have a device tree background and
> there in 99% of the cases the device tree needs a fix ;)
> 
> But let me ask the other way around, then: What will it hurt or break if
> we keep the pr_err() like Remo did? Even knowing that its not perfect?
> But knowing that it will give at least a note that something is wrong
> with at least a starting point for searching what needs to be fixed. I
> mean even if we don't get the device, we will get the affected node we
> can search for which device uses it as "required".
> 
> Could we somehow agree that in 90% of the cases this should be catched
> at device (driver) development time, already?

I don't see why *catching* such errors needs pr_err() in core code; without it
you still get a Result as return value that you need to handle in some way.

> And therefore it should be
> beneficial if we don't require each and every driver to be "bloated"
> with checking this individually?

I guess you mean "bloated with *printing* this individually", rather than
"checking".

This is where we disagree: I think it is "bloating" the core kernel instead if
we start adding error prints to core code, where a proper error code is
propagated up to the driver.

I did say that I would agree to a certain extend with this specific one if we
could print it properly, since it is designed to leave no doubt that returning
an error code from required() is fatal for the driver. But I'm not even sure
about this anymore.

I still haven't read a reason why this one is so crucial to print from core
code, while for other things that are always fatal (e.g. request_irq()) we
don't.

However, if you really think we need a common helper that prints something in
the error case, maybe we can add an *additional* helper

	pub fn required_by(self, dev: &Device) -> Result<T>

and document that it is the same as required(), with an additional error print
in case of failure for the given device.

- Danilo

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Rob Herring]

On Sun, Apr 27, 2025 at 03:12:18PM -0700, John Hubbard wrote:
> On 4/26/25 2:50 PM, Remo Senekowitsch wrote:
> > On Sat Apr 26, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
> > > On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
> > > > On 26.04.25 16:19, Danilo Krummrich wrote:
> > > > > On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
> > > > > > On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
> ...
> > > > > Why do you think it is important to force this error print by having it in
> > > > > PropertyGuard::required() and even take an additional device reference for this
> > > > > purpose, rather than leaving it to the driver when to print a message for an
> > > > > error condition that makes it fail to probe()?
> > > > 
> > > > To my understanding doing the error print in "core" was proposed by
> > > > Rob [1]:
> > > 
> > > That is fine, though it doesn't answer my question above. :)
> > 
> > If the question is addressed to me, I don't think it is important.
> > I don't have a particular preference either way. I'm just trying to
> 
> Generally, printing in libraries an lower level routines (in this case,
> "core") is undesirable. We'll do it anyway, sometimes:
> 
>     a) Behind a *_DEBUG configuration, to debug the core itself, or
> 
>     b) Desperation: hard to recover from errors, that the upper layers
>        for some reason lack context to provide an adequate error
>        message for.
> 
> The idea is that the lower level you are in the software stack, the
> more rare printing should be.

If that's a kernel style/requirement, I've never heard that. About the 
only coding style in this area I'm aware of don't print messages on 
kmalloc failure because the core does. It's the same concept here.

When practically every caller is printing a message, it should go in the 
called function. It's not really different than anything else we do. If 
we have 2 functions commonly called in sequence, we combine them into a 
single helper function. 

It's a pattern we have in the C API that I'd rather not repeat with 
Rust.

Rob

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[John Hubbard]

On 4/28/25 1:18 PM, Rob Herring wrote:
> On Sun, Apr 27, 2025 at 03:12:18PM -0700, John Hubbard wrote:
>> On 4/26/25 2:50 PM, Remo Senekowitsch wrote:
>>> On Sat Apr 26, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
>>>> On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
>>>>> On 26.04.25 16:19, Danilo Krummrich wrote:
>>>>>> On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
>>>>>>> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
>> ...
>> The idea is that the lower level you are in the software stack, the
>> more rare printing should be.
> 
> If that's a kernel style/requirement, I've never heard that. About the 
> only coding style in this area I'm aware of don't print messages on 
> kmalloc failure because the core does. It's the same concept here.
> 
> When practically every caller is printing a message, it should go in the 

If *every* caller, without exception, today and tomorrow, including 
callers that expect failure--if all of those require printing a message,
then yes, it's time to print from the lower level routine.

Short of that, you end up with potentially excessive, and definitely
unrequested and/or undesirable printing.

> called function. It's not really different than anything else we do. If 
> we have 2 functions commonly called in sequence, we combine them into a 
> single helper function. 
> 

Right. And the rules are similar: the lower level has to be accurately
factored out. Not approximately.

thanks,
-- 
John Hubbard

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Rob Herring]

On Mon, Apr 28, 2025 at 06:09:36PM +0200, Danilo Krummrich wrote:
> On Mon, Apr 28, 2025 at 07:03:07AM +0200, Dirk Behme wrote:
> > On 27/04/2025 14:23, Danilo Krummrich wrote:
> > > On Sun, Apr 27, 2025 at 08:11:58AM +0200, Dirk Behme wrote:
> > >> On 26.04.25 12:15, Danilo Krummrich wrote:
> > >>> On Sat, Apr 26, 2025 at 08:19:09AM +0200, Dirk Behme wrote:
> > >>>> On 25.04.25 17:35, Danilo Krummrich wrote:
> > >>>>> On Fri, Apr 25, 2025 at 05:01:26PM +0200, Remo Senekowitsch wrote:
> > >>>>>> +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() {
> > >>>>>> +            pr_err!(
> > >>>>>> +                "{}: property '{}' is missing\n",
> > >>>>>> +                self.fwnode.display_path(),
> > >>>>>> +                self.name
> > >>>>>> +            );
> > >>>>>
> > >>>>> Hm, we can't use the device pointer of the fwnode_handle, since it is not
> > >>>>> guaranteed to be valid, hence the pr_*() print...
> > >>>>>
> > >>>>> Anyways, I'm not sure we need to print here at all. If a driver wants to print
> > >>>>> that it is unhappy about a missing required property it can do so by itself, I
> > >>>>> think.
> > >>>>
> > >>>> Hmm, the driver said by using 'required' that it *is* required. So a
> > >>>> missing property is definitely an error here. Else it would have used
> > >>>> 'optional'. Which doesn't print in case the property is missing.
> > >>>>
> > >>>> If I remember correctly having 'required' and 'optional' is the result
> > >>>> of some discussion on Zulip. And one conclusion of that discussion was
> > >>>> to move checking & printing the error out of the individual drivers
> > >>>> into a central place to avoid this error checking & printing in each
> > >>>> and every driver. I think the idea is that the drivers just have to do
> > >>>> ...required()?; and that's it, then.
> > >>>
> > >>> Yes, I get the idea.
> > >>>
> > >>> If it'd be possible to use dev_err!() instead I wouldn't object in this specific
> > >>> case. But this code is used by drivers from probe(), hence printing the error
> > >>> without saying for which device it did occur is a bit pointless.
> > >>
> > >> Thinking a little about this, yes, we don't know the device here. But:
> > >> Does the device matter here?
> > > 
> > > If the above fails it means that for a (specific) device a driver expects that
> > > a specific property of some firmware node is present. So, yes, I think it does
> > > matter.
> > > 
> > >> There is nothing wrong with the (unknown)
> > >> device, no? What is wrong here is the firmware (node). It misses
> > >> something.
> > > 
> > > How do we know the firmware node is wrong? Maybe the driver has wrong
> > > expectations for this device?
> > > 
> > >> And this is exactly what the message tells: "There is an
> > >> error due to the missing node 'name' in 'path', please fix it". That
> > >> should be sufficient to identify the firmware/device tree description
> > >> and fix it.
> > > 
> > > I think we can't always fix them, even if they're wrong. How do we fix ACPI
> > > firmware nodes for instance?
> > 
> > So the argument here is that the device (driver) is expecting something
> > to be "required" is wrong and might need to be fixed. Not the firmware.
> > Yes, ok, that is a valid argument. I have a device tree background and
> > there in 99% of the cases the device tree needs a fix ;)
> > 
> > But let me ask the other way around, then: What will it hurt or break if
> > we keep the pr_err() like Remo did? Even knowing that its not perfect?
> > But knowing that it will give at least a note that something is wrong
> > with at least a starting point for searching what needs to be fixed. I
> > mean even if we don't get the device, we will get the affected node we
> > can search for which device uses it as "required".
> > 
> > Could we somehow agree that in 90% of the cases this should be catched
> > at device (driver) development time, already?
> 
> I don't see why *catching* such errors needs pr_err() in core code; without it
> you still get a Result as return value that you need to handle in some way.
> 
> > And therefore it should be
> > beneficial if we don't require each and every driver to be "bloated"
> > with checking this individually?
> 
> I guess you mean "bloated with *printing* this individually", rather than
> "checking".
> 
> This is where we disagree: I think it is "bloating" the core kernel instead if
> we start adding error prints to core code, where a proper error code is
> propagated up to the driver.

1 or more error strings in every single driver is what bloats the 
kernel, not 1 string. It's all kernel code and memory usage whether it's 
core or drivers.

> I did say that I would agree to a certain extend with this specific one if we
> could print it properly, since it is designed to leave no doubt that returning
> an error code from required() is fatal for the driver. But I'm not even sure
> about this anymore.
> 
> I still haven't read a reason why this one is so crucial to print from core
> code, while for other things that are always fatal (e.g. request_irq()) we
> don't.

request_irq() is not always fatal. Some drivers fallback to polling. In 
general, we've been adding _optional() variants of functions to return 
NULL rather than errors which is handled silently by subsequent API 
calls. Secondarily, those print errors in the non-optional case. It's 
not real consistent in this area, but something we should improve.

> However, if you really think we need a common helper that prints something in
> the error case, maybe we can add an *additional* helper
> 
> 	pub fn required_by(self, dev: &Device) -> Result<T>
> 
> and document that it is the same as required(), with an additional error print
> in case of failure for the given device.

One thing that's really hard to debug in C drivers is where an 
error came from. You can for example turn on initcall_debug and see that 
a driver probe returned an error. It's virtually impossible to tell 
where that originated from. The only way to tell is with prints. That is 
probably the root of why probe has so many error prints. I think we can 
do a lot better with rust given Result can hold more than just an int. 
We obviously can't get back to the origin if that was C code, but just 
if we know exactly which call from probe failed that would be a huge 
improvement.

Rob

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Rob Herring]

On Mon, Apr 28, 2025 at 01:25:03PM -0700, John Hubbard wrote:
> On 4/28/25 1:18 PM, Rob Herring wrote:
> > On Sun, Apr 27, 2025 at 03:12:18PM -0700, John Hubbard wrote:
> >> On 4/26/25 2:50 PM, Remo Senekowitsch wrote:
> >>> On Sat Apr 26, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
> >>>> On Sat, Apr 26, 2025 at 04:35:07PM +0200, Dirk Behme wrote:
> >>>>> On 26.04.25 16:19, Danilo Krummrich wrote:
> >>>>>> On Sat, Apr 26, 2025 at 01:08:39PM +0200, Remo Senekowitsch wrote:
> >>>>>>> On Sat Apr 26, 2025 at 12:15 PM CEST, Danilo Krummrich wrote:
> >> ...
> >> The idea is that the lower level you are in the software stack, the
> >> more rare printing should be.
> > 
> > If that's a kernel style/requirement, I've never heard that. About the 
> > only coding style in this area I'm aware of don't print messages on 
> > kmalloc failure because the core does. It's the same concept here.
> > 
> > When practically every caller is printing a message, it should go in the 
> 
> If *every* caller, without exception, today and tomorrow, including 
> callers that expect failure--if all of those require printing a message,
> then yes, it's time to print from the lower level routine.

We do know for 2 reasons. The first is we document with schema whether a 
property is required or not. That is a contract between the firmware and 
the OS. Changing what's required breaks that contract. Second, the 
caller indicates whether the property is required or not. We already do 
this with subsystems that are indirectly accessing properties (e.g. 
clk_get() and clk_get_optional()).

But see my other reply. We are perhaps arguing about the symptoms rather 
than what is the root cause for having prints in the first place.

Rob

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Danilo Krummrich]

On Mon, Apr 28, 2025 at 03:48:40PM -0500, Rob Herring wrote:
> 
> One thing that's really hard to debug in C drivers is where an 
> error came from. You can for example turn on initcall_debug and see that 
> a driver probe returned an error. It's virtually impossible to tell 
> where that originated from. The only way to tell is with prints. That is 
> probably the root of why probe has so many error prints. I think we can 
> do a lot better with rust given Result can hold more than just an int. 

This I fully agree with, not sure if the solution is to put more stuff into the
Result type though. However, there are things like #[track_caller] (also
recently mentioned by Benno), which might be a good candidate for improving this
situation.

As mentioned, for now let's go with

	pub fn required_by(self, dev: &Device) -> Result<T>

additional to required() for this purpose to get a proper dev_err() print.

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Remo Senekowitsch]

On Mon Apr 28, 2025 at 11:21 PM CEST, Danilo Krummrich wrote:
> On Mon, Apr 28, 2025 at 03:48:40PM -0500, Rob Herring wrote:
>> 
>> One thing that's really hard to debug in C drivers is where an 
>> error came from. You can for example turn on initcall_debug and see that 
>> a driver probe returned an error. It's virtually impossible to tell 
>> where that originated from. The only way to tell is with prints. That is 
>> probably the root of why probe has so many error prints. I think we can 
>> do a lot better with rust given Result can hold more than just an int. 
>
> This I fully agree with, not sure if the solution is to put more stuff into the
> Result type though. However, there are things like #[track_caller] (also
> recently mentioned by Benno), which might be a good candidate for improving this
> situation.
>
> As mentioned, for now let's go with
>
> 	pub fn required_by(self, dev: &Device) -> Result<T>
>
> additional to required() for this purpose to get a proper dev_err() print.

Could it make sense to _replace_ `required` with `required_by` ?
Otherwise `required` sits a little awkwardly between `optional` and
`required_by`. I can't think of a situation where `required` would be
preferred.

Re: [PATCH v3 3/7] rust: property: Introduce PropertyGuard

[Danilo Krummrich]

On Mon, Apr 28, 2025 at 11:50:19PM +0200, Remo Senekowitsch wrote:
> On Mon Apr 28, 2025 at 11:21 PM CEST, Danilo Krummrich wrote:
> > On Mon, Apr 28, 2025 at 03:48:40PM -0500, Rob Herring wrote:
> >> 
> >> One thing that's really hard to debug in C drivers is where an 
> >> error came from. You can for example turn on initcall_debug and see that 
> >> a driver probe returned an error. It's virtually impossible to tell 
> >> where that originated from. The only way to tell is with prints. That is 
> >> probably the root of why probe has so many error prints. I think we can 
> >> do a lot better with rust given Result can hold more than just an int. 
> >
> > This I fully agree with, not sure if the solution is to put more stuff into the
> > Result type though. However, there are things like #[track_caller] (also
> > recently mentioned by Benno), which might be a good candidate for improving this
> > situation.
> >
> > As mentioned, for now let's go with
> >
> > 	pub fn required_by(self, dev: &Device) -> Result<T>
> >
> > additional to required() for this purpose to get a proper dev_err() print.
> 
> Could it make sense to _replace_ `required` with `required_by` ?
> Otherwise `required` sits a little awkwardly between `optional` and
> `required_by`. I can't think of a situation where `required` would be
> preferred.

Fine with me; required() also seems not useful to implement required_by().

However, I think we should revisit those generic error prints once we tackled
the issue of ergonomics in finding the source of an error in general.

Re: [PATCH v3 1/7] rust: property: Move property_present to separate file

[Dirk Behme]

On 25/04/2025 17:01, Remo Senekowitsch wrote:
> Not all property-related APIs can be exposed directly on a device.
> For example, iterating over child nodes of a device will yield
> fwnode_handle. Thus, in order to access properties on these child nodes,
> the property access methods must be implemented on the abstraction over
> fwnode_handle.
> 
> While it's possible to expose similar methods on `Device` directly for
> convenience, those methods would have to get the `FwNode` first, which
> is a fallible operation, making the API inconsistent. For this reason,
> such duplicated methods are omitted. Users who need to read properties
> of a device will have to explictily get the `FwNode` first (handle the
Typo: explictily -> explicitly

Dirk

Re: [PATCH v3 5/7] rust: property: Add child accessor and iterator

[Dirk Behme]

On 25/04/2025 17:01, Remo Senekowitsch wrote:
> Allow Rust drivers to access children of a fwnode either by name or by
> iterating over all of them.
> 
> In C, there is the function `fwnode_get_next_child_node` for iteration
> and the macro `fwnode_for_each_child_node` that helps with handling the
> pointers. Instead of a macro, a native iterator is used in Rust such
> that regular for-loops can be used.
> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/device/property.rs | 79 +++++++++++++++++++++++++++++++++-
>  1 file changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index 9505cc35d..0a0cb0c02 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -13,7 +13,7 @@
>      error::{to_result, Result},
>      prelude::*,
>      str::{CStr, CString},
> -    types::Opaque,
> +    types::{ARef, Opaque},
>  };
>  
>  impl Device {
> @@ -52,6 +52,27 @@ pub fn fwnode(&self) -> Option<&FwNode> {
>  pub struct FwNode(Opaque<bindings::fwnode_handle>);
>  
>  impl FwNode {
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that:
> +    /// - The reference count was incremented at least once.
> +    /// - They relinquish that increment. That is, if there is only one
> +    ///   increment, callers must not use the underlying object anymore -- it is
> +    ///   only safe to do so via the newly created `ARef<FwNode>`.
> +    unsafe fn from_raw(raw: *mut bindings::fwnode_handle) -> ARef<Self> {
> +        // SAFETY: As per the safety requirements of this function:
> +        // - `NonNull::new_unchecked`:
> +        //   - `raw` is not null
> +        // - `ARef::from_raw`:
> +        //   - `raw` has an incremented refcount
> +        //   - that increment is relinquished, i.e. it won't be decremented
> +        //     elsewhere.


Quite minor: There is some inconsistency on using the '.' above. The two
`raw` sentences don't have it while the last 'that increment ...' has it.


> +        // CAST: It is safe to cast from a `*mut fwnode_handle` to
> +        // `*mut FwNode`, because `FwNode` is  defined as a
> +        // `#[repr(transparent)]` wrapper around `fwnode_handle`.
> +        unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(raw.cast())) }
> +    }
> +
>      /// Obtain the raw `struct fwnode_handle *`.
>      pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
>          self.0.get()
> @@ -238,6 +259,62 @@ pub fn property_read<'fwnode, 'name, T: Property>(
>              name,
>          }
>      }
> +
> +    /// Returns first matching named child node handle.
> +    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<Self>> {
> +        // SAFETY: `self` and `name` are valid by their type invariants.
> +        let child =
> +            unsafe { bindings::fwnode_get_named_child_node(self.as_raw(), name.as_char_ptr()) };
> +        if child.is_null() {
> +            return None;
> +        }
> +        // SAFETY:
> +        // - `fwnode_get_named_child_node` returns a pointer with its refcount
> +        //   incremented.
> +        // - That increment is relinquished, i.e. the underlying object is not
> +        //   used anymore except via the newly created `ARef`.
> +        Some(unsafe { Self::from_raw(child) })
> +    }
> +
> +    /// Returns an iterator over a node's children.
> +    pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
> +        let mut prev: Option<ARef<FwNode>> = None;
> +
> +        core::iter::from_fn(move || {
> +            let prev_ptr = match prev.take() {
> +                None => ptr::null_mut(),
> +                Some(prev) => {
> +                    // We will pass `prev` to `fwnode_get_next_child_node`,
> +                    // which decrements its refcount, so we use
> +                    // `ARef::into_raw` to avoid decrementing the refcount
> +                    // twice.
> +                    let prev = ARef::into_raw(prev);
> +                    prev.as_ptr().cast()
> +                }
> +            };
> +            // SAFETY:
> +            // - `self.as_raw()` is valid by its type invariant.
> +            // - `prev_ptr` may be null, which is allowed and corresponds to
> +            //   getting the first child. Otherwise, `prev_ptr` is valid, as it
> +            //   is the stored return value from the previous invocation.
> +            // - `prev_ptr` has its refount incremented.
> +            // - The increment of `prev_ptr` is relinquished, i.e. the
> +            //   underlying object won't be unsed anymore.

Typo: unsed -> used (?)

Dirk

Re: [PATCH v3 2/7] rust: property: Enable printing fwnode name and path

[Dirk Behme]

On 25/04/2025 17:01, Remo Senekowitsch wrote:
> Add two new public methods `display_name` and `display_path` to
> `FwNode`. They can be used by driver authors for logging purposes. In
> addition, they will be used by core property abstractions for automatic
> logging, for example when a driver attempts to read a required but
> missing property.
> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/device/property.rs | 78 ++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/rust/kernel/device/property.rs b/rust/kernel/device/property.rs
> index d89715f7d..28850aa3b 100644
> --- a/rust/kernel/device/property.rs
> +++ b/rust/kernel/device/property.rs
> @@ -49,6 +49,84 @@ pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
....
> +    /// Returns an object that implements [`Display`](core::fmt::Display) for
> +    /// printing the full path of a node.
> +    pub fn display_path(&self) -> impl core::fmt::Display + '_ {
> +        struct FwNodeDisplayPath<'a>(&'a FwNode);
> +
> +        impl core::fmt::Display for FwNodeDisplayPath<'_> {
> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> +                // The logic here is the same as the one in lib/vsprintf.c
> +                // (fwnode_full_name_string).
> +
> +                let num_parents = unsafe { bindings::fwnode_count_parents(self.0.as_raw()) };

Missing a safety comment.

Dirk