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

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

[Remo Senekowitsch]

More Rust bindings for device property reads

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.

Rob Herring and Dirk Behme did several review iterations over on Zulip
already[1], I'm thankful for their great input.

This is my first time posting to the mailing list, please let me know if
I did anything wrong.

Remo

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

[PATCH 01/10] rust: Move property_present to property.rs

[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 APIs has to be duplicated on a fwnode as they are in C.

A related discussion can be found on the R4L Zulip[1].

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

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/helpers/helpers.c  |  1 +
 rust/helpers/property.c | 13 ++++++++
 rust/kernel/device.rs   |  7 ----
 rust/kernel/lib.rs      |  1 +
 rust/kernel/property.rs | 73 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 88 insertions(+), 7 deletions(-)
 create mode 100644 rust/helpers/property.c
 create mode 100644 rust/kernel/property.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..c37c74488
--- /dev/null
+++ b/rust/helpers/property.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/property.h>
+
+struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
+{
+	return dev_fwnode(dev);
+}
+
+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.rs
index db2d9658b..d5e6a19ff 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,7 +6,6 @@
 
 use crate::{
     bindings,
-    str::CStr,
     types::{ARef, Opaque},
 };
 use core::{fmt, ptr};
@@ -181,12 +180,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/lib.rs b/rust/kernel/lib.rs
index 496ed32b0..ca233fd20 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -67,6 +67,7 @@
 pub mod platform;
 pub mod prelude;
 pub mod print;
+pub mod property;
 pub mod rbtree;
 pub mod revocable;
 pub mod security;
diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
new file mode 100644
index 000000000..b0a4bb63a
--- /dev/null
+++ b/rust/kernel/property.rs
@@ -0,0 +1,73 @@
+// 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.
+    fn fwnode(&self) -> &FwNode {
+        // SAFETY: `self` is valid.
+        let fwnode_handle = unsafe { bindings::dev_fwnode(self.as_raw()) };
+        if fwnode_handle.is_null() {
+            panic!("fwnode_handle cannot be null");
+        }
+        // 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.
+        unsafe { &*fwnode_handle.cast() }
+    }
+
+    /// Checks if property is present or not.
+    pub fn property_present(&self, name: &CStr) -> bool {
+        self.fwnode().property_present(name)
+    }
+}
+
+/// 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

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:40PM +0100, 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 APIs has to be duplicated on a fwnode as they are in C.

s/has/have/

> 
> A related discussion can be found on the R4L Zulip[1].
> 
> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697

Useful below the '---', but I don't think we want to keep this link 
forever. And who knows how long it will be valid? The commit msg needs 
to stand on its own, and I think it does.

> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/helpers/helpers.c  |  1 +
>  rust/helpers/property.c | 13 ++++++++
>  rust/kernel/device.rs   |  7 ----
>  rust/kernel/lib.rs      |  1 +
>  rust/kernel/property.rs | 73 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 88 insertions(+), 7 deletions(-)
>  create mode 100644 rust/helpers/property.c
>  create mode 100644 rust/kernel/property.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..c37c74488
> --- /dev/null
> +++ b/rust/helpers/property.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/property.h>
> +
> +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> +{
> +	return dev_fwnode(dev);
> +}
> +
> +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.rs
> index db2d9658b..d5e6a19ff 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -6,7 +6,6 @@
>  
>  use crate::{
>      bindings,
> -    str::CStr,
>      types::{ARef, Opaque},
>  };
>  use core::{fmt, ptr};
> @@ -181,12 +180,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/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0..ca233fd20 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -67,6 +67,7 @@
>  pub mod platform;
>  pub mod prelude;
>  pub mod print;
> +pub mod property;
>  pub mod rbtree;
>  pub mod revocable;
>  pub mod security;
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> new file mode 100644
> index 000000000..b0a4bb63a
> --- /dev/null
> +++ b/rust/kernel/property.rs
> @@ -0,0 +1,73 @@
> +// 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.
> +    fn fwnode(&self) -> &FwNode {
> +        // SAFETY: `self` is valid.
> +        let fwnode_handle = unsafe { bindings::dev_fwnode(self.as_raw()) };
> +        if fwnode_handle.is_null() {
> +            panic!("fwnode_handle cannot be null");

It's usually not a good idea to panic the kernel especially with 
something a driver calls as that's probably recoverable.

Users/drivers testing fwnode_handle/of_node for NULL is pretty common. 
Though often that's a legacy code path, so maybe not allowing NULL is 
fine for now.

> +        }
> +        // 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.
> +        unsafe { &*fwnode_handle.cast() }
> +    }
> +
> +    /// Checks if property is present or not.
> +    pub fn property_present(&self, name: &CStr) -> bool {
> +        self.fwnode().property_present(name)
> +    }
> +}

The C developer in me wants to put this after the FwNode stuff since 
this uses it.

> +
> +/// 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
> 

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Rob Herring]

On Wed, Mar 26, 2025 at 03:51:06PM -0500, Rob Herring wrote:
> On Wed, Mar 26, 2025 at 06:13:40PM +0100, 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 APIs has to be duplicated on a fwnode as they are in C.
> 
> s/has/have/
> 
> > 
> > A related discussion can be found on the R4L Zulip[1].
> > 
> > [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697
> 
> Useful below the '---', but I don't think we want to keep this link 
> forever. And who knows how long it will be valid? The commit msg needs 
> to stand on its own, and I think it does.
> 
> > 
> > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> > ---
> >  rust/helpers/helpers.c  |  1 +
> >  rust/helpers/property.c | 13 ++++++++
> >  rust/kernel/device.rs   |  7 ----
> >  rust/kernel/lib.rs      |  1 +
> >  rust/kernel/property.rs | 73 +++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 88 insertions(+), 7 deletions(-)
> >  create mode 100644 rust/helpers/property.c
> >  create mode 100644 rust/kernel/property.rs

Also, property.rs needs to be added to MAINTAINERS. I guess it goes 
under driver core with Greg.

Rob

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Remo Senekowitsch]

On Wed Mar 26, 2025 at 9:51 PM CET, Rob Herring wrote:
> On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote:
>>
>> +impl Device {
>> +    /// Obtain the fwnode corresponding to the device.
>> +    fn fwnode(&self) -> &FwNode {
>> +        // SAFETY: `self` is valid.
>> +        let fwnode_handle = unsafe { bindings::dev_fwnode(self.as_raw()) };
>> +        if fwnode_handle.is_null() {
>> +            panic!("fwnode_handle cannot be null");
>
> It's usually not a good idea to panic the kernel especially with 
> something a driver calls as that's probably recoverable.
>
> Users/drivers testing fwnode_handle/of_node for NULL is pretty common. 
> Though often that's a legacy code path, so maybe not allowing NULL is 
> fine for now.

Just to be clear on this, should I keep this as is, or return a result?
In the latter case, all the duplicated methods on `Device` that just
call `self.fwnode().same_method()` would have a result in their function
signatur as well. That includes `property_present`, `read_property`
and `children`.

>> +        }
>> +        // 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.
>> +        unsafe { &*fwnode_handle.cast() }
>> +    }
>> +
>> +    /// Checks if property is present or not.
>> +    pub fn property_present(&self, name: &CStr) -> bool {
>> +        self.fwnode().property_present(name)
>> +    }
>> +}
>
> The C developer in me wants to put this after the FwNode stuff since 
> this uses it.

Is that just a comment or a call to action? :-)

Remo

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Andrew Ballance]

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> +        // 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.
> +        unsafe { &*fwnode_handle.cast() }


this safety comment should say why it is safe to cast from a
struct fwnode_handle* to a *const FwNode

Andrew

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Andy Shevchenko]

On Wed, Mar 26, 2025 at 06:13:40PM +0100, 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 APIs has to be duplicated on a fwnode as they are in C.
> 
> A related discussion can be found on the R4L Zulip[1].
> 
> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697

You can make the above to be a Link tag like

Link: ... [1]

> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>

...

> +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> +{
> +	return dev_fwnode(dev);
> +}

Why not const? For most of the property retrieval APIs the parameter is const.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Rob Herring]

On Thu, Mar 27, 2025 at 3:37 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Mar 26, 2025 at 06:13:40PM +0100, 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 APIs has to be duplicated on a fwnode as they are in C.
> >
> > A related discussion can be found on the R4L Zulip[1].
> >
> > [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697
>
> You can make the above to be a Link tag like
>
> Link: ... [1]
>
> > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>
> ...
>
> > +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> > +{
> > +     return dev_fwnode(dev);
> > +}
>
> Why not const? For most of the property retrieval APIs the parameter is const.

Because you might need to modify the refcount.

Though the rust code should probably use __dev_fwnode() and/or
__dev_fwnode_const() directly and avoid the need for the helper here.

Rob

Re: [PATCH 01/10] rust: Move property_present to property.rs

[Andy Shevchenko]

On Thu, Mar 27, 2025 at 08:55:42AM -0500, Rob Herring wrote:
> On Thu, Mar 27, 2025 at 3:37 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote:

...

> > > +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> > > +{
> > > +     return dev_fwnode(dev);
> > > +}
> >
> > Why not const? For most of the property retrieval APIs the parameter is const.
> 
> Because you might need to modify the refcount.
> 
> Though the rust code should probably use __dev_fwnode() and/or
> __dev_fwnode_const() directly and avoid the need for the helper here.

Indeed, that would help at least to understand the intention.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 02/10] rust: Add an Integer trait

[Remo Senekowitsch]

From: "Rob Herring (Arm)" <robh@kernel.org>

Add an "Integer" trait similar to crate::num::Integer. This is useful
for implementing generic methods which operate on different sizes of
integers. One example is reading DT/ACPI firmware properties.

This was originally proposed by Alice Ryhl[1].

[1] https://lore.kernel.org/rust-for-linux/CAH5fLgiXPZqKpWSSNdx-Ww-E9h2tOLcF3_8Y4C_JQ0eU8EMwFw@mail.gmail.com/

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 rust/kernel/types.rs | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 2bbaab83b..21647b7ba 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -3,10 +3,11 @@
 //! Kernel types.
 
 use crate::init::{self, PinInit};
+use crate::transmute::{AsBytes, FromBytes};
 use core::{
     cell::UnsafeCell,
     marker::{PhantomData, PhantomPinned},
-    mem::{ManuallyDrop, MaybeUninit},
+    mem::{size_of, ManuallyDrop, MaybeUninit},
     ops::{Deref, DerefMut},
     ptr::NonNull,
 };
@@ -553,6 +554,25 @@ pub enum Either<L, R> {
     Right(R),
 }
 
+/// Trait defined for all integer types similar to `crate::num::Integer`
+pub trait Integer: FromBytes + AsBytes + Copy {
+    /// Size of the integer in bytes
+    const SIZE: usize;
+}
+
+macro_rules! impl_int {
+    ($($typ:ty),* $(,)?) => {$(
+        impl Integer for $typ {
+            const SIZE: usize = size_of::<Self>();
+        }
+    )*};
+}
+
+impl_int! {
+    u8, u16, u32, u64, usize,
+    i8, i16, i32, i64, isize,
+}
+
 /// Zero-sized type to mark types not [`Send`].
 ///
 /// Add this type as a field to your struct if your type should not be sent to a different task.
-- 
2.49.0

Re: [PATCH 02/10] rust: Add an Integer trait

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:41PM +0100, Remo Senekowitsch wrote:
> From: "Rob Herring (Arm)" <robh@kernel.org>
> 
> Add an "Integer" trait similar to crate::num::Integer. This is useful
> for implementing generic methods which operate on different sizes of
> integers. One example is reading DT/ACPI firmware properties.
> 
> This was originally proposed by Alice Ryhl[1].
> 
> [1] https://lore.kernel.org/rust-for-linux/CAH5fLgiXPZqKpWSSNdx-Ww-E9h2tOLcF3_8Y4C_JQ0eU8EMwFw@mail.gmail.com/
> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

This needs your Signed-off-by too because you are sending it.

> ---
>  rust/kernel/types.rs | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 2bbaab83b..21647b7ba 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -3,10 +3,11 @@
>  //! Kernel types.
>  
>  use crate::init::{self, PinInit};
> +use crate::transmute::{AsBytes, FromBytes};
>  use core::{
>      cell::UnsafeCell,
>      marker::{PhantomData, PhantomPinned},
> -    mem::{ManuallyDrop, MaybeUninit},
> +    mem::{size_of, ManuallyDrop, MaybeUninit},
>      ops::{Deref, DerefMut},
>      ptr::NonNull,
>  };
> @@ -553,6 +554,25 @@ pub enum Either<L, R> {
>      Right(R),
>  }
>  
> +/// Trait defined for all integer types similar to `crate::num::Integer`
> +pub trait Integer: FromBytes + AsBytes + Copy {
> +    /// Size of the integer in bytes
> +    const SIZE: usize;
> +}
> +
> +macro_rules! impl_int {
> +    ($($typ:ty),* $(,)?) => {$(
> +        impl Integer for $typ {
> +            const SIZE: usize = size_of::<Self>();
> +        }
> +    )*};
> +}
> +
> +impl_int! {
> +    u8, u16, u32, u64, usize,
> +    i8, i16, i32, i64, isize,
> +}
> +
>  /// Zero-sized type to mark types not [`Send`].
>  ///
>  /// Add this type as a field to your struct if your type should not be sent to a different task.
> -- 
> 2.49.0
> 

[PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Remo Senekowitsch]

The rust bindings for reading device properties has a single
implementation supporting differing sizes of integers. The fwnode C API
already has a similar interface, but it is not exposed with the
fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.

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/base/property.c  | 9 +++++----
 include/linux/property.h | 2 ++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c1392743d..64d5b7055 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -292,10 +292,10 @@ int device_property_match_string(const struct device *dev, const char *propname,
 }
 EXPORT_SYMBOL_GPL(device_property_match_string);
 
-static int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
-					  const char *propname,
-					  unsigned int elem_size, void *val,
-					  size_t nval)
+int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
+				   const char *propname,
+				   unsigned int elem_size, void *val,
+				   size_t nval)
 {
 	int ret;
 
@@ -310,6 +310,7 @@ static int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
 	return fwnode_call_int_op(fwnode->secondary, property_read_int_array, propname,
 				  elem_size, val, nval);
 }
+EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
 
 /**
  * fwnode_property_read_u8_array - return a u8 array property of firmware node
diff --git a/include/linux/property.h b/include/linux/property.h
index e214ecd24..441a1ad76 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -57,6 +57,8 @@ bool fwnode_property_present(const struct fwnode_handle *fwnode,
 			     const char *propname);
 bool fwnode_property_read_bool(const struct fwnode_handle *fwnode,
 			     const char *propname);
+int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
+				   unsigned int elem_size, void *val, size_t nval);
 int fwnode_property_read_u8_array(const struct fwnode_handle *fwnode,
 				  const char *propname, u8 *val,
 				  size_t nval);
-- 
2.49.0

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> The rust bindings for reading device properties has a single
> implementation supporting differing sizes of integers. The fwnode C API
> already has a similar interface, but it is not exposed with the
> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.

...

> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);

I'm not sure about this. We have a lot of assumptions in the code that the
arrays beneath are only represented by the selected number of integer types.
This opens a Pandora's box, e.g., reading in u24, which is not supported by
the upper layers..

> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
> +				   unsigned int elem_size, void *val, size_t nval);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Remo Senekowitsch]

On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
> On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
>> The rust bindings for reading device properties has a single
>> implementation supporting differing sizes of integers. The fwnode C API
>> already has a similar interface, but it is not exposed with the
>> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>
> ...
>
>> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
>
> I'm not sure about this. We have a lot of assumptions in the code that the
> arrays beneath are only represented by the selected number of integer types.
> This opens a Pandora's box, e.g., reading in u24, which is not supported by
> the upper layers..
>
>> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
>> +				   unsigned int elem_size, void *val, size_t nval);

Here's an alternative approach using a macro to map each integer type explicitly
to its corresponding read function. There are some additional changes that will
be necessary to make the rest work, but this is the gist of it.

+macro_rules! impl_property_for_int {
+    ($($int:ty: $f:ident),* $(,)?) => {
+        $(
+            impl<const N: usize> Property for [$int; N] {
+                fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+                    let mut val: [MaybeUninit<$int>; N] = [const { MaybeUninit::uninit() }; N];
+
+                    // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is valid
+                    // because `fwnode` is valid. `val.as_ptr` is valid because `val` is valid.
+                    let ret = unsafe {
+                        bindings::$f(
+                            fwnode.as_raw(),
+                            name.as_char_ptr(),
+                            val.as_mut_ptr().cast(),
+                            val.len(),
+                        )
+                    };
+                    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 Property for $int {
+                fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+                    let val: [_; 1] = <[$int; 1] as Property>::read(fwnode, name)?;
+                    Ok(val[0])
+                }
+            }
+        )*
+    };
+}
+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,
+}

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
> On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
> > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> >> The rust bindings for reading device properties has a single
> >> implementation supporting differing sizes of integers. The fwnode C API
> >> already has a similar interface, but it is not exposed with the
> >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.

...

> >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> >
> > I'm not sure about this. We have a lot of assumptions in the code that the
> > arrays beneath are only represented by the selected number of integer types.
> > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > the upper layers..
> >
> >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
> >> +				   unsigned int elem_size, void *val, size_t nval);
> 
> Here's an alternative approach using a macro to map each integer type explicitly
> to its corresponding read function. There are some additional changes that will
> be necessary to make the rest work, but this is the gist of it.

I don;'t know Rust to tell anything about this, but at least it feels much
better approach.

> +macro_rules! impl_property_for_int {
> +    ($($int:ty: $f:ident),* $(,)?) => {
> +        $(
> +            impl<const N: usize> Property for [$int; N] {
> +                fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> +                    let mut val: [MaybeUninit<$int>; N] = [const { MaybeUninit::uninit() }; N];
> +
> +                    // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is valid
> +                    // because `fwnode` is valid. `val.as_ptr` is valid because `val` is valid.
> +                    let ret = unsafe {
> +                        bindings::$f(
> +                            fwnode.as_raw(),
> +                            name.as_char_ptr(),
> +                            val.as_mut_ptr().cast(),
> +                            val.len(),
> +                        )
> +                    };
> +                    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 Property for $int {
> +                fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> +                    let val: [_; 1] = <[$int; 1] as Property>::read(fwnode, name)?;
> +                    Ok(val[0])
> +                }
> +            }
> +        )*
> +    };
> +}
> +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,

Dunno what these mean. Are they signed integers? Do you really need this kind
of types to be supported? Properties usually unsigned and I never saw (but it
doesn't mean they are not exist) the negative values there.  Does DT schema
even allow this? Does DT validator complain or not on the negative values
supplied in the DTS?

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Thu, Apr 3, 2025 at 8:29 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
> > On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > >> The rust bindings for reading device properties has a single
> > >> implementation supporting differing sizes of integers. The fwnode C API
> > >> already has a similar interface, but it is not exposed with the
> > >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>
> ...
>
> > >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> > >
> > > I'm not sure about this. We have a lot of assumptions in the code that the
> > > arrays beneath are only represented by the selected number of integer types.
> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > > the upper layers..
> > >
> > >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
> > >> +                             unsigned int elem_size, void *val, size_t nval);
> >
> > Here's an alternative approach using a macro to map each integer type explicitly
> > to its corresponding read function. There are some additional changes that will
> > be necessary to make the rest work, but this is the gist of it.
>
> I don;'t know Rust to tell anything about this, but at least it feels much
> better approach.

I know a little Rust and it is much worse. It is implementing the same
code 8 times instead of 1 time just to work-around the C API.

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Remo Senekowitsch]

On Thu Apr 3, 2025 at 6:08 PM CEST, Rob Herring wrote:
> On Thu, Apr 3, 2025 at 8:29 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
>> > On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
>> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
>> > >> The rust bindings for reading device properties has a single
>> > >> implementation supporting differing sizes of integers. The fwnode C API
>> > >> already has a similar interface, but it is not exposed with the
>> > >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>>
>> ...
>>
>> > >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
>> > >
>> > > I'm not sure about this. We have a lot of assumptions in the code that the
>> > > arrays beneath are only represented by the selected number of integer types.
>> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
>> > > the upper layers..
>> > >
>> > >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
>> > >> +                             unsigned int elem_size, void *val, size_t nval);
>> >
>> > Here's an alternative approach using a macro to map each integer type explicitly
>> > to its corresponding read function. There are some additional changes that will
>> > be necessary to make the rest work, but this is the gist of it.
>>
>> I don;'t know Rust to tell anything about this, but at least it feels much
>> better approach.
>
> I know a little Rust and it is much worse. It is implementing the same
> code 8 times instead of 1 time just to work-around the C API.

You mean it's worse because it will generate too much code during compile time?

Remo

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Remo Senekowitsch]

On Thu Apr 3, 2025 at 6:08 PM CEST, Rob Herring wrote:
> On Thu, Apr 3, 2025 at 8:29 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
>> > On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
>> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
>> > >> The rust bindings for reading device properties has a single
>> > >> implementation supporting differing sizes of integers. The fwnode C API
>> > >> already has a similar interface, but it is not exposed with the
>> > >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>>
>> ...
>>
>> > >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
>> > >
>> > > I'm not sure about this. We have a lot of assumptions in the code that the
>> > > arrays beneath are only represented by the selected number of integer types.
>> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
>> > > the upper layers..
>> > >
>> > >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
>> > >> +                             unsigned int elem_size, void *val, size_t nval);
>> >
>> > Here's an alternative approach using a macro to map each integer type explicitly
>> > to its corresponding read function. There are some additional changes that will
>> > be necessary to make the rest work, but this is the gist of it.
>>
>> I don;'t know Rust to tell anything about this, but at least it feels much
>> better approach.
>
> I know a little Rust and it is much worse. It is implementing the same
> code 8 times instead of 1 time just to work-around the C API.

I prepared a functioning version of the macro-based approach. I'll post
the patch for reference and discussion. We don't have to go with it.

Remo

---
 rust/kernel/property.rs | 124 +++++++++++++++++++++++++---------------
 1 file changed, 77 insertions(+), 47 deletions(-)

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index ceedd1a82..78dcc189e 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -4,7 +4,7 @@
 //!
 //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
 
-use core::{ffi::c_void, mem::MaybeUninit, ptr};
+use core::{mem::MaybeUninit, ptr};
 
 use crate::{
     alloc::KVec,
@@ -13,7 +13,7 @@
     error::{to_result, Result},
     prelude::*,
     str::{CStr, CString},
-    types::{ARef, Integer, Opaque},
+    types::{ARef, Opaque},
 };
 
 impl Device {
@@ -43,7 +43,7 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
     }
 
     /// Returns firmware property `name` integer array values in a KVec
-    pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
+    pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
         &'fwnode self,
         name: &'name CStr,
         len: usize,
@@ -52,7 +52,7 @@ pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
     }
 
     /// Returns integer array length for firmware property `name`
-    pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
+    pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
         self.fwnode().property_count_elem::<T>(name)
     }
 
@@ -148,24 +148,17 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
     }
 
     /// Returns firmware property `name` integer array values in a KVec
-    pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
+    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: `name` is non-null and null-terminated. `self.as_raw` is valid
-        // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
-        let err = unsafe {
-            bindings::fwnode_property_read_int_array(
-                self.as_raw(),
-                name.as_char_ptr(),
-                T::SIZE as u32,
-                val.as_ptr() as *mut c_void,
-                len,
-            )
-        };
+        // 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 { T::read_array_out_param(self, name, val.as_mut_ptr(), len) };
         let res = if err < 0 {
             Err(Error::from_errno(err))
         } else {
@@ -181,19 +174,11 @@ pub fn property_read_array_vec<'fwnode, 'name, T: Integer>(
     }
 
     /// Returns integer array length for firmware property `name`
-    pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
+    pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
         // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
         // because `self` is valid. Passing null pointer buffer is valid to obtain
         // the number of elements in the property array.
-        let ret = unsafe {
-            bindings::fwnode_property_read_int_array(
-                self.as_raw(),
-                name.as_char_ptr(),
-                T::SIZE as u32,
-                ptr::null_mut(),
-                0,
-            )
-        };
+        let ret = unsafe { T::read_array_out_param(self, name, ptr::null_mut(), 0) };
         to_result(ret)?;
         Ok(ret as usize)
     }
@@ -201,8 +186,8 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<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, integers and arrays
-    /// of integers.
+    /// 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
@@ -300,6 +285,9 @@ pub fn property_get_reference_args(
             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(),
@@ -388,34 +376,76 @@ fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
         Ok(str.try_into()?)
     }
 }
-impl<T: Integer, const N: usize> Property for [T; N] {
+/// Implemented for all integers that can be read as properties.
+///
+/// This helper trait is needed to associate the integer types of various sizes
+/// with their corresponding `fwnode_property_read_*_array` functions.
+pub trait PropertyInt: Property {
+    /// # 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(
+        fwnode: &FwNode,
+        name: &CStr,
+        out_param: *mut Self,
+        len: 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 Property for $int {
+            fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+                let val: [_; 1] = <[$int; 1] as Property>::read(fwnode, name)?;
+                Ok(val[0])
+            }
+        }
+        impl PropertyInt for $int {
+            unsafe fn read_array_out_param(
+                fwnode: &FwNode,
+                name: &CStr,
+                out_param: *mut Self,
+                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 {
+                    bindings::$f(fwnode.as_raw(), name.as_char_ptr(), out_param, len)
+                }
+            }
+        }
+    )* };
+}
+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,
+}
+impl<T: PropertyInt, const N: usize> Property for [T; N] {
     fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
         let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
 
-        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is valid
-        // because `fwnode` is valid. `val.as_ptr` is valid because `val` is valid.
-        let ret = unsafe {
-            bindings::fwnode_property_read_int_array(
-                fwnode.as_raw(),
-                name.as_char_ptr(),
-                T::SIZE as u32,
-                val.as_mut_ptr().cast(),
-                val.len(),
-            )
-        };
+        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
+        // valid because `fwnode` is valid. `val.as_ptr` is valid because `val`
+        // is valid. Casting from `*mut MaybeUninit<T>` to `*mut T` is safe
+        // because `MaybeUninit<T>` has the same memory layout as `T`.
+        let ret = unsafe { T::read_array_out_param(fwnode, name, val.as_mut_ptr().cast(), N) };
         to_result(ret)?;
 
         // SAFETY: `val` is always initialized when
-        // fwnode_property_read_int_array is successful.
+        // fwnode_property_read_$t_array is successful.
         Ok(val.map(|v| unsafe { v.assume_init() }))
     }
 }
-impl<T: Integer> Property for T {
-    fn read(fwnode: &FwNode, name: &CStr) -> Result<T> {
-        let val: [_; 1] = <[T; 1] as Property>::read(fwnode, name)?;
-        Ok(val[0])
-    }
-}
 
 /// The number of arguments of a reference.
 pub enum NArgs<'a> {
-- 
2.49.0

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Thu, Apr 3, 2025 at 12:04 PM Remo Senekowitsch <remo@buenzli.dev> wrote:
>
> On Thu Apr 3, 2025 at 6:08 PM CEST, Rob Herring wrote:
> > On Thu, Apr 3, 2025 at 8:29 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> >>
> >> On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
> >> > On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
> >> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> >> > >> The rust bindings for reading device properties has a single
> >> > >> implementation supporting differing sizes of integers. The fwnode C API
> >> > >> already has a similar interface, but it is not exposed with the
> >> > >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
> >>
> >> ...
> >>
> >> > >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> >> > >
> >> > > I'm not sure about this. We have a lot of assumptions in the code that the
> >> > > arrays beneath are only represented by the selected number of integer types.
> >> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> >> > > the upper layers..
> >> > >
> >> > >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
> >> > >> +                             unsigned int elem_size, void *val, size_t nval);
> >> >
> >> > Here's an alternative approach using a macro to map each integer type explicitly
> >> > to its corresponding read function. There are some additional changes that will
> >> > be necessary to make the rest work, but this is the gist of it.
> >>
> >> I don;'t know Rust to tell anything about this, but at least it feels much
> >> better approach.
> >
> > I know a little Rust and it is much worse. It is implementing the same
> > code 8 times instead of 1 time just to work-around the C API.
>
> I prepared a functioning version of the macro-based approach. I'll post
> the patch for reference and discussion. We don't have to go with it.

Please wait for the discussion to finish.

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Remo Senekowitsch]

On Thu Apr 3, 2025 at 6:08 PM CEST, Rob Herring wrote:
> On Thu, Apr 3, 2025 at 8:29 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> On Wed, Apr 02, 2025 at 06:04:13PM +0200, Remo Senekowitsch wrote:
>> > On Thu Mar 27, 2025 at 9:41 AM CET, Andy Shevchenko wrote:
>> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
>> > >> The rust bindings for reading device properties has a single
>> > >> implementation supporting differing sizes of integers. The fwnode C API
>> > >> already has a similar interface, but it is not exposed with the
>> > >> fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>>
>> ...
>>
>> > >> +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
>> > >
>> > > I'm not sure about this. We have a lot of assumptions in the code that the
>> > > arrays beneath are only represented by the selected number of integer types.
>> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
>> > > the upper layers..
>> > >
>> > >> +int fwnode_property_read_int_array(const struct fwnode_handle *fwnode, const char *propname,
>> > >> +                             unsigned int elem_size, void *val, size_t nval);
>> >
>> > Here's an alternative approach using a macro to map each integer type explicitly
>> > to its corresponding read function. There are some additional changes that will
>> > be necessary to make the rest work, but this is the gist of it.
>>
>> I don;'t know Rust to tell anything about this, but at least it feels much
>> better approach.
>
> I know a little Rust and it is much worse. It is implementing the same
> code 8 times instead of 1 time just to work-around the C API.

Can you please elaborate on this concern?

The previous approach uses generics, which means the function is also
"copy-pasted" for each concrete type it is used with at compile time,
just like with a macro. So using a macro wouldn't be worse than generics
if binary size is the concern.

If the concern is about readability, I have managed to move all logic
outside the macro body, all that remains is the association between type
and `*_read_array` function.

Remo

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Thu, Mar 27, 2025 at 3:41 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > The rust bindings for reading device properties has a single
> > implementation supporting differing sizes of integers. The fwnode C API
> > already has a similar interface, but it is not exposed with the
> > fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>
> ...
>
> > +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
>
> I'm not sure about this. We have a lot of assumptions in the code that the
> arrays beneath are only represented by the selected number of integer types.
> This opens a Pandora's box, e.g., reading in u24, which is not supported by
> the upper layers..

We can probably drop the export if it is just that which you object to.

Without this function, the implementation is just multiple levels of
switch statements to work-around what's needed for the nicest rust
API.

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 11:04:32AM -0500, Rob Herring wrote:
> On Thu, Mar 27, 2025 at 3:41 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > > The rust bindings for reading device properties has a single
> > > implementation supporting differing sizes of integers. The fwnode C API
> > > already has a similar interface, but it is not exposed with the
> > > fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.

...

> > > +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> >
> > I'm not sure about this. We have a lot of assumptions in the code that the
> > arrays beneath are only represented by the selected number of integer types.
> > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > the upper layers..
> 
> We can probably drop the export if it is just that which you object to.

Yes, this is main point, but dropping it does not prevent from still using in
the complied-in code. Is it possible to hide it better?

> Without this function, the implementation is just multiple levels of
> switch statements to work-around what's needed for the nicest rust
> API.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Thu, Apr 3, 2025 at 11:15 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Apr 03, 2025 at 11:04:32AM -0500, Rob Herring wrote:
> > On Thu, Mar 27, 2025 at 3:41 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > > > The rust bindings for reading device properties has a single
> > > > implementation supporting differing sizes of integers. The fwnode C API
> > > > already has a similar interface, but it is not exposed with the
> > > > fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>
> ...
>
> > > > +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> > >
> > > I'm not sure about this. We have a lot of assumptions in the code that the
> > > arrays beneath are only represented by the selected number of integer types.
> > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > > the upper layers..
> >
> > We can probably drop the export if it is just that which you object to.
>
> Yes, this is main point, but dropping it does not prevent from still using in
> the complied-in code. Is it possible to hide it better?

Don't put any declaration in the header and declare it in the rust
code? But lack of declaration generates warnings.

Also, all the backends will reject an arbitrary size. So your worry
about u24 or other odd sizes isn't really valid. But if you want to be
doubly paranoid for when we add a new firmware backend (shoot me now),
you could move this from the swnode implementation to the fwnode
implementation:

        if (!is_power_of_2(elem_size) || elem_size > sizeof(u64))
                return -ENXIO;

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 11:36:38AM -0500, Rob Herring wrote:
> On Thu, Apr 3, 2025 at 11:15 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Apr 03, 2025 at 11:04:32AM -0500, Rob Herring wrote:
> > > On Thu, Mar 27, 2025 at 3:41 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > > > > The rust bindings for reading device properties has a single
> > > > > implementation supporting differing sizes of integers. The fwnode C API
> > > > > already has a similar interface, but it is not exposed with the
> > > > > fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.

...

> > > > > +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> > > >
> > > > I'm not sure about this. We have a lot of assumptions in the code that the
> > > > arrays beneath are only represented by the selected number of integer types.
> > > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > > > the upper layers..
> > >
> > > We can probably drop the export if it is just that which you object to.
> >
> > Yes, this is main point, but dropping it does not prevent from still using in
> > the complied-in code. Is it possible to hide it better?
> 
> Don't put any declaration in the header and declare it in the rust
> code? But lack of declaration generates warnings.

Exactly. And I believe we have the typed versions of int_array for a reason.
Otherwise what's the point in having them to begin with?
(The first what comes to my mind is a compile time type checking, so we don't
 try to load u8 with u32 data or any other dirty tricks.)

Maybe it deserves a header that can be included explicitly in the rust stuff
and being checked at compile time to avoid people using that? Can we achieve
something like C preprocessor

#ifndef FOO
#error This header must not be included directly
#endif

> Also, all the backends will reject an arbitrary size. So your worry
> about u24 or other odd sizes isn't really valid. But if you want to be
> doubly paranoid for when we add a new firmware backend (shoot me now),
> you could move this from the swnode implementation to the fwnode
> implementation:
> 
>         if (!is_power_of_2(elem_size) || elem_size > sizeof(u64))
>                 return -ENXIO;

That might work. But still an interface of int_array seems lower by
level than typed ones.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Thu, Apr 3, 2025 at 12:55 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Apr 03, 2025 at 11:36:38AM -0500, Rob Herring wrote:
> > On Thu, Apr 3, 2025 at 11:15 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Apr 03, 2025 at 11:04:32AM -0500, Rob Herring wrote:
> > > > On Thu, Mar 27, 2025 at 3:41 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Wed, Mar 26, 2025 at 06:13:42PM +0100, Remo Senekowitsch wrote:
> > > > > > The rust bindings for reading device properties has a single
> > > > > > implementation supporting differing sizes of integers. The fwnode C API
> > > > > > already has a similar interface, but it is not exposed with the
> > > > > > fwnode_property_ API. Add the fwnode_property_read_int_array() wrapper.
>
> ...
>
> > > > > > +EXPORT_SYMBOL_GPL(fwnode_property_read_int_array);
> > > > >
> > > > > I'm not sure about this. We have a lot of assumptions in the code that the
> > > > > arrays beneath are only represented by the selected number of integer types.
> > > > > This opens a Pandora's box, e.g., reading in u24, which is not supported by
> > > > > the upper layers..
> > > >
> > > > We can probably drop the export if it is just that which you object to.
> > >
> > > Yes, this is main point, but dropping it does not prevent from still using in
> > > the complied-in code. Is it possible to hide it better?
> >
> > Don't put any declaration in the header and declare it in the rust
> > code? But lack of declaration generates warnings.
>
> Exactly. And I believe we have the typed versions of int_array for a reason.
> Otherwise what's the point in having them to begin with?
> (The first what comes to my mind is a compile time type checking, so we don't
>  try to load u8 with u32 data or any other dirty tricks.)
>
> Maybe it deserves a header that can be included explicitly in the rust stuff
> and being checked at compile time to avoid people using that? Can we achieve
> something like C preprocessor
>
> #ifndef FOO
> #error This header must not be included directly
> #endif

Looks there are RUST_BINDGEN or __BINDGEN__ define which could be used
here. Don't need a header, just wrap the declaration. No idea which
one would be preferred as there's exactly 1 use of each. Miguel?

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Miguel Ojeda]

On Thu, Apr 3, 2025 at 8:48 PM Rob Herring <robh@kernel.org> wrote:
>
> Looks there are RUST_BINDGEN or __BINDGEN__ define which could be used
> here. Don't need a header, just wrap the declaration. No idea which
> one would be preferred as there's exactly 1 use of each. Miguel?

If you mean bcachefs' `RUST_BINDGEN`, then I think that one comes from
bcachefs-tools, i.e. we don't define it.

Cheers,
Miguel

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Thu, Apr 03, 2025 at 10:36:40PM +0200, Miguel Ojeda wrote:
> On Thu, Apr 3, 2025 at 8:48 PM Rob Herring <robh@kernel.org> wrote:
> >
> > Looks there are RUST_BINDGEN or __BINDGEN__ define which could be used
> > here. Don't need a header, just wrap the declaration. No idea which
> > one would be preferred as there's exactly 1 use of each. Miguel?
> 
> If you mean bcachefs' `RUST_BINDGEN`, then I think that one comes from
> bcachefs-tools, i.e. we don't define it.

But is there a way to have a function that is not really exported to be used in
Rust only code?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Rob Herring]

On Fri, Apr 4, 2025 at 6:00 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Apr 03, 2025 at 10:36:40PM +0200, Miguel Ojeda wrote:
> > On Thu, Apr 3, 2025 at 8:48 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > Looks there are RUST_BINDGEN or __BINDGEN__ define which could be used
> > > here. Don't need a header, just wrap the declaration. No idea which
> > > one would be preferred as there's exactly 1 use of each. Miguel?
> >
> > If you mean bcachefs' `RUST_BINDGEN`, then I think that one comes from
> > bcachefs-tools, i.e. we don't define it.
>
> But is there a way to have a function that is not really exported to be used in
> Rust only code?

#ifdef __BINDGEN__
int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
const char *propname,
                                unsigned int elem_size, void *val, size_t nval);
#endif

Rob

Re: [PATCH 03/10] device property: Add fwnode_property_read_int_array()

[Andy Shevchenko]

On Fri, Apr 04, 2025 at 09:12:57AM -0500, Rob Herring wrote:
> On Fri, Apr 4, 2025 at 6:00 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Apr 03, 2025 at 10:36:40PM +0200, Miguel Ojeda wrote:
> > > On Thu, Apr 3, 2025 at 8:48 PM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > Looks there are RUST_BINDGEN or __BINDGEN__ define which could be used
> > > > here. Don't need a header, just wrap the declaration. No idea which
> > > > one would be preferred as there's exactly 1 use of each. Miguel?
> > >
> > > If you mean bcachefs' `RUST_BINDGEN`, then I think that one comes from
> > > bcachefs-tools, i.e. we don't define it.
> >
> > But is there a way to have a function that is not really exported to be used in
> > Rust only code?
> 
> #ifdef __BINDGEN__
> int fwnode_property_read_int_array(const struct fwnode_handle *fwnode,
> const char *propname,
>                                 unsigned int elem_size, void *val, size_t nval);
> #endif

Okay. This can be working compromise. Meanwhile I leave the discussion go on
how to proceed the best on the Rust code side. It seems to me if the macro
approach is indeed looks nice (let's say comparatively nice as this piece in C
header) I would prefer that we do not export it at all.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 04/10] rust: 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
sizes of properties (e.g. u8, u16, u32).

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/property.rs | 153 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 151 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index b0a4bb63a..4756ea766 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -4,9 +4,17 @@
 //!
 //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
 
-use core::ptr;
+use core::{ffi::c_void, mem::MaybeUninit, ptr};
 
-use crate::{bindings, device::Device, str::CStr, types::Opaque};
+use crate::{
+    alloc::KVec,
+    bindings,
+    device::Device,
+    error::{to_result, Result},
+    prelude::*,
+    str::{CStr, CString},
+    types::{Integer, Opaque},
+};
 
 impl Device {
     /// Obtain the fwnode corresponding to the device.
@@ -26,6 +34,41 @@ fn fwnode(&self) -> &FwNode {
     pub fn property_present(&self, name: &CStr) -> bool {
         self.fwnode().property_present(name)
     }
+
+    /// Returns if a firmware property `name` is true or false
+    pub fn property_read_bool(&self, name: &CStr) -> bool {
+        self.fwnode().property_read_bool(name)
+    }
+
+    /// Returns the index of matching string `match_str` for firmware string property `name`
+    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
+        self.fwnode().property_read_string(name)
+    }
+
+    /// Returns the index of matching string `match_str` for firmware string property `name`
+    pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
+        self.fwnode().property_match_string(name, match_str)
+    }
+
+    /// Returns firmware property `name` integer scalar value
+    pub fn property_read<T: Integer>(&self, name: &CStr) -> Result<T> {
+        self.fwnode().property_read(name)
+    }
+
+    /// Returns firmware property `name` integer array values
+    pub fn property_read_array<T: Integer, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
+        self.fwnode().property_read_array(name)
+    }
+
+    /// Returns firmware property `name` integer array values in a KVec
+    pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
+        self.fwnode().property_read_array_vec(name, len)
+    }
+
+    /// Returns integer array length for firmware property `name`
+    pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
+        self.fwnode().property_count_elem::<T>(name)
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -57,6 +100,112 @@ pub fn property_present(&self, name: &CStr) -> bool {
         // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
         unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
     }
+
+    /// Returns if a firmware property `name` is true or false
+    pub fn property_read_bool(&self, name: &CStr) -> bool {
+        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
+        // because `self` is valid.
+        unsafe { bindings::fwnode_property_read_bool(self.as_raw(), name.as_char_ptr()) }
+    }
+
+    /// Returns the index of matching string `match_str` for firmware string property `name`
+    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
+        let mut str = core::ptr::null_mut();
+        let pstr: *mut _ = &mut str;
+
+        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is
+        // valid because `self` is valid.
+        let ret = unsafe {
+            bindings::fwnode_property_read_string(self.as_raw(), name.as_char_ptr(), pstr as _)
+        };
+        to_result(ret)?;
+
+        // SAFETY: `pstr` contains a non-null ptr on success
+        let str = unsafe { CStr::from_char_ptr(*pstr) };
+        Ok(str.try_into()?)
+    }
+
+    /// 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 scalar value
+    pub fn property_read<T: Integer>(&self, name: &CStr) -> Result<T> {
+        let val: [_; 1] = Self::property_read_array(self, name)?;
+        Ok(val[0])
+    }
+
+    /// Returns firmware property `name` integer array values
+    pub fn property_read_array<T: Integer, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
+        let val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
+
+        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
+        // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
+        let ret = unsafe {
+            bindings::fwnode_property_read_int_array(
+                self.as_raw(),
+                name.as_char_ptr(),
+                T::SIZE as u32,
+                val.as_ptr() as *mut c_void,
+                val.len(),
+            )
+        };
+        to_result(ret)?;
+
+        // SAFETY: `val` is always initialized when
+        // fwnode_property_read_int_array is successful.
+        Ok(val.map(|v| unsafe { v.assume_init() }))
+    }
+
+    /// Returns firmware property `name` integer array values in a KVec
+    pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
+        let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
+
+        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
+        // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
+        to_result(unsafe {
+            bindings::fwnode_property_read_int_array(
+                self.as_raw(),
+                name.as_char_ptr(),
+                T::SIZE as u32,
+                val.as_ptr() as *mut c_void,
+                len,
+            )
+        })?;
+
+        // SAFETY: fwnode_property_read_int_array() writes exactly `len` entries on success
+        unsafe { val.set_len(len) }
+        Ok(val)
+    }
+
+    /// Returns integer array length for firmware property `name`
+    pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
+        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
+        // because `self` is valid. Passing null pointer buffer is valid to obtain
+        // the number of elements in the property array.
+        let ret = unsafe {
+            bindings::fwnode_property_read_int_array(
+                self.as_raw(),
+                name.as_char_ptr(),
+                T::SIZE as u32,
+                ptr::null_mut(),
+                0,
+            )
+        };
+        to_result(ret)?;
+        Ok(ret as usize)
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
-- 
2.49.0

Re: [PATCH 04/10] rust: Add bindings for reading device properties

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:43PM +0100, Remo Senekowitsch wrote:
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
> 
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> sizes of properties (e.g. u8, u16, u32).
> 
> Co-developed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/property.rs | 153 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 151 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index b0a4bb63a..4756ea766 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -4,9 +4,17 @@
>  //!
>  //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
>  
> -use core::ptr;
> +use core::{ffi::c_void, mem::MaybeUninit, ptr};
>  
> -use crate::{bindings, device::Device, str::CStr, types::Opaque};
> +use crate::{
> +    alloc::KVec,
> +    bindings,
> +    device::Device,
> +    error::{to_result, Result},
> +    prelude::*,
> +    str::{CStr, CString},
> +    types::{Integer, Opaque},
> +};
>  
>  impl Device {
>      /// Obtain the fwnode corresponding to the device.
> @@ -26,6 +34,41 @@ fn fwnode(&self) -> &FwNode {
>      pub fn property_present(&self, name: &CStr) -> bool {
>          self.fwnode().property_present(name)
>      }
> +
> +    /// Returns if a firmware property `name` is true or false
> +    pub fn property_read_bool(&self, name: &CStr) -> bool {
> +        self.fwnode().property_read_bool(name)
> +    }
> +
> +    /// Returns the index of matching string `match_str` for firmware string property `name`

Comment doesn't match the function.

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

Same comment copy-n-paste mismatch.

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

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

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

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

Rob

Re: [PATCH 04/10] rust: Add bindings for reading device properties

[Remo Senekowitsch]

On Wed Mar 26, 2025 at 10:27 PM CET, Rob Herring wrote:
> On Wed, Mar 26, 2025 at 06:13:43PM +0100, Remo Senekowitsch wrote:
>> +    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
>> +        let mut str = core::ptr::null_mut();
>> +        let pstr: *mut _ = &mut str;
>> +
>> +        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is
>> +        // valid because `self` is valid.
>> +        let ret = unsafe {
>> +            bindings::fwnode_property_read_string(self.as_raw(), name.as_char_ptr(), pstr as _)
>> +        };
>> +        to_result(ret)?;
>> +
>> +        // SAFETY: `pstr` contains a non-null ptr on success
>> +        let str = unsafe { CStr::from_char_ptr(*pstr) };
>> +        Ok(str.try_into()?)
>> +    }
>
> There's a problem with the C version of this function that I'd like to 
> not repeat in Rust especially since ownership is clear. 
>
> The issue is that we never know when the returned string is no longer 
> needed. For DT overlays, we need to be able free the string when/if an 
> overlay is removed. Though overlays are somewhat orthogonal to this. 
> It's really just when the property's node refcount goes to 0 that the 
> property value could be freed.
>
> So this function should probably return a copy of the string that the 
> caller owns.

I think the function already does that. The variable `str` is of type `&CStr`,
so the expression `str.try_into()?` on the last line calls the implementation
of `impl TryFrom<&CStr> for CString`, which is documented here:
https://rust.docs.kernel.org/kernel/str/struct.CString.html#impl-TryFrom%3C%26CStr%3E-for-CString

And looking at the source, it shows that the `CString` is created with a new
owned memory allocation:
https://rust.docs.kernel.org/src/kernel/str.rs.html#878-890

Remo

[PATCH 05/10] rust: Read properties via single generic method

[Remo Senekowitsch]

This can probably be squashed into its parent. I haven't done that
yet in case there is something wrong with the generic approach and the
previous one is preferred.

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

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index 4756ea766..4a03008ce 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -35,31 +35,11 @@ pub fn property_present(&self, name: &CStr) -> bool {
         self.fwnode().property_present(name)
     }
 
-    /// Returns if a firmware property `name` is true or false
-    pub fn property_read_bool(&self, name: &CStr) -> bool {
-        self.fwnode().property_read_bool(name)
-    }
-
-    /// Returns the index of matching string `match_str` for firmware string property `name`
-    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
-        self.fwnode().property_read_string(name)
-    }
-
     /// Returns the index of matching string `match_str` for firmware string property `name`
     pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
         self.fwnode().property_match_string(name, match_str)
     }
 
-    /// Returns firmware property `name` integer scalar value
-    pub fn property_read<T: Integer>(&self, name: &CStr) -> Result<T> {
-        self.fwnode().property_read(name)
-    }
-
-    /// Returns firmware property `name` integer array values
-    pub fn property_read_array<T: Integer, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
-        self.fwnode().property_read_array(name)
-    }
-
     /// Returns firmware property `name` integer array values in a KVec
     pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
         self.fwnode().property_read_array_vec(name, len)
@@ -69,6 +49,11 @@ pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Re
     pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
         self.fwnode().property_count_elem::<T>(name)
     }
+
+    /// Returns firmware property `name` integer scalar value
+    pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
+        self.fwnode().property_read(name)
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -101,30 +86,6 @@ pub fn property_present(&self, name: &CStr) -> bool {
         unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
     }
 
-    /// Returns if a firmware property `name` is true or false
-    pub fn property_read_bool(&self, name: &CStr) -> bool {
-        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
-        // because `self` is valid.
-        unsafe { bindings::fwnode_property_read_bool(self.as_raw(), name.as_char_ptr()) }
-    }
-
-    /// Returns the index of matching string `match_str` for firmware string property `name`
-    pub fn property_read_string(&self, name: &CStr) -> Result<CString> {
-        let mut str = core::ptr::null_mut();
-        let pstr: *mut _ = &mut str;
-
-        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is
-        // valid because `self` is valid.
-        let ret = unsafe {
-            bindings::fwnode_property_read_string(self.as_raw(), name.as_char_ptr(), pstr as _)
-        };
-        to_result(ret)?;
-
-        // SAFETY: `pstr` contains a non-null ptr on success
-        let str = unsafe { CStr::from_char_ptr(*pstr) };
-        Ok(str.try_into()?)
-    }
-
     /// 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
@@ -140,34 +101,6 @@ pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usi
         Ok(ret as usize)
     }
 
-    /// Returns firmware property `name` integer scalar value
-    pub fn property_read<T: Integer>(&self, name: &CStr) -> Result<T> {
-        let val: [_; 1] = Self::property_read_array(self, name)?;
-        Ok(val[0])
-    }
-
-    /// Returns firmware property `name` integer array values
-    pub fn property_read_array<T: Integer, const N: usize>(&self, name: &CStr) -> Result<[T; N]> {
-        let val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
-
-        // SAFETY: `name` is non-null and null-terminated. `self.as_raw` is valid
-        // because `self` is valid. `val.as_ptr` is valid because `val` is valid.
-        let ret = unsafe {
-            bindings::fwnode_property_read_int_array(
-                self.as_raw(),
-                name.as_char_ptr(),
-                T::SIZE as u32,
-                val.as_ptr() as *mut c_void,
-                val.len(),
-            )
-        };
-        to_result(ret)?;
-
-        // SAFETY: `val` is always initialized when
-        // fwnode_property_read_int_array is successful.
-        Ok(val.map(|v| unsafe { v.assume_init() }))
-    }
-
     /// Returns firmware property `name` integer array values in a KVec
     pub fn property_read_array_vec<T: Integer>(&self, name: &CStr, len: usize) -> Result<KVec<T>> {
         let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
@@ -206,6 +139,38 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
         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, integers and arrays
+    /// of 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 crate::{device::Device, types::CString};
+    /// fn examples(dev: &Device) -> Result {
+    ///     let fwnode = dev.fwnode();
+    ///     let b: bool = fwnode.property_read("some-bool")?;
+    ///     let s: CString = fwnode.property_read("some-str")?;
+    /// }
+    /// ```
+    pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
+        T::read(&self, name)
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
@@ -220,3 +185,65 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
         unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
     }
 }
+
+/// Implemented for all types that can be read as properties.
+///
+/// This is used to make [FwNode::property_read] generic over the type of
+/// property being read.
+pub trait Property: Sized {
+    /// Used to make [FwNode::property_read] generic.
+    fn read(fwnode: &FwNode, name: &CStr) -> Result<Self>;
+}
+
+impl Property for bool {
+    fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is valid
+        // because `fwnode` is valid.
+        Ok(unsafe { bindings::fwnode_property_read_bool(fwnode.as_raw(), name.as_char_ptr()) })
+    }
+}
+impl Property for CString {
+    fn read(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()?)
+    }
+}
+impl<T: Integer, const N: usize> Property for [T; N] {
+    fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
+
+        // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is valid
+        // because `fwnode` is valid. `val.as_ptr` is valid because `val` is valid.
+        let ret = unsafe {
+            bindings::fwnode_property_read_int_array(
+                fwnode.as_raw(),
+                name.as_char_ptr(),
+                T::SIZE as u32,
+                val.as_mut_ptr().cast(),
+                val.len(),
+            )
+        };
+        to_result(ret)?;
+
+        // SAFETY: `val` is always initialized when
+        // fwnode_property_read_int_array is successful.
+        Ok(val.map(|v| unsafe { v.assume_init() }))
+    }
+}
+impl<T: Integer> Property for T {
+    fn read(fwnode: &FwNode, name: &CStr) -> Result<T> {
+        let val: [_; 1] = <[T; 1] as Property>::read(fwnode, name)?;
+        Ok(val[0])
+    }
+}
-- 
2.49.0

Re: [PATCH 05/10] rust: Read properties via single generic method

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:44PM +0100, Remo Senekowitsch wrote:
> This can probably be squashed into its parent. I haven't done that
> yet in case there is something wrong with the generic approach and the
> previous one is preferred.

Looks good to me other than the returning of strings issue. And yes it 
should be squashed. (Really, it should have been in this series so 
reviewers aren't reviewing code you delete in the next patch).

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

[PATCH 06/10] rust: property: Add child accessor and iterator

[Remo Senekowitsch]

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

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index 4a03008ce..dc927ad93 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -13,7 +13,7 @@
     error::{to_result, Result},
     prelude::*,
     str::{CStr, CString},
-    types::{Integer, Opaque},
+    types::{ARef, Integer, Opaque},
 };
 
 impl Device {
@@ -54,6 +54,16 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
     pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
         self.fwnode().property_read(name)
     }
+
+    /// Returns first matching named child node handle.
+    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
+        self.fwnode().get_child_by_name(name)
+    }
+
+    /// Returns an iterator over a node's children.
+    pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
+        self.fwnode().children()
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -75,6 +85,11 @@ pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
 pub struct FwNode(Opaque<bindings::fwnode_handle>);
 
 impl FwNode {
+    // SAFETY: `raw` must have its refcount incremented.
+    unsafe fn from_raw(raw: *mut bindings::fwnode_handle) -> ARef<Self> {
+        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()
@@ -171,6 +186,46 @@ pub fn property_count_elem<T: Integer>(&self, name: &CStr) -> Result<usize> {
     pub fn property_read<T: Property>(&self, name: &CStr) -> Result<T> {
         T::read(&self, 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.
+        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 refcount incremented.
+        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()
+                }
+            };
+            let next = unsafe { bindings::fwnode_get_next_child_node(self.as_raw(), prev_ptr) };
+            if next.is_null() {
+                return None;
+            }
+            // SAFETY: `fwnode_get_next_child_node` returns a pointer with
+            // refcount incremented.
+            let next = unsafe { FwNode::from_raw(next) };
+            prev = Some(next.clone());
+            Some(next)
+        })
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
-- 
2.49.0

Re: [PATCH 06/10] rust: property: Add child accessor and iterator

[Andrew Ballance]

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
>  impl FwNode {
> +    // SAFETY: `raw` must have its refcount incremented.
> +    unsafe fn from_raw(raw: *mut bindings::fwnode_handle) -> ARef<Self> {
> +        unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(raw.cast())) }
> +    }

this safety comment should say why it is ok to cast from a
struct fwnode_handle* to a *const FwNode

> +                    // 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()
> +                }
> +            };
> +            let next = unsafe { bindings::fwnode_get_next_child_node(self.as_raw(), prev_ptr) };

this is missing a safety comment

> +            if next.is_null() {
> +                return None;
> +            }

Andrew

Re: [PATCH 06/10] rust: property: Add child accessor and iterator

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:45PM +0100, Remo Senekowitsch wrote:
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>

Needs a commit msg.

> ---
>  rust/kernel/property.rs | 57 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)

[PATCH 07/10] rust: Add arrayvec

[Remo Senekowitsch]

This patch is basically a proof of concept intendend to gather feedback
about how to do this properly. Normally I would want to use the crate
from crates.io[1], but that's not an option in the kernel. We could also
vendor the entire source code of arrayvec. I'm not sure if people will
be happy with that.

[1] https://crates.io/crates/arrayvec

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 rust/kernel/arrayvec.rs | 81 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs      |  1 +
 2 files changed, 82 insertions(+)
 create mode 100644 rust/kernel/arrayvec.rs

diff --git a/rust/kernel/arrayvec.rs b/rust/kernel/arrayvec.rs
new file mode 100644
index 000000000..041e7dcce
--- /dev/null
+++ b/rust/kernel/arrayvec.rs
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Provides [ArrayVec], a stack-allocated vector with static capacity.
+
+use core::mem::MaybeUninit;
+
+/// A stack-allocated vector with statically fixed capacity.
+///
+/// This can be useful to avoid heap allocation and still ensure safety where a
+/// small but dynamic number of elements is needed.
+///
+/// For example, consider a function that returns a variable number of values,
+/// but no more than 8. In C, one might achieve this by passing a pointer to
+/// a stack-allocated array as an out-parameter and making the function return
+/// the actual number of elements. This is not safe, because nothing prevents
+/// the caller from reading elements from the array that weren't actually
+/// initialized by the function. `ArrayVec` solves this problem, users are
+/// prevented from accessing uninitialized elements.
+///
+/// This basically exists already (in a much more mature form) on crates.io:
+/// <https://crates.io/crates/arrayvec>
+#[derive(Debug)]
+pub struct ArrayVec<const N: usize, T> {
+    array: [core::mem::MaybeUninit<T>; N],
+    len: usize,
+}
+
+impl<const N: usize, T> ArrayVec<N, T> {
+    /// Adds a new element to the end of the vector.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the vector is already full.
+    pub fn push(&mut self, elem: T) {
+        if self.len == N {
+            panic!("OOM")
+        }
+        self.array[self.len] = MaybeUninit::new(elem);
+        self.len += 1;
+    }
+
+    /// Returns the length of the vector.
+    pub fn len(&self) -> usize {
+        self.len
+    }
+}
+
+impl<const N: usize, T> Default for ArrayVec<N, T> {
+    fn default() -> Self {
+        Self {
+            array: [const { MaybeUninit::uninit() }; N],
+            len: 0,
+        }
+    }
+}
+
+impl<const N: usize, T> AsRef<[T]> for ArrayVec<N, T> {
+    fn as_ref(&self) -> &[T] {
+        // SAFETY: As per the type invariant, all elements at index < self.len
+        // are initialized.
+        unsafe { core::mem::transmute(&self.array[..self.len]) }
+    }
+}
+
+impl<const N: usize, T> AsMut<[T]> for ArrayVec<N, T> {
+    fn as_mut(&mut self) -> &mut [T] {
+        // SAFETY: As per the type invariant, all elements at index < self.len
+        // are initialized.
+        unsafe { core::mem::transmute(&mut self.array[..self.len]) }
+    }
+}
+
+impl<const N: usize, T> Drop for ArrayVec<N, T> {
+    fn drop(&mut self) {
+        unsafe {
+            let slice: &mut [T] =
+                core::slice::from_raw_parts_mut(self.array.as_mut_ptr().cast(), self.len);
+            core::ptr::drop_in_place(slice);
+        }
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ca233fd20..0777f7a42 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -36,6 +36,7 @@
 pub use ffi;
 
 pub mod alloc;
+pub mod arrayvec;
 #[cfg(CONFIG_BLOCK)]
 pub mod block;
 #[doc(hidden)]
-- 
2.49.0

Re: [PATCH 07/10] rust: Add arrayvec

[Andrew Ballance]

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> +pub struct ArrayVec<const N: usize, T> {
> +    array: [core::mem::MaybeUninit<T>; N],
> +    len: usize,
> +}

I would reverse the order of the generic arguments so T is first so
that it matches the declaration of an array: [T; N] 

> +    /// Adds a new element to the end of the vector.
> +    ///
> +    /// # Panics
> +    ///
> +    /// Panics if the vector is already full.
> +    pub fn push(&mut self, elem: T) {
> +        if self.len == N {
> +            panic!("OOM")
> +        }
> +        self.array[self.len] = MaybeUninit::new(elem);
> +        self.len += 1;
> +    }

This function should not panic. I would rewrite it so that its signature is
pub fn push(&mut self, elem: T) -> Result<(), T>

> +impl<const N: usize, T> AsRef<[T]> for ArrayVec<N, T> {
> +    fn as_ref(&self) -> &[T] {
> +        // SAFETY: As per the type invariant, all elements at index < self.len
> +        // are initialized.
> +        unsafe { core::mem::transmute(&self.array[..self.len]) }
> +    }
> +}
> +
> +impl<const N: usize, T> AsMut<[T]> for ArrayVec<N, T> {
> +    fn as_mut(&mut self) -> &mut [T] {
> +        // SAFETY: As per the type invariant, all elements at index < self.len
> +        // are initialized.
> +        unsafe { core::mem::transmute(&mut self.array[..self.len]) }
> +    }
> +}

I would replace both uses of transmute here with slice::from_raw_parts
like you did in drop. 

also it's not needed but you could also impl Deref and DerefMut for ArrayVec
because you have already impl'ed AsRef and AsMut so it should be simple

> +impl<const N: usize, T> Drop for ArrayVec<N, T> {
> +    fn drop(&mut self) {
> +        unsafe {

this is missing a safety comment

> +            let slice: &mut [T] =
> +                core::slice::from_raw_parts_mut(self.array.as_mut_ptr().cast(), self.len);
> +            core::ptr::drop_in_place(slice);
> +        }
> +    }
> +}

Andrew

Re: [PATCH 07/10] rust: Add arrayvec

[Danilo Krummrich]

On Wed, Mar 26, 2025 at 06:13:46PM +0100, Remo Senekowitsch wrote:
> This patch is basically a proof of concept intendend to gather feedback
> about how to do this properly. Normally I would want to use the crate
> from crates.io[1], but that's not an option in the kernel. We could also
> vendor the entire source code of arrayvec. I'm not sure if people will
> be happy with that.

Do we really need this? The only user in this series I could spot was
property_get_reference_args(). And I think that with a proper abstraction of
struct fwnode_reference_args we could avoid to copy memory at all.

If it turns out we actually need something like this, I'd prefer to move it to
rust/kernel/alloc/ and see if it's worth to derive a common trait that maybe can
share a few things between ArrayVec and Vec.

> 
> [1] https://crates.io/crates/arrayvec
> 
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/arrayvec.rs | 81 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs      |  1 +
>  2 files changed, 82 insertions(+)
>  create mode 100644 rust/kernel/arrayvec.rs
> 
> diff --git a/rust/kernel/arrayvec.rs b/rust/kernel/arrayvec.rs
> new file mode 100644
> index 000000000..041e7dcce
> --- /dev/null
> +++ b/rust/kernel/arrayvec.rs
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Provides [ArrayVec], a stack-allocated vector with static capacity.
> +
> +use core::mem::MaybeUninit;
> +
> +/// A stack-allocated vector with statically fixed capacity.
> +///
> +/// This can be useful to avoid heap allocation and still ensure safety where a
> +/// small but dynamic number of elements is needed.
> +///
> +/// For example, consider a function that returns a variable number of values,
> +/// but no more than 8. In C, one might achieve this by passing a pointer to
> +/// a stack-allocated array as an out-parameter and making the function return
> +/// the actual number of elements. This is not safe, because nothing prevents
> +/// the caller from reading elements from the array that weren't actually
> +/// initialized by the function. `ArrayVec` solves this problem, users are
> +/// prevented from accessing uninitialized elements.
> +///
> +/// This basically exists already (in a much more mature form) on crates.io:
> +/// <https://crates.io/crates/arrayvec>
> +#[derive(Debug)]
> +pub struct ArrayVec<const N: usize, T> {
> +    array: [core::mem::MaybeUninit<T>; N],
> +    len: usize,
> +}
> +
> +impl<const N: usize, T> ArrayVec<N, T> {
> +    /// Adds a new element to the end of the vector.
> +    ///
> +    /// # Panics
> +    ///
> +    /// Panics if the vector is already full.
> +    pub fn push(&mut self, elem: T) {
> +        if self.len == N {
> +            panic!("OOM")

Please do not panic, this should return a Result instead.

[PATCH 08/10] rust: property: Add property_get_reference_args

[Remo Senekowitsch]

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

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index dc927ad93..f1d0a33ba 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -8,6 +8,7 @@
 
 use crate::{
     alloc::KVec,
+    arrayvec::ArrayVec,
     bindings,
     device::Device,
     error::{to_result, Result},
@@ -64,6 +65,20 @@ pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
     pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
         self.fwnode().children()
     }
+
+    /// Finds a reference with arguments.
+    pub fn property_get_reference_args(
+        &self,
+        prop: &CStr,
+        nargs: NArgs<'_>,
+        index: u32,
+    ) -> Result<(
+        ARef<FwNode>,
+        ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>,
+    )> {
+        self.fwnode()
+            .property_get_reference_args(prop, nargs, index)
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -226,6 +241,45 @@ 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<(
+        ARef<Self>,
+        ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>,
+    )> {
+        let mut out_args = bindings::fwnode_reference_args::default();
+
+        let (nargs_prop, nargs) = match nargs {
+            NArgs::Prop(nargs_prop) => (nargs_prop.as_char_ptr(), 0),
+            NArgs::N(nargs) => (ptr::null(), nargs),
+        };
+
+        let ret = unsafe {
+            bindings::fwnode_property_get_reference_args(
+                self.0.get(),
+                prop.as_char_ptr(),
+                nargs_prop,
+                nargs,
+                index,
+                &mut out_args,
+            )
+        };
+        to_result(ret)?;
+
+        let node = unsafe { FwNode::from_raw(out_args.fwnode) };
+        let mut args = ArrayVec::default();
+
+        for i in 0..out_args.nargs {
+            args.push(out_args.args[i as usize]);
+        }
+
+        Ok((node, args))
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
@@ -302,3 +356,12 @@ fn read(fwnode: &FwNode, name: &CStr) -> Result<T> {
         Ok(val[0])
     }
 }
+
+/// 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),
+}
-- 
2.49.0

Re: [PATCH 08/10] rust: property: Add property_get_reference_args

[Andrew Ballance]

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> +    pub fn property_get_reference_args(
> +        &self,
> +        prop: &CStr,
> +        nargs: NArgs<'_>,
> +        index: u32,
> +    ) -> Result<(
> +        ARef<Self>,
> +        ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>,
> +    )> {
> +        let mut out_args = bindings::fwnode_reference_args::default();
> +
> +        let (nargs_prop, nargs) = match nargs {
> +            NArgs::Prop(nargs_prop) => (nargs_prop.as_char_ptr(), 0),
> +            NArgs::N(nargs) => (ptr::null(), nargs),
> +        };
> +
> +        let ret = unsafe {
> +            bindings::fwnode_property_get_reference_args(
> +                self.0.get(),
> +                prop.as_char_ptr(),
> +                nargs_prop,
> +                nargs,
> +                index,
> +                &mut out_args,
> +            )
> +        };
> +        to_result(ret)?;
> +
> +        let node = unsafe { FwNode::from_raw(out_args.fwnode) };
> +        let mut args = ArrayVec::default();
> +
> +        for i in 0..out_args.nargs {
> +            args.push(out_args.args[i as usize]);
> +        }
> +
> +        Ok((node, args))
> +    }

This function is missing safety comments.

Andrew

Re: [PATCH 08/10] rust: property: Add property_get_reference_args

[Miguel Ojeda]

On Wed, Mar 26, 2025 at 10:07 PM Andrew Ballance
<andrewjballance@gmail.com> wrote:
>
> This function is missing safety comments.

Yeah -- that should be spotted by Clippy.

Remo: did you build with `CLIPPY=1`?

Thanks!

Cheers,
Miguel

Re: [PATCH 08/10] rust: property: Add property_get_reference_args

[Remo Senekowitsch]

On Wed Mar 26, 2025 at 10:25 PM CET, Miguel Ojeda wrote:
>
> Remo: did you build with `CLIPPY=1`?

I didn't! Usually I have rust-analyzer configured with clippy, but that doesn't seem to work here.
I added it to my build scripts and can see the warnings now, future patches should be free of clippy warnings.

Thanks for the tip!

Remo

Re: [PATCH 08/10] rust: property: Add property_get_reference_args

[Danilo Krummrich]

On Wed, Mar 26, 2025 at 06:13:47PM +0100, Remo Senekowitsch wrote:
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
>  rust/kernel/property.rs | 63 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index dc927ad93..f1d0a33ba 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -8,6 +8,7 @@
>  
>  use crate::{
>      alloc::KVec,
> +    arrayvec::ArrayVec,
>      bindings,
>      device::Device,
>      error::{to_result, Result},
> @@ -64,6 +65,20 @@ pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
>      pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
>          self.fwnode().children()
>      }
> +
> +    /// Finds a reference with arguments.
> +    pub fn property_get_reference_args(
> +        &self,
> +        prop: &CStr,
> +        nargs: NArgs<'_>,
> +        index: u32,
> +    ) -> Result<(
> +        ARef<FwNode>,
> +        ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>,
> +    )> {
> +        self.fwnode()
> +            .property_get_reference_args(prop, nargs, index)
> +    }
>  }
>  
>  /// A reference-counted fwnode_handle.
> @@ -226,6 +241,45 @@ 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<(
> +        ARef<Self>,
> +        ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>,
> +    )> {
> +        let mut out_args = bindings::fwnode_reference_args::default();
> +
> +        let (nargs_prop, nargs) = match nargs {
> +            NArgs::Prop(nargs_prop) => (nargs_prop.as_char_ptr(), 0),
> +            NArgs::N(nargs) => (ptr::null(), nargs),
> +        };
> +
> +        let ret = unsafe {
> +            bindings::fwnode_property_get_reference_args(
> +                self.0.get(),
> +                prop.as_char_ptr(),
> +                nargs_prop,
> +                nargs,
> +                index,
> +                &mut out_args,
> +            )
> +        };
> +        to_result(ret)?;
> +
> +        let node = unsafe { FwNode::from_raw(out_args.fwnode) };
> +        let mut args = ArrayVec::default();
> +
> +        for i in 0..out_args.nargs {
> +            args.push(out_args.args[i as usize]);
> +        }

Why the copy? Can't you just write an abstraction for struct
fwnode_reference_args and just return an instance of that?

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

[Remo Senekowitsch]

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

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

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

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

[Andrew Ballance]

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> +/// 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> {

nit: when linking items in rustdoc comments you should suround them like [`this`]

Andrew

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

[Rob Herring]

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

We'll want to squash this one too.

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

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

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

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

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

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

[PATCH 10/10] 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         | 56 +++++++++++++++++++-
 2 files changed, 58 insertions(+), 1 deletion(-)

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..ed25a3781 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,
@@ -28,6 +28,60 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin
             dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
         }
 
+        let dev = pdev.as_ref();
+        if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
+        {
+            dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx);
+        }
+
+        if let Ok(str) = dev
+            .property_read::<CString>(c_str!("compatible"))
+            .required()
+        {
+            dev_info!(pdev.as_ref(), "compatible string = {:?}\n", str);
+        }
+
+        let prop = dev
+            .property_read::<bool>(c_str!("test,bool-prop"))
+            .required()?;
+        dev_info!(dev, "bool prop is {}\n", prop);
+
+        if dev.property_present(c_str!("test,u32-prop")) {
+            dev_info!(dev, "'test,u32-prop' is present\n");
+        }
+
+        let prop = dev
+            .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 _ = dev
+            .property_read::<u32>(c_str!("test,u32-required-prop"))
+            .required()?;
+
+        let prop: u32 = dev.property_read(c_str!("test,u32-prop")).required()?;
+        dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
+
+        let prop: [i16; 4] = dev.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",
+            dev.property_count_elem::<u16>(c_str!("test,i16-array"))
+                .unwrap()
+        );
+
+        let prop: KVec<i16> = dev
+            .property_read_array_vec(c_str!("test,i16-array"), 4)?
+            .required()?;
+        dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop);
+
         let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
 
         Ok(drvdata.into())
-- 
2.49.0

Re: [PATCH 10/10] samples: rust: platform: Add property read examples

[Rob Herring]

On Wed, Mar 26, 2025 at 06:13:49PM +0100, Remo Senekowitsch wrote:
> 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         | 56 +++++++++++++++++++-
>  2 files changed, 58 insertions(+), 1 deletion(-)
> 
> 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..ed25a3781 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,
> @@ -28,6 +28,60 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin
>              dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
>          }
>  
> +        let dev = pdev.as_ref();

We should move this to the top and replace all the 'pdev.as_ref()' with 
'dev'.

> +        if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
> +        {
> +            dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx);

Like here. (Looks like this is my fault.)

> +        }
> +
> +        if let Ok(str) = dev
> +            .property_read::<CString>(c_str!("compatible"))
> +            .required()
> +        {
> +            dev_info!(pdev.as_ref(), "compatible string = {:?}\n", str);
> +        }
> +
> +        let prop = dev
> +            .property_read::<bool>(c_str!("test,bool-prop"))
> +            .required()?;

The 'required' is kind of odd for boolean properties. They are never 
required as not present is the only way to to get false.

> +        dev_info!(dev, "bool prop is {}\n", prop);
> +
> +        if dev.property_present(c_str!("test,u32-prop")) {
> +            dev_info!(dev, "'test,u32-prop' is present\n");
> +        }
> +
> +        let prop = dev
> +            .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 _ = dev
> +            .property_read::<u32>(c_str!("test,u32-required-prop"))
> +            .required()?;
> +
> +        let prop: u32 = dev.property_read(c_str!("test,u32-prop")).required()?;
> +        dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
> +
> +        let prop: [i16; 4] = dev.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",
> +            dev.property_count_elem::<u16>(c_str!("test,i16-array"))
> +                .unwrap()
> +        );
> +
> +        let prop: KVec<i16> = dev
> +            .property_read_array_vec(c_str!("test,i16-array"), 4)?
> +            .required()?;
> +        dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop);
> +
>          let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?;
>  
>          Ok(drvdata.into())
> -- 
> 2.49.0
> 

Re: [PATCH 10/10] samples: rust: platform: Add property read examples

[Remo Senekowitsch]

On Wed Mar 26, 2025 at 11:01 PM CET, Rob Herring wrote:
>>
>> +        let prop = dev
>> +            .property_read::<bool>(c_str!("test,bool-prop"))
>> +            .required()?;
>
> The 'required' is kind of odd for boolean properties. They are never 
> required as not present is the only way to to get false.

Agreed. I can think of a few alternatives:

* Make the trait `Property` more flexible to allow each implementor to specify
  what its output type for the `read` function is, via an associated type.
  I really don't like this idea, because overly generic APIs can mess with type
  inference and become less ergonomic because of it.

* Use `propert_present` instead. That doesn't perfectly express the intention,
  because it doesn't warn if the property is present but has a type other than
  bool.

* Add an additional inherent method `property_read_bool`, which returns a plain
  `bool` instead of `PropertyGuard<bool>`. Then there will be three slightly
  different ways to read a bool: `property_present`, `property_read_bool` and
  `property_read::<bool>`. Maybe that's confusing.

* Add `property_read_bool` and remove `impl Property for bool`. That would avoid
  confusion between `property_read_bool` and `property_read::<bool>`, only the
  former would work.

Re: [PATCH 10/10] samples: rust: platform: Add property read examples

[Rob Herring]

On Wed, Mar 26, 2025 at 5:24 PM Remo Senekowitsch <remo@buenzli.dev> wrote:
>
> On Wed Mar 26, 2025 at 11:01 PM CET, Rob Herring wrote:
> >>
> >> +        let prop = dev
> >> +            .property_read::<bool>(c_str!("test,bool-prop"))
> >> +            .required()?;
> >
> > The 'required' is kind of odd for boolean properties. They are never
> > required as not present is the only way to to get false.
>
> Agreed. I can think of a few alternatives:
>
> * Make the trait `Property` more flexible to allow each implementor to specify
>   what its output type for the `read` function is, via an associated type.
>   I really don't like this idea, because overly generic APIs can mess with type
>   inference and become less ergonomic because of it.
>
> * Use `propert_present` instead. That doesn't perfectly express the intention,
>   because it doesn't warn if the property is present but has a type other than
>   bool.

Right. I've been cleaning up the tree to use of_property_read_bool()
on bools and of_property_present() on non-bools, so don't want to go
back to 1 function. The C code now warns on a mismatch.

> * Add an additional inherent method `property_read_bool`, which returns a plain
>   `bool` instead of `PropertyGuard<bool>`. Then there will be three slightly
>   different ways to read a bool: `property_present`, `property_read_bool` and
>   `property_read::<bool>`. Maybe that's confusing.
>
> * Add `property_read_bool` and remove `impl Property for bool`. That would avoid
>   confusion between `property_read_bool` and `property_read::<bool>`, only the
>   former would work.

I think I would go with this option. Easier to add another way later
than remove one.

Rob

Re: [PATCH 10/10] samples: rust: platform: Add property read examples

[Danilo Krummrich]

On Wed, Mar 26, 2025 at 06:13:49PM +0100, Remo Senekowitsch wrote:
> --- 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,
> @@ -28,6 +28,60 @@ fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin
>              dev_info!(pdev.as_ref(), "Probed with info: '{}'.\n", info.0);
>          }
>  
> +        let dev = pdev.as_ref();
> +        if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
> +        {
> +            dev_info!(pdev.as_ref(), "matched compatible string idx = {}\n", idx);
> +        }
> +
> +        if let Ok(str) = dev
> +            .property_read::<CString>(c_str!("compatible"))
> +            .required()
> +        {
> +            dev_info!(pdev.as_ref(), "compatible string = {:?}\n", str);
> +        }
> +
> +        let prop = dev
> +            .property_read::<bool>(c_str!("test,bool-prop"))
> +            .required()?;
> +        dev_info!(dev, "bool prop is {}\n", prop);
> +
> +        if dev.property_present(c_str!("test,u32-prop")) {
> +            dev_info!(dev, "'test,u32-prop' is present\n");
> +        }
> +
> +        let prop = dev
> +            .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
> +        );

Printing the default value looks more like a test, rather than a sample.

> +
> +        // Missing property without a default will print an error
> +        let _ = dev
> +            .property_read::<u32>(c_str!("test,u32-required-prop"))
> +            .required()?;
> +
> +        let prop: u32 = dev.property_read(c_str!("test,u32-prop")).required()?;
> +        dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);
> +
> +        let prop: [i16; 4] = dev.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",
> +            dev.property_count_elem::<u16>(c_str!("test,i16-array"))
> +                .unwrap()

Please no unwrap() in the kernel, it may panic.

> +        );
> +
> +        let prop: KVec<i16> = dev
> +            .property_read_array_vec(c_str!("test,i16-array"), 4)?
> +            .required()?;
> +        dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop);
> +

Please move this code to a new function, e.g.

	impl SampleDriver {
	   fn properties_parse(dev: &device::Device)) -> Result;
	}

in order to keep probe() as clean as possible. If we put too much stuff in
there, it might become too confusing to serve as a simple example showing how to
implement a platform driver in Rust.

Besides that, are we sure we really want to print everything?

Re: [PATCH 0/10] More Rust bindings for device property reads

[Andrew Ballance]

Hi thanks for sending these in.

On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> This is my first time posting to the mailing list, please let me know if
> I did anything wrong.

you probably will have to resubmit in the patch canonical format[1]

I would recomend reading the docs but in summary:
 - all of your patches should have the subsystem in the subject line.
 - patch 6 is missing a commit message
 - commit messages should be written in the imperitive tone
 - a couple of your commit messages give a reason for the changes but
   do not have a summary of the changes
 - for your v2 you should add a diffstat to your cover letter running
   `git format-patch --base=auto --cover-letter` does this automatically
   for you

Andrew


[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

Re: [PATCH 0/10] More Rust bindings for device property reads

[Andy Shevchenko]

On Wed, Mar 26, 2025 at 03:54:09PM -0500, Andrew Ballance wrote:
> Hi thanks for sending these in.
> 
> On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: 
> > This is my first time posting to the mailing list, please let me know if
> > I did anything wrong.
> 
> you probably will have to resubmit in the patch canonical format[1]
> 
> I would recomend reading the docs but in summary:
>  - all of your patches should have the subsystem in the subject line.
>  - patch 6 is missing a commit message
>  - commit messages should be written in the imperitive tone
>  - a couple of your commit messages give a reason for the changes but
>    do not have a summary of the changes

>  - for your v2 you should add a diffstat to your cover letter running
>    `git format-patch --base=auto --cover-letter` does this automatically
>    for you

Also -v<X> gives the proper versioning.

> [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 0/5] 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.

Rob Herring and Dirk Behme did several review iterations over on Zulip
already[1], I'm thankful for their great input.

This is my second-ever patch series. I read through the documentation
for sending patches[2] carefully, trying to apply everything to the best
of my understanding. Please point out any specifics if I got something
wrong.

Best regards,
Remo

changes in v2:
- Add property.rs to maintainers file
- Add and improve lots of safety comments
- Use `__dev_fwnode` istead of `dev_fwnode` to express the intent better
- Make `get_child_by_name` not return a `PropertyGuard`
- Squash the patch with the generic read method
- Squash the PropertyGuard patch
- Remove `arrayvec`, abstract over `fwnode_reference_args` instead
- Log the full fwnode path in case of a missing property
- Remove integer trait and export of fwnode_property_read_int_array
  These are not needed now that a macro is used to associate each
  integer type with its corresponding `read_*_array` function.
- Replace `impl Property for bool` with a standalone function
  `property_read_bool` with an infallible function signature
- Extract parsing of properties into separate function in sample
  platform driver

Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/with/507874342 [1]
Link: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format [2]

Remo Senekowitsch (5):
  rust: Move property_present to separate file
  rust: 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                                  |   1 +
 drivers/of/unittest-data/tests-platform.dtsi |   3 +
 rust/helpers/helpers.c                       |   1 +
 rust/helpers/property.c                      |   8 +
 rust/kernel/device.rs                        |   7 -
 rust/kernel/lib.rs                           |   1 +
 rust/kernel/property.rs                      | 608 +++++++++++++++++++
 samples/rust/rust_driver_platform.rs         |  69 ++-
 8 files changed, 688 insertions(+), 10 deletions(-)
 create mode 100644 rust/helpers/property.c
 create mode 100644 rust/kernel/property.rs

-- 
2.49.0

[PATCH v2 1/5] rust: 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,
duplicate the APIs on a fwnode as they are in C.

Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
 MAINTAINERS             |  1 +
 rust/helpers/helpers.c  |  1 +
 rust/helpers/property.c |  8 +++++
 rust/kernel/device.rs   |  7 ----
 rust/kernel/lib.rs      |  1 +
 rust/kernel/property.rs | 75 +++++++++++++++++++++++++++++++++++++++++
 6 files changed, 86 insertions(+), 7 deletions(-)
 create mode 100644 rust/helpers/property.c
 create mode 100644 rust/kernel/property.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index c8d9e8187..a34471662 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7117,6 +7117,7 @@ F:	rust/kernel/device_id.rs
 F:	rust/kernel/devres.rs
 F:	rust/kernel/driver.rs
 F:	rust/kernel/platform.rs
+F:	rust/kernel/property.rs
 F:	samples/rust/rust_driver_platform.rs
 
 DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
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.rs
index db2d9658b..d5e6a19ff 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,7 +6,6 @@
 
 use crate::{
     bindings,
-    str::CStr,
     types::{ARef, Opaque},
 };
 use core::{fmt, ptr};
@@ -181,12 +180,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/lib.rs b/rust/kernel/lib.rs
index 496ed32b0..ca233fd20 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -67,6 +67,7 @@
 pub mod platform;
 pub mod prelude;
 pub mod print;
+pub mod property;
 pub mod rbtree;
 pub mod revocable;
 pub mod security;
diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
new file mode 100644
index 000000000..f6e6c980d
--- /dev/null
+++ b/rust/kernel/property.rs
@@ -0,0 +1,75 @@
+// 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.
+    fn fwnode(&self) -> &FwNode {
+        // SAFETY: `self` is valid.
+        let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
+        if fwnode_handle.is_null() {
+            panic!("fwnode_handle cannot be null");
+        }
+        // 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`.
+        unsafe { &*fwnode_handle.cast() }
+    }
+
+    /// Checks if property is present or not.
+    pub fn property_present(&self, name: &CStr) -> bool {
+        self.fwnode().property_present(name)
+    }
+}
+
+/// 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

Re: [PATCH v2 1/5] rust: Move property_present to separate file

[Danilo Krummrich]

On Mon, Apr 14, 2025 at 05:26:26PM +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,
> duplicate the APIs on a fwnode as they are in C.

What do you mean with "duplicate the APIs"?

Also, this patch does three separate things.

  (1) move property_present() to separate file
  (2) implement FwNode abstraction
  (3) implement Device::fwnode()

I'd rather keep property_present() in device.rs and also move fwnode() into
device.rs, even though in C it's in property.c. impl Device blocks should be in
device.rs.

Hence, please split this in two patches, one for (2) and one for (3).

> +++ b/rust/kernel/property.rs
> @@ -0,0 +1,75 @@
> +// 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.
> +    fn fwnode(&self) -> &FwNode {
> +        // SAFETY: `self` is valid.
> +        let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
> +        if fwnode_handle.is_null() {
> +            panic!("fwnode_handle cannot be null");

Please don't use panic!() unless you hit an error condition that is impossible
to handle.

If __dev_fwnode() can ever return NULL, make this function return
Option<&FwNode> instead.

Re: [PATCH v2 1/5] rust: Move property_present to separate file

[Remo Senekowitsch]

On Mon Apr 14, 2025 at 6:00 PM CEST, Danilo Krummrich wrote:
> On Mon, Apr 14, 2025 at 05:26:26PM +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,
>> duplicate the APIs on a fwnode as they are in C.
>
> What do you mean with "duplicate the APIs"?

The property related methods on `Device` and `FwNode` are mostly
the same. Same names, same function signatures. The methods on
`Device` simply call `self.fwnode()` and then the same method on
`FwNode`. I was asked to do this so driver authors don't have to write
`dev.fwnode().read_propert()` etc. every time they want to do something
on the device itself. This is the case here for `property_present` as
well as the methods added in the following patches.

> Also, this patch does three separate things.
>
>   (1) move property_present() to separate file
>   (2) implement FwNode abstraction
>   (3) implement Device::fwnode()
>
> I'd rather keep property_present() in device.rs and also move fwnode() into
> device.rs, even though in C it's in property.c. impl Device blocks should be in
> device.rs.

I was asked to move it by Rob Herring on Zulip[1]. Dirk Behme also
agreed with it[2]. I have no strong opinion on this, hopefully you can
come to an agreement. Whatever it is, I'll happily implement it.

>> +++ b/rust/kernel/property.rs
>> @@ -0,0 +1,75 @@
>> +// 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.
>> +    fn fwnode(&self) -> &FwNode {
>> +        // SAFETY: `self` is valid.
>> +        let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
>> +        if fwnode_handle.is_null() {
>> +            panic!("fwnode_handle cannot be null");
>
> Please don't use panic!() unless you hit an error condition that is impossible
> to handle.
>
> If __dev_fwnode() can ever return NULL, make this function return
> Option<&FwNode> instead.

Rob Herring mentioned this in the previous thread already:

> Users/drivers testing fwnode_handle/of_node for NULL is pretty common.
> Though often that's a legacy code path, so maybe not allowing NULL is
> fine for now.

I asked a follow-up question about how to proceed but got no reply[3].

Best regards,
Remo

Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/495807346 [1]
Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/495945200 [2]
Link: https://lore.kernel.org/rust-for-linux/D96HP1UV49SY.1U1BFA14P8XHE@buenzli.dev/T/#mc7967352b7bd1be812806939150511c1a5018bb1 [3]

Re: [PATCH v2 1/5] rust: Move property_present to separate file

[Danilo Krummrich]

On Mon, Apr 14, 2025 at 06:40:54PM +0200, Remo Senekowitsch wrote:
> On Mon Apr 14, 2025 at 6:00 PM CEST, Danilo Krummrich wrote:
> > On Mon, Apr 14, 2025 at 05:26:26PM +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,
> >> duplicate the APIs on a fwnode as they are in C.
> >
> > What do you mean with "duplicate the APIs"?
> 
> The property related methods on `Device` and `FwNode` are mostly
> the same. Same names, same function signatures. The methods on
> `Device` simply call `self.fwnode()` and then the same method on
> `FwNode`. I was asked to do this so driver authors don't have to write
> `dev.fwnode().read_propert()` etc. every time they want to do something
> on the device itself. This is the case here for `property_present` as
> well as the methods added in the following patches.

Yeah, I noticed when I continued with patch 2 of this series.

> > Also, this patch does three separate things.
> >
> >   (1) move property_present() to separate file
> >   (2) implement FwNode abstraction
> >   (3) implement Device::fwnode()
> >
> > I'd rather keep property_present() in device.rs and also move fwnode() into
> > device.rs, even though in C it's in property.c. impl Device blocks should be in
> > device.rs.
> 
> I was asked to move it by Rob Herring on Zulip[1]. Dirk Behme also
> agreed with it[2]. I have no strong opinion on this, hopefully you can
> come to an agreement. Whatever it is, I'll happily implement it.

Please also see my comment in patch 2. If we think device.rs becomes too messy
with that, we should convert device.rs to its own module, i.e.
rust/kernel/device/mod.rs and then have rust/kernel/device/property.rs instead.

> >> +++ b/rust/kernel/property.rs
> >> @@ -0,0 +1,75 @@
> >> +// 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.
> >> +    fn fwnode(&self) -> &FwNode {
> >> +        // SAFETY: `self` is valid.
> >> +        let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
> >> +        if fwnode_handle.is_null() {
> >> +            panic!("fwnode_handle cannot be null");
> >
> > Please don't use panic!() unless you hit an error condition that is impossible
> > to handle.
> >
> > If __dev_fwnode() can ever return NULL, make this function return
> > Option<&FwNode> instead.
> 
> Rob Herring mentioned this in the previous thread already:
> 
> > Users/drivers testing fwnode_handle/of_node for NULL is pretty common.
> > Though often that's a legacy code path, so maybe not allowing NULL is
> > fine for now.
> 
> I asked a follow-up question about how to proceed but got no reply[3].

I NULL is a possible return value of __dev_fwnode() (which seems to be the
case), there's no other option than makeing fwnode() fallible.

> 
> Best regards,
> Remo
> 
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/495807346 [1]
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/495945200 [2]
> Link: https://lore.kernel.org/rust-for-linux/D96HP1UV49SY.1U1BFA14P8XHE@buenzli.dev/T/#mc7967352b7bd1be812806939150511c1a5018bb1 [3]

Re: [PATCH v2 1/5] rust: Move property_present to separate file

[Remo Senekowitsch]

On Mon Apr 14, 2025 at 8:00 PM CEST, Danilo Krummrich wrote:
> On Mon, Apr 14, 2025 at 06:40:54PM +0200, Remo Senekowitsch wrote:
>> On Mon Apr 14, 2025 at 6:00 PM CEST, Danilo Krummrich wrote:
>> > On Mon, Apr 14, 2025 at 05:26:26PM +0200, Remo Senekowitsch wrote:
>> >> +impl Device {
>> >> +    /// Obtain the fwnode corresponding to the device.
>> >> +    fn fwnode(&self) -> &FwNode {
>> >> +        // SAFETY: `self` is valid.
>> >> +        let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
>> >> +        if fwnode_handle.is_null() {
>> >> +            panic!("fwnode_handle cannot be null");
>> >
>> > Please don't use panic!() unless you hit an error condition that is impossible
>> > to handle.
>> >
>> > If __dev_fwnode() can ever return NULL, make this function return
>> > Option<&FwNode> instead.
>> 
>> Rob Herring mentioned this in the previous thread already:
>> 
>> > Users/drivers testing fwnode_handle/of_node for NULL is pretty common.
>> > Though often that's a legacy code path, so maybe not allowing NULL is
>> > fine for now.
>> 
>> I asked a follow-up question about how to proceed but got no reply[3].
>
> I NULL is a possible return value of __dev_fwnode() (which seems to be the
> case), there's no other option than makeing fwnode() fallible.

That makes sense. In that case, I think we shouldn't duplicate the
methods on `Device` and `FwNode`, because their signatures would be
different. E.g. `Device::property_present` would have to return
`Option<bool>` while `FwNode::property_present` would return `bool`.
I think it would be cleaner to force driver authors to do something
like `if let Some(fwnode) = dev.fwnode() { /* read properties */ }`.

[PATCH v2 2/5] rust: 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.

The PropertyGuard 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.

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/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 383 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index f6e6c980d..0d4ea3168 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -4,9 +4,17 @@
 //!
 //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
 
-use core::ptr;
+use core::{mem::MaybeUninit, ptr};
 
-use crate::{bindings, device::Device, str::CStr, types::Opaque};
+use crate::{
+    alloc::KVec,
+    bindings, c_str,
+    device::Device,
+    error::{to_result, Result},
+    prelude::*,
+    str::{BStr, CStr, CString},
+    types::Opaque,
+};
 
 impl Device {
     /// Obtain the fwnode corresponding to the device.
@@ -28,6 +36,38 @@ fn fwnode(&self) -> &FwNode {
     pub fn property_present(&self, name: &CStr) -> bool {
         self.fwnode().property_present(name)
     }
+
+    /// Returns firmware property `name` boolean value
+    pub fn property_read_bool(&self, name: &CStr) -> bool {
+        self.fwnode().property_read_bool(name)
+    }
+
+    /// Returns the index of matching string `match_str` for firmware string property `name`
+    pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
+        self.fwnode().property_match_string(name, match_str)
+    }
+
+    /// Returns firmware property `name` integer 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>>> {
+        self.fwnode().property_read_array_vec(name, len)
+    }
+
+    /// Returns integer array length for firmware property `name`
+    pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
+        self.fwnode().property_count_elem::<T>(name)
+    }
+
+    /// Returns firmware property `name` integer scalar value
+    pub fn property_read<'fwnode, 'name, T: Property>(
+        &'fwnode self,
+        name: &'name CStr,
+    ) -> PropertyGuard<'fwnode, 'name, T> {
+        self.fwnode().property_read(name)
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -59,6 +99,150 @@ 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 crate::{device::Device, types::CString};
+    /// fn examples(dev: &Device) -> Result {
+    ///     let fwnode = dev.fwnode();
+    ///     let b: bool = fwnode.property_read("some-bool").required()?;
+    ///     if let Some(s) = fwnode.property_read::<CString>("some-str").optional() {
+    ///         // ...
+    ///     }
+    /// }
+    /// ```
+    pub fn property_read<'fwnode, 'name, T: Property>(
+        &'fwnode self,
+        name: &'name CStr,
+    ) -> PropertyGuard<'fwnode, 'name, T> {
+        PropertyGuard {
+            inner: T::read(self, name),
+            fwnode: self,
+            name,
+        }
+    }
+
+    /// helper used to display name or path of a fwnode
+    ///
+    /// # Safety
+    ///
+    /// Callers must provide a valid format string for a fwnode.
+    unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
+        let mut buf = [0; 256];
+        // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
+        // valid because `self` is valid.
+        let written = unsafe {
+            bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
+        };
+        // SAFETY: `written` is smaller or equal to `buf.len()`.
+        let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
+        write!(f, "{}", BStr::from_bytes(b))
+    }
+
+    /// 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 + use<'_> {
+        struct FwNodeDisplayName<'a>(&'a FwNode);
+
+        impl core::fmt::Display for FwNodeDisplayName<'_> {
+            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+                // SAFETY: "%pfwP" is a valid format string for fwnode
+                unsafe { self.0.fmt(f, c_str!("%pfwP")) }
+            }
+        }
+
+        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 + use<'_> {
+        struct FwNodeDisplayPath<'a>(&'a FwNode);
+
+        impl core::fmt::Display for FwNodeDisplayPath<'_> {
+            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+                // SAFETY: "%pfwf" is a valid format string for fwnode
+                unsafe { self.0.fmt(f, c_str!("%pfwf")) }
+            }
+        }
+
+        FwNodeDisplayPath(self)
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
@@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
         unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
     }
 }
