Not all property-related APIs can be exposed directly on a device.
For example, iterating over child nodes of a device will yield
fwnode_handle. Thus, in order to access properties on these child nodes,
the APIs has to be duplicated on a fwnode as they are in C.
A related discussion can be found on the R4L Zulip[1].
[1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697
Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
---
rust/helpers/helpers.c | 1 +
rust/helpers/property.c | 13 ++++++++
rust/kernel/device.rs | 7 ----
rust/kernel/lib.rs | 1 +
rust/kernel/property.rs | 73 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 88 insertions(+), 7 deletions(-)
create mode 100644 rust/helpers/property.c
create mode 100644 rust/kernel/property.rs
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0640b7e11..b4eec5bf2 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -23,6 +23,7 @@
#include "platform.c"
#include "pci.c"
#include "pid_namespace.c"
+#include "property.c"
#include "rbtree.c"
#include "rcu.c"
#include "refcount.c"
diff --git a/rust/helpers/property.c b/rust/helpers/property.c
new file mode 100644
index 000000000..c37c74488
--- /dev/null
+++ b/rust/helpers/property.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/property.h>
+
+struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
+{
+ return dev_fwnode(dev);
+}
+
+void rust_helper_fwnode_handle_put(struct fwnode_handle *fwnode)
+{
+ fwnode_handle_put(fwnode);
+}
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index db2d9658b..d5e6a19ff 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,7 +6,6 @@
use crate::{
bindings,
- str::CStr,
types::{ARef, Opaque},
};
use core::{fmt, ptr};
@@ -181,12 +180,6 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
)
};
}
-
- /// Checks if property is present or not.
- pub fn property_present(&self, name: &CStr) -> bool {
- // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
- unsafe { bindings::device_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
- }
}
// SAFETY: Instances of `Device` are always reference-counted.
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0..ca233fd20 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -67,6 +67,7 @@
pub mod platform;
pub mod prelude;
pub mod print;
+pub mod property;
pub mod rbtree;
pub mod revocable;
pub mod security;
diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
new file mode 100644
index 000000000..b0a4bb63a
--- /dev/null
+++ b/rust/kernel/property.rs
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Unified device property interface.
+//!
+//! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
+
+use core::ptr;
+
+use crate::{bindings, device::Device, str::CStr, types::Opaque};
+
+impl Device {
+ /// Obtain the fwnode corresponding to the device.
+ fn fwnode(&self) -> &FwNode {
+ // SAFETY: `self` is valid.
+ let fwnode_handle = unsafe { bindings::dev_fwnode(self.as_raw()) };
+ if fwnode_handle.is_null() {
+ panic!("fwnode_handle cannot be null");
+ }
+ // SAFETY: `fwnode_handle` is valid. Its lifetime is tied to `&self`. We
+ // return a reference instead of an `ARef<FwNode>` because `dev_fwnode()`
+ // doesn't increment the refcount.
+ unsafe { &*fwnode_handle.cast() }
+ }
+
+ /// Checks if property is present or not.
+ pub fn property_present(&self, name: &CStr) -> bool {
+ self.fwnode().property_present(name)
+ }
+}
+
+/// A reference-counted fwnode_handle.
+///
+/// This structure represents the Rust abstraction for a
+/// C `struct fwnode_handle`. This implementation abstracts the usage of an
+/// already existing C `struct fwnode_handle` within Rust code that we get
+/// passed from the C side.
+///
+/// # Invariants
+///
+/// A `FwNode` instance represents a valid `struct fwnode_handle` created by the
+/// C portion of the kernel.
+///
+/// Instances of this type are always reference-counted, that is, a call to
+/// `fwnode_handle_get` ensures that the allocation remains valid at least until
+/// the matching call to `fwnode_handle_put`.
+#[repr(transparent)]
+pub struct FwNode(Opaque<bindings::fwnode_handle>);
+
+impl FwNode {
+ /// Obtain the raw `struct fwnode_handle *`.
+ pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
+ self.0.get()
+ }
+
+ /// Checks if property is present or not.
+ pub fn property_present(&self, name: &CStr) -> bool {
+ // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
+ unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
+ }
+}
+
+// SAFETY: Instances of `FwNode` are always reference-counted.
+unsafe impl crate::types::AlwaysRefCounted for FwNode {
+ fn inc_ref(&self) {
+ // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
+ unsafe { bindings::fwnode_handle_get(self.as_raw()) };
+ }
+
+ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
+ // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+ unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
+ }
+}
--
2.49.0On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote:
> Not all property-related APIs can be exposed directly on a device.
> For example, iterating over child nodes of a device will yield
> fwnode_handle. Thus, in order to access properties on these child nodes,
> the APIs has to be duplicated on a fwnode as they are in C.
>
> A related discussion can be found on the R4L Zulip[1].
>
> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697
You can make the above to be a Link tag like
Link: ... [1]
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
...
> +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> +{
> + return dev_fwnode(dev);
> +}
Why not const? For most of the property retrieval APIs the parameter is const.
--
With Best Regards,
Andy Shevchenko
On Thu, Mar 27, 2025 at 3:37 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote:
> > Not all property-related APIs can be exposed directly on a device.
> > For example, iterating over child nodes of a device will yield
> > fwnode_handle. Thus, in order to access properties on these child nodes,
> > the APIs has to be duplicated on a fwnode as they are in C.
> >
> > A related discussion can be found on the R4L Zulip[1].
> >
> > [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697
>
> You can make the above to be a Link tag like
>
> Link: ... [1]
>
> > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
>
> ...
>
> > +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> > +{
> > + return dev_fwnode(dev);
> > +}
>
> Why not const? For most of the property retrieval APIs the parameter is const.
Because you might need to modify the refcount.
Though the rust code should probably use __dev_fwnode() and/or
__dev_fwnode_const() directly and avoid the need for the helper here.
Rob
On Thu, Mar 27, 2025 at 08:55:42AM -0500, Rob Herring wrote:
> On Thu, Mar 27, 2025 at 3:37 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote:
...
> > > +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> > > +{
> > > + return dev_fwnode(dev);
> > > +}
> >
> > Why not const? For most of the property retrieval APIs the parameter is const.
>
> Because you might need to modify the refcount.
>
> Though the rust code should probably use __dev_fwnode() and/or
> __dev_fwnode_const() directly and avoid the need for the helper here.
Indeed, that would help at least to understand the intention.
--
With Best Regards,
Andy Shevchenko
On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote:
> + // SAFETY: `fwnode_handle` is valid. Its lifetime is tied to `&self`. We
> + // return a reference instead of an `ARef<FwNode>` because `dev_fwnode()`
> + // doesn't increment the refcount.
> + unsafe { &*fwnode_handle.cast() }
this safety comment should say why it is safe to cast from a
struct fwnode_handle* to a *const FwNode
Andrew
On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote:
> Not all property-related APIs can be exposed directly on a device.
> For example, iterating over child nodes of a device will yield
> fwnode_handle. Thus, in order to access properties on these child nodes,
> the APIs has to be duplicated on a fwnode as they are in C.
s/has/have/
>
> A related discussion can be found on the R4L Zulip[1].
>
> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697
Useful below the '---', but I don't think we want to keep this link
forever. And who knows how long it will be valid? The commit msg needs
to stand on its own, and I think it does.
>
> Signed-off-by: Remo Senekowitsch <remo@buenzli.dev>
> ---
> rust/helpers/helpers.c | 1 +
> rust/helpers/property.c | 13 ++++++++
> rust/kernel/device.rs | 7 ----
> rust/kernel/lib.rs | 1 +
> rust/kernel/property.rs | 73 +++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 88 insertions(+), 7 deletions(-)
> create mode 100644 rust/helpers/property.c
> create mode 100644 rust/kernel/property.rs
>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 0640b7e11..b4eec5bf2 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -23,6 +23,7 @@
> #include "platform.c"
> #include "pci.c"
> #include "pid_namespace.c"
> +#include "property.c"
> #include "rbtree.c"
> #include "rcu.c"
> #include "refcount.c"
> diff --git a/rust/helpers/property.c b/rust/helpers/property.c
> new file mode 100644
> index 000000000..c37c74488
> --- /dev/null
> +++ b/rust/helpers/property.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/property.h>
> +
> +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev)
> +{
> + return dev_fwnode(dev);
> +}
> +
> +void rust_helper_fwnode_handle_put(struct fwnode_handle *fwnode)
> +{
> + fwnode_handle_put(fwnode);
> +}
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index db2d9658b..d5e6a19ff 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -6,7 +6,6 @@
>
> use crate::{
> bindings,
> - str::CStr,
> types::{ARef, Opaque},
> };
> use core::{fmt, ptr};
> @@ -181,12 +180,6 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
> )
> };
> }
> -
> - /// Checks if property is present or not.
> - pub fn property_present(&self, name: &CStr) -> bool {
> - // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
> - unsafe { bindings::device_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
> - }
> }
>
> // SAFETY: Instances of `Device` are always reference-counted.
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0..ca233fd20 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -67,6 +67,7 @@
> pub mod platform;
> pub mod prelude;
> pub mod print;
> +pub mod property;
> pub mod rbtree;
> pub mod revocable;
> pub mod security;
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> new file mode 100644
> index 000000000..b0a4bb63a
> --- /dev/null
> +++ b/rust/kernel/property.rs
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Unified device property interface.
> +//!
> +//! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
> +
> +use core::ptr;
> +
> +use crate::{bindings, device::Device, str::CStr, types::Opaque};
> +
> +impl Device {
> + /// Obtain the fwnode corresponding to the device.
> + fn fwnode(&self) -> &FwNode {
> + // SAFETY: `self` is valid.
> + let fwnode_handle = unsafe { bindings::dev_fwnode(self.as_raw()) };
> + if fwnode_handle.is_null() {
> + panic!("fwnode_handle cannot be null");
It's usually not a good idea to panic the kernel especially with
something a driver calls as that's probably recoverable.
Users/drivers testing fwnode_handle/of_node for NULL is pretty common.
Though often that's a legacy code path, so maybe not allowing NULL is
fine for now.
> + }
> + // SAFETY: `fwnode_handle` is valid. Its lifetime is tied to `&self`. We
> + // return a reference instead of an `ARef<FwNode>` because `dev_fwnode()`
> + // doesn't increment the refcount.
> + unsafe { &*fwnode_handle.cast() }
> + }
> +
> + /// Checks if property is present or not.
> + pub fn property_present(&self, name: &CStr) -> bool {
> + self.fwnode().property_present(name)
> + }
> +}
The C developer in me wants to put this after the FwNode stuff since
this uses it.
> +
> +/// A reference-counted fwnode_handle.
> +///
> +/// This structure represents the Rust abstraction for a
> +/// C `struct fwnode_handle`. This implementation abstracts the usage of an
> +/// already existing C `struct fwnode_handle` within Rust code that we get
> +/// passed from the C side.
> +///
> +/// # Invariants
> +///
> +/// A `FwNode` instance represents a valid `struct fwnode_handle` created by the
> +/// C portion of the kernel.
> +///
> +/// Instances of this type are always reference-counted, that is, a call to
> +/// `fwnode_handle_get` ensures that the allocation remains valid at least until
> +/// the matching call to `fwnode_handle_put`.
> +#[repr(transparent)]
> +pub struct FwNode(Opaque<bindings::fwnode_handle>);
> +
> +impl FwNode {
> + /// Obtain the raw `struct fwnode_handle *`.
> + pub(crate) fn as_raw(&self) -> *mut bindings::fwnode_handle {
> + self.0.get()
> + }
> +
> + /// Checks if property is present or not.
> + pub fn property_present(&self, name: &CStr) -> bool {
> + // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
> + unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
> + }
> +}
> +
> +// SAFETY: Instances of `FwNode` are always reference-counted.
> +unsafe impl crate::types::AlwaysRefCounted for FwNode {
> + fn inc_ref(&self) {
> + // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> + unsafe { bindings::fwnode_handle_get(self.as_raw()) };
> + }
> +
> + unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> + // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> + unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> + }
> +}
> --
> 2.49.0
>
On Wed Mar 26, 2025 at 9:51 PM CET, Rob Herring wrote:
> On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote:
>>
>> +impl Device {
>> + /// Obtain the fwnode corresponding to the device.
>> + fn fwnode(&self) -> &FwNode {
>> + // SAFETY: `self` is valid.
>> + let fwnode_handle = unsafe { bindings::dev_fwnode(self.as_raw()) };
>> + if fwnode_handle.is_null() {
>> + panic!("fwnode_handle cannot be null");
>
> It's usually not a good idea to panic the kernel especially with
> something a driver calls as that's probably recoverable.
>
> Users/drivers testing fwnode_handle/of_node for NULL is pretty common.
> Though often that's a legacy code path, so maybe not allowing NULL is
> fine for now.
Just to be clear on this, should I keep this as is, or return a result?
In the latter case, all the duplicated methods on `Device` that just
call `self.fwnode().same_method()` would have a result in their function
signatur as well. That includes `property_present`, `read_property`
and `children`.
>> + }
>> + // SAFETY: `fwnode_handle` is valid. Its lifetime is tied to `&self`. We
>> + // return a reference instead of an `ARef<FwNode>` because `dev_fwnode()`
>> + // doesn't increment the refcount.
>> + unsafe { &*fwnode_handle.cast() }
>> + }
>> +
>> + /// Checks if property is present or not.
>> + pub fn property_present(&self, name: &CStr) -> bool {
>> + self.fwnode().property_present(name)
>> + }
>> +}
>
> The C developer in me wants to put this after the FwNode stuff since
> this uses it.
Is that just a comment or a call to action? :-)
Remo
On Wed, Mar 26, 2025 at 03:51:06PM -0500, Rob Herring wrote: > On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote: > > Not all property-related APIs can be exposed directly on a device. > > For example, iterating over child nodes of a device will yield > > fwnode_handle. Thus, in order to access properties on these child nodes, > > the APIs has to be duplicated on a fwnode as they are in C. > > s/has/have/ > > > > > A related discussion can be found on the R4L Zulip[1]. > > > > [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697 > > Useful below the '---', but I don't think we want to keep this link > forever. And who knows how long it will be valid? The commit msg needs > to stand on its own, and I think it does. > > > > > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev> > > --- > > rust/helpers/helpers.c | 1 + > > rust/helpers/property.c | 13 ++++++++ > > rust/kernel/device.rs | 7 ---- > > rust/kernel/lib.rs | 1 + > > rust/kernel/property.rs | 73 +++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 88 insertions(+), 7 deletions(-) > > create mode 100644 rust/helpers/property.c > > create mode 100644 rust/kernel/property.rs Also, property.rs needs to be added to MAINTAINERS. I guess it goes under driver core with Greg. Rob
© 2016 - 2025 Red Hat, Inc.