| Message ID | 20250326171411.590681-2-remo@buenzli.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | More Rust bindings for device property reads | expand |
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 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 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
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.
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.
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()) } + } +}
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