| Message ID | 20250605161956.3658374-1-igor.korotin.linux@gmail.com |
|---|---|
| Headers | show |
| Series | rust: Add ACPI match table support for Rust drivers | expand |
On Thu, Jun 05, 2025 at 05:19:56PM +0100, Igor Korotin wrote: > This patch series introduces support for ACPI match tables in Rust > drivers. Which commit does this patch series apply to? In general, I recommend to use '--base' on 'git format-patch'.
On Thu, Jun 05, 2025 at 05:27:26PM +0100, Igor Korotin wrote: > From: Igor Korotin <igor.korotin.linux@gmail.com> > > Extend the `Adapter` trait to support ACPI device identification. > > This mirrors the existing Open Firmware (OF) support (`of_id_table`) and > enables Rust drivers to match and retrieve ACPI-specific device data > when `CONFIG_ACPI` is enabled. > > To avoid breaking compilation, a stub implementation of `acpi_id_table()` > is added to the Platform adapter; the full implementation will be provided > in a subsequent patch. > --- > rust/bindings/bindings_helper.h | 1 + > rust/kernel/driver.rs | 58 ++++++++++++++++++++++++++++++--- > rust/kernel/platform.rs | 5 +++ > 3 files changed, 60 insertions(+), 4 deletions(-) > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index e0bcd130b494..d974fc6c141f 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -6,6 +6,7 @@ > * Sorted alphabetically. > */ > > +#include <linux/acpi.h> > #include <kunit/test.h> > #include <linux/blk-mq.h> > #include <linux/blk_types.h> > diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs > index ec9166cedfa7..d4098596188a 100644 > --- a/rust/kernel/driver.rs > +++ b/rust/kernel/driver.rs > @@ -6,7 +6,7 @@ > //! register using the [`Registration`] class. > > use crate::error::{Error, Result}; > -use crate::{device, of, str::CStr, try_pin_init, types::Opaque, ThisModule}; > +use crate::{device, of, acpi, str::CStr, try_pin_init, types::Opaque, ThisModule}; > use core::pin::Pin; > use pin_init::{pin_data, pinned_drop, PinInit}; > > @@ -141,6 +141,38 @@ pub trait Adapter { > /// The type holding driver private data about each device id supported by the driver. > type IdInfo: 'static; > > + /// The [`acpi::IdTable`] of the corresponding driver > + fn acpi_id_table() -> Option<acpi::IdTable<Self::IdInfo>>; > + > + /// Returns the driver's private data from the matching entry in the [`acpi::IdTable`], if any. > + /// > + /// If this returns `None`, it means there is no match with an entry in the [`acpi::IdTable`]. > + #[cfg(CONFIG_ACPI)] > + fn acpi_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> { > + let table = Self::acpi_id_table()?; > + > + // SAFETY: > + // - `table` has static lifetime, hence it's valid for read, > + // - `dev` is guaranteed to be valid while it's alive, and so is `pdev.as_ref().as_raw()`. > + let raw_id = unsafe { bindings::acpi_match_device(table.as_ptr(), dev.as_raw()) }; > + > + if raw_id.is_null() { > + None > + } else { > + // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and > + // does not add additional invariants, so it's safe to transmute. > + let id = unsafe { &*raw_id.cast::<acpi::DeviceId>() }; > + > + Some(table.info(<acpi::DeviceId as crate::device_id::RawDeviceId>::index(id))) > + } > + } > + > + #[cfg(not(CONFIG_ACPI))] > + #[allow(missing_docs)] > + fn acpi_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> { > + None > + } > + > /// The [`of::IdTable`] of the corresponding driver. > fn of_id_table() -> Option<of::IdTable<Self::IdInfo>>; > > @@ -178,9 +210,27 @@ fn of_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> { > /// If this returns `None`, it means that there is no match in any of the ID tables directly > /// associated with a [`device::Device`]. > fn id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> { > - let id = Self::of_id_info(dev); > - if id.is_some() { > - return id; > + // SAFETY: `id_info` is called from `Adapter::probe_callback` with a valid `dev` argument. > + let fwnode = unsafe{ (*dev.as_raw()).fwnode}; There is an abstraction for FwNode on the list [1] that I plan to merge soon. Generally, it would make sense to build on top of that. However, I don't understand why we need this and the subsequent is_acpi_device_node() and is_of_node() checks. Instead, I think we can keep the existing code and just add the following. let id = Self::acpi_id_info(dev); if id.is_some() { return id; } [1] https://lore.kernel.org/lkml/20250530192856.1177011-1-remo@buenzli.dev/ > + > + // SAFETY: `bindings::is_acpi_device_node` checks `fwnode` before accessing `fwnode->ops`, > + // and only compares it with the address of `acpi_device_fwnode_ops`. > + if unsafe { bindings::is_acpi_device_node(fwnode) } { As mentioned above, I think we don't need this check. > + let id = Self::acpi_id_info(dev); > + > + if id.is_some() { > + return id; > + } > + } > + > + // SAFETY: `bindings::is_of_node` checks `fwnode` before accessing `fwnode->ops`, > + // and only compares it with the address of `of_fwnode_ops`. > + if unsafe { bindings::is_of_node(fwnode) } { Same here. > + let id = Self::of_id_info(dev); > + > + if id.is_some() { > + return id; > + } > } > > None > diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs > index fd4a494f30e8..3cc9fe6ccfcf 100644 > --- a/rust/kernel/platform.rs > +++ b/rust/kernel/platform.rs > @@ -5,6 +5,7 @@ > //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h) > > use crate::{ > + acpi, > bindings, device, driver, > error::{to_result, Result}, > of, > @@ -95,6 +96,10 @@ impl<T: Driver + 'static> driver::Adapter for Adapter<T> { > fn of_id_table() -> Option<of::IdTable<Self::IdInfo>> { > T::OF_ID_TABLE > } > + > + fn acpi_id_table() -> Option<acpi::IdTable<Self::IdInfo>> { > + None > + } > } > > /// Declares a kernel module that exposes a single platform driver. > -- > 2.43.0 >
On Fri, Jun 6, 2025 at 2:50 PM Danilo Krummrich <dakr@kernel.org> wrote: > However, I don't understand why we need this and the subsequent > is_acpi_device_node() and is_of_node() checks. The idea is to avoid unnecessary table lookups when both OF and ACPI match tables are present. If we already know the fwnode type, these simple pointer comparisons (is_acpi_device_node() / is_of_node()) let us skip the irrelevant match function. Those checks are cheap (just pointer comparisons), while acpi_match_device() and of_match_device() iterate over tables. So yeah, it’s a bit ugly, but it can save some CPU cycles during enumeration. Thanks, Igor
On Fri, Jun 06, 2025 at 03:26:13PM +0100, Igor Korotin wrote: > On Fri, Jun 6, 2025 at 2:50 PM Danilo Krummrich <dakr@kernel.org> wrote: > > However, I don't understand why we need this and the subsequent > > is_acpi_device_node() and is_of_node() checks. > > The idea is to avoid unnecessary table lookups when both OF and ACPI > match tables are present. Ok, that's fair -- let's build it on top of the FwNode abstractions though.
On Fri, Jun 6, 2025 at 3:32 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Fri, Jun 06, 2025 at 03:26:13PM +0100, Igor Korotin wrote: > > On Fri, Jun 6, 2025 at 2:50 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > However, I don't understand why we need this and the subsequent > > > is_acpi_device_node() and is_of_node() checks. > > > > The idea is to avoid unnecessary table lookups when both OF and ACPI > > match tables are present. > > Ok, that's fair -- let's build it on top of the FwNode abstractions though. I'm ok with the FwNode abstractions. Just to make sure I understood you correctly: I'll need to wait until these FwNode abstractions are pushed to the rust-next branch, reimplement what is necessary and send v3. Is this the course of actions? Thanks Igor
On Fri, Jun 06, 2025 at 03:58:11PM +0100, Igor Korotin wrote: > On Fri, Jun 6, 2025 at 3:32 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > On Fri, Jun 06, 2025 at 03:26:13PM +0100, Igor Korotin wrote: > > > On Fri, Jun 6, 2025 at 2:50 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > > However, I don't understand why we need this and the subsequent > > > > is_acpi_device_node() and is_of_node() checks. > > > > > > The idea is to avoid unnecessary table lookups when both OF and ACPI > > > match tables are present. > > > > Ok, that's fair -- let's build it on top of the FwNode abstractions though. > > I'm ok with the FwNode abstractions. Just to make sure I understood > you correctly: > I'll need to wait until these FwNode abstractions are pushed to the > rust-next branch, reimplement what is necessary and send v3. Is this > the course of actions? Not all Rust code goes through the Rust-for-Linux tree, it depends on who maintains the code. For the FwNode and device property series I pointed you to, the code is maintained by the driver-core maintainers and hence will go through the driver-core tree [1]. (You can always check the corresponding entries in the MAINTAINERS file, they document, who maintains a file and which tree changes go through. For instance, if you want to know this for a specific file, you can run ./scripts/get_maintainer.pl path/to/file and the script tells you everything you need to know. In general, before submitting patches you should run this script on your patches to find out who you should send them to.) However, there is no need to wait until the FwNode and device property series lands in driver-core-next, you can just fetch the patch series from the mailing list and build your patches on top of that. If you do this, you should make sure to mention the exact series you build on top of in the cover letter, ideally with a lore link to the specific version of the series. [1] https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/
On Fri, Jun 06, 2025 at 03:26:13PM +0100, Igor Korotin wrote: > On Fri, Jun 6, 2025 at 2:50 PM Danilo Krummrich <dakr@kernel.org> wrote: > > However, I don't understand why we need this and the subsequent > > is_acpi_device_node() and is_of_node() checks. > > The idea is to avoid unnecessary table lookups when both OF and ACPI > match tables are present. If we already know the fwnode type, these > simple pointer comparisons (is_acpi_device_node() / is_of_node()) let > us skip the irrelevant match function. > > Those checks are cheap (just pointer comparisons), while > acpi_match_device() and of_match_device() iterate over tables. > > So yeah, it’s a bit ugly, but it can save some CPU cycles during enumeration. You have loads of CPU cycles during enumeration, keep things simple first, only attempt to optimize things later on if it is actually measureable. thanks, greg k-h
On 6/6/25 5:29 PM, Greg Kroah-Hartman wrote: > On Fri, Jun 06, 2025 at 03:26:13PM +0100, Igor Korotin wrote: >> On Fri, Jun 6, 2025 at 2:50 PM Danilo Krummrich <dakr@kernel.org> wrote: >>> However, I don't understand why we need this and the subsequent >>> is_acpi_device_node() and is_of_node() checks. >> >> The idea is to avoid unnecessary table lookups when both OF and ACPI >> match tables are present. If we already know the fwnode type, these >> simple pointer comparisons (is_acpi_device_node() / is_of_node()) let >> us skip the irrelevant match function. >> >> Those checks are cheap (just pointer comparisons), while >> acpi_match_device() and of_match_device() iterate over tables. >> >> So yeah, it’s a bit ugly, but it can save some CPU cycles during enumeration. > > You have loads of CPU cycles during enumeration, keep things simple > first, only attempt to optimize things later on if it is actually > measureable. I'm fine either way, I don't expect much value in optimizing this and at the same time I don't see doing it adds significant complexity either. If Greg prefers not to have this optimization to begin with, let's go without it please.
On Fri, Jun 6, 2025 at 4:39 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On 6/6/25 5:29 PM, Greg Kroah-Hartman wrote: > > On Fri, Jun 06, 2025 at 03:26:13PM +0100, Igor Korotin wrote: > >> On Fri, Jun 6, 2025 at 2:50 PM Danilo Krummrich <dakr@kernel.org> wrote: > >>> However, I don't understand why we need this and the subsequent > >>> is_acpi_device_node() and is_of_node() checks. > >> > >> The idea is to avoid unnecessary table lookups when both OF and ACPI > >> match tables are present. If we already know the fwnode type, these > >> simple pointer comparisons (is_acpi_device_node() / is_of_node()) let > >> us skip the irrelevant match function. > >> > >> Those checks are cheap (just pointer comparisons), while > >> acpi_match_device() and of_match_device() iterate over tables. > >> > >> So yeah, it’s a bit ugly, but it can save some CPU cycles during enumeration. > > > > You have loads of CPU cycles during enumeration, keep things simple > > first, only attempt to optimize things later on if it is actually > > measureable. > > I'm fine either way, I don't expect much value in optimizing this and at the > same time I don't see doing it adds significant complexity either. > > If Greg prefers not to have this optimization to begin with, let's go without > it please. I'm ok with this as well. Will remove those checks. I assume that I don't need to align with `FwNode` abstractions in that case. Also I'll drop `is_of_node` rust helper in v3. Thanks for the review and your comments, guys Cheers Igor
This patch series introduces support for ACPI match tables in Rust drivers. Currently, Rust abstractions support only Open Firmware (OF) device matching. This series extends the driver model to support ACPI-based matching, enabling Rust drivers to bind to ACPI-described devices. Changes include: - A new `acpi::DeviceId` abstraction for working with `struct acpi_device_id`. - A helper function `is_of_node()` for determining fwnode types. - Updates to the core `Adapter` trait and `platform::Driver` to support optional ACPI ID tables. - A sample implementation in the Rust platform driver, demonstrating multi-bus matching. This is especially useful for writing drivers that work across platforms using both OF and ACPI. Tested using QEMU with a custom SSDT that creates an ACPI device matching the sample Rust platform driver. Igor Korotin (5): rust: acpi: add `acpi::DeviceId` abstraction rust: helpers: Add `is_of_node` helper function rust: driver: Add ACPI id table support to Adapter trait rust: platform: Add ACPI match table support to `Driver` trait samples: rust: add ACPI match table example to platform driver Changelog --------- v2: - Removed misleading comment in `acpi::DeviceID` implementation. - Removed unnecessary casting in `acpi::DeviceID::new`. - Moved `pub mod acpi` to correct alphabetical position in `rust/kernel/lib.rs`. - Link to v1: https://lore.kernel.org/rust-for-linux/20250530123815.1766726-1-igor.korotin.linux@gmail.com/ MAINTAINERS | 2 + rust/bindings/bindings_helper.h | 1 + rust/helpers/helpers.c | 1 + rust/helpers/of.c | 6 +++ rust/kernel/acpi.rs | 62 ++++++++++++++++++++++++++++ rust/kernel/driver.rs | 58 ++++++++++++++++++++++++-- rust/kernel/lib.rs | 1 + rust/kernel/platform.rs | 17 +++++++- samples/rust/rust_driver_platform.rs | 41 +++++++++++++++++- 9 files changed, 183 insertions(+), 6 deletions(-) create mode 100644 rust/helpers/of.c create mode 100644 rust/kernel/acpi.rs