rust: Add ACPI match table support for Rust drivers

rust/kernel/acpi.rs new file +9 -0

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

rust/kernel/acpi.rs line 38

Quoted 10 lines · Igor Korotin

rust/kernel/acpi.rs new file +10 -0

+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/[email protected]

rust/kernel/driver.rs +18 -0

@@ -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 {

rust/kernel/driver.rs +4 -0

+    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>>;
 

Quoted 22 lines · Igor Korotin

rust/kernel/driver.rs +18 -0

@@ -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 {

rust/kernel/driver.rs pub trait Adapter { · line 171

Quoted 10 lines

rust/kernel/driver.rs +10 -0

+            // 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.

Quoted 7 lines

rust/kernel/driver.rs +4 -0

+    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 fn of_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> { · line 216

Quoted 8 lines

rust/kernel/driver.rs +4 -0

@@ -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?

Quoted 41 lines · Benno Lossin

Quoted 22 lines · Igor Korotin

rust/kernel/driver.rs +18 -0

@@ -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 {

rust/kernel/driver.rs pub trait Adapter { · line 171

Quoted 10 lines

rust/kernel/driver.rs +10 -0

+            // 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 */

Quoted 1 line

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.

Quoted 7 lines

Quoted 7 lines

rust/kernel/driver.rs +4 -0

+    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>>;
 

Quoted 11 lines

rust/kernel/driver.rs fn of_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> { · line 216

Quoted 8 lines

rust/kernel/driver.rs +4 -0

@@ -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.

Quoted 42 lines · Danilo Krummrich

Quoted 41 lines · Benno Lossin

Quoted 22 lines · Igor Korotin

rust/kernel/driver.rs +18 -0

@@ -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 {

rust/kernel/driver.rs pub trait Adapter { · line 171

Quoted 10 lines

rust/kernel/driver.rs +10 -0

+            // 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 */

Quoted 4 lines

Quoted 1 line

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

Quoted 19 lines

Quoted 18 lines

Quoted 8 lines

rust/kernel/driver.rs +4 -0

+    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 fn of_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> { · line 216

Quoted 7 lines

rust/kernel/driver.rs +4 -0

@@ -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?

Quoted 2 lines

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?

Quoted 45 lines · Benno Lossin

Quoted 44 lines · Danilo Krummrich

Quoted 42 lines · Benno Lossin

Quoted 22 lines · Igor Korotin

rust/kernel/driver.rs +18 -0

@@ -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 {

rust/kernel/driver.rs pub trait Adapter { · line 171

Quoted 10 lines

rust/kernel/driver.rs +10 -0

+            // 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.

Quoted 4 lines

Quoted 2 lines

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.

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.

Quoted 4 lines · <[email protected]> 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?

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.

Quoted 46 lines · Danilo Krummrich

Quoted 45 lines · Benno Lossin

Quoted 44 lines · Danilo Krummrich

Quoted 42 lines · Benno Lossin

Quoted 22 lines · Igor Korotin

rust/kernel/driver.rs +18 -0

@@ -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 {

rust/kernel/driver.rs pub trait Adapter { · line 171

Quoted 10 lines

rust/kernel/driver.rs +10 -0

+            // 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.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

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.

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 | 96 +++++++++++++++++++++++++++-
 1 file changed, 95 insertions(+), 1 deletion(-)

samples/rust/rust_driver_platform.rs +0 -0

index e3992e7a71e9..be7c311dca07 100644

samples/rust/rust_driver_platform.rs +5 -0

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

Quoted 16 lines · 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 <[email protected]>

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

samples/rust/rust_driver_platform.rs +0 -0

index e3992e7a71e9..be7c311dca07 100644
@@ -17,10 +17,104 @@ struct SampleDriver {
     [(of::DeviceId::new(c_str!("test,rust-device")), Info(42))]
 );
 

samples/rust/rust_driver_platform.rs struct SampleDriver { · line 20

Quoted 1 line

samples/rust/rust_driver_platform.rs +1 -0

+// ACPI match table test

This looks great, let's move it to the crate-level documentation.

That way it gets rendered :)

Extend the Rust sample platform driver to probe using device/driver name

samples/rust/rust_driver_platform.rs +16 -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>,