linux-kernel - Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to Adapter trait

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <DAI2ZYFSORPD.3LJ9L234S6LZ2@kernel.org>
Date: Mon, 09 Jun 2025 16:44:33 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Igor Korotin" <igor.korotin.linux@...il.com>, "Danilo Krummrich"
 <dakr@...nel.org>
Cc: <ojeda@...nel.org>, <alex.gaynor@...il.com>, <rafael@...nel.org>,
 <gregkh@...uxfoundation.org>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <linux-acpi@...r.kernel.org>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <benno.lossin@...ton.me>, <a.hindborg@...nel.org>, <aliceryhl@...gle.com>,
 <tmgross@...ch.edu>, <lenb@...nel.org>, <wedsonaf@...il.com>,
 <viresh.kumar@...aro.org>, <alex.hung@....com>,
 <dingxiangfei2009@...il.com>
Subject: Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to
 Adapter trait

On Mon Jun 9, 2025 at 3:09 PM CEST, Igor Korotin wrote:
> On Sun, Jun 8, 2025 at 12:50 PM Danilo Krummrich <dakr@...nel.org> wrote:
>> On Sun, Jun 08, 2025 at 01:46:17PM +0200, Benno Lossin wrote:
>> > On Sun Jun 8, 2025 at 12:48 PM CEST, Danilo Krummrich wrote:
>> > > On Sun, Jun 08, 2025 at 09:54:30AM +0200, Benno Lossin wrote:
>> > >> On Fri Jun 6, 2025 at 7:08 PM CEST, Igor Korotin wrote:
>> > >> > @@ -141,6 +141,38 @@ pub trait Adapter {
>> > >> >      /// The type holding driver private data about each device id supported by the driver.
>> > >> >      type IdInfo: 'static;
>> > >> >
>> > >> > +    /// The [`acpi::IdTable`] of the corresponding driver
>> > >> > +    fn acpi_id_table() -> Option<acpi::IdTable<Self::IdInfo>>;
>> > >> > +
>> > >> > +    /// Returns the driver's private data from the matching entry in the [`acpi::IdTable`], if any.
>> > >> > +    ///
>> > >> > +    /// If this returns `None`, it means there is no match with an entry in the [`acpi::IdTable`].
>> > >> > +    #[cfg(CONFIG_ACPI)]
>> > >> > +    fn acpi_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
>> > >> > +        let table = Self::acpi_id_table()?;
>> > >> > +
>> > >> > +        // SAFETY:
>> > >> > +        // - `table` has static lifetime, hence it's valid for read,
>> > >> > +        // - `dev` is guaranteed to be valid while it's alive, and so is `pdev.as_ref().as_raw()`.
>> > >> > +        let raw_id = unsafe { bindings::acpi_match_device(table.as_ptr(), dev.as_raw()) };
>> > >> > +
>> > >> > +        if raw_id.is_null() {
>> > >> > +            None
>> > >> > +        } else {
>> > >> > +            // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and
>> > >> > +            // does not add additional invariants, so it's safe to transmute.
>> > >> > +            let id = unsafe { &*raw_id.cast::<acpi::DeviceId>() };
>> > >> > +
>> > >> > +            Some(table.info(<acpi::DeviceId as crate::device_id::RawDeviceId>::index(id)))
>> > >> > +        }
>> > >> > +    }
>> > >> > +
>> > >> > +    #[cfg(not(CONFIG_ACPI))]
>> > >> > +    #[allow(missing_docs)]
>> > >>
>> > >> I think we should change this to one single definition and do
>> > >>
>> > >>     if cfg!(not(CONFIG_ACPI)) {
>> > >>         return None;
>> > >>     }
>> > >>     /* body from above */
>> > >>
>> > >> In a single function instead.
>> > >
>> > > Generally, that's fine, but in this case I'd rather keep it as it is for
>> > > consistency with the rest of the file.
>> >
>> > Then let's also change the OF bindings in this file to that style :)
>>
>> Fine for me.
>>
>> @Igor: If you do so, please do it in a seaparate patch.
>
> That's definitely not possible with `if cfg!(not(CONFIG_ACPI))`
> because it is a runtime condition and it breaks compilation in case
> either `CONFIG_OF` or `CONFIG_ACPI` is not set, the Rust compiler
> can't find `of_match_device`/`acpi_match_device` because in that case
> they are defined as `static inline` and are not parsed by bindgen.
>
> Alternatively the following pattern works:
>
>     fn foo() {
>         #[cfg(not(CONFIG_ACPI))] {
>             None
>         }
>
>         #[cfg(CONFIG_ACPI)] {
>              // some logic.
>         }
>     }

We might want to add a `cfg_if!` macro that expands to this.

> Also the argument `dev` of `both of_id_info` and `acpi_id_info`  must be
> renamed into `_dev` to mark it as unused in case CONFIG_ACPI/CONFIG_OF
> is off and the argument is not used.

You can add a `let _ = dev;` in the `not` "branch".

---
Cheers,
Benno