+
+/// 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(fwnode: &FwNode, name: &CStr) -> Result<Self>;
+}
+
+impl Property for CString {
+    fn read(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(
+        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(
+                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(fwnode.as_raw(), name.as_char_ptr(), out_param, len) }
+}
+impl<T: PropertyInt, const N: usize> Property for [T; N] {
+    fn read(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(fwnode: &FwNode, name: &CStr) -> Result<Self> {
+        let val: [_; 1] = <[T; 1] as Property>::read(fwnode, name)?;
+        Ok(val[0])
+    }
+}
+
+/// A helper for reading device properties.
+///
+/// Use [`Self::required`] if a missing property is considered a bug and
+/// [`Self::optional`] otherwise.
+///
+/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
+pub struct PropertyGuard<'fwnode, 'name, T> {
+    /// The result of reading the property.
+    inner: Result<T>,
+    /// The fwnode of the property, used for logging in the "required" case.
+    fwnode: &'fwnode FwNode,
+    /// The name of the property, used for logging in the "required" case.
+    name: &'name CStr,
+}
+
+impl<T> PropertyGuard<'_, '_, T> {
+    /// Access the property, indicating it is required.
+    ///
+    /// If the property is not present, the error is automatically logged. If a
+    /// missing property is not an error, use [`Self::optional`] instead.
+    pub fn required(self) -> Result<T> {
+        if self.inner.is_err() {
+            // Get the device associated with the fwnode for device-associated
+            // logging.
+            // TODO: Are we allowed to do this? The field `fwnode_handle.dev`
+            // has a somewhat vague comment, which could mean we're not
+            // supposed to access it:
+            // https://elixir.bootlin.com/linux/v6.13.6/source/include/linux/fwnode.h#L51
+            // SAFETY: According to the invariant of FwNode, it is valid.
+            let dev = unsafe { (*self.fwnode.as_raw()).dev };
+
+            if dev.is_null() {
+                pr_err!(
+                    "{}: property '{}' is missing\n",
+                    self.fwnode.display_path(),
+                    self.name
+                );
+            } else {
+                // SAFETY: If dev is not null, it points to a valid device.
+                let dev: &Device = unsafe { &*dev.cast() };
+                dev_err!(
+                    dev,
+                    "{}: property '{}' is missing\n",
+                    self.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

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Danilo Krummrich]

On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch wrote:
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
> 
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> types of properties where appropriate.
> 
> The PropertyGuard 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.

The patch adds a lot of thing, i.e.
  * implement PropertyInt
  * implement PropertyGuard
  * extend FwNode by a lot of functions
  * extend Device by some property functions

I see that from v1 a lot of things have been squashed, likely because there are
a few circular dependencies. Is there really no reasonable way to break this
down a bit?

> 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/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 383 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index f6e6c980d..0d4ea3168 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -4,9 +4,17 @@
>  //!
>  //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
>  
> -use core::ptr;
> +use core::{mem::MaybeUninit, ptr};
>  
> -use crate::{bindings, device::Device, str::CStr, types::Opaque};
> +use crate::{
> +    alloc::KVec,
> +    bindings, c_str,
> +    device::Device,
> +    error::{to_result, Result},
> +    prelude::*,
> +    str::{BStr, CStr, CString},
> +    types::Opaque,
> +};
>  
>  impl Device {
>      /// Obtain the fwnode corresponding to the device.
> @@ -28,6 +36,38 @@ fn fwnode(&self) -> &FwNode {
>      pub fn property_present(&self, name: &CStr) -> bool {
>          self.fwnode().property_present(name)
>      }
> +
> +    /// Returns firmware property `name` boolean value
> +    pub fn property_read_bool(&self, name: &CStr) -> bool {
> +        self.fwnode().property_read_bool(name)
> +    }
> +
> +    /// Returns the index of matching string `match_str` for firmware string property `name`
> +    pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> +        self.fwnode().property_match_string(name, match_str)
> +    }
> +
> +    /// Returns firmware property `name` integer 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>>> {
> +        self.fwnode().property_read_array_vec(name, len)
> +    }
> +
> +    /// Returns integer array length for firmware property `name`
> +    pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
> +        self.fwnode().property_count_elem::<T>(name)
> +    }
> +
> +    /// Returns firmware property `name` integer scalar value
> +    pub fn property_read<'fwnode, 'name, T: Property>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, T> {
> +        self.fwnode().property_read(name)
> +    }
>  }

Okay, I start to see why you have all those Device functions in a separate file.
:)

I think we should move device.rs into its own module, i.e.
rust/kernel/device/mod.rs and then create rust/kernel/device/property.rs.

>  
>  /// A reference-counted fwnode_handle.
> @@ -59,6 +99,150 @@ 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 crate::{device::Device, types::CString};
> +    /// fn examples(dev: &Device) -> Result {
> +    ///     let fwnode = dev.fwnode();
> +    ///     let b: bool = fwnode.property_read("some-bool").required()?;
> +    ///     if let Some(s) = fwnode.property_read::<CString>("some-str").optional() {
> +    ///         // ...
> +    ///     }
> +    /// }
> +    /// ```
> +    pub fn property_read<'fwnode, 'name, T: Property>(
> +        &'fwnode self,
> +        name: &'name CStr,
> +    ) -> PropertyGuard<'fwnode, 'name, T> {
> +        PropertyGuard {
> +            inner: T::read(self, name),
> +            fwnode: self,
> +            name,
> +        }
> +    }
> +
> +    /// helper used to display name or path of a fwnode
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must provide a valid format string for a fwnode.
> +    unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
> +        let mut buf = [0; 256];
> +        // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
> +        // valid because `self` is valid.
> +        let written = unsafe {
> +            bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
> +        };

Why do we need this? Can't we use write! right away?

> +        // SAFETY: `written` is smaller or equal to `buf.len()`.
> +        let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
> +        write!(f, "{}", BStr::from_bytes(b))
> +    }
> +
> +    /// 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 + use<'_> {
> +        struct FwNodeDisplayName<'a>(&'a FwNode);
> +
> +        impl core::fmt::Display for FwNodeDisplayName<'_> {
> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> +                // SAFETY: "%pfwP" is a valid format string for fwnode
> +                unsafe { self.0.fmt(f, c_str!("%pfwP")) }
> +            }
> +        }
> +
> +        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 + use<'_> {
> +        struct FwNodeDisplayPath<'a>(&'a FwNode);
> +
> +        impl core::fmt::Display for FwNodeDisplayPath<'_> {
> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> +                // SAFETY: "%pfwf" is a valid format string for fwnode
> +                unsafe { self.0.fmt(f, c_str!("%pfwf")) }
> +            }
> +        }
> +
> +        FwNodeDisplayPath(self)
> +    }
>  }
>  
>  // SAFETY: Instances of `FwNode` are always reference-counted.
> @@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>      }
>  }
> +
> +/// 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(fwnode: &FwNode, name: &CStr) -> Result<Self>;
> +}
> +
> +impl Property for CString {
> +    fn read(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()?)
> +    }
> +}

