| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aAupcPT7GWgndVuA@pollux>
Date: Fri, 25 Apr 2025 17:25:36 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Remo Senekowitsch <remo@...nzli.dev>
Cc: Rob Herring <robh@...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>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Dirk Behme <dirk.behme@...bosch.com>, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 1/7] rust: property: Move property_present to separate
file
On Fri, Apr 25, 2025 at 05:01:24PM +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,
> the property access methods must be implemented on the abstraction over
> fwnode_handle.
>
> While it's possible to expose similar methods on `Device` directly for
> convenience, those methods would have to get the `FwNode` first, which
> is a fallible operation, making the API inconsistent. For this reason,
> such duplicated methods are omitted. Users who need to read properties
> of a device will have to explictily get the `FwNode` first (handle the
> `None` case) and then read properties on that.
I think I mentioned that in v2 [1]; when the commit subject says "rust:
property: Move property_present to separate", the commit shouldn't do anything
beyond this scope.
I can see that you switch from device_property_present() to
fwnode_property_present(), without fixing users, so obviously the former is
unused.
Please make the implementation of the FwNode abstraction and the removal of
device_property_present() separate commits.
[1] https://lore.kernel.org/lkml/Z_0xGRsI74PsAL_E@cassiopeiae/
>
> Signed-off-by: Remo Senekowitsch <remo@...nzli.dev>
> ---
> MAINTAINERS | 3 +-
> rust/helpers/helpers.c | 1 +
> rust/helpers/property.c | 8 +++
> rust/kernel/{device.rs => device/mod.rs} | 9 +--
> rust/kernel/device/property.rs | 70 ++++++++++++++++++++++++
> 5 files changed, 83 insertions(+), 8 deletions(-)
> create mode 100644 rust/helpers/property.c
> rename rust/kernel/{device.rs => device/mod.rs} (97%)
> create mode 100644 rust/kernel/device/property.rs
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c8d9e8187..4585f9e7f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7112,7 +7112,8 @@ F: include/linux/kobj*
> F: include/linux/property.h
> F: include/linux/sysfs.h
> F: lib/kobj*
> -F: rust/kernel/device.rs
> +F: rust/kernel/device/mod.rs
> +F: rust/kernel/device/property.rs
This should just be
F: rust/kernel/device/
> F: rust/kernel/device_id.rs
> F: rust/kernel/devres.rs
> F: rust/kernel/driver.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..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/mod.rs
> similarity index 97%
> rename from rust/kernel/device.rs
> rename to rust/kernel/device/mod.rs
> index db2d9658b..e49107452 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device/mod.rs
> @@ -6,7 +6,6 @@
>
> use crate::{
> bindings,
> - str::CStr,
> types::{ARef, Opaque},
> };
> use core::{fmt, ptr};
> @@ -14,6 +13,8 @@
> #[cfg(CONFIG_PRINTK)]
> use crate::c_str;
>
> +pub mod property;
> +
> /// A reference-counted device.
> ///
> /// This structure represents the Rust abstraction for a C `struct device`. This implementation
> @@ -181,12 +182,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/device/property.rs b/rust/kernel/device/property.rs
> new file mode 100644
> index 000000000..d89715f7d
> --- /dev/null
> +++ b/rust/kernel/device/property.rs
> @@ -0,0 +1,70 @@
> +// 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.
> + pub fn fwnode(&self) -> Option<&FwNode> {
> + // SAFETY: `self` is valid.
> + let fwnode_handle = unsafe { bindings::__dev_fwnode(self.as_raw()) };
> + if fwnode_handle.is_null() {
> + return None;
> + }
> + // 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`.
> + Some(unsafe { &*fwnode_handle.cast() })
> + }
> +}
Given that the cover letter says "Remove the duplicated property reading methods
on Device.", I assume that's the only Device method you introduce? If so, please
keep this one in the impl block of device/mod.rs.
Please also rebase onto driver-core-next and put this method in the following
impl block.
impl<Ctx: DeviceContext> Device<Ctx>
I assume this is valid to call from any device context.
Powered by blists - more mailing lists