linux-kernel - Re: [PATCH v2 09/10] rust: pci: add basic PCI device / driver abstractions

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <877cedi98j.fsf@metaspace.dk>
Date: Tue, 25 Jun 2024 12:53:48 +0200
From: Andreas Hindborg <nmi@...aspace.dk>
To: Danilo Krummrich <dakr@...hat.com>
Cc: gregkh@...uxfoundation.org,  rafael@...nel.org,  bhelgaas@...gle.com,
  ojeda@...nel.org,  alex.gaynor@...il.com,  wedsonaf@...il.com,
  boqun.feng@...il.com,  gary@...yguo.net,  bjorn3_gh@...tonmail.com,
  benno.lossin@...ton.me,  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,
  rust-for-linux@...r.kernel.org,  linux-kernel@...r.kernel.org,
  linux-pci@...r.kernel.org
Subject: Re: [PATCH v2 09/10] rust: pci: add basic PCI device / driver
 abstractions

Hi Danilo,

Thanks for working on this. I just finished rebasing the Rust NVMe
driver on these patches, and I have just one observation for now.

Danilo Krummrich <dakr@...hat.com> writes:

[...]

> +pub trait Driver {
> +    /// Data stored on device by driver.
> +    ///
> +    /// Corresponds to the data set or retrieved via the kernel's
> +    /// `pci_{set,get}_drvdata()` functions.
> +    ///
> +    /// Require that `Data` implements `ForeignOwnable`. We guarantee to
> +    /// never move the underlying wrapped data structure.
> +    ///
> +    /// TODO: Use associated_type_defaults once stabilized:
> +    ///
> +    /// `type Data: ForeignOwnable = ();`
> +    type Data: ForeignOwnable;
> +
> +    /// The type holding information about each device id supported by the driver.
> +    ///
> +    /// TODO: Use associated_type_defaults once stabilized:
> +    ///
> +    /// type IdInfo: 'static = ();
> +    type IdInfo: 'static;
> +
> +    /// The table of device ids supported by the driver.
> +    const ID_TABLE: IdTable<'static, DeviceId, Self::IdInfo>;
> +
> +    /// PCI driver probe.
> +    ///
> +    /// Called when a new platform device is added or discovered.
> +    /// Implementers should attempt to initialize the device here.
> +    fn probe(dev: &mut Device, id: Option<&Self::IdInfo>) -> Result<Self::Data>;

Since you changed the `Device` representation to be basically an `ARef`,
the `&mut` makes no sense. I think we should either pass by value or
immutable reference.


Best regards,
Andreas


> +
> +    /// PCI driver remove.
> +    ///
> +    /// Called when a platform device is removed.
> +    /// Implementers should prepare the device for complete removal here.
> +    fn remove(data: &Self::Data);
> +}
> +
> +/// The PCI device representation.
> +///
> +/// A PCI device is based on an always reference counted `device:Device` instance. Cloning a PCI
> +/// device, hence, also increments the base device' reference count.
> +#[derive(Clone)]
> +pub struct Device(ARef<device::Device>);
> +