Re: [PATCH v3 2/9] gpu: nova-core: gsp: add NvStatus enum for RM control errors

[Eliot Courtney]

On Mon Apr 6, 2026 at 5:05 AM JST, John Hubbard wrote:
> On 3/25/26 5:13 AM, Eliot Courtney wrote:
>> Add NvStatus enum that wraps the raw NV_STATUS u32 codes returned by RM
>> control RPCs.
>> 
>> Signed-off-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
>> ---
>>   drivers/gpu/nova-core/gsp/fw.rs | 401 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 401 insertions(+)
>> 
>> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
>> index 0c8a74f0e8ac..cb4bda253193 100644
>> --- a/drivers/gpu/nova-core/gsp/fw.rs
>> +++ b/drivers/gpu/nova-core/gsp/fw.rs
>> @@ -97,6 +97,407 @@ pub(in crate::gsp) fn advance_cpu_write_ptr(qs: &Coherent<GspMem>, count: u32) {
>>   pub(crate) const GSP_MSG_QUEUE_ELEMENT_SIZE_MAX: usize =
>>       num::u32_as_usize(bindings::GSP_MSG_QUEUE_ELEMENT_SIZE_MAX);
>>   
>> +/// Status code returned by GSP-RM RPCs.
>> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
>> +pub(crate) enum NvStatus {
>> +    Ok,
>
> As mentioned in my respose to patch 1, these are hand-written and won't
> automatically pick up anything new from bindgen(1).
>
> If for some reason we don't go directly to generating Rust bindings from
> GSP-RM builds, then we could somewhat mitigate the problem with a
> declarative macro that generates the enum, the From<u32> impl, and the
> From<NvStatus> for Result impl from a single table.
>
> That would at least prevent internal drift between the three, but it
> still would not detect new binding constants.
>
> It would also add yet another macro, sigh.

Yeah I feel we have a lot of macros. But, at least for From<u32> we can
use FromPrimitive once that is ready (see my other email, maybe better
for me to switch it to TryFrom and get rid of Unknown). As for the
mapping, we have to think about it manually somewhere, so either it's
here in a From<> impl, or it's in the bindings generator or upstream of
that.

>
>> +    AlreadySignalled,
>> +    BrokenFb,
> ...
>> +    Unknown(u32),
>> +}
>> +
>> +impl From<NvStatus> for Result {
>> +    fn from(status: NvStatus) -> Self {
>
> This discards the NvStatus variant. The caller sees only a kernel errno.
> We need to retain the rich RM error codes for debugging. For example,
> a caller that gets Err(ENODEV) cannot tell whether RM returned
> CardNotPresent, InvalidDevice, FabricManagerNotPresent, or
> ReductionManagerNotAvailable. Something like:
>
>      fn send(self, ...) -> Result<T::Reply, NvStatus>
>
> would let the caller log or match on the original status, and convert
> to errno only when propagating upward.

SGTM, let me push NvStatus handling a bit up the caller chain then.
I was thinking it might be nice to not carry the openrm naming, and
instead call it e.g. GspMsgRmStatus.

But, I don't think we should use it as is as the error variant of
Result, because it contains 'Ok'. A better shape might be:

GspMsgRmStatus {Ok, Warning(GspMsgRmWarning), Error(GspMsgRmError)}

Another issue is that RmControl::send needs to also propagate the
kernel::error::Error from calling the cmdq. So we would need a wrapping
enum, e.g. enum GspRpcError {Transport(Error), Rm(GspMsgRmError)}. The
(currently unchecked) rpc_result field that comes back from GSP messages
can contain NV_STATUS values (as well as some other ones), so
probably Cmdq should be returning this too.

So if we are fine to just log warnings instead of surfacing them to the
caller, we could do:

fn send(...) -> Result<T::Reply, GspRpcError>

WDYT?

If we go this direction, I might create another patch series for
defining GspRpcError and adding support to Cmdq for returning that,
based on the RPC header's `rpc_result`, since we need to pass that
through here for RM control.

>
>> +        match status {
>> +            NvStatus::Ok => Ok(()),
> ...
>> +
>> +            NvStatus::Timeout => Err(ETIMEDOUT),
>> +
>> +            _ => Err(EIO),
>
> This will cause failures for NV_WARN_* items, which is a problem.
> RM warnings should not be converted into errors.

Do you reckon we will want to do anything other than just log the
warning?

>
> thanks,