Message-ID: <20250326205106.GB2787672-robh@kernel.org>
Date: Wed, 26 Mar 2025 15:51:06 -0500
From: Rob Herring <robh@...nel.org>
To: Remo Senekowitsch <remo@...nzli.dev>
Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Daniel Scally <djrscally@...il.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Dirk Behme <dirk.behme@...bosch.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>,
	Saravana Kannan <saravanak@...gle.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
	devicetree@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 01/10] rust: Move property_present to property.rs

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@...nzli.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
> 

Powered by blists - more mailing lists