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@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)?
`&**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.
