`Gpu` currently owns the state needed to unload the GSP directly. This means that `unload_bundle` has to be the last initialized field: once GSP boot succeeds, any later initialization failure would leave `Gpu` partially initialized, and its `PinnedDrop` implementation would not run.
This prevents adding fallible `Gpu` fields that need to query the GSP after it has booted. Move the GSP state and unload bundle into a dedicated pinned `GspResources` object. Once that subobject has been initialized, its `PinnedDrop` implementation will run even if initialization of a later `Gpu` field fails, ensuring that the GSP unload sequence is executed. Signed-off-by: Alexandre Courbot <[email protected]> Reviewed-by: Eliot Courtney <[email protected]> --- drivers/gpu/nova-core/gpu.rs | 90 ++++++++++++++++++++++++++------------------ 1 file changed, 53 insertions(+), 37 deletions(-) diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs index b3c91731db45..b5cffcee9817 100644 --- a/drivers/gpu/nova-core/gpu.rs +++ b/drivers/gpu/nova-core/gpu.rs @@ -262,35 +262,62 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { } } -/// Structure holding the resources required to operate the GPU. +/// Self-contained resources to operate and drop the GSP. #[pin_data(PinnedDrop)] -pub(crate) struct Gpu<'gpu> { +struct GspResources<'gpu> { /// Device owning the GPU. device: &'gpu device::Device<device::Bound>, - spec: Spec, /// MMIO mapping of PCI BAR 0. bar: Bar0<'gpu>, - /// System memory page required for flushing all pending GPU-side memory writes done through - /// PCIE into system memory, via sysmembar (A GPU-initiated HW memory-barrier operation). - sysmem_flush: SysmemFlush<'gpu>, /// GSP falcon instance, used for GSP boot up and cleanup. gsp_falcon: Falcon<GspFalcon>, /// SEC2 falcon instance, used for GSP boot up and cleanup. sec2_falcon: Falcon<Sec2Falcon>, - /// GSP runtime data. Temporarily an empty placeholder. + /// GSP runtime data. #[pin] gsp: Gsp, /// GSP unload firmware bundle, if any. unload_bundle: Option<gsp::UnloadBundle>, } +/// Structure holding the resources required to operate the GPU. +#[pin_data] +pub(crate) struct Gpu<'gpu> { + spec: Spec, + /// GSP and its resources. + #[pin] + gsp_resources: GspResources<'gpu>, + /// System memory page required for flushing all pending GPU-side memory writes done through + /// PCIE into system memory, via sysmembar (A GPU-initiated HW memory-barrier operation). + /// + /// Must be kept declared *after* `gsp_resources`, as the latter's `PinnedDrop` implementation + /// requires the sysmem flush page to be in place. + sysmem_flush: SysmemFlush<'gpu>, +} + +#[pinned_drop] +impl PinnedDrop for GspResources<'_> { + 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 + .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)); + } +} + impl<'gpu> Gpu<'gpu> { pub(crate) fn new( pdev: &'gpu pci::Device<device::Core<'_>>, bar: Bar0<'gpu>, ) -> impl PinInit<Self, Error> + 'gpu { try_pin_init!(Self { - device: pdev.as_ref(), spec: Spec::new(pdev.as_ref(), bar).inspect(|spec| { dev_info!(pdev,"NVIDIA ({})\n", spec); })?, @@ -308,40 +335,29 @@ pub(crate) fn new( .inspect_err(|_| dev_err!(pdev, "GFW boot did not complete\n"))?; }, + // Initialize this early because `gsp_resources` depends on it. sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?, - gsp_falcon: Falcon::new( - pdev.as_ref(), - spec.chipset, - ) - .inspect(|falcon| falcon.clear_swgen0_intr(bar))?, + gsp_resources <- try_pin_init!(GspResources { + device: pdev.as_ref(), - sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset)?, + bar, - gsp <- Gsp::new(pdev), + gsp_falcon: Falcon::new( + pdev.as_ref(), + spec.chipset, + ) + .inspect(|falcon| falcon.clear_swgen0_intr(bar))?, - // This member must be initialized last, so the `UnloadBundle` can never be dropped from - // outside of the constructed `Gpu`, ensuring that the unload sequence is properly run - // in case of failure. - unload_bundle: gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)?, - bar, + sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset)?, + + gsp <- Gsp::new(pdev), + + // This member must be initialized last, so the `UnloadBundle` can never be dropped + // from outside of the constructed `GspResources`, ensuring that the unload sequence + // is properly run in case of failure. + unload_bundle: gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)?, + }), }) } } - -#[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 - .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)); - } -} -- 2.54.0
