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

[Danilo Krummrich <dakr@kernel.org>]

On Thu, Mar 13, 2025 at 10:44:38AM +0000, Benno Lossin wrote:
> 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.

Yeah, that's basically a bug that I never fixed (until now), since making it
take a mutable reference would not have changed anything in terms of
accessibility.

> 
> > 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@kernel.org>
> 
> Two small nits below, but it already looks good:
> 
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> 
> > ---
> >  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)?

`&**self` gives us a `Device` (i.e. `pci::Device`), not a `device::Device`.

> 
> > @@ -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()`.

Yeah, having to write `&**pdev` was bothering me too, and I actually tried what
you suggest, but it didn't compile -- I'll double check.