Re: [PATCH 3/4] rust: pci: fix unrestricted &mut pci::Device

[Benno Lossin]

On Thu Mar 13, 2025 at 3:13 AM CET, Danilo Krummrich wrote:
> As by now, pci::Device is implemented as:
>
> 	#[derive(Clone)]
> 	pub struct Device(ARef<device::Device>);
>
> This may be convenient, but has the implication that drivers can call
> device methods that require a mutable reference concurrently at any
> point of time.

Which methods take mutable references? The `set_master` method you
mentioned also took a shared reference before this patch.

> Instead define pci::Device as
>
> 	pub struct Device<Ctx: DeviceContext = Normal>(
> 		Opaque<bindings::pci_dev>,
> 		PhantomData<Ctx>,
> 	);
>
> and manually implement the AlwaysRefCounted trait.
>
> With this we can implement methods that should only be called from
> bus callbacks (such as probe()) for pci::Device<Core>. Consequently, we
> make this type accessible in bus callbacks only.
>
> Arbitrary references taken by the driver are still of type
> ARef<pci::Device> and hence don't provide access to methods that are
> reserved for bus callbacks.
>
> Fixes: 1bd8b6b2c5d3 ("rust: pci: add basic PCI device / driver abstractions")
> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>

Two small nits below, but it already looks good:

Reviewed-by: Benno Lossin <benno.lossin@xxxxxxxxx>

> ---
>  drivers/gpu/nova-core/driver.rs |   4 +-
>  rust/kernel/pci.rs              | 126 ++++++++++++++++++++------------
>  samples/rust/rust_driver_pci.rs |   8 +-
>  3 files changed, 85 insertions(+), 53 deletions(-)
>

> @@ -351,20 +361,8 @@ fn deref(&self) -> &Self::Target {
>  }
>  
>  impl Device {

One alternative to implementing `Deref` below would be to change this to
`impl<Ctx: DeviceContext> Device<Ctx>`. But then one would lose the
ability to just do `&pdev` to get a `Device` from a `Device<Core>`... So
I think the deref way is better. Just wanted to mention this in case
someone re-uses this pattern.

> -    /// Create a PCI Device instance from an existing `device::Device`.
> -    ///
> -    /// # Safety
> -    ///
> -    /// `dev` must be an `ARef<device::Device>` whose underlying `bindings::device` is a member of
> -    /// a `bindings::pci_dev`.
> -    pub unsafe fn from_dev(dev: ARef<device::Device>) -> Self {
> -        Self(dev)
> -    }
> -
>      fn as_raw(&self) -> *mut bindings::pci_dev {
> -        // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device`
> -        // embedded in `struct pci_dev`.
> -        unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) as _ }
> +        self.0.get()
>      }
>  
>      /// Returns the PCI vendor ID.

>  impl AsRef<device::Device> for Device {
>      fn as_ref(&self) -> &device::Device {
> -        &self.0
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> +        // `struct pci_dev`.
> +        let dev = unsafe { addr_of_mut!((*self.as_raw()).dev) };
> +
> +        // SAFETY: `dev` points to a valid `struct device`.
> +        unsafe { device::Device::as_ref(dev) }

Why not use `&**self` instead (ie go through the `Deref` impl)?

> @@ -77,7 +77,7 @@ fn probe(pdev: &mut pci::Device, info: &Self::IdInfo) -> Result<Pin<KBox<Self>>>
>  
>          let drvdata = KBox::new(
>              Self {
> -                pdev: pdev.clone(),
> +                pdev: (&**pdev).into(),

It might be possible to do:

    impl From<&pci::Device<Core>> for ARef<pci::Device> { ... }

Then this line could become `pdev: pdev.into()`.

---
Cheers,
Benno

>                  bar,
>              },
>              GFP_KERNEL,