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

Andreas Hindborg
2024-06-25T10:59:33Z

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@redhat.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>);
> +