Danilo Krummrich
2025-Aug-30 13:32 UTC
[PATCH] gpu: nova-core: take advantage of pci::Device::unbind()
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 at 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
Danilo Krummrich
2025-Aug-30 13:51 UTC
[PATCH] gpu: nova-core: take advantage of pci::Device::unbind()
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()); } }
Alexandre Courbot
2025-Aug-31 13:50 UTC
[PATCH] gpu: nova-core: take advantage of pci::Device::unbind()
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 at 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.
Danilo Krummrich
2025-Sep-01 10:41 UTC
[PATCH] gpu: nova-core: take advantage of pci::Device::unbind()
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.