mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Gladyshev Ilya <foxido@foxido.dev>
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Cc: Gary Guo <gary@garyguo.net>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Len Brown <lenb@kernel.org>, Miguel Ojeda <ojeda@kernel.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	Benno Lossin <lossin@kernel.org>,
	Andreas Hindborg <a.hindborg@kernel.org>,
	Alice Ryhl <aliceryhl@google.com>,
	Trevor Gross <tmgross@umich.edu>,
	Danilo Krummrich <dakr@kernel.org>,
	Tamir Duberstein <tamird@gmail.com>, Armin Wolf <W_Armin@gmx.de>,
	platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org,
	linux-acpi@vger.kernel.org, foxido@foxido.dev
Subject: Re: [PATCH 2/3] rust: implement wrapper for acpi_object
Date: Fri, 9 Jan 2026 13:57:03 +0300	[thread overview]
Message-ID: <b8e2c3ed-7db1-48ca-8dc8-64dfa437c595@foxido.dev> (raw)
In-Reply-To: <CANiq72=TAXwjjxFiKiiwh9m_rRK_yUVS4b+2st=QJWErz5qTpQ@mail.gmail.com>

On 1/8/26 21:46, Miguel Ojeda wrote:
>> 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.
> 
> If this refers to the `ACPI_TYPE_*` constants, there seems to be a
> comment in the C docs that requires it to be kept in sync already with
> elsewhere, so perhaps it could be reasonable to add one more place to
> sync? (Though I don't see the mentioned arrays from a quick grep?)
> 
>       * NOTE: Types must be kept in sync with the global acpi_ns_properties
>       * and acpi_ns_type_names arrays.
> 
> [...snip about bindgen...]

If I understand correctly, acpi_object is something untrusted in general 
and broken hardware can provide arbitrary _type value. So ergonomics of 
enum solution would be kinda strange:

```
type_id() -> Result<Enum> // Result because it can be invalid

// ...

// '?' here makes it look like non-trivial operation
if x.type_id()? == Enum::Buffer
```

And it's unclear should ACPI_TYPE_INVALID be Ok(INVALID) or Err...

Also users of AcpiObject generally doesn't care if received object has 
valid, but unexpected type or invalid type, so this conversion int->enum 
will introduce (small but) unneeded overhead.

That's why I preferred int-style approach, matching C side. Here is 
alternative proposal from my side:

1. Enum with type values like you want
2. This Enum implements .id() method that returns underlying <int>
3. AcpiObject::type_id() still returns <int> value
4. There is method to convert int into enum (like TryFrom)

Probably you can 'newtype' <int> and implement TryFrom AcpiTypeId -> 
Enum and write something like

```
match x.type_id().try_into()? /* or as_enum() or smth */ {
   Enum::Integer => /* ... */,
   Enum::String => /* ... */
}

```


---
Sorry for my chaotic explanations...



  reply	other threads:[~2026-01-09 10:57 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-01-07 20:35 [PATCH 0/3] rust: WMI abstractions Gladyshev Ilya
2026-01-07 20:35 ` [PATCH 1/3] rust: enable const_intrinsic_copy feature Gladyshev Ilya
2026-01-07 20:35 ` [PATCH 2/3] rust: implement wrapper for acpi_object Gladyshev Ilya
2026-01-08 13:21   ` Gary Guo
2026-01-08 17:11     ` Gladyshev Ilya
2026-01-08 18:46       ` Miguel Ojeda
2026-01-09 10:57         ` Gladyshev Ilya [this message]
2026-01-11 17:57           ` Miguel Ojeda
2026-01-08 20:06       ` Gary Guo
2026-01-09 10:30         ` Gladyshev Ilya
2026-01-07 20:35 ` [PATCH 3/3] rust: add WMI abstractions Gladyshev Ilya
2026-01-08 19:48   ` Kari Argillander
2026-01-09 11:12     ` Gladyshev Ilya
2026-01-09 11:17       ` Kari Argillander
2026-01-08 20:48   ` Kari Argillander
2026-01-09 11:01     ` Gladyshev Ilya
2026-01-09 11:15       ` Kari Argillander
2026-01-09 11:31         ` Gladyshev Ilya

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b8e2c3ed-7db1-48ca-8dc8-64dfa437c595@foxido.dev \
    --to=foxido@foxido.dev \
    --cc=W_Armin@gmx.de \
    --cc=a.hindborg@kernel.org \
    --cc=aliceryhl@google.com \
    --cc=boqun.feng@gmail.com \
    --cc=dakr@kernel.org \
    --cc=gary@garyguo.net \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=miguel.ojeda.sandonis@gmail.com \
    --cc=ojeda@kernel.org \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=tamird@gmail.com \
    --cc=tmgross@umich.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome