[PATCH v3 0/4] rust: Add ACPI match table support for Rust drivers

[PATCH v3 0/4] rust: Add ACPI match table support for Rust drivers

[Igor Korotin]

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`.
  - 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 (4):
  rust: acpi: add `acpi::DeviceId` abstraction
  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
---------
v3:
 - Removed fwnode type check in `Adapter::id_info` per Greg's and Danilo's
   comments
 - Removed `is_of_node` rust helper, due to unnecessity. 
 - Fixed example code in `rust_driver_platform.rs` per Danilo's comment
 - Added an instruction of testing ACPI using QEMU with a custom SSDT
 - Fixed minor code formatting issues.
 - Link to v2: https://lore.kernel.org/rust-for-linux/20250605161956.3658374-1-igor.korotin.linux@gmail.com/
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                          |  1 +
 rust/bindings/bindings_helper.h      |  1 +
 rust/kernel/acpi.rs                  | 61 +++++++++++++++++
 rust/kernel/driver.rs                | 39 ++++++++++-
 rust/kernel/lib.rs                   |  1 +
 rust/kernel/platform.rs              | 18 +++++-
 samples/rust/rust_driver_platform.rs | 97 +++++++++++++++++++++++++++-
 7 files changed, 214 insertions(+), 4 deletions(-)
 create mode 100644 rust/kernel/acpi.rs


base-commit: 9857af0fcff385c75433f2162c30c62eb912ef6d
-- 
2.43.0

[PATCH v3 1/4] rust: acpi: add `acpi::DeviceId` abstraction

[Igor Korotin]

`acpi::DeviceId` is an abstraction around `struct acpi_device_id`.

This is used by subsequent patches, in particular the i2c driver
abstractions, to create ACPI device ID tables.

Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
---
 MAINTAINERS         |  1 +
 rust/kernel/acpi.rs | 61 +++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs  |  1 +
 3 files changed, 63 insertions(+)
 create mode 100644 rust/kernel/acpi.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 731f09909a92..44ae63e4b3b8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -302,6 +302,7 @@ F:	include/linux/acpi.h
 F:	include/linux/fwnode.h
 F:	include/linux/fw_table.h
 F:	lib/fw_table.c
+F:	rust/kernel/acpi.rs
 F:	tools/power/acpi/
 
 ACPI APEI
diff --git a/rust/kernel/acpi.rs b/rust/kernel/acpi.rs
new file mode 100644
index 000000000000..f9a98dc4eb8a
--- /dev/null
+++ b/rust/kernel/acpi.rs
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Advanced Configuration and Power Interface abstractions.
+
+use crate::{bindings, device_id::RawDeviceId, prelude::*};
+
+/// IdTable type for ACPI drivers.
+pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
+
+/// An ACPI device id.
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+pub struct DeviceId(bindings::acpi_device_id);
+
+// SAFETY:
+// * `DeviceId` is a `#[repr(transparent)` wrapper of `struct acpi_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::acpi_device_id;
+
+    const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::acpi_device_id, driver_data);
+
+    fn index(&self) -> usize {
+        self.0.driver_data as _
+    }
+}
+
+impl DeviceId {
+    const ACPI_ID_LEN: usize = 16;
+
+    /// Create a new device id from an ACPI 'id' string.
+    pub const fn new(id: &'static CStr) -> Self {
+        assert!(id.len() <= Self::ACPI_ID_LEN, "ID exceeds 16 bytes");
+        let src = id.as_bytes_with_nul();
+        // Replace with `bindings::acpi_device_id::default()` once stabilized for `const`.
+        // SAFETY: FFI type is valid to be zero-initialized.
+        let mut acpi: bindings::acpi_device_id = unsafe { core::mem::zeroed() };
+        let mut i = 0;
+        while i < src.len() {
+            acpi.id[i] = src[i];
+            i += 1;
+        }
+
+        Self(acpi)
+    }
+}
+
+/// Create an ACPI `IdTable` with an "alias" for modpost.
+#[macro_export]
+macro_rules! acpi_device_table {
+    ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => {
+        const $table_name: $crate::device_id::IdArray<
+            $crate::acpi::DeviceId,
+            $id_info_type,
+            { $table_data.len() },
+        > = $crate::device_id::IdArray::new($table_data);
+
+        $crate::module_device_table!("acpi", $module_table_name, $table_name);
+    };
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index de07aadd1ff5..3c17e60e3093 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -37,6 +37,7 @@
 
 pub use ffi;
 
+pub mod acpi;
 pub mod alloc;
 #[cfg(CONFIG_BLOCK)]
 pub mod block;
-- 
2.43.0

[PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

[Igor Korotin]

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.

Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/driver.rs           | 39 ++++++++++++++++++++++++++++++++-
 rust/kernel/platform.rs         |  6 ++++-
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ab37e1d35c70..002d84f06b42 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..638f1d270af8 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::{acpi, device, of, 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,6 +210,11 @@ 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::acpi_id_info(dev);
+        if id.is_some() {
+            return id;
+        }
+
         let id = Self::of_id_info(dev);
         if id.is_some() {
             return id;
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 4917cb34e2fe..dd77934937d3 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -5,7 +5,7 @@
 //! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
 
 use crate::{
-    bindings, device, driver,
+    acpi, bindings, device, driver,
     error::{to_result, Result},
     of,
     prelude::*,
@@ -95,6 +95,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

[PATCH v3 3/4] rust: platform: Add ACPI match table support to `Driver` trait

[Igor Korotin]

Extend the `platform::Driver` trait to support ACPI device matching by
adding the `ACPI_ID_TABLE` constant.

This allows Rust platform drivers to define ACPI match tables alongside
their existing OF match tables.

These changes mirror the existing OF support and allow Rust platform
drivers to match devices based on ACPI identifiers.

To avoid breaking compilation, a stub ACPI match table definition is
added to the Rust sample platform driver. Functional support for ACPI
matching in the sample driver will be provided in a follow-up patch.

Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
---
 rust/kernel/platform.rs              | 14 ++++++++++++--
 samples/rust/rust_driver_platform.rs |  3 ++-
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index dd77934937d3..78010a613c54 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -38,12 +38,18 @@ unsafe fn register(
             None => core::ptr::null(),
         };
 
+        let acpi_table = match T::ACPI_ID_TABLE {
+            Some(table) => table.as_ptr(),
+            None => core::ptr::null(),
+        };
+
         // SAFETY: It's safe to set the fields of `struct platform_driver` on initialization.
         unsafe {
             (*pdrv.get()).driver.name = name.as_char_ptr();
             (*pdrv.get()).probe = Some(Self::probe_callback);
             (*pdrv.get()).remove = Some(Self::remove_callback);
             (*pdrv.get()).driver.of_match_table = of_table;
+            (*pdrv.get()).driver.acpi_match_table = acpi_table;
         }
 
         // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
@@ -97,7 +103,7 @@ fn of_id_table() -> Option<of::IdTable<Self::IdInfo>> {
     }
 
     fn acpi_id_table() -> Option<acpi::IdTable<Self::IdInfo>> {
-        None
+        T::ACPI_ID_TABLE
     }
 }
 
@@ -128,7 +134,7 @@ macro_rules! module_platform_driver {
 /// # Example
 ///
 ///```
-/// # use kernel::{bindings, c_str, device::Core, of, platform};
+/// # use kernel::{acpi, bindings, c_str, device::Core, of, platform};
 ///
 /// struct MyDriver;
 ///
@@ -144,6 +150,7 @@ macro_rules! module_platform_driver {
 /// impl platform::Driver for MyDriver {
 ///     type IdInfo = ();
 ///     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
+///     const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
 ///
 ///     fn probe(
 ///         _pdev: &platform::Device<Core>,
@@ -164,6 +171,9 @@ pub trait Driver: Send {
     /// The table of OF device ids supported by the driver.
     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>>;
 
+    /// The table of ACPI device ids supported by the driver.
+    const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>>;
+
     /// Platform driver probe.
     ///
     /// Called when a new platform device is added or discovered.
diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
index 8b42b3cfb363..e3992e7a71e9 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -2,7 +2,7 @@
 
 //! Rust Platform driver sample.
 
-use kernel::{c_str, device::Core, of, platform, prelude::*, types::ARef};
+use kernel::{acpi, c_str, device::Core, of, platform, prelude::*, types::ARef};
 
 struct SampleDriver {
     pdev: ARef<platform::Device>,
@@ -20,6 +20,7 @@ struct SampleDriver {
 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;
 
     fn probe(
         pdev: &platform::Device<Core>,
-- 
2.43.0

[PATCH v3 4/4] samples: rust: add ACPI match table example to platform driver

[Igor Korotin]

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 <igor.korotin.linux@gmail.com>
---
 samples/rust/rust_driver_platform.rs | 96 +++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 1 deletion(-)

diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
index e3992e7a71e9..be7c311dca07 100644
--- a/samples/rust/rust_driver_platform.rs
+++ b/samples/rust/rust_driver_platform.rs
@@ -17,10 +17,104 @@ struct SampleDriver {
     [(of::DeviceId::new(c_str!("test,rust-device")), Info(42))]
 );
 
+// ACPI match table test
+//
+// This demonstrates how to test an ACPI-based Rust platform driver using QEMU
+// with a custom SSDT.
+//
+// Steps:
+//
+// 1. **Create an SSDT source file** (`ssdt.dsl`) with the following content:
+//
+//     ```asl
+//     DefinitionBlock ("", "SSDT", 2, "TEST", "VIRTACPI", 0x00000001)
+//     {
+//         Scope (\_SB)
+//         {
+//             Device (T432)
+//             {
+//                 Name (_HID, "TEST4321")  // ACPI hardware ID to match
+//                 Name (_UID, 1)
+//                 Name (_STA, 0x0F)        // Device present, enabled
+//                 Name (_CRS, ResourceTemplate ()
+//                 {
+//                     Memory32Fixed (ReadWrite, 0xFED00000, 0x1000)
+//                 })
+//             }
+//         }
+//     }
+//     ```
+//
+// 2. **Compile the table**:
+//
+//     ```sh
+//     iasl -tc ssdt.dsl
+//     ```
+//
+//     This generates `ssdt.aml`
+//
+// 3. **Run QEMU** with the compiled AML file:
+//
+//     ```sh
+//     qemu-system-x86_64 -m 512M \
+//         -enable-kvm \
+//         -kernel path/to/bzImage \
+//         -append "root=/dev/sda console=ttyS0" \
+//         -hda rootfs.img \
+//         -serial stdio \
+//         -acpitable file=ssdt.aml
+//     ```
+//
+//     Requirements:
+//     - The `rust_driver_platform` must be present either:
+//         - built directly into the kernel (`bzImage`), or
+//         - available as a `.ko` file and loadable from `rootfs.img`
+//
+// 4. **Verify it worked** by checking `dmesg`:
+//
+//     ```
+//     rust_driver_platform TEST4321:00: Probed with info: '0'.
+//     ```
+//
+// This demonstrates ACPI table matching using a custom ID in QEMU with a minimal SSDT
+
+kernel::acpi_device_table!(
+    ACPI_TABLE,
+    MODULE_ACPI_TABLE,
+    <SampleDriver as platform::Driver>::IdInfo,
+    [(acpi::DeviceId::new(c_str!("TEST4321")), Info(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:
+///
+/// ```ignore
+/// impl platform::Driver for SampleDriver {
+///     type IdInfo = ();
+///     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");
+///
+///         let drvdata = KBox::new(Self { pdev: pdev.into() }, GFP_KERNEL)?;
+///
+///         Ok(drvdata.into())
+///     }
+/// }
+/// ```
 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>,
-- 
2.43.0

Re: [PATCH v3 3/4] rust: platform: Add ACPI match table support to `Driver` trait

[kernel test robot]

Hi Igor,

kernel test robot noticed the following build errors:

[auto build test ERROR on 9857af0fcff385c75433f2162c30c62eb912ef6d]

url:    https://github.com/intel-lab-lkp/linux/commits/Igor-Korotin/rust-acpi-add-acpi-DeviceId-abstraction/20250607-011123
base:   9857af0fcff385c75433f2162c30c62eb912ef6d
patch link:    https://lore.kernel.org/r/20250606170905.3881900-1-igor.korotin.linux%40gmail.com
patch subject: [PATCH v3 3/4] rust: platform: Add ACPI match table support to `Driver` trait
config: x86_64-buildonly-randconfig-001-20250607 (https://download.01.org/0day-ci/archive/20250607/202506071640.KIUrSBGt-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250607/202506071640.KIUrSBGt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506071640.KIUrSBGt-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error[E0425]: cannot find value `ACPI_TABLE` in this scope
   --> rust/doctests_kernel_generated.rs:3527:71
   |
   3527 |      const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
   |                                                                       ^^^^^^^^^^ not found in this scope

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 1/4] rust: acpi: add `acpi::DeviceId` abstraction

[Benno Lossin]

On Fri Jun 6, 2025 at 7:06 PM CEST, Igor Korotin wrote:
> +impl DeviceId {
> +    const ACPI_ID_LEN: usize = 16;
> +
> +    /// Create a new device id from an ACPI 'id' string.
> +    pub const fn new(id: &'static CStr) -> Self {
> +        assert!(id.len() <= Self::ACPI_ID_LEN, "ID exceeds 16 bytes");
> +        let src = id.as_bytes_with_nul();
> +        // Replace with `bindings::acpi_device_id::default()` once stabilized for `const`.
> +        // SAFETY: FFI type is valid to be zero-initialized.
> +        let mut acpi: bindings::acpi_device_id = unsafe { core::mem::zeroed() };

This can be made safe using this series:

    https://lore.kernel.org/all/20250523145125.523275-1-lossin@kernel.org

---
Cheers,
Benno

> +        let mut i = 0;
> +        while i < src.len() {
> +            acpi.id[i] = src[i];
> +            i += 1;
> +        }
> +
> +        Self(acpi)
> +    }
> +}

Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

[Benno Lossin]

On Fri Jun 6, 2025 at 7:08 PM CEST, Igor Korotin wrote:
> @@ -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)]

I think we should change this to one single definition and do

    if cfg!(not(CONFIG_ACPI)) {
        return None;
    }
    /* body from above */

In a single function instead.

> +    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,6 +210,11 @@ 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::acpi_id_info(dev);
> +        if id.is_some() {
> +            return id;
> +        }

Is a driver only going to have one id_info? Or is there some kind of
precedence?

---
Cheers,
Benno

Re: [PATCH v3 4/4] samples: rust: add ACPI match table example to platform driver

[Benno Lossin]

On Fri Jun 6, 2025 at 7:10 PM CEST, Igor Korotin wrote:
> 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 <igor.korotin.linux@gmail.com>
> ---
>  samples/rust/rust_driver_platform.rs | 96 +++++++++++++++++++++++++++-
>  1 file changed, 95 insertions(+), 1 deletion(-)
>
> diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
> index e3992e7a71e9..be7c311dca07 100644
> --- a/samples/rust/rust_driver_platform.rs
> +++ b/samples/rust/rust_driver_platform.rs
> @@ -17,10 +17,104 @@ struct SampleDriver {
>      [(of::DeviceId::new(c_str!("test,rust-device")), Info(42))]
>  );
>  
> +// ACPI match table test

This looks great, let's move it to the crate-level documentation. That
way it gets rendered :)

---
Cheers,
Benno

> +//
> +// This demonstrates how to test an ACPI-based Rust platform driver using QEMU
> +// with a custom SSDT.
> +//
> +// Steps:

Re: [PATCH v3 1/4] rust: acpi: add `acpi::DeviceId` abstraction

[Danilo Krummrich]

On Sun, Jun 08, 2025 at 09:48:00AM +0200, Benno Lossin wrote:
> On Fri Jun 6, 2025 at 7:06 PM CEST, Igor Korotin wrote:
> > +impl DeviceId {
> > +    const ACPI_ID_LEN: usize = 16;
> > +
> > +    /// Create a new device id from an ACPI 'id' string.
> > +    pub const fn new(id: &'static CStr) -> Self {
> > +        assert!(id.len() <= Self::ACPI_ID_LEN, "ID exceeds 16 bytes");
> > +        let src = id.as_bytes_with_nul();
> > +        // Replace with `bindings::acpi_device_id::default()` once stabilized for `const`.
> > +        // SAFETY: FFI type is valid to be zero-initialized.
> > +        let mut acpi: bindings::acpi_device_id = unsafe { core::mem::zeroed() };
> 
> This can be made safe using this series:
> 
>     https://lore.kernel.org/all/20250523145125.523275-1-lossin@kernel.org

Indeed, I did not mention this though since I think this series should not
depend on the one above. They'll land through different trees and the
improvement can still be made later on.

Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

[Danilo Krummrich]

On Sun, Jun 08, 2025 at 09:54:30AM +0200, Benno Lossin wrote:
> On Fri Jun 6, 2025 at 7:08 PM CEST, Igor Korotin wrote:
> > @@ -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)]
> 
> I think we should change this to one single definition and do
> 
>     if cfg!(not(CONFIG_ACPI)) {
>         return None;
>     }
>     /* body from above */
> 
> In a single function instead.

Generally, that's fine, but in this case I'd rather keep it as it is for
consistency with the rest of the file.

> > +    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,6 +210,11 @@ 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::acpi_id_info(dev);
> > +        if id.is_some() {
> > +            return id;
> > +        }
> 
> Is a driver only going to have one id_info? Or is there some kind of
> precedence?

A driver potentially has lots of them, but the device is only matching a single
entry in one of the driver's ID tables and hence a single ID info.

Re: [PATCH v3 4/4] samples: rust: add ACPI match table example to platform driver

[Danilo Krummrich]

On Sun, Jun 08, 2025 at 09:58:46AM +0200, Benno Lossin wrote:
> On Fri Jun 6, 2025 at 7:10 PM CEST, Igor Korotin wrote:
> > 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 <igor.korotin.linux@gmail.com>
> > ---
> >  samples/rust/rust_driver_platform.rs | 96 +++++++++++++++++++++++++++-
> >  1 file changed, 95 insertions(+), 1 deletion(-)
> >
> > diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs
> > index e3992e7a71e9..be7c311dca07 100644
> > --- a/samples/rust/rust_driver_platform.rs
> > +++ b/samples/rust/rust_driver_platform.rs
> > @@ -17,10 +17,104 @@ struct SampleDriver {
> >      [(of::DeviceId::new(c_str!("test,rust-device")), Info(42))]
> >  );
> >  
> > +// ACPI match table test
> 
> This looks great, let's move it to the crate-level documentation.

+1

> That way it gets rendered :)

Just to be precise, I think the build system does not yet render driver
documentation, but it should be in the future.

Re: [PATCH v3 3/4] rust: platform: Add ACPI match table support to `Driver` trait

[Danilo Krummrich]

On Fri, Jun 06, 2025 at 06:09:05PM +0100, Igor Korotin wrote:
> @@ -144,6 +150,7 @@ macro_rules! module_platform_driver {
>  /// impl platform::Driver for MyDriver {
>  ///     type IdInfo = ();
>  ///     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +///     const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);

This breaks the doctest. You need to define ACPI_TABLE above, just like OF_TABLE
is defined above.

If you enable CONFIG_RUST_KERNEL_DOCTESTS=y it will compile the doctests and run
them as kunit test.

Re: [PATCH v3 4/4] samples: rust: add ACPI match table example to platform driver

[Danilo Krummrich]

On Fri, Jun 06, 2025 at 06:10:33PM +0100, Igor Korotin wrote:
> Extend the Rust sample platform driver to probe using device/driver name
> +/// 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:
> +///
> +/// ```ignore
> +/// impl platform::Driver for SampleDriver {
> +///     type IdInfo = ();
> +///     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");
> +///
> +///         let drvdata = KBox::new(Self { pdev: pdev.into() }, GFP_KERNEL)?;
> +///
> +///         Ok(drvdata.into())
> +///     }
> +/// }
> +/// ```

What I meant with [1] was that I think we should make this code compile and
remove everything that's not needed, i.e.:

	///```
	/// # use kernel::{acpi, device::Core, of, platform};
	///
	/// struct MyDriver;
	///
	/// impl platform::Driver for MyDriver {
	///     type IdInfo = ();
	///     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>,
	///         _id_info: Option<&Self::IdInfo>,
	///     ) -> Result<Pin<KBox<Self>>> {
	///         Err(ENODEV)
	///     }
	/// }
	///```

However, given that we can't run doctests from drivers yet, we should just
remove this doctest I think. It much more belongs into rust/kernel/platform.rs
anyways (where we already have a similar one).

[1] https://lore.kernel.org/lkml/aEL0AGBZqDp1lMFe@pollux/

Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

[Benno Lossin]

On Sun Jun 8, 2025 at 12:48 PM CEST, Danilo Krummrich wrote:
> On Sun, Jun 08, 2025 at 09:54:30AM +0200, Benno Lossin wrote:
>> On Fri Jun 6, 2025 at 7:08 PM CEST, Igor Korotin wrote:
>> > @@ -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)]
>> 
>> I think we should change this to one single definition and do
>> 
>>     if cfg!(not(CONFIG_ACPI)) {
>>         return None;
>>     }
>>     /* body from above */
>> 
>> In a single function instead.
>
> Generally, that's fine, but in this case I'd rather keep it as it is for
> consistency with the rest of the file.

Then let's also change the OF bindings in this file to that style :)

>> > +    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,6 +210,11 @@ 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::acpi_id_info(dev);
>> > +        if id.is_some() {
>> > +            return id;
>> > +        }
>> 
>> Is a driver only going to have one id_info? Or is there some kind of
>> precedence?
>
> A driver potentially has lots of them, but the device is only matching a single
> entry in one of the driver's ID tables and hence a single ID info.

Ah so if `of_id_info` and `acpi_id_info` return `Some(_)`, then both
values are the same?

---
Cheers,
Benno

Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

[Danilo Krummrich]

On Sun, Jun 08, 2025 at 01:46:17PM +0200, Benno Lossin wrote:
> On Sun Jun 8, 2025 at 12:48 PM CEST, Danilo Krummrich wrote:
> > On Sun, Jun 08, 2025 at 09:54:30AM +0200, Benno Lossin wrote:
> >> On Fri Jun 6, 2025 at 7:08 PM CEST, Igor Korotin wrote:
> >> > @@ -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)]
> >> 
> >> I think we should change this to one single definition and do
> >> 
> >>     if cfg!(not(CONFIG_ACPI)) {
> >>         return None;
> >>     }
> >>     /* body from above */
> >> 
> >> In a single function instead.
> >
> > Generally, that's fine, but in this case I'd rather keep it as it is for
> > consistency with the rest of the file.
> 
> Then let's also change the OF bindings in this file to that style :)

Fine for me.

@Igor: If you do so, please do it in a seaparate patch.

> >> > +    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,6 +210,11 @@ 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::acpi_id_info(dev);
> >> > +        if id.is_some() {
> >> > +            return id;
> >> > +        }
> >> 
> >> Is a driver only going to have one id_info? Or is there some kind of
> >> precedence?
> >
> > A driver potentially has lots of them, but the device is only matching a single
> > entry in one of the driver's ID tables and hence a single ID info.
> 
> Ah so if `of_id_info` and `acpi_id_info` return `Some(_)`, then both
> values are the same?

No, if one of them returns Some(_), the other one will always return None. Or
phrased differently, the first match will always be the only match.

Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

[Igor Korotin]

On Sun, Jun 8, 2025 at 12:50 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sun, Jun 08, 2025 at 01:46:17PM +0200, Benno Lossin wrote:
> > On Sun Jun 8, 2025 at 12:48 PM CEST, Danilo Krummrich wrote:
> > > On Sun, Jun 08, 2025 at 09:54:30AM +0200, Benno Lossin wrote:
> > >> On Fri Jun 6, 2025 at 7:08 PM CEST, Igor Korotin wrote:
> > >> > @@ -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)]
> > >>
> > >> I think we should change this to one single definition and do
> > >>
> > >>     if cfg!(not(CONFIG_ACPI)) {
> > >>         return None;
> > >>     }
> > >>     /* body from above */
> > >>
> > >> In a single function instead.
> > >
> > > Generally, that's fine, but in this case I'd rather keep it as it is for
> > > consistency with the rest of the file.
> >
> > Then let's also change the OF bindings in this file to that style :)
>
> Fine for me.
>
> @Igor: If you do so, please do it in a seaparate patch.

That's definitely not possible with `if cfg!(not(CONFIG_ACPI))`
because it is a runtime condition and it breaks compilation in case
either `CONFIG_OF` or `CONFIG_ACPI` is not set, the Rust compiler
can't find `of_match_device`/`acpi_match_device` because in that case
they are defined as `static inline` and are not parsed by bindgen.

Alternatively the following pattern works:

    fn foo() {
        #[cfg(not(CONFIG_ACPI))] {
            None
        }

        #[cfg(CONFIG_ACPI)] {
             // some logic.
        }
    }

Also the argument `dev` of `both of_id_info` and `acpi_id_info`  must be
renamed into `_dev` to mark it as unused in case CONFIG_ACPI/CONFIG_OF
is off and the argument is not used.

Let me know if this pattern is acceptable. To my opinion it is not much
different from the original 2 functions conditioned by #[cfg] and
requires some nasty changes.

Thanks
Igor

Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

[Miguel Ojeda]

On Mon, Jun 9, 2025 at 3:09 PM Igor Korotin
<igor.korotin.linux@gmail.com> wrote:
>
> Let me know if this pattern is acceptable. To my opinion it is not much
> different from the original 2 functions conditioned by #[cfg] and
> requires some nasty changes.

In general, the more local a `cfg` can be made, the better, because we
can share more, e.g. the docs and signature.

But maybe in this case it doesn't work -- what would be the "nasty
changes" required?

Cheers,
Miguel

Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

[Igor Korotin]

> <igor.korotin.linux@gmail.com> wrote:
> >
> > Let me know if this pattern is acceptable. To my opinion it is not much
> > different from the original 2 functions conditioned by #[cfg] and
> > requires some nasty changes.
>
> In general, the more local a `cfg` can be made, the better, because we
> can share more, e.g. the docs and signature.
>
> But maybe in this case it doesn't work -- what would be the "nasty
> changes" required?

In particular on this example:
    fn of_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
        #[cfg(not(CONFIG_OF))] {
            None
        }

        #[cfg(CONFIG_OF))] {
            // the rest of logic
        }
    }

