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
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
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
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
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
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.0On 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.
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]
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]
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 */ }`.
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.0Hi 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() {
/// // ...
/// }
/// }
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
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
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
>
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.
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.
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
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]
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
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.
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.
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
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.0On 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
>
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.0Add 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.0Hi 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(())
> + }
> +}
> +
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
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
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
© 2016 - 2025 Red Hat, Inc.