| 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: <20241022230351.GA1848992-robh@kernel.org>
Date: Tue, 22 Oct 2024 18:03:51 -0500
From: Rob Herring <robh@...nel.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com,
ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
tmgross@...ch.edu, a.hindborg@...sung.com, aliceryhl@...gle.com,
airlied@...il.com, fujita.tomonori@...il.com, lina@...hilina.net,
pstanner@...hat.com, ajanulgu@...hat.com, lyude@...hat.com,
daniel.almeida@...labora.com, saravanak@...gle.com,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 14/16] rust: of: add `of::DeviceId` abstraction
On Tue, Oct 22, 2024 at 11:31:51PM +0200, Danilo Krummrich wrote:
> `of::DeviceId` is an abstraction around `struct of_device_id`.
>
> This is used by subsequent patches, in particular the platform bus
> abstractions, to create OF device ID tables.
>
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> ---
> MAINTAINERS | 1 +
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/lib.rs | 1 +
> rust/kernel/of.rs | 63 +++++++++++++++++++++++++++++++++
> 4 files changed, 66 insertions(+)
> create mode 100644 rust/kernel/of.rs
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d9c512a3e72b..87eb9a7869eb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17340,6 +17340,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
> F: Documentation/ABI/testing/sysfs-firmware-ofw
> F: drivers/of/
> F: include/linux/of*.h
> +F: rust/kernel/of.rs
> F: scripts/dtc/
> F: tools/testing/selftests/dt/
> K: of_overlay_notifier_
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index cd4edd6496ae..312f03cbdce9 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -15,6 +15,7 @@
> #include <linux/firmware.h>
> #include <linux/jiffies.h>
> #include <linux/mdio.h>
> +#include <linux/of_device.h>
Technically, you don't need this for *this* patch. You need
mod_devicetable.h for of_device_id. Best to not rely on implicit
includes. I've tried removing it and it still built, so I guess there is
another implicit include somewhere...
> #include <linux/pci.h>
> #include <linux/phy.h>
> #include <linux/refcount.h>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 3ec690eb6d43..5946f59f1688 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -51,6 +51,7 @@
> pub mod list;
> #[cfg(CONFIG_NET)]
> pub mod net;
> +pub mod of;
> pub mod page;
> pub mod prelude;
> pub mod print;
> diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs
> new file mode 100644
> index 000000000000..a37629997974
> --- /dev/null
> +++ b/rust/kernel/of.rs
> @@ -0,0 +1,63 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Open Firmware abstractions.
s/Open Firmware/Devicetree/
Or keep both that prior versions of this code had. Most of DT/OF today
is not OpenFirmware.
> +//!
> +//! C header: [`include/linux/of_*.h`](srctree/include/linux/of_*.h)
I haven't quite figured out how this gets used. I guess just a link in
documentation? I somewhat doubt this file is going to handle all DT
abstractions. That might become quite long. Most of of_address.h and
of_irq.h I actively don't want to see Rust bindings for because they
are mainly used by higher level interfaces (e.g. platform dev
resources). There's a slew of "don't add new users" APIs which I need to
document. Also, the main header is of.h which wasn't included here.
As of now, only the mod_devicetable.h header is used by this file, so I
think you should just put it until that changes. Maybe there would be
some savings if all of mod_devicetable.h was handled by 1 rust file?
> +
> +use crate::{bindings, device_id::RawDeviceId, prelude::*};
> +
> +/// An open firmware device id.
> +#[derive(Clone, Copy)]
> +pub struct DeviceId(bindings::of_device_id);
> +
> +// SAFETY:
> +// * `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and does not add
> +// additional invariants, so it's safe to transmute to `RawType`.
> +// * `DRIVER_DATA_OFFSET` is the offset to the `data` field.
> +unsafe impl RawDeviceId for DeviceId {
> + type RawType = bindings::of_device_id;
> +
> + const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::of_device_id, data);
> +
> + fn index(&self) -> usize {
> + self.0.data as _
> + }
> +}
> +
> +impl DeviceId {
> + /// Create a new device id from an OF 'compatible' string.
> + pub const fn new(compatible: &'static CStr) -> Self {
> + let src = compatible.as_bytes_with_nul();
> + // Replace with `bindings::of_device_id::default()` once stabilized for `const`.
> + // SAFETY: FFI type is valid to be zero-initialized.
> + let mut of: bindings::of_device_id = unsafe { core::mem::zeroed() };
> +
> + let mut i = 0;
> + while i < src.len() {
> + of.compatible[i] = src[i] as _;
> + i += 1;
> + }
AFAICT, this loop will go away when C char maps to u8. Perhaps a note
to that extent or commented code implementing that.
> +
> + Self(of)
> + }
> +
> + /// The compatible string of the embedded `struct bindings::of_device_id` as `&CStr`.
> + pub fn compatible<'a>(&self) -> &'a CStr {
> + // SAFETY: `self.compatible` is a valid `char` pointer.
> + unsafe { CStr::from_char_ptr(self.0.compatible.as_ptr()) }
> + }
I don't think we need this. The usage model is checking does a node's
compatible string(s) match a compatible in the table. Most of the time
we don't even need that. We just need the match data.
Rob
Powered by blists - more mailing lists