| Message ID | 20241022213221.2383-15-dakr@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Device / Driver PCI / Platform Rust abstractions | expand |
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@kernel.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
On Tue, Oct 22, 2024 at 06:03:51PM -0500, Rob Herring wrote: > 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@kernel.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... True, however mod_devicetable.h is already needed for a previous patch "rust: pci: add basic PCI device / driver abstractions" already. So, I'll add it there and remove the of_device.h include here. > > > #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. I think for now it's indeed meaningless and we should just remove it. If subsequent patches start adding abstractions for things like properties, device nodes, etc. they can add it back in. > > 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? AFAIK, in C we have all those device ID structs in mod_devicetable.h, such that in can easily be included in scripts/mod/file2alias.c. But I think implementing all Rust abstractions for those in a single file would be a bit odd. I'd rather put them together with the corresponding bus abstractions. OF and ACPI may be a bit of an exception. > > > + > > +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. That's true, I'll add a note. > > > + > > + 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. Right, I think I just added it for the sample driver, we can get rid of it. > > Rob >
Hi Danilo, On Tue Oct 22, 2024 at 2:31 PM PDT, Danilo Krummrich wrote: > +/// 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 DeviceId is missing the `#[repr(transparent)]`. BR, Fabien
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> #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. +//! +//! C header: [`include/linux/of_*.h`](srctree/include/linux/of_*.h) + +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; + } + + 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()) } + } +} + +/// Create an OF `IdTable` with an "alias" for modpost. +#[macro_export] +macro_rules! of_device_table { + ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => { + const $table_name: $crate::device_id::IdArray< + $crate::of::DeviceId, + $id_info_type, + { $table_data.len() }, + > = $crate::device_id::IdArray::new($table_data); + + $crate::module_device_table!("of", $module_table_name, $table_name); + }; +}
`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@kernel.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