I think it would be pretty weird to have a function CString::read() that takes a
FwNode argument, no? Same for all the other types below.

I assume you do this for

	pub fn property_read<'fwnode, 'name, T: Property>(
	   &'fwnode self,
	   name: &'name CStr,
	)

but given that you have to do the separate impls anyways, is there so much value
having the generic variant? You could still generate all the
property_read_{int}() variants with a macro.

If you really want a generic property_read(), I think you should create new
types instead and implement the Property trait for them instead.

> +/// 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(
> +        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(
> +                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(fwnode.as_raw(), name.as_char_ptr(), out_param, len) }
> +}
> +impl<T: PropertyInt, const N: usize> Property for [T; N] {
> +    fn read(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(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> +        let val: [_; 1] = <[T; 1] as Property>::read(fwnode, name)?;
> +        Ok(val[0])
> +    }
> +}
> +
> +/// A helper for reading device properties.
> +///
> +/// Use [`Self::required`] if a missing property is considered a bug and
> +/// [`Self::optional`] otherwise.
> +///
> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> +pub struct PropertyGuard<'fwnode, 'name, T> {
> +    /// The result of reading the property.
> +    inner: Result<T>,
> +    /// The fwnode of the property, used for logging in the "required" case.
> +    fwnode: &'fwnode FwNode,
> +    /// The name of the property, used for logging in the "required" case.
> +    name: &'name CStr,
> +}
> +
> +impl<T> PropertyGuard<'_, '_, T> {
> +    /// Access the property, indicating it is required.
> +    ///
> +    /// If the property is not present, the error is automatically logged. If a
> +    /// missing property is not an error, use [`Self::optional`] instead.
> +    pub fn required(self) -> Result<T> {
> +        if self.inner.is_err() {
> +            // Get the device associated with the fwnode for device-associated
> +            // logging.
> +            // TODO: Are we allowed to do this? The field `fwnode_handle.dev`
> +            // has a somewhat vague comment, which could mean we're not
> +            // supposed to access it:
> +            // https://elixir.bootlin.com/linux/v6.13.6/source/include/linux/fwnode.h#L51
> +            // SAFETY: According to the invariant of FwNode, it is valid.
> +            let dev = unsafe { (*self.fwnode.as_raw()).dev };

I don't think this is valid it do, AFAICS, a firmware node handle doesn't own a
reference to the device pointer.

> +
> +            if dev.is_null() {
> +                pr_err!(
> +                    "{}: property '{}' is missing\n",
> +                    self.fwnode.display_path(),
> +                    self.name
> +                );
> +            } else {
> +                // SAFETY: If dev is not null, it points to a valid device.
> +                let dev: &Device = unsafe { &*dev.cast() };
> +                dev_err!(
> +                    dev,
> +                    "{}: property '{}' is missing\n",
> +                    self.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
> 

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Danilo Krummrich]

On Mon, Apr 14, 2025 at 07:44:36PM +0200, Danilo Krummrich wrote:
> On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch wrote:
> > The device property API is a firmware agnostic API for reading
> > properties from firmware (DT/ACPI) devices nodes and swnodes.
> > 
> > While the C API takes a pointer to a caller allocated variable/buffer,
> > the rust API is designed to return a value and can be used in struct
> > initialization. Rust generics are also utilized to support different
> > types of properties where appropriate.
> > 
> > The PropertyGuard 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.
> 
> The patch adds a lot of thing, i.e.
>   * implement PropertyInt

I meant the Property trait and all its impls of course. :)

>   * implement PropertyGuard
>   * extend FwNode by a lot of functions
>   * extend Device by some property functions

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Remo Senekowitsch]

On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote:
> On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch wrote:
>> The device property API is a firmware agnostic API for reading
>> properties from firmware (DT/ACPI) devices nodes and swnodes.
>> 
>> While the C API takes a pointer to a caller allocated variable/buffer,
>> the rust API is designed to return a value and can be used in struct
>> initialization. Rust generics are also utilized to support different
>> types of properties where appropriate.
>> 
>> The PropertyGuard 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.
>
> The patch adds a lot of thing, i.e.
>   * implement PropertyInt
>   * implement PropertyGuard
>   * extend FwNode by a lot of functions
>   * extend Device by some property functions
>
> I see that from v1 a lot of things have been squashed, likely because there are
> a few circular dependencies. Is there really no reasonable way to break this
> down a bit?

I was explicitly asked to do this in the previous thread[1]. I'm happy
to invest time into organizing files and commits exactly the way people
want, but squashing and splitting the same commits back and forth
between subsequent patch series is a waste of my time.

Do reviewers not typically read the review comments of others as well?
What can I do to avoid this situation and make progress instead of
running in circles?

Link: https://lore.kernel.org/rust-for-linux/20250326171411.590681-1-remo@buenzli.dev/T/#m68b99b283a2e62726ee039bb2394d0741b31e330 [1]

>> +    /// helper used to display name or path of a fwnode
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must provide a valid format string for a fwnode.
>> +    unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
>> +        let mut buf = [0; 256];
>> +        // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
>> +        // valid because `self` is valid.
>> +        let written = unsafe {
>> +            bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
>> +        };
>
> Why do we need this? Can't we use write! right away?

I don't know how, can you be more specific? I'm not too familiar with
how these formatting specifiers work under the hood, but on the face of
it, Rust and C seem very different.

>> +        // SAFETY: `written` is smaller or equal to `buf.len()`.
>> +        let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
>> +        write!(f, "{}", BStr::from_bytes(b))
>> +    }
>> +
>> +    /// 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 + use<'_> {
>> +        struct FwNodeDisplayName<'a>(&'a FwNode);
>> +
>> +        impl core::fmt::Display for FwNodeDisplayName<'_> {
>> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
>> +                // SAFETY: "%pfwP" is a valid format string for fwnode
>> +                unsafe { self.0.fmt(f, c_str!("%pfwP")) }
>> +            }
>> +        }
>> +
>> +        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 + use<'_> {
>> +        struct FwNodeDisplayPath<'a>(&'a FwNode);
>> +
>> +        impl core::fmt::Display for FwNodeDisplayPath<'_> {
>> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
>> +                // SAFETY: "%pfwf" is a valid format string for fwnode
>> +                unsafe { self.0.fmt(f, c_str!("%pfwf")) }
>> +            }
>> +        }
>> +
>> +        FwNodeDisplayPath(self)
>> +    }
>>  }
>>  
>>  // SAFETY: Instances of `FwNode` are always reference-counted.
>> @@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
>>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
>>      }
>>  }
>> +
>> +/// 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(fwnode: &FwNode, name: &CStr) -> Result<Self>;
>> +}
>> +
>> +impl Property for CString {
>> +    fn read(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()?)
>> +    }
>> +}
>
> I think it would be pretty weird to have a function CString::read() that takes a
> FwNode argument, no? Same for all the other types below.
>
> I assume you do this for
>
> 	pub fn property_read<'fwnode, 'name, T: Property>(
> 	   &'fwnode self,
> 	   name: &'name CStr,
> 	)
>
> but given that you have to do the separate impls anyways, is there so much value
> having the generic variant? You could still generate all the
> property_read_{int}() variants with a macro.
>
> If you really want a generic property_read(), I think you should create new
> types instead and implement the Property trait for them instead.

