Message-ID: <20260108132141.6cce4827.gary@garyguo.net>
Date: Thu, 8 Jan 2026 13:21:41 +0000
From: Gary Guo <gary@...yguo.net>
To: Gladyshev Ilya <foxido@...ido.dev>
Cc: "foxido @ foxido . dev-cc= Rafael J. Wysocki" <rafael@...nel.org>, Len
 Brown <lenb@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, Boqun Feng
 <boqun.feng@...il.com>, Benno Lossin <lossin@...nel.org>, Andreas Hindborg
 <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross
 <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Tamir Duberstein
 <tamird@...il.com>, Armin Wolf <W_Armin@....de>,
 <platform-driver-x86@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH 2/3] rust: implement wrapper for acpi_object

On Wed,  7 Jan 2026 23:35:32 +0300
Gladyshev Ilya <foxido@...ido.dev> wrote:

> ACPI Object is represented via union on C-side. On Rust side, this union
> is transparently wrapped for each ACPI Type, with individual methods and
> Defer implementation to represented type (integer, string, buffer, etc).
> 
> Signed-off-by: Gladyshev Ilya <foxido@...ido.dev>


Hi Gladyshev,

I've checked the `acpi_object` implementation on the C side and it appears
that the buffer is not owned by the object (however managed externally,
could either be resting in ACPI tables directly or be allocated).

Therefore, you might want to carry a lifetime to represent the lifetime of
underlying buffers?

> ---
>  rust/kernel/acpi.rs | 91 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 91 insertions(+)
> 
> diff --git a/rust/kernel/acpi.rs b/rust/kernel/acpi.rs
> index 9b8efa623130..ac1a9f305f6c 100644
> --- a/rust/kernel/acpi.rs
> +++ b/rust/kernel/acpi.rs
> @@ -2,6 +2,8 @@
>  
>  //! Advanced Configuration and Power Interface abstractions.
>  
> +use core::ops::Deref;
> +
>  use crate::{
>      bindings,
>      device_id::{RawDeviceId, RawDeviceIdIndex},
> @@ -63,3 +65,92 @@ macro_rules! acpi_device_table {
>          $crate::module_device_table!("acpi", $module_table_name, $table_name);
>      };
>  }
> +
> +/// An ACPI object.
> +///
> +/// This structure represents the Rust abstraction for a C [`struct acpi_object`].
> +/// You probably want to convert it into actual object type (e.g [`AcpiBuffer`]).
> +///
> +/// # Example
> +/// ```
> +/// # use kernel::prelude::*;
> +/// use kernel::acpi::{AcpiObject, AcpiBuffer};
> +///
> +/// fn read_first_acpi_byte(obj: &AcpiObject) -> Result<u8> {
> +///     let buf: &AcpiBuffer = obj.try_into()?;
> +///
> +///     Ok(buf[0])
> +/// }
> +/// ```
> +///
> +/// [`struct acpi_object`]: srctree/include/acpi/actypes.h
> +#[repr(transparent)]
> +pub struct AcpiObject(bindings::acpi_object);
> +
> +impl AcpiObject {
> +    /// Returns object type id (see [`actypes.h`](srctree/include/acpi/actypes.h)).
> +    pub fn type_id(&self) -> u32 {
> +        // SAFETY: `type` field is valid in all union variants.
> +        unsafe { self.0.type_ }
> +    }

This should probably be an enum instead of just integer.

> +}
> +
> +/// Generate wrapper type for AcpiObject subtype.
> +///
> +/// For given subtype implements
> +/// - `#[repr(transparent)]` type wrapper,
> +/// - `TryFrom<&AcpiObject> for &SubType` trait,
> +/// - unsafe from_unchecked() for 'trusted' conversion.
> +macro_rules! acpi_object_subtype {
> +    ($subtype_name:ident <- ($acpi_type:ident, $field_name:ident, $union_type:ty)) => {
> +        /// Wraps `acpi_object` subtype.
> +        #[repr(transparent)]
> +        pub struct $subtype_name($union_type);

Instead of wrapping the bindgen-generated subtypes, I would rather this to
be a transparent wrapper of `acpi_object`, with an invariant that the
specific union field is active.

This way you do not have to name the bindgen-generated names.

> +
> +        impl<'a> TryFrom<&'a AcpiObject> for &'a $subtype_name {
> +            type Error = Error;
> +
> +            fn try_from(value: &'a AcpiObject) -> core::result::Result<Self, Self::Error> {
> +                if (value.type_id() != $subtype_name::ACPI_TYPE) {
> +                    return Err(EINVAL);
> +                }
> +
> +                // SAFETY: Requested cast is valid because we validated type_id
> +                Ok(unsafe { $subtype_name::from_unchecked(&value) })
> +            }
> +        }

It feels like this can be better implemented by having a sealed trait for
all possible ACPI object types?

> +
> +        impl $subtype_name {
> +            /// Int value, representing this ACPI type (see [`acpitypes.h`]).
> +            ///
> +            /// [`acpitypes.h`]: srctree/include/linux/acpitypes.h
> +            pub const ACPI_TYPE: u32 = bindings::$acpi_type;
> +
> +            /// Converts opaque AcpiObject reference into exact ACPI type reference.
> +            ///
> +            /// # Safety
> +            ///
> +            /// - Requested cast should be valid (value.type_id() is `Self::ACPI_TYPE`).
> +            pub unsafe fn from_unchecked(value: &AcpiObject) -> &Self {
> +                // SAFETY:
> +                // - $field_name is currently active union's field due to external safety contract,
> +                // - Transmuting to `repr(transparent)` wrapper is safe.
> +                unsafe {
> +                    ::core::mem::transmute::<&$union_type, &$subtype_name>(&value.0.$field_name)
> +                }
> +            }
> +        }
> +    };
> +}
> +
> +acpi_object_subtype!(AcpiBuffer
> +    <- (ACPI_TYPE_BUFFER, buffer, bindings::acpi_object__bindgen_ty_3));
> +
> +impl Deref for AcpiBuffer {
> +    type Target = [u8];
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: (pointer, length) indeed represents byte slice.
> +        unsafe { ::core::slice::from_raw_parts(self.0.pointer, self.0.length as usize) }
> +    }
> +}

Powered by blists - more mailing lists