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

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

[Danilo Krummrich]

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
+        }));
+    }
 }

base-commit: 09f90256e8902793f594517ef440698585eb3595
-- 
2.51.0

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

[Danilo Krummrich]

On 8/30/25 3:32 PM, Danilo Krummrich wrote:
> +    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
> +        }));
> +    }
>   }

Actually, inspect() + is_err() is much nicer:

diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 2db9afdc6087..ca4ea5749975 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -303,10 +303,10 @@ pub(crate) fn new(

      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
-        }));
+        kernel::warn_on!(self
+            .bar
+            .access(pdev.as_ref())
+            .inspect(|bar| self.sysmem_flush.unregister(bar))
+            .is_err());
      }
  }

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

[Alexandre Courbot]

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.

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

[Danilo Krummrich]

On Sun Aug 31, 2025 at 3:50 PM CEST, Alexandre Courbot wrote:
>> +    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.

pub(in crate::driver) doesn't work, since hierarchically it's a sibling. :(

However, I can add a doc-comment to make it a bit more obvious.

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

[Alexandre Courbot]

On Mon Sep 1, 2025 at 7:41 PM JST, Danilo Krummrich wrote:
> On Sun Aug 31, 2025 at 3:50 PM CEST, Alexandre Courbot wrote:
>>> +    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.
>
> pub(in crate::driver) doesn't work, since hierarchically it's a sibling. :(

Argh. TIL.

>
> However, I can add a doc-comment to make it a bit more obvious.

Would it also help if we made `Gpu::unbind` take a
`pci::Device<device::Core>`? That way, driver functions that only have a
bound device could not invoke it.

(also, should we make the argument a `device::Device` instead of a
`pci::Device`?)

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

[Danilo Krummrich]

On Mon Sep 1, 2025 at 3:13 PM CEST, Alexandre Courbot wrote:
> On Mon Sep 1, 2025 at 7:41 PM JST, Danilo Krummrich wrote:
>> On Sun Aug 31, 2025 at 3:50 PM CEST, Alexandre Courbot wrote:
>>>> +    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.
>>
>> pub(in crate::driver) doesn't work, since hierarchically it's a sibling. :(
>
> Argh. TIL.
>
>>
>> However, I can add a doc-comment to make it a bit more obvious.
>
> Would it also help if we made `Gpu::unbind` take a
> `pci::Device<device::Core>`? That way, driver functions that only have a
> bound device could not invoke it.

This was my intention, but somehow I typed Bound.

> (also, should we make the argument a `device::Device` instead of a
> `pci::Device`?)

I think it makes sense to abstract the specific bus device, since long term we
also want to support tegra.