Yeah, that would be workable. On the other hand, it's not unusual in
Rust to implement traits on foreign types, right? If the problem is
the non-descriptive name "read" then we can change it to something more
verbose. Maybe `CStr::read_from_fwnode_property` or something. It's not
meant to be used directly, a verbose name wouldn't cause any damage.

I think making the function generic can be nice to work with, especially
if type inference is working. But I'm fine with either.

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Danilo Krummrich]

On Tue, Apr 15, 2025 at 01:55:42AM +0200, Remo Senekowitsch wrote:
> On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote:
> > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch wrote:
> >> The device property API is a firmware agnostic API for reading
> >> properties from firmware (DT/ACPI) devices nodes and swnodes.
> >> 
> >> While the C API takes a pointer to a caller allocated variable/buffer,
> >> the rust API is designed to return a value and can be used in struct
> >> initialization. Rust generics are also utilized to support different
> >> types of properties where appropriate.
> >> 
> >> The PropertyGuard 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.
> >
> > The patch adds a lot of thing, i.e.
> >   * implement PropertyInt
> >   * implement PropertyGuard
> >   * extend FwNode by a lot of functions
> >   * extend Device by some property functions
> >
> > I see that from v1 a lot of things have been squashed, likely because there are
> > a few circular dependencies. Is there really no reasonable way to break this
> > down a bit?
> 
> I was explicitly asked to do this in the previous thread[1].

