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: <aEV43zxZmWvDgKES@pollux>
Date: Sun, 8 Jun 2025 13:49:51 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <lossin@...nel.org>
Cc: Igor Korotin <igor.korotin.linux@...il.com>, 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, Igor Korotin <igorkor.3vium@...il.com>
Subject: Re: [PATCH v3 2/4] rust: driver: Add ACPI id table support to
 Adapter trait

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.

> >> > +    fn acpi_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> {
> >> > +        None
> >> > +    }
> >> > +
> >> >      /// The [`of::IdTable`] of the corresponding driver.
> >> >      fn of_id_table() -> Option<of::IdTable<Self::IdInfo>>;
> >> >  
> >> > @@ -178,6 +210,11 @@ fn of_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> {
> >> >      /// If this returns `None`, it means that there is no match in any of the ID tables directly
> >> >      /// associated with a [`device::Device`].
> >> >      fn id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
> >> > +        let id = Self::acpi_id_info(dev);
> >> > +        if id.is_some() {
> >> > +            return id;
> >> > +        }
> >> 
> >> Is a driver only going to have one id_info? Or is there some kind of
> >> precedence?
> >
> > A driver potentially has lots of them, but the device is only matching a single
> > entry in one of the driver's ID tables and hence a single ID info.
> 
> Ah so if `of_id_info` and `acpi_id_info` return `Some(_)`, then both
> values are the same?

No, if one of them returns Some(_), the other one will always return None. Or
phrased differently, the first match will always be the only match.