The `dev` is marked as unused if `CONFIG_OF` is not set. So we have 3 options:
1. rename it to `_dev`. This is nasty to my opinion, because it is misleading.
2. add #[alloc(unused_variables)] on top of it. Also not good since it
will suppress
all other possible unused_variables as well.
3. The third option is `let _ = dev;` in `#[cfg(not(CONFIG_OF))]`
section. I came to
it while I was writing this reply. This looks like the best option of
three in my opinion.

Thanks
Igor

Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

[Benno Lossin]

On Mon Jun 9, 2025 at 3:09 PM CEST, Igor Korotin wrote:
> On Sun, Jun 8, 2025 at 12:50 PM Danilo Krummrich <dakr@kernel.org> wrote:
>> On Sun, Jun 08, 2025 at 01:46:17PM +0200, Benno Lossin wrote:
>> > On Sun Jun 8, 2025 at 12:48 PM CEST, Danilo Krummrich wrote:
>> > > On Sun, Jun 08, 2025 at 09:54:30AM +0200, Benno Lossin wrote:
>> > >> On Fri Jun 6, 2025 at 7:08 PM CEST, Igor Korotin wrote:
>> > >> > @@ -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)]
>> > >>
>> > >> I think we should change this to one single definition and do
>> > >>
>> > >>     if cfg!(not(CONFIG_ACPI)) {
>> > >>         return None;
>> > >>     }
>> > >>     /* body from above */
>> > >>
>> > >> In a single function instead.
>> > >
>> > > Generally, that's fine, but in this case I'd rather keep it as it is for
>> > > consistency with the rest of the file.
>> >
>> > Then let's also change the OF bindings in this file to that style :)
>>
>> Fine for me.
>>
>> @Igor: If you do so, please do it in a seaparate patch.
>
> That's definitely not possible with `if cfg!(not(CONFIG_ACPI))`
> because it is a runtime condition and it breaks compilation in case
> either `CONFIG_OF` or `CONFIG_ACPI` is not set, the Rust compiler
> can't find `of_match_device`/`acpi_match_device` because in that case
> they are defined as `static inline` and are not parsed by bindgen.
>
> Alternatively the following pattern works:
>
>     fn foo() {
>         #[cfg(not(CONFIG_ACPI))] {
>             None
>         }
>
>         #[cfg(CONFIG_ACPI)] {
>              // some logic.
>         }
>     }

We might want to add a `cfg_if!` macro that expands to this.

> Also the argument `dev` of `both of_id_info` and `acpi_id_info`  must be
> renamed into `_dev` to mark it as unused in case CONFIG_ACPI/CONFIG_OF
> is off and the argument is not used.

You can add a `let _ = dev;` in the `not` "branch".

---
Cheers,
Benno

Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

[Miguel Ojeda]

On Mon, Jun 9, 2025 at 4:07 PM Igor Korotin
<igor.korotin.linux@gmail.com> wrote:
>
> 1. rename it to `_dev`. This is nasty to my opinion, because it is misleading.

Yes, please do not change the name that would be use in the "normal/full case".

> 2. add #[alloc(unused_variables)] on top of it. Also not good since it
> will suppress
> all other possible unused_variables as well.

This can be done conditionally with `cfg_attr`, but it is not great.

> 3. The third option is `let _ = dev;` in `#[cfg(not(CONFIG_OF))]`
> section. I came to
> it while I was writing this reply. This looks like the best option of
> three in my opinion.

Yeah.

Cheers,
Miguel