I'm well aware that you were asked to do so and that one reason was that
subsequent patches started deleting code that was added in previous ones
(hence my suspicion of circular dependencies and that splitting up things might
not be super trivial).

> I'm happy
> to invest time into organizing files and commits exactly the way people
> want, but squashing and splitting the same commits back and forth
> between subsequent patch series is a waste of my time.

I don't think you were asked to go back and forth, but whether you see a
reasonable way to break things down a bit, where "reasonable" means without
deleting code that was just added.

> Do reviewers not typically read the review comments of others as well?

I think mostly they do, but maintainers and reviewers are rather busy people.
So, I don't think you can expect everyone to follow every thread, especially
when they get lengthy.

> What can I do to avoid this situation and make progress instead of
> running in circles?

I suggest to investigate whether it can be split it up in a reasonable way and
subsequently answer the question.

With your contribution you attempt to add a rather large portion of pretty core
code. This isn't an easy task and quite some discussion is totally expected;
please don't get frustrated, the series goes pretty well. :)

> 
> Link: https://lore.kernel.org/rust-for-linux/20250326171411.590681-1-remo@buenzli.dev/T/#m68b99b283a2e62726ee039bb2394d0741b31e330 [1]
> 
> >> +    /// helper used to display name or path of a fwnode
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// Callers must provide a valid format string for a fwnode.
> >> +    unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
> >> +        let mut buf = [0; 256];
> >> +        // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
> >> +        // valid because `self` is valid.
> >> +        let written = unsafe {
> >> +            bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
> >> +        };
> >
> > Why do we need this? Can't we use write! right away?
> 
> I don't know how, can you be more specific? I'm not too familiar with
> how these formatting specifiers work under the hood, but on the face of
> it, Rust and C seem very different.

See below.

