On Thu May 21, 2026 at 10:50 PM JST, Alexandre Courbot wrote: > When probing the driver, the FWSEC-FRTS firmware creates a WPR2 secure > memory region to store the GSP firmware, and the Booter Loader loads and > starts that firmware into the GSP, making it run in RISC-V mode. > > These operations need to be reverted upon unloading, particularly the > WPR2 secure region creation, as its presence prevents the driver from > subsequently probing. > > Thus, prepare the Booter Unloader and FWSEC-SB firmwares when booting > the GSP, so they can be executed at unbind time to put the GPU into a > state where it can be probed again. > > Signed-off-by: Alexandre Courbot <[email protected]> > ---
After seeing the bundle moved outwards, I realised that it has the same issue that SysmemFlush does, i.e. if probe fails it does not reset the GSP. A lot of the time during development I will break things badly enough that probe fails, so it would be nice if this is supported. OTOH, this gets the probe suceed and unload case working which is important and this is a definite improvement, so for this version and the previous version of the patch: Reviewed-by: Eliot Courtney <[email protected]> I also had a brief go at making this work on Drop, here is the diff on top of this series. I can send this as a follow up if you would like after cleaning it up, or lmk wdyt: diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs index 20d38a64dcc7..6571a8c59d09 100644 --- a/drivers/gpu/nova-core/driver.rs +++ b/drivers/gpu/nova-core/driver.rs @@ -1,7 +1,5 @@ // SPDX-License-Identifier: GPL-2.0 -use core::cell::Cell; - use kernel::{ auxiliary, device::Core, @@ -23,10 +21,7 @@ types::ForLt, }; -use crate::{ - gpu::Gpu, - gsp, // -}; +use crate::gpu::Gpu; /// Counter for generating unique auxiliary device IDs. static AUXILIARY_ID_COUNTER: Atomic<u32> = Atomic::new(0); @@ -38,10 +33,6 @@ pub(crate) struct NovaCore<'bound> { bar: pci::Bar<'bound, BAR0_SIZE>, #[allow(clippy::type_complexity)] _reg: Devres<auxiliary::Registration<ForLt!(())>>, - /// GSP unload bundle, if any. - /// - /// Stored into a `Cell` so it can be [taken](Cell::take) without a mutable reference. - unload_bundle: Cell<Option<gsp::UnloadBundle>>, } const BAR0_SIZE: usize = SZ_16M; @@ -103,8 +94,6 @@ fn probe<'bound>( // other threads of execution. unsafe { pdev.dma_set_mask_and_coherent(DmaMask::new::<GPU_DMA_BITS>())? }; - let mut unload_bundle = None; - Ok(try_pin_init!(NovaCore { bar: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0")?, // TODO: Use `&bar` self-referential pin-init syntax once available. @@ -112,7 +101,7 @@ fn probe<'bound>( // SAFETY: `bar` is initialized before this expression is evaluated // (`try_pin_init!()` initializes fields in declaration order), lives at a pinned // stable address, and is dropped after `gpu` (struct field drop order). - gpu <- Gpu::new(pdev, unsafe { &*core::ptr::from_ref(bar) }, &mut unload_bundle), + gpu <- Gpu::new(pdev, unsafe { &*core::ptr::from_ref(bar) }), _reg: auxiliary::Registration::new( pdev.as_ref(), c"nova-drm", @@ -122,15 +111,7 @@ fn probe<'bound>( crate::MODULE_NAME, (), )?, - unload_bundle: Cell::new(unload_bundle), })) }) } - - fn unbind<'bound>( - dev: &'bound pci::Device<kernel::device::Core>, - this: Pin<&'bound Self::Data<'bound>>, - ) { - this.gpu.unbind(dev, this.unload_bundle.take()) - } } diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs index 5af04901b512..605eaba5d90a 100644 --- a/drivers/gpu/nova-core/gpu.rs +++ b/drivers/gpu/nova-core/gpu.rs @@ -246,8 +246,10 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { } /// Structure holding the resources required to operate the GPU. -#[pin_data] +#[pin_data(PinnedDrop)] pub(crate) struct Gpu<'bound> { + /// Device owning the GPU. + device: &'bound device::Device<device::Bound>, spec: Spec, /// MMIO mapping of PCI BAR 0. bar: &'bound Bar0, @@ -261,24 +263,23 @@ pub(crate) struct Gpu<'bound> { /// GSP runtime data. Temporarily an empty placeholder. #[pin] gsp: Gsp, + /// GSP unload firmware bundle, if any. + unload_bundle: Option<gsp::UnloadBundle>, } impl<'bound> Gpu<'bound> { /// Create a new [`Gpu`] instance. /// /// `pdev` is the PCI device for the GPU, `bar` is its `Bar0` mapping. - /// - /// `unload_bundle` is an output parameter, where the [GSP unload bundle](gsp::UnloadBundle) is - /// to be written. The driver layer will pass the written value back to [`Gpu::unbind`]. pub(crate) fn new<'init>( pdev: &'bound pci::Device<device::Bound>, bar: &'bound Bar0, - unload_bundle: &'init mut Option<gsp::UnloadBundle>, ) -> impl PinInit<Self, Error> + 'init where 'bound: 'init, { try_pin_init!(Self { + device: pdev.as_ref(), spec: Spec::new(pdev.as_ref(), bar).inspect(|spec| { dev_info!(pdev,"NVIDIA ({})\n", spec); })?, @@ -303,24 +304,24 @@ pub(crate) fn new<'init>( gsp <- Gsp::new(pdev), - _: { *unload_bundle = gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)? }, + unload_bundle: gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)?, }) } +} - pub(crate) fn unbind( - &self, - pdev: &'bound pci::Device<device::Core>, - unload_bundle: Option<gsp::UnloadBundle>, - ) { - let _ = self +#[pinned_drop] +impl PinnedDrop for Gpu<'_> { + fn drop(self: Pin<&mut Self>) { + let this = self.project(); + let device = *this.device; + let bar = *this.bar; + let bundle = this.unload_bundle.take(); + + let _ = this .gsp - .unload( - pdev.as_ref(), - self.bar, - &self.gsp_falcon, - &self.sec2_falcon, - unload_bundle, - ) - .inspect_err(|e| dev_err!(pdev, "failed to unload GSP: {:?}\n", e)); + .as_ref() + .get_ref() + .unload(device, bar, &*this.gsp_falcon, &*this.sec2_falcon, bundle) + .inspect_err(|e| dev_err!(device, "failed to unload GSP: {:?}\n", e)); } }
