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,
