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

[John Hubbard <jhubbard@nvidia.com>]

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@nvidia.com>
> ---
>   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.

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

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

thanks,
-- 
John Hubbard