linux-kernel - Re: [PATCH v5 13/16] rust: driver: implement `Adapter`

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: <CAPFo5V+WMxS5joVOJrxr_vaEpo+vdTcfffbpWDe2dtw7kP6rnw@mail.gmail.com>
Date: Wed, 11 Dec 2024 18:10:24 -0800
From: Fabien Parent <fabien.parent@...aro.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com, 
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com, 
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, 
	tmgross@...ch.edu, a.hindborg@...sung.com, aliceryhl@...gle.com, 
	airlied@...il.com, fujita.tomonori@...il.com, lina@...hilina.net, 
	pstanner@...hat.com, ajanulgu@...hat.com, lyude@...hat.com, robh@...nel.org, 
	daniel.almeida@...labora.com, saravanak@...gle.com, dirk.behme@...bosch.com, 
	j@...nau.net, chrisi.schrefl@...il.com, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH v5 13/16] rust: driver: implement `Adapter`

Hi Danilo,

On Tue, Dec 10, 2024 at 2:51 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> In order to not duplicate code in bus specific implementations (e.g.
> platform), implement a generic `driver::Adapter` to represent the
> connection of matched drivers and devices.
>
> Bus specific `Adapter` implementations can simply implement this trait
> to inherit generic functionality, such as matching OF or ACPI device IDs
> and ID table entries.
>
> Suggested-by: Rob Herring (Arm) <robh@...nel.org>
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> ---
>  rust/kernel/driver.rs | 59 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> index ab0bb46fe2cc..d169899a5da1 100644
> --- a/rust/kernel/driver.rs
> +++ b/rust/kernel/driver.rs
> @@ -6,7 +6,9 @@
>  //! register using the [`Registration`] class.
>
>  use crate::error::{Error, Result};
> -use crate::{init::PinInit, str::CStr, try_pin_init, types::Opaque, ThisModule};
> +use crate::{
> +    device, device_id, init::PinInit, of, str::CStr, try_pin_init, types::Opaque, ThisModule,
> +};
>  use core::pin::Pin;
>  use macros::{pin_data, pinned_drop};
>
> @@ -114,3 +116,58 @@ macro_rules! module_driver {
>          }
>      }
>  }
> +
> +/// The bus independent adapter to match a drivers and a devices.
> +///
> +/// This trait should be implemented by the bus specific adapter, which represents the connection
> +/// of a device and a driver.
> +///
> +/// It provides bus independent functions for device / driver interactions.
> +pub trait Adapter {
> +    /// The type holding driver private data about each device id supported by the driver.
> +    type IdInfo: 'static;
> +
> +    /// The [`of::IdTable`] of the corresponding driver.
> +    fn of_id_table() -> of::IdTable<Self::IdInfo>;

I think we may want this to return an Option<of::IdTable<Self::IdInfo>>
instead. I don't think we want to force every bus abstraction to have
to implement every possible IdTable that this adapter will support.

For instance if your driver only supports ACPI, it will still be required
to provide an empty OF table because the bus abstraction needs it
for implementing this trait.

> +
> +    /// Returns the driver's private data from the matching entry in the [`of::IdTable`], if any.
> +    ///
> +    /// If this returns `None`, it means there is no match with an entry in the [`of::IdTable`].
> +    #[cfg(CONFIG_OF)]
> +    fn of_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
> +        let table = Self::of_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::of_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::<of::DeviceId>() };
> +
> +            Some(table.info(<of::DeviceId as device_id::RawDeviceId>::index(id)))
> +        }
> +    }
> +
> +    #[cfg(not(CONFIG_OF))]
> +    fn of_id_info(_dev: &device::Device) -> Option<&'static Self::IdInfo> {
> +        None
> +    }
> +
> +    /// Returns the driver's private data from the matching entry of any of the ID tables, if any.
> +    ///
> +    /// 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::of_id_info(dev);
> +        if id.is_some() {
> +            return id;
> +        }
> +
> +        None
> +    }
> +}
> --
> 2.47.0
>