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 */ }`.
© 2016 - 2025 Red Hat, Inc.