Re: [PATCH v2 10/10] gpu: nova-core: wait for RISC-V HALTED on FSP unload

[Alexandre Courbot]

On Fri Jul 3, 2026 at 3:22 AM PDT, Eliot Courtney wrote:
> Currently the code waits for "not active" but this is not the same as
> halted as there are more than two states. Match openrm here and wait for
> halted instead.
>
> Fixes: c7fea1f70944 ("gpu: nova-core: add non-sec2 unload path")
> Signed-off-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/falcon.rs           | 11 +++++++++++
>  drivers/gpu/nova-core/falcon/hal.rs       |  5 +++++
>  drivers/gpu/nova-core/falcon/hal/ga102.rs |  7 +++++++
>  drivers/gpu/nova-core/falcon/hal/tu102.rs |  4 ++++
>  drivers/gpu/nova-core/gsp/hal/gh100.rs    | 11 +++++++++--
>  5 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 78948cc8bff3..ec286017535f 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -749,11 +749,22 @@ pub(crate) fn signature_reg_fuse_version(
>  
>      /// Check if the RISC-V core is active.
>      ///
> +    /// Note that this does not imply that the RISC-V core is halted if it returns `false`.

I would say "guarantee" instead of "imply" here, wdyt?

> +    ///
>      /// Returns `true` if the RISC-V core is active, `false` otherwise.
>      pub(crate) fn is_riscv_active(&self) -> bool {
>          self.hal.is_riscv_active(self)
>      }
>  
> +    /// Checks whether the RISC-V core is halted.
> +    ///
> +    /// Note that this does not imply that the RISC-V core is active if it returns `false`.

Same here.

> +    ///
> +    /// Returns [`ENOTSUPP`] if the status is not available.
> +    pub(crate) fn is_riscv_halted(&self) -> Result<bool> {
> +        self.hal.is_riscv_halted(self)
> +    }
> +
>      /// Load a firmware image into Falcon memory, using the preferred method for the current
>      /// chipset.
>      pub(crate) fn load<F: FalconFirmware<Target = E> + FalconDmaLoadable>(&self, fw: &F) -> Result {
> diff --git a/drivers/gpu/nova-core/falcon/hal.rs b/drivers/gpu/nova-core/falcon/hal.rs
> index ee4a017f3a4c..7e532889a1f4 100644
> --- a/drivers/gpu/nova-core/falcon/hal.rs
> +++ b/drivers/gpu/nova-core/falcon/hal.rs
> @@ -53,6 +53,11 @@ fn signature_reg_fuse_version(
>      /// Returns `true` if the RISC-V core is active, `false` otherwise.
>      fn is_riscv_active(&self, falcon: &Falcon<'_, E>) -> bool;
>  
> +    /// Checks whether the RISC-V core is halted.
> +    ///
> +    /// Returns [`ENOTSUPP`] if the chipset does not expose RISC-V halt status.
> +    fn is_riscv_halted(&self, falcon: &Falcon<'_, E>) -> Result<bool>;

It's a bit unfortunate that we add a runtime error check for something
that is essentially known to never fail (the GSP's GH100 HAL being the
sole caller of this method). I cannot think of a better design at
the moment, but this hints that our HAL could be improved as I think we
already have several instances of the same pattern elsewhere.

Not a big deal anyway - if you can confirm my wording nit above, this
one should be good to merge as well.