Message-Id: <DF4PPPGQBPNA.13TJNSRSRUW0A@kernel.org>
Date: Mon, 22 Dec 2025 12:35:08 +0100
From: "Danilo Krummrich" <dakr@...nel.org>
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>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice
 Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "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: [RFC PATCH 1/3] rust: implement wrapper for acpi_object

On Sun Dec 21, 2025 at 7:22 PM CET, Gladyshev Ilya wrote:
> +/// 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.

I think this is a good place to link the corresponding types.

> +///
> +/// # Example
> +/// ```
> +/// # use kernel::prelude::*
> +/// use kernel::acpi::{AcpiObject};

Braces not needed.

> +///
> +/// fn read_first_acpi_byte(obj: &AcpiObject) -> Result<u8> {
> +///     if obj.type_id() != AcpiBuffer::ACPI_TYPE {
> +///         return Err(EINVAL);
> +///     }

Given the try_into() conversion below this check is unnecessary.

> +///     let obj: &AcpiBuffer = obj.try_into()?;
> +///
> +///     Ok(obj.payload()[0])
> +/// }
> +/// ```
> +#[repr(transparent)]
> +pub struct AcpiObject(bindings::acpi_object);
> +
> +impl AcpiObject {
> +    /// Returns object type (see `acpitypes.h`)
> +    pub fn type_id(&self) -> u32 {
> +        // SAFETY: `type` field is valid in all union variants

Here and in a lot of other places, please end with a period.

> +        unsafe { self.0.type_ }
> +    }
> +}
> +
> +/// Generate AcpiObject subtype
> +///
> +/// For given subtype implements
> +/// - `TryFrom<&AcpiObject> for &SubType` trait
> +/// - unsafe try_from_unchecked() with same semantics, but without type check
> +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);
> +
> +        impl TryFrom<&AcpiObject> for &$subtype_name {
> +            type Error = Error;
> +
> +            fn try_from(value: &AcpiObject) -> core::result::Result<Self, Self::Error> {
> +                // SAFETY: type_ field present in all union types and is always valid
> +                let real_type = unsafe { value.0.type_ };
> +
> +                if (real_type != $subtype_name::ACPI_TYPE) {
> +                    return Err(EINVAL);
> +                }

This should just be

	if (value.type_id() != $subtype_name::ACPI_TYPE) {
	    return Err(EINVAL);
	}

> +
> +                // SAFETY: We validated union subtype

When writing safety comments, please read the safety documentation of the
corresponding function and try to cover all requirements listed as bullet
points.

> +                Ok(unsafe {
> +                    ::core::mem::transmute::<&$union_type, &$subtype_name>(&value.0.$field_name)
> +                })
> +            }
> +        }
> +
> +        impl $subtype_name {
> +            /// This ACPI type int value (see `acpitypes.h`)
> +            pub const ACPI_TYPE: u32 = bindings::$acpi_type;
> +
> +            /// Converts AcpiObject reference into exact ACPI type wrapper
> +            ///
> +            /// # Safety
> +            ///
> +            /// Assumes that value is correct (`Self`) subtype
> +            pub unsafe fn try_from_unchecked(value: &AcpiObject) -> &Self {

The name try_from_unchecked() implies that the function is fallible, but it
isn't. I suggest calling it something along the lines of cast_unchecked().

> +                // SAFETY: Only unsafety comes from unchecked transformation and
> +                // we transfered
> +                unsafe {
> +                    ::core::mem::transmute::<&$union_type, &$subtype_name>(&value.0.$field_name)
> +                }
> +            }
> +        }
> +    };
> +}
> +
> +acpi_object_subtype!(AcpiInteger
> +    <- (ACPI_TYPE_INTEGER, integer, bindings::acpi_object__bindgen_ty_1));
> +acpi_object_subtype!(AcpiString
> +    <- (ACPI_TYPE_STRING, string, bindings::acpi_object__bindgen_ty_2));
> +acpi_object_subtype!(AcpiBuffer
> +    <- (ACPI_TYPE_BUFFER, buffer, bindings::acpi_object__bindgen_ty_3));
> +acpi_object_subtype!(AcpiPackage
> +    <- (ACPI_TYPE_PACKAGE, package, bindings::acpi_object__bindgen_ty_4));
> +acpi_object_subtype!(AcpiReference
> +    <- (ACPI_TYPE_LOCAL_REFERENCE, reference, bindings::acpi_object__bindgen_ty_5));
> +acpi_object_subtype!(AcpiProcessor
> +    <- (ACPI_TYPE_PROCESSOR, processor, bindings::acpi_object__bindgen_ty_6));
> +acpi_object_subtype!(AcpiPowerResource
> +    <- (ACPI_TYPE_POWER, power_resource, bindings::acpi_object__bindgen_ty_7));
> +
> +impl AcpiBuffer {
> +    /// Get Buffer's content
> +    pub fn payload(&self) -> &[u8] {
> +        // SAFETY: (pointer, length) indeed represents byte slice
> +        unsafe { ::core::slice::from_raw_parts(self.0.pointer, self.0.length as usize) }
> +    }
> +}

What about the values of the other types? How are they accessed?

Also, I think it would be better to use a Deref impl rather than a method.

Powered by blists - more mailing lists