> 
> >> +        // SAFETY: `written` is smaller or equal to `buf.len()`.
> >> +        let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
> >> +        write!(f, "{}", BStr::from_bytes(b))
> >> +    }
> >> +
> >> +    /// 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 + use<'_> {
> >> +        struct FwNodeDisplayName<'a>(&'a FwNode);
> >> +
> >> +        impl core::fmt::Display for FwNodeDisplayName<'_> {
> >> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> >> +                // SAFETY: "%pfwP" is a valid format string for fwnode
> >> +                unsafe { self.0.fmt(f, c_str!("%pfwP")) }

I think this could just use write!() and fwnode_get_name(), right?

> >> +            }
> >> +        }
> >> +
> >> +        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 + use<'_> {
> >> +        struct FwNodeDisplayPath<'a>(&'a FwNode);
> >> +
> >> +        impl core::fmt::Display for FwNodeDisplayPath<'_> {
> >> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> >> +                // SAFETY: "%pfwf" is a valid format string for fwnode
> >> +                unsafe { self.0.fmt(f, c_str!("%pfwf")) }

This one is indeed a bit more tricky, because it comes from
fwnode_full_name_string() in lib/vsprintf.c.

Maybe it would be better to replicate the loop within fwnode_full_name_string()
and call write! from there.

> >> +            }
> >> +        }
> >> +
> >> +        FwNodeDisplayPath(self)
> >> +    }
> >>  }
> >>  
> >>  // SAFETY: Instances of `FwNode` are always reference-counted.
> >> @@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> >>      }
> >>  }
> >> +
> >> +/// 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(fwnode: &FwNode, name: &CStr) -> Result<Self>;
> >> +}
> >> +
> >> +impl Property for CString {
> >> +    fn read(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()?)
> >> +    }
> >> +}
> >
> > I think it would be pretty weird to have a function CString::read() that takes a
> > FwNode argument, no? Same for all the other types below.
> >
> > I assume you do this for
> >
> > 	pub fn property_read<'fwnode, 'name, T: Property>(
> > 	   &'fwnode self,
> > 	   name: &'name CStr,
> > 	)
> >
> > but given that you have to do the separate impls anyways, is there so much value
> > having the generic variant? You could still generate all the
> > property_read_{int}() variants with a macro.
> >
> > If you really want a generic property_read(), I think you should create new
> > types instead and implement the Property trait for them instead.
> 
> Yeah, that would be workable. On the other hand, it's not unusual in
> Rust to implement traits on foreign types, right? If the problem is
> the non-descriptive name "read" then we can change it to something more
> verbose. Maybe `CStr::read_from_fwnode_property` or something. It's not
> meant to be used directly, a verbose name wouldn't cause any damage.

Yeah, if we keep this approach, I'd prefer a more descriptive name.

However, I'd like to hear some more opinions from other members of the Rust team
on this one.

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Remo Senekowitsch]

On Tue Apr 15, 2025 at 11:48 AM CEST, Danilo Krummrich wrote:
> On Tue, Apr 15, 2025 at 01:55:42AM +0200, Remo Senekowitsch wrote:
>> On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote:
>> > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch wrote:
>> >> The device property API is a firmware agnostic API for reading
>> >> properties from firmware (DT/ACPI) devices nodes and swnodes.
>> >> 
>> >> While the C API takes a pointer to a caller allocated variable/buffer,
>> >> the rust API is designed to return a value and can be used in struct
>> >> initialization. Rust generics are also utilized to support different
>> >> types of properties where appropriate.
>> >> 
>> >> The PropertyGuard 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.
>> >
>> > The patch adds a lot of thing, i.e.
>> >   * implement PropertyInt
>> >   * implement PropertyGuard
>> >   * extend FwNode by a lot of functions
>> >   * extend Device by some property functions
>> >
>> > I see that from v1 a lot of things have been squashed, likely because there are
>> > a few circular dependencies. Is there really no reasonable way to break this
>> > down a bit?
>> 
>> I was explicitly asked to do this in the previous thread[1].
>
> I'm well aware that you were asked to do so and that one reason was that
> subsequent patches started deleting code that was added in previous ones
> (hence my suspicion of circular dependencies and that splitting up things might
> not be super trivial).
>
>> I'm happy
>> to invest time into organizing files and commits exactly the way people
>> want, but squashing and splitting the same commits back and forth
>> between subsequent patch series is a waste of my time.
>
> I don't think you were asked to go back and forth, but whether you see a
> reasonable way to break things down a bit, where "reasonable" means without
> deleting code that was just added.

I was asked to squash two specific commits. The first was making the
read method generic. That was the one that deleted much code. Totally
reasonable, and the generic stuff might be discarded anyway, so I won't
be moving stuff back and forth.

However, the second commit was the one introducing PropertyGuard. That
was a beautifully atomic commit, no circular dependencies in sight. If
this commit is to be split up, one of the smaller ones would without
doubt look exactly the same as the one before. I provided a link[1]
to the exact email telling me to squash that exact patch to avoid any
confusion.

>> Do reviewers not typically read the review comments of others as well?
>
> I think mostly they do, but maintainers and reviewers are rather busy people.
> So, I don't think you can expect everyone to follow every thread, especially
> when they get lengthy.
>
>> What can I do to avoid this situation and make progress instead of
>> running in circles?
>
> I suggest to investigate whether it can be split it up in a reasonable way and
> subsequently answer the question.

The point is that I agree with you that the PropertyGuard patch can be
split out. And possibly more: I think a reasonable person could make a
separate commit for every `property_read_<type>` method. And I'm happy
to do that if it's the way to go.  But if I do that, send a v3 and then
someone else asks me to squash it again (because they didn't read this
exchange between you and me...) then I would've wasted my time.

> With your contribution you attempt to add a rather large portion of pretty core
> code. This isn't an easy task and quite some discussion is totally expected;
> please don't get frustrated, the series goes pretty well. :)

I'm trying, but I can't say I have a good first impression of doing
code review over a mailing list. Doing open-heart surgery with a
pickfork and writing documents in Microsoft Word both seem like more
productive and enjoyable activities to me.
 
Link: https://lore.kernel.org/rust-for-linux/20250326171411.590681-1-remo@buenzli.dev/T/#m68b99b283a2e62726ee039bb2394d0741b31e330 [1]

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Rob Herring]

On Tue, Apr 15, 2025 at 6:11 AM Remo Senekowitsch <remo@buenzli.dev> wrote:
>
> On Tue Apr 15, 2025 at 11:48 AM CEST, Danilo Krummrich wrote:
> > On Tue, Apr 15, 2025 at 01:55:42AM +0200, Remo Senekowitsch wrote:
> >> On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote:
> >> > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch wrote:
> >> >> The device property API is a firmware agnostic API for reading
> >> >> properties from firmware (DT/ACPI) devices nodes and swnodes.
> >> >>
> >> >> While the C API takes a pointer to a caller allocated variable/buffer,
> >> >> the rust API is designed to return a value and can be used in struct
> >> >> initialization. Rust generics are also utilized to support different
> >> >> types of properties where appropriate.
> >> >>
> >> >> The PropertyGuard 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.
> >> >
> >> > The patch adds a lot of thing, i.e.
> >> >   * implement PropertyInt
> >> >   * implement PropertyGuard
> >> >   * extend FwNode by a lot of functions
> >> >   * extend Device by some property functions
> >> >
> >> > I see that from v1 a lot of things have been squashed, likely because there are
> >> > a few circular dependencies. Is there really no reasonable way to break this
> >> > down a bit?
> >>
> >> I was explicitly asked to do this in the previous thread[1].
> >
> > I'm well aware that you were asked to do so and that one reason was that
> > subsequent patches started deleting code that was added in previous ones
> > (hence my suspicion of circular dependencies and that splitting up things might
> > not be super trivial).
> >
> >> I'm happy
> >> to invest time into organizing files and commits exactly the way people
> >> want, but squashing and splitting the same commits back and forth
> >> between subsequent patch series is a waste of my time.
> >
> > I don't think you were asked to go back and forth, but whether you see a
> > reasonable way to break things down a bit, where "reasonable" means without
> > deleting code that was just added.
>
> I was asked to squash two specific commits. The first was making the
> read method generic. That was the one that deleted much code. Totally
> reasonable, and the generic stuff might be discarded anyway, so I won't
> be moving stuff back and forth.
>
> However, the second commit was the one introducing PropertyGuard. That
> was a beautifully atomic commit, no circular dependencies in sight. If
> this commit is to be split up, one of the smaller ones would without
> doubt look exactly the same as the one before. I provided a link[1]
> to the exact email telling me to squash that exact patch to avoid any
> confusion.

Adding PropertyGuard changes the API. I don't think it makes sense to
review the API without it and then again with it.

It might be reasonable to split adding the fwnode bindings and then
the Device version. But those are in separate files anyway, so it's
not really making review easier.

> >> Do reviewers not typically read the review comments of others as well?
> >
> > I think mostly they do, but maintainers and reviewers are rather busy people.
> > So, I don't think you can expect everyone to follow every thread, especially
> > when they get lengthy.

Please understand maintainers are reviewing 10s to 100s of patches a
week. I don't necessarily remember my own comments from last week.

> >> What can I do to avoid this situation and make progress instead of
> >> running in circles?
> >
> > I suggest to investigate whether it can be split it up in a reasonable way and
> > subsequently answer the question.
>
> The point is that I agree with you that the PropertyGuard patch can be
> split out. And possibly more: I think a reasonable person could make a
> separate commit for every `property_read_<type>` method. And I'm happy
> to do that if it's the way to go.  But if I do that, send a v3 and then
> someone else asks me to squash it again (because they didn't read this
> exchange between you and me...) then I would've wasted my time.

A commit per method really seems excessive to me. I prefer to look at
the API as a whole, and to do that with separate patches only makes
that harder.

A reasonable split here is possibly splitting the fwnode and the
Device versions of the API. In any case, I think we've discussed this
enough and I don't care to discuss it more, so whatever reasonable
split you come up with is fine with me.

> > With your contribution you attempt to add a rather large portion of pretty core
> > code. This isn't an easy task and quite some discussion is totally expected;
> > please don't get frustrated, the series goes pretty well. :)
>
> I'm trying, but I can't say I have a good first impression of doing
> code review over a mailing list. Doing open-heart surgery with a
> pickfork and writing documents in Microsoft Word both seem like more
> productive and enjoyable activities to me.

Mailing lists are a good scapegoat, but you can have 2 reviewers with
2 different opinions anywhere. That's just the nature of a large,
distributed project with lots of reviewers.

Rob

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Danilo Krummrich]

On Tue, Apr 15, 2025 at 07:46:19AM -0500, Rob Herring wrote:
> 
> A reasonable split here is possibly splitting the fwnode and the
> Device versions of the API. In any case, I think we've discussed this
> enough and I don't care to discuss it more, so whatever reasonable
> split you come up with is fine with me.

+1

Let's wait for some more feedback on the Property trait impl for primitives,
CString, etc. before moving on.

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Remo Senekowitsch]

On Tue Apr 15, 2025 at 2:46 PM CEST, Rob Herring wrote:
>
> Adding PropertyGuard changes the API. I don't think it makes sense to
> review the API without it and then again with it.

We could add the PropertyGuard first and introduce the users later.
Then they are in separate patches and the API doesn't change.

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Gary Guo]

On Tue, 15 Apr 2025 11:48:07 +0200
Danilo Krummrich <dakr@kernel.org> wrote:

> On Tue, Apr 15, 2025 at 01:55:42AM +0200, Remo Senekowitsch wrote:
> > On Mon Apr 14, 2025 at 7:44 PM CEST, Danilo Krummrich wrote:  
> > > On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch wrote:  
> > >> The device property API is a firmware agnostic API for reading
> > >> properties from firmware (DT/ACPI) devices nodes and swnodes.
> > >> 
> > >> While the C API takes a pointer to a caller allocated variable/buffer,
> > >> the rust API is designed to return a value and can be used in struct
> > >> initialization. Rust generics are also utilized to support different
> > >> types of properties where appropriate.
> > >> 
> > >> The PropertyGuard 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.  
> > >
> > > The patch adds a lot of thing, i.e.
> > >   * implement PropertyInt
> > >   * implement PropertyGuard
> > >   * extend FwNode by a lot of functions
> > >   * extend Device by some property functions
> > >
> > > I see that from v1 a lot of things have been squashed, likely because there are
> > > a few circular dependencies. Is there really no reasonable way to break this
> > > down a bit?  
> > 
> > I was explicitly asked to do this in the previous thread[1].  
> 
> I'm well aware that you were asked to do so and that one reason was that
> subsequent patches started deleting code that was added in previous ones
> (hence my suspicion of circular dependencies and that splitting up things might
> not be super trivial).
> 
> > I'm happy
> > to invest time into organizing files and commits exactly the way people
> > want, but squashing and splitting the same commits back and forth
> > between subsequent patch series is a waste of my time.  
> 
> I don't think you were asked to go back and forth, but whether you see a
> reasonable way to break things down a bit, where "reasonable" means without
> deleting code that was just added.
> 
> > Do reviewers not typically read the review comments of others as well?  
> 
> I think mostly they do, but maintainers and reviewers are rather busy people.
> So, I don't think you can expect everyone to follow every thread, especially
> when they get lengthy.
> 
> > What can I do to avoid this situation and make progress instead of
> > running in circles?  
> 
> I suggest to investigate whether it can be split it up in a reasonable way and
> subsequently answer the question.
> 
> With your contribution you attempt to add a rather large portion of pretty core
> code. This isn't an easy task and quite some discussion is totally expected;
> please don't get frustrated, the series goes pretty well. :)
> 
> > 
> > Link: https://lore.kernel.org/rust-for-linux/20250326171411.590681-1-remo@buenzli.dev/T/#m68b99b283a2e62726ee039bb2394d0741b31e330 [1]
> >   
> > >> +    /// helper used to display name or path of a fwnode
> > >> +    ///
> > >> +    /// # Safety
> > >> +    ///
> > >> +    /// Callers must provide a valid format string for a fwnode.
> > >> +    unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
> > >> +        let mut buf = [0; 256];
> > >> +        // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
> > >> +        // valid because `self` is valid.
> > >> +        let written = unsafe {
> > >> +            bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
> > >> +        };  
> > >
> > > Why do we need this? Can't we use write! right away?  
> > 
> > I don't know how, can you be more specific? I'm not too familiar with
> > how these formatting specifiers work under the hood, but on the face of
> > it, Rust and C seem very different.  
> 
> See below.
> 
> >   
> > >> +        // SAFETY: `written` is smaller or equal to `buf.len()`.
> > >> +        let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
> > >> +        write!(f, "{}", BStr::from_bytes(b))
> > >> +    }
> > >> +
> > >> +    /// 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 + use<'_> {
> > >> +        struct FwNodeDisplayName<'a>(&'a FwNode);
> > >> +
> > >> +        impl core::fmt::Display for FwNodeDisplayName<'_> {
> > >> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> > >> +                // SAFETY: "%pfwP" is a valid format string for fwnode
> > >> +                unsafe { self.0.fmt(f, c_str!("%pfwP")) }  
> 
> I think this could just use write!() and fwnode_get_name(), right?
> 
> > >> +            }
> > >> +        }
> > >> +
> > >> +        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 + use<'_> {
> > >> +        struct FwNodeDisplayPath<'a>(&'a FwNode);
> > >> +
> > >> +        impl core::fmt::Display for FwNodeDisplayPath<'_> {
> > >> +            fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> > >> +                // SAFETY: "%pfwf" is a valid format string for fwnode
> > >> +                unsafe { self.0.fmt(f, c_str!("%pfwf")) }  
> 
> This one is indeed a bit more tricky, because it comes from
> fwnode_full_name_string() in lib/vsprintf.c.
> 
> Maybe it would be better to replicate the loop within fwnode_full_name_string()
> and call write! from there.
> 
> > >> +            }
> > >> +        }
> > >> +
> > >> +        FwNodeDisplayPath(self)
> > >> +    }
> > >>  }
> > >>  
> > >>  // SAFETY: Instances of `FwNode` are always reference-counted.
> > >> @@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> > >>          unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> > >>      }
> > >>  }
> > >> +
> > >> +/// 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(fwnode: &FwNode, name: &CStr) -> Result<Self>;
> > >> +}
> > >> +
> > >> +impl Property for CString {
> > >> +    fn read(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()?)
> > >> +    }
> > >> +}  
> > >
> > > I think it would be pretty weird to have a function CString::read() that takes a
> > > FwNode argument, no? Same for all the other types below.
> > >
> > > I assume you do this for
> > >
> > > 	pub fn property_read<'fwnode, 'name, T: Property>(
> > > 	   &'fwnode self,
> > > 	   name: &'name CStr,
> > > 	)
> > >
> > > but given that you have to do the separate impls anyways, is there so much value
> > > having the generic variant? You could still generate all the
> > > property_read_{int}() variants with a macro.
> > >
> > > If you really want a generic property_read(), I think you should create new
> > > types instead and implement the Property trait for them instead.  
> > 
> > Yeah, that would be workable. On the other hand, it's not unusual in
> > Rust to implement traits on foreign types, right? If the problem is
> > the non-descriptive name "read" then we can change it to something more
> > verbose. Maybe `CStr::read_from_fwnode_property` or something. It's not
> > meant to be used directly, a verbose name wouldn't cause any damage.  
> 
> Yeah, if we keep this approach, I'd prefer a more descriptive name.
> 
> However, I'd like to hear some more opinions from other members of the Rust team
> on this one.

I think the trait approach is what people quite typically use. The
additional functions are usually not an issue, as they won't be
available if you don't import the trait. Using names like
`read_from_fwnode_property` would be remove any concern for me.

Best,
Gary

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Dirk Behme]

Hi Remo,

On 14/04/2025 17:26, Remo Senekowitsch wrote:
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
> 
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> types of properties where appropriate.
> 
> The PropertyGuard 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.
> 
> 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/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 383 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index f6e6c980d..0d4ea3168 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
...
> +    /// helper used to display name or path of a fwnode
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must provide a valid format string for a fwnode.
> +    unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
> +        let mut buf = [0; 256];
> +        // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
> +        // valid because `self` is valid.
> +        let written = unsafe {
> +            bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
> +        };
> +        // SAFETY: `written` is smaller or equal to `buf.len()`.
> +        let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
> +        write!(f, "{}", BStr::from_bytes(b))
> +    }
> +
> +    /// 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 + use<'_> {


I don't know about the details but with rustc 1.81 [1] I'm getting [2].
Just doing what is proposed seems to "fix" it:

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 3b1af034e902e..eadf7501d499b 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -26,6 +26,7 @@
 #![feature(const_mut_refs)]
 #![feature(const_ptr_write)]
 #![feature(const_refs_to_cell)]
+#![feature(precise_capturing)]

 // Ensure conditional compilation based on the kernel configuration works;
 // otherwise we may silently break things like initcall handling.

Just want to mention it because I think the minimal rustc version we
have to support is 1.78.

Best regards

Dirk

P.S.: Many thanks for working on this! :)

[1]

$ rustc --version
rustc 1.81.0 (eeb90cda1 2024-09-04)

[2]

error[E0658]: precise captures on `impl Trait` are experimental
   --> rust/kernel/property.rs:256:61
    |
