Message-ID: <7b8130de-8096-4fcb-be84-c13209638b25@foxido.dev>
Date: Thu, 8 Jan 2026 20:11:19 +0300
From: Gladyshev Ilya <foxido@...ido.dev>
To: Gary Guo <gary@...yguo.net>
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 1/8/26 16:21, Gary Guo wrote:
> 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).
Hm, I looked through ACPI_FREE() call sites and acpi_evaluate_object() 
implementation, and it seems to me that the acpi_object's lifetime is 
the same as its internal buffer. Overall, it is indeed managed 
externally, but acpi_object and acpi_object::buffer->pointer live 
together. I’m not an ACPI expert, though, so maybe I’m missing something.

Anyway, the current Rust setup seems fine for now:
0. AcpiObject validity is guaranteed by whoever constructed/passed it (C 
side for WMI abstractions, for example)
1. You can only convert &AcpiObject to &AcpiSubType (reference to 
reference), so AcpiSubType can't outlive AcpiObject
2. You can't steal the data pointer from &AcpiSubType either, because 
the Deref impl is "logically safe" and only gives you a reference to the 
inner data, which can't outlive AcpiSubType's reference -> can't outlive 
AcpiObject.

So for now until AcpiObject lives _less_ than it's inner data, 
everything is OK.

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

Using an enum would require keeping Rust's enum synced with the C side, 
as well as implementing some simple but non-trivial checks that the 
`type_` field is a valid enum value (and the valid range isn't 
continuous). I think that keeping it as an integer is simpler and better 
matches C side.

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

Sounds reasonable, thanks!

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

I don’t see any particular benefit except moving the `impl TryFrom` out 
of macro-generated code, but you’re probably right -- I’ll try 
implementing it this way.

(will rustdoc show TryFrom on the AcpiSubType's page if it's implemented 
via sealed trait?)

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