Message-Id: <DF4Q1I15MG7E.Q8234J76FQ1O@kernel.org>
Date: Mon, 22 Dec 2025 12:50:32 +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 2/3] rust: introduce WMI abstractions

On Sun Dec 21, 2025 at 7:22 PM CET, Gladyshev Ilya wrote:
> diff --git a/rust/kernel/wmi.rs b/rust/kernel/wmi.rs
> new file mode 100644
> index 000000000000..018e88d01e8c
> --- /dev/null
> +++ b/rust/kernel/wmi.rs
> @@ -0,0 +1,267 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstractions for the WMI devices.
> +//!
> +//! C header: [`include/linux/wmi.h`](srctree/include/linux/wmi.h)
> +
> +use crate::{
> +    acpi::AcpiObject,
> +    device,
> +    device_id::{RawDeviceId, RawDeviceIdIndex},
> +    driver,
> +    error::{from_result, to_result, VTABLE_DEFAULT_ERROR},
> +    prelude::*,
> +    types::Opaque,
> +};
> +use core::{
> +    marker::PhantomData,
> +    mem::MaybeUninit,
> +    ptr::{addr_of_mut, NonNull},
> +};
> +use macros::vtable;

Please use kernel vertical style.

[1] https://docs.kernel.org/rust/coding-guidelines.html#imports

> +
> +/// [`IdTable`](kernel::device_id::IdTable) type for WMI.
> +pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
> +
> +/// The WMI driver trait.
> +#[vtable]
> +pub trait Driver: Send {
> +    /// The type holding information about each one of the device ids supported by the driver.
> +    type IdInfo: 'static;
> +
> +    /// The table of device ids supported by the driver.
> +    const TABLE: IdTable<Self::IdInfo>;
> +
> +    /// WMI driver probe.
> +    ///
> +    /// Called when a new WMI device is bound to this driver.
> +    /// Implementers should attempt to initialize the driver here.
> +    fn probe(dev: &Device<device::Core>, id_info: &Self::IdInfo) -> impl PinInit<Self, Error>;
> +
> +    /// WMI device notify.
> +    ///
> +    /// Called when new WMI event received from bounded device
> +    fn notify(&self, _dev: &Device<device::Core>, _event: &AcpiObject) {

Please use Pin<&Self> instead.

> +        build_error!(VTABLE_DEFAULT_ERROR);
> +    }
> +
> +    /// WMI driver remove.
> +    fn remove(self: Pin<KBox<Self>>, _dev: &Device<device::Core>) {

We can't pass the driver's device private data by value here. As long as the
device is not fully unbound, there may still be calls to Device::drvdata();
please see also [2]). Hence, please use Pin<&Self> instead.

Also, please call this method unbind() to be consistent with other busses.

[2] https://lore.kernel.org/rust-for-linux/DEZMS6Y4A7XE.XE7EUBT5SJFJ@kernel.org/

> +        build_error!(VTABLE_DEFAULT_ERROR);
> +    }
> +}

<snip>

> +impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> +    fn as_ref(&self) -> &device::Device<Ctx> {
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> +        // `struct platform_device`.
> +        let dev = unsafe { addr_of_mut!((*self.inner.get()).dev) };

Please use &raw mut instead.

> +
> +        // SAFETY: `dev` points to a valid `struct device`.
> +        unsafe { device::Device::from_raw(dev) }
> +    }
> +}
> +
> +kernel::impl_device_context_deref!(unsafe { Device });
> +kernel::impl_device_context_into_aref!(Device);
> +
> +// SAFETY: Instances of `Device` are always reference-counted.
> +unsafe impl crate::sync::aref::AlwaysRefCounted for Device {
> +    fn inc_ref(&self) {
> +        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> +        unsafe { bindings::get_device(self.as_ref().as_raw()) };
> +    }
> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> +        unsafe { bindings::put_device(&raw mut (*obj.as_ref().as_raw()).dev) }
> +    }
> +}
> +
> +/// Abstraction for the WMI device ID structure, i.e. [`struct wmi_device_id`].
> +#[repr(transparent)]
> +pub struct DeviceId(bindings::wmi_device_id);
> +
> +impl DeviceId {
> +    /// Constructs new DeviceId from GUID string
> +    pub const fn new(guid: &[u8; bindings::UUID_STRING_LEN as usize]) -> Self {
> +        // SAFETY: FFI type is valid to be zero-initialized.
> +        let mut inner: bindings::wmi_device_id = unsafe { MaybeUninit::zeroed().assume_init() };
> +
> +        build_assert!(inner.guid_string.len() == bindings::UUID_STRING_LEN as usize + 1);
> +
> +        // SAFETY: It's safe to copy UUID_STRING_LEN, because we validated lengths
> +        // Also we leave last byte zeroed -> guid_string is valid C string
> +        unsafe {
> +            ::core::ptr::copy_nonoverlapping(
> +                guid.as_ptr(),
> +                &raw mut inner.guid_string[0],
> +                bindings::UUID_STRING_LEN as usize,
> +            );
> +        }

This does not compile without const_intrinsic_copy. I think we can enable it
though, since it should be stable since 1.83.

> +
> +        Self(inner)
> +    }
> +}

Powered by blists - more mailing lists