Re: [PATCH] gpu: nova-core: take advantage of pci::Device::unbind()

["Alexandre Courbot" <acourbot@nvidia.com>]

On Sat Aug 30, 2025 at 10:32 PM JST, Danilo Krummrich wrote:
> Now that we have pci::Device::unbind() we can unregister the sysmem
> flush page with a direct access the I/O resource, i.e. without RCU read
> side critical section.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  drivers/gpu/nova-core/driver.rs |  4 ++++
>  drivers/gpu/nova-core/gpu.rs    | 20 ++++++++++----------
>  2 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
> index 274989ea1fb4..02514e1e2529 100644
> --- a/drivers/gpu/nova-core/driver.rs
> +++ b/drivers/gpu/nova-core/driver.rs
> @@ -54,4 +54,8 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
>  
>          Ok(this)
>      }
> +
> +    fn unbind(pdev: &pci::Device<Core>, this: Pin<&Self>) {
> +        this.gpu.unbind(pdev);
> +    }
>  }
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 8caecaf7dfb4..2db9afdc6087 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -163,7 +163,7 @@ fn new(bar: &Bar0) -> Result<Spec> {
>  }
>  
>  /// Structure holding the resources required to operate the GPU.
> -#[pin_data(PinnedDrop)]
> +#[pin_data]
>  pub(crate) struct Gpu {
>      spec: Spec,
>      /// MMIO mapping of PCI BAR 0
> @@ -174,15 +174,6 @@ pub(crate) struct Gpu {
>      sysmem_flush: SysmemFlush,
>  }
>  
> -#[pinned_drop]
> -impl PinnedDrop for Gpu {
> -    fn drop(self: Pin<&mut Self>) {
> -        // Unregister the sysmem flush page before we release it.
> -        self.bar
> -            .try_access_with(|b| self.sysmem_flush.unregister(b));
> -    }
> -}
> -
>  impl Gpu {
>      /// Helper function to load and run the FWSEC-FRTS firmware and confirm that it has properly
>      /// created the WPR2 region.
> @@ -309,4 +300,13 @@ pub(crate) fn new(
>              sysmem_flush,
>          }))
>      }
> +
> +    pub(crate) fn unbind(&self, pdev: &pci::Device<device::Bound>) {
> +        // Unregister the sysmem flush page before we release it.
> +        kernel::warn_on!(self.bar.access(pdev.as_ref()).map_or(true, |bar| {
> +            self.sysmem_flush.unregister(bar);
> +
> +            false
> +        }));
> +    }

Maybe I'm overtly cautious, but this method can be called from a large
part of the driver, leaving the Gpu device in a half-unbound state. The
`PinnedDrop` approach had the benefit of not allowing this.

One way to solve the problem would be to make this method `pub(in
crate::driver)`, so other modules cannot call it.