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

[Eliot Courtney]

On Thu Jul 23, 2026 at 12:28 PM JST, Alexandre Courbot wrote:
> 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?

Yes I think "guarantee" is better here since otherwise you need the
mathematical reading of "imply" to not think it's an if-and-only-if.

>
>> +    ///
>>      /// 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.

Yeah, I couldn't think of a good way to do this either and I was unhappy
about it. Wording nit sgtm. Thanks!