256 |     pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
    |                                                             ^^^
    |
    = note: see issue #123432
<https://github.com/rust-lang/rust/issues/123432> for more information
    = help: add `#![feature(precise_capturing)]` to the crate attributes
to enable
    = note: this compiler was built on 2024-09-04; consider upgrading it
if it is out of date

error[E0658]: precise captures on `impl Trait` are experimental
   --> rust/kernel/property.rs:271:61
    |
271 |     pub fn display_path(&self) -> impl core::fmt::Display + use<'_> {
    |                                                             ^^^
    |
    = note: see issue #123432
<https://github.com/rust-lang/rust/issues/123432> for more information
    = help: add `#![feature(precise_capturing)]` to the crate attributes
to enable
    = note: this compiler was built on 2024-09-04; consider upgrading it
if it is out of date

error: aborting due to 2 previous errors

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Remo Senekowitsch]

Hi Dirk,

On Wed Apr 23, 2025 at 2:29 PM CEST, Dirk Behme wrote:
> Hi Remo,
>
> On 14/04/2025 17:26, Remo Senekowitsch wrote:
>> The device property API is a firmware agnostic API for reading
>> properties from firmware (DT/ACPI) devices nodes and swnodes.
>> 
>> While the C API takes a pointer to a caller allocated variable/buffer,
>> the rust API is designed to return a value and can be used in struct
>> initialization. Rust generics are also utilized to support different
>> types of properties where appropriate.
>> 
>> The PropertyGuard 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.
>> 
>> 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/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 383 insertions(+), 2 deletions(-)
>> 
>> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
>> index f6e6c980d..0d4ea3168 100644
>> --- a/rust/kernel/property.rs
>> +++ b/rust/kernel/property.rs
> ...
>> +    /// helper used to display name or path of a fwnode
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must provide a valid format string for a fwnode.
>> +    unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
>> +        let mut buf = [0; 256];
>> +        // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
>> +        // valid because `self` is valid.
>> +        let written = unsafe {
>> +            bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
>> +        };
>> +        // SAFETY: `written` is smaller or equal to `buf.len()`.
>> +        let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
>> +        write!(f, "{}", BStr::from_bytes(b))
>> +    }
>> +
>> +    /// 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 + use<'_> {
>
>
> I don't know about the details but with rustc 1.81 [1] I'm getting [2].
> Just doing what is proposed seems to "fix" it:
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 3b1af034e902e..eadf7501d499b 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -26,6 +26,7 @@
>  #![feature(const_mut_refs)]
>  #![feature(const_ptr_write)]
>  #![feature(const_refs_to_cell)]
> +#![feature(precise_capturing)]
>
>  // Ensure conditional compilation based on the kernel configuration works;
>  // otherwise we may silently break things like initcall handling.
>
> Just want to mention it because I think the minimal rustc version we
> have to support is 1.78.

Thanks for catching this! I'll make sure to compile with 1.78 from now
on. Luckily we don't need to activate an unstable feature, there is an
"older" capturing syntax that works here. (`'_` instead of `use<'_>`)

> Best regards
>
> Dirk
>
> P.S.: Many thanks for working on this! :)
>
> [1]
>
> $ rustc --version
> rustc 1.81.0 (eeb90cda1 2024-09-04)
>
> [2]
>
> error[E0658]: precise captures on `impl Trait` are experimental
>    --> rust/kernel/property.rs:256:61
>     |
> 256 |     pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
>     |                                                             ^^^
>     |
>     = note: see issue #123432
> <https://github.com/rust-lang/rust/issues/123432> for more information
>     = help: add `#![feature(precise_capturing)]` to the crate attributes
> to enable
>     = note: this compiler was built on 2024-09-04; consider upgrading it
> if it is out of date
>
> error[E0658]: precise captures on `impl Trait` are experimental
>    --> rust/kernel/property.rs:271:61
>     |
> 271 |     pub fn display_path(&self) -> impl core::fmt::Display + use<'_> {
>     |                                                             ^^^
>     |
>     = note: see issue #123432
> <https://github.com/rust-lang/rust/issues/123432> for more information
>     = help: add `#![feature(precise_capturing)]` to the crate attributes
> to enable
>     = note: this compiler was built on 2024-09-04; consider upgrading it
> if it is out of date
>
> error: aborting due to 2 previous errors

--
Best regards,
Remo

Re: [PATCH v2 2/5] rust: Add bindings for reading device properties

[Dirk Behme]

Hi Remo,

On 14/04/2025 17:26, Remo Senekowitsch wrote:
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
> 
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> types of properties where appropriate.
> 
> The PropertyGuard 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.
> 
> 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/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 383 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index f6e6c980d..0d4ea3168 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -4,9 +4,17 @@
....
> +    /// 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 crate::{device::Device, types::CString};
> +    /// fn examples(dev: &Device) -> Result {
> +    ///     let fwnode = dev.fwnode();
> +    ///     let b: bool = fwnode.property_read("some-bool").required()?;
> +    ///     if let Some(s) = fwnode.property_read::<CString>("some-str").optional() {
> +    ///         // ...
> +    ///     }
> +    /// }
> +    /// ```

With CONFIG_RUST_KERNEL_DOCTESTS=y I had to change this example to [1]
to make it buildable.

Best regards

Dirk

[1]

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index 17ad176378206..bd43c910786d6 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -214,11 +214,10 @@ pub fn property_count_elem<T: PropertyInt>(&self,
name: &CStr) -> Result<usize>
     /// # Examples
     ///
     /// ```
-    /// # use crate::{device::Device, types::CString};
-    /// fn examples(dev: &Device) -> Result {
-    ///     let fwnode = dev.fwnode();
-    ///     let b: bool = fwnode.property_read("some-bool").required()?;
-    ///     if let Some(s) =
fwnode.property_read::<CString>("some-str").optional() {
+    /// # use kernel::{c_str, property::FwNode, str::CString};
+    /// fn examples(fwnode: &FwNode) -> () {
+    ///     let b: bool = fwnode.property_read_bool(c_str!("some-bool"));
+    ///     if let Some(s) =
fwnode.property_read::<CString>(c_str!("some-str")).optional() {
     ///         // ...
     ///     }
     /// }

[PATCH v2 3/5] 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/property.rs | 75 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index 0d4ea3168..d9fb773b6 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -13,7 +13,7 @@
     error::{to_result, Result},
     prelude::*,
     str::{BStr, CStr, CString},
-    types::Opaque,
+    types::{ARef, Opaque},
 };
 
 impl Device {
@@ -68,6 +68,16 @@ pub fn property_read<'fwnode, 'name, T: Property>(
     ) -> PropertyGuard<'fwnode, 'name, T> {
         self.fwnode().property_read(name)
     }
+
+    /// Returns first matching named child node handle.
+    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
+        self.fwnode().get_child_by_name(name)
+    }
+
+    /// Returns an iterator over a node's children.
+    pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
+        self.fwnode().children()
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -89,6 +99,22 @@ pub fn property_read<'fwnode, 'name, T: Property>(
 pub struct FwNode(Opaque<bindings::fwnode_handle>);
 
 impl FwNode {
+    /// # Safety
+    ///
+    /// Callers must ensure that the reference count was incremented at least
+    /// once, and that they are properly relinquishing one 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`].
+    unsafe fn from_raw(raw: *mut bindings::fwnode_handle) -> ARef<Self> {
+        // SAFETY: As per the safety requirement, raw has an incremented
+        // refcount which won't be decremented elsewhere. It also it not null.
+        // 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()
@@ -243,6 +269,53 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
 
         FwNodeDisplayPath(self)
     }
+
+    /// 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.
+        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 refcount incremented.
+        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. `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 and it won't be decremented anymore on the Rust
+            // side. `fwnode_get_next_child_node` decrements the refcount of
+            // `prev_ptr`, so the refcount is handled correctly.
+            let next = unsafe { bindings::fwnode_get_next_child_node(self.as_raw(), prev_ptr) };
+            if next.is_null() {
+                return None;
+            }
+            // SAFETY: `fwnode_get_next_child_node` returns a pointer with
+            // refcount incremented.
+            let next = unsafe { FwNode::from_raw(next) };
+            prev = Some(next.clone());
+            Some(next)
+        })
+    }
 }
 
 // SAFETY: Instances of `FwNode` are always reference-counted.
-- 
2.49.0

Re: [PATCH v2 3/5] rust: property: Add child accessor and iterator

[Danilo Krummrich]

On Mon, Apr 14, 2025 at 05:26:28PM +0200, Remo Senekowitsch wrote:
>  impl Device {
> @@ -68,6 +68,16 @@ pub fn property_read<'fwnode, 'name, T: Property>(
>      ) -> PropertyGuard<'fwnode, 'name, T> {
>          self.fwnode().property_read(name)
>      }
> +
> +    /// Returns first matching named child node handle.
> +    pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
> +        self.fwnode().get_child_by_name(name)
> +    }
> +
> +    /// Returns an iterator over a node's children.
> +    pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
> +        self.fwnode().children()
> +    }
>  }

Since those functions are within the impl Device block, please move them to
device.rs.

>  
>  /// A reference-counted fwnode_handle.
> @@ -89,6 +99,22 @@ pub fn property_read<'fwnode, 'name, T: Property>(
>  pub struct FwNode(Opaque<bindings::fwnode_handle>);
>  
>  impl FwNode {
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the reference count was incremented at least
> +    /// once, and that they are properly relinquishing one 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`].

Please compile multiple safety requirements into a list.

> +    unsafe fn from_raw(raw: *mut bindings::fwnode_handle) -> ARef<Self> {
> +        // SAFETY: As per the safety requirement, raw has an incremented
> +        // refcount which won't be decremented elsewhere. It also it not null.

Same here.

> +        // 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`.

This should be CAST: instead.

> +        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()
> @@ -243,6 +269,53 @@ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
>  
>          FwNodeDisplayPath(self)
>      }
> +
> +    /// 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.

I'd say that `&self` is valid by its type invariant.

> +        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 refcount incremented.

Please cover all safety requirements from from_raw().

> +        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. `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 and it won't be decremented anymore on the Rust
> +            // side. `fwnode_get_next_child_node` decrements the refcount of
> +            // `prev_ptr`, so the refcount is handled correctly.

Please also compile this into a list.

> +            let next = unsafe { bindings::fwnode_get_next_child_node(self.as_raw(), prev_ptr) };
> +            if next.is_null() {
> +                return None;
> +            }
> +            // SAFETY: `fwnode_get_next_child_node` returns a pointer with
> +            // refcount incremented.

You rather need to justify why the next you pass into from_raw() is valid.

> +            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 v2 4/5] 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/property.rs | 79 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
index d9fb773b6..17ad17637 100644
--- a/rust/kernel/property.rs
+++ b/rust/kernel/property.rs
@@ -78,6 +78,17 @@ pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> {
     pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a {
         self.fwnode().children()
     }
+
+    /// Finds a reference with arguments.
+    pub fn property_get_reference_args(
+        &self,
+        prop: &CStr,
+        nargs: NArgs<'_>,
+        index: u32,
+    ) -> Result<FwNodeReferenceArgs> {
+        self.fwnode()
+            .property_get_reference_args(prop, nargs, index)
+    }
 }
 
 /// A reference-counted fwnode_handle.
@@ -316,6 +327,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.
@@ -451,6 +521,15 @@ fn read(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 v2 5/5] 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         | 69 +++++++++++++++++++-
 2 files changed, 69 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..0284f1840 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,81 @@ 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<()> {
+        if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
+        {
+            dev_info!(dev, "matched compatible string idx = {}\n", idx);
+        }
+
+        if let Ok(str) = dev
+            .property_read::<CString>(c_str!("compatible"))
+            .required()
+        {
+            dev_info!(dev, "compatible string = {:?}\n", str);
+        }
+
+        let prop = dev.property_read_bool(c_str!("test,bool-prop"));
+        dev_info!(dev, "bool prop is {}\n", prop);
+
+        if dev.property_present(c_str!("test,u32-prop")) {
+            dev_info!(dev, "'test,u32-prop' is present\n");
+        }
+
+        let prop = dev
+            .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 _ = dev
+            .property_read::<u32>(c_str!("test,u32-required-prop"))
+            .required()?;
+
+        let prop: u32 = dev.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] = dev.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",
+            dev.property_count_elem::<u16>(c_str!("test,i16-array"))?,
+        );
+
+        let prop: KVec<i16> = dev
+            .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 v2 5/5] samples: rust: platform: Add property read examples

[Dirk Behme]

Hi Remo,

On 14/04/2025 17:26, Remo Senekowitsch wrote:
> 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         | 69 +++++++++++++++++++-
>  2 files changed, 69 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)>;


Is "test,u32-required-prop" missing here? See below ...


>  			};
>  		};
>  
> diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
> index 8120609e2..0284f1840 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,81 @@ 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<()> {
> +        if let Ok(idx) = dev.property_match_string(c_str!("compatible"), c_str!("test,rust-device"))
> +        {
> +            dev_info!(dev, "matched compatible string idx = {}\n", idx);
> +        }
> +
> +        if let Ok(str) = dev
> +            .property_read::<CString>(c_str!("compatible"))
> +            .required()
> +        {
> +            dev_info!(dev, "compatible string = {:?}\n", str);
> +        }
> +
> +        let prop = dev.property_read_bool(c_str!("test,bool-prop"));
> +        dev_info!(dev, "bool prop is {}\n", prop);
> +
> +        if dev.property_present(c_str!("test,u32-prop")) {
> +            dev_info!(dev, "'test,u32-prop' is present\n");
> +        }
> +
> +        let prop = dev
> +            .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 _ = dev
> +            .property_read::<u32>(c_str!("test,u32-required-prop"))
> +            .required()?;


On x86 using above dtsi it seems it error exits here:

[    2.178227] rust_driver_platform
testcase-data:platform-tests:test-device@2: Probed with info: '42'.


[    2.178715] rust_driver_platform
testcase-data:platform-tests:test-device@2: matched compatible string
idx = 0

[    2.179122] rust_driver_platform
testcase-data:platform-tests:test-device@2: compatible string =
"test,rust-device"

[    2.179563] rust_driver_platform
testcase-data:platform-tests:test-device@2: bool prop is false


[    2.179841] rust_driver_platform
testcase-data:platform-tests:test-device@2: 'test,u32-prop' is present


[    2.180200] rust_driver_platform
testcase-data:platform-tests:test-device@2: 'test,u32-optional-prop' is
0x12 (default = 0x12)

[    2.180598] rust_driver_platform
testcase-data:platform-tests:test-device@2:
/testcase-data/platform-tests/test-device@2: property
'test,u32-required-prop' is missing
[    2.181244] rust_driver_platform
testcase-data:platform-tests:test-device@2: probe with driver
rust_driver_platform failed with error -22

I'm not sure if this is what we want?

Best regards

Dirk


> +        let prop: u32 = dev.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] = dev.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",
> +            dev.property_count_elem::<u16>(c_str!("test,i16-array"))?,
> +        );
> +
> +        let prop: KVec<i16> = dev
> +            .property_read_array_vec(c_str!("test,i16-array"), 4)?
> +            .required()?;
> +        dev_info!(dev, "'test,i16-array' is KVec {:?}\n", prop);
> +
> +        Ok(())
> +    }
> +}
> +

Re: [PATCH v2 5/5] samples: rust: platform: Add property read examples

[Dirk Behme]

On 23/04/2025 14:39, Dirk Behme wrote:
> Hi Remo,
> 
> On 14/04/2025 17:26, Remo Senekowitsch wrote:
>> 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         | 69 +++++++++++++++++++-
>>  2 files changed, 69 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)>;
> 
> 
> Is "test,u32-required-prop" missing here? See below ...

Ah, no, sorry, "test,u32-required-prop" isn't there intentionally to
demonstrate the error handling on missing required properties. In this
case I would propose to just remove the '?' to make the sample code not
exit on this intended error:

diff --git a/samples/rust/rust_driver_platform.rs
b/samples/rust/rust_driver_platform.rs
index 01902956e1ca7..b6cbd721a8882 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -75,7 +75,7 @@ fn properties_parse(dev: &kernel::device::Device) ->
Result<()> {
         // Missing property without a default will print an error
         let _ = dev
             .property_read::<u32>(c_str!("test,u32-required-prop"))
-            .required()?;
+            .required();

         let prop: u32 =
dev.property_read(c_str!("test,u32-prop")).required()?;
         dev_info!(dev, "'test,u32-prop' is {:#x}\n", prop);

Best regards

Dirk

Re: [PATCH v2 0/5] More Rust bindings for device property reads

[Miguel Ojeda]

On Mon, Apr 14, 2025 at 5:26 PM Remo Senekowitsch <remo@buenzli.dev> wrote:
>
> changes in v2:

If possible (no need to resend, it is just a suggestion), please
submit new versions (especially multi-patch ones) in a new thread,
otherwise in some clients (including lore.kernel.org) it gets harder
to read.

> Link: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format [2]

By the way, since I am here: to link to docs, nowadays there is
docs.kernel.org which is nicer/shorter:

    https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format

I hope that helps!

Cheers,
Miguel

Re: [PATCH v2 0/5] More Rust bindings for device property reads

[Remo Senekowitsch]

On Mon Apr 14, 2025 at 5:38 PM CEST, Miguel Ojeda wrote:
> On Mon, Apr 14, 2025 at 5:26 PM Remo Senekowitsch <remo@buenzli.dev> wrote:
>>
>> changes in v2:
>
> If possible (no need to resend, it is just a suggestion), please
> submit new versions (especially multi-patch ones) in a new thread,
> otherwise in some clients (including lore.kernel.org) it gets harder
> to read.

Okay, will do next time.

Junio Hamano recently wrote on the Git mailing list[1] that he wants
his contributors to do this, so the original message-ID can be used as
a "patch set ID". A quick glace at the archive[2] confirms that people
work this way. I assumed the kernel community would appreciate this
as well.

Link: https://lore.kernel.org/git/xmqqy0w8ng5r.fsf@gitster.g/ [1]
Link: https://lore.kernel.org/git/ [2]

Best regards,
Remo

Re: [PATCH v2 0/5] More Rust bindings for device property reads

[Miguel Ojeda]

On Mon, Apr 14, 2025 at 6:07 PM Remo Senekowitsch <remo@buenzli.dev> wrote:
>
> Junio Hamano recently wrote on the Git mailing list[1] that he wants
> his contributors to do this, so the original message-ID can be used as
> a "patch set ID". A quick glace at the archive[2] confirms that people
> work this way. I assumed the kernel community would appreciate this
> as well.

I see, thanks for the reference!

I don't think it is a good idea for long series, given how some
clients render things, e.g. GMail just shows the new version in a
single linear thread and it gets really confusing.

Even in a nested/tree render, like Lore's option, it is harder to see
and/or compare the versions, in my opinion. Perhaps Lore could be
improved here (e.g. offering (auto-)folding of versions) -- most of
the time we only care about the last version (and perhaps last - 1)
anyway.

I think `b4` behavior of subject+author works OK, but an explicit
change-id would be nice. Reusing the Message-ID for that would only
really work well if we could control the UI for everyone, I think.

Cheers,
Miguel