Re: [PATCH] rust: acpi: add `acpi::DeviceId` abstraction

[Danilo Krummrich <dakr@kernel.org>]

On Fri, May 30, 2025 at 01:38:06PM +0100, Igor Korotin wrote:
> `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>

As Greg mentioned it would be nice to see the subsequent patches.

Also, for this to be useful you need to implement the generic glue code in
rust/kernel/driver.rs in the generic Adapter trait.

You also may want to connect it to the platform bus abstraction, just like the
OF bindings are, since I assume that's the bus you care about?

> ---
>  MAINTAINERS         |  1 +
>  rust/kernel/acpi.rs | 63 +++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs  |  1 +
>  3 files changed, 65 insertions(+)
>  create mode 100644 rust/kernel/acpi.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b659fb27ab63..5f8dfae08454 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..bbd38910736c
> --- /dev/null
> +++ b/rust/kernel/acpi.rs
> @@ -0,0 +1,63 @@
> +// 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() };

The patch did not land yet, but for this there's the following improvement
upcoming:

https://lore.kernel.org/rust-for-linux/20250523145125.523275-13-lossin@kernel.org/

> +        // TODO: Use `clone_from_slice` once the corresponding types do match.

clone_from_slice() can't be used from const context, this comment is wrong in
of.rs (where you probably copied it from).

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

You can drop this cast, it shouldn't be needed anymore.

> +            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 15c5f72976cd..05f1d3870bf7 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -78,6 +78,7 @@
>  #[cfg(CONFIG_NET)]
>  pub mod net;
>  pub mod of;
> +pub mod acpi;

As mentioned by Greg, please keep this ordered.