rust: Add ACPI match table support for Rust drivers

From: Igor Korotin <[email protected]>

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(-)

rust/bindings/bindings_helper.h +1 -0

index e0bcd130b494..d974fc6c141f 100644
@@ -6,6 +6,7 @@
  * Sorted alphabetically.
  */
 
+#include <linux/acpi.h>
 #include <kunit/test.h>
 #include <linux/blk-mq.h>
 #include <linux/blk_types.h>

rust/kernel/driver.rs +33 -1

index ec9166cedfa7..d4098596188a 100644
@@ -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>>;
 

rust/kernel/driver.rs +5 -0

+            let id = Self::of_id_info(dev);
+
+            if id.is_some() {
+                return id;
+            }
         }
 
         None

rust/kernel/platform.rs +5 -0

index fd4a494f30e8..3cc9fe6ccfcf 100644
@@ -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.

Signature · 1 line

2.43.0

However, I don't understand why we need this and the subsequent is_acpi_device_node() and is_of_node() checks.

Quoted 2 lines · Danilo Krummrich

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.

Quoted 6 lines · Igor Korotin

Quoted 2 lines · Danilo Krummrich

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.

Quoted 9 lines · Danilo Krummrich

Quoted 6 lines · Igor Korotin

Quoted 2 lines · Danilo Krummrich

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.

Quoted 1 line

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?

Quoted 2 lines · Danilo Krummrich

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.

Quoted 9 lines · Igor Korotin

Quoted 2 lines · Danilo Krummrich

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.

Quoted 4 lines

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.

Quoted 10 lines · Greg Kroah-Hartman

Quoted 9 lines · Igor Korotin

Quoted 2 lines · Danilo Krummrich

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.

Quoted 8 lines

Quoted 4 lines

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.

Extend the Rust sample platform driver to probe using device/driver name matching, OF ID table matching, or ACPI ID table matching.

Signed-off-by Igor Korotin <[email protected]>

---
 samples/rust/rust_driver_platform.rs | 40 +++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

samples/rust/rust_driver_platform.rs +0 -0

index e3992e7a71e9..ee0780c1d6ae 100644

samples/rust/rust_driver_platform.rs +22 -0

+);
+
+/// OF/ACPI match tables for Platform Driver implementation
+///
+/// The platform::Driver requires declaration of both OF_ID_TABLE and
+/// ACPI_ID_TABLE, but if driver is not going to use either of them
+/// it can implement one of them or both as None.
+///
+/// # Example:
+///
+///```
+/// impl platform::Driver for SampleDriver {
+///     type IdInfo = Info;
+///     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
+///     const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = None;
+///
+///     fn probe(
+///         pdev: &platform::Device<Core>,
+///         info: Option<&Self::IdInfo>,
+///     ) -> Result<Pin<KBox<Self>>> {
+///         dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n");
+///

samples/rust/rust_driver_platform.rs +2 -1

+
 impl platform::Driver for SampleDriver {
     type IdInfo = Info;
     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
-    const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = None;
+    const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
 
     fn probe(
         pdev: &platform::Device<Core>,

Signature · 1 line

2.43.0

This patch series introduces support for ACPI match tables in Rust drivers.