Re: [PATCH v3 1/5] nouveau/gsp/rpc: Document RPC function return value contracts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The newly added kernel-doc comments for `r535_gsp_rpc_send` and `r535_gsp_rpc_push` incorrectly document their return values regarding the POLL policy.

Pre-existing issues:
- [High] r535_gsp_msgq_recv() returns NULL instead of an error pointer on invalid RPC length, leading to unchecked propagation and widespread NULL pointer dereferences.
--

commit 0752081ac429b7a0554a2c49b5c123044b8bdc1f
Author: Hongling Zeng <zenghongling@kylinos.cn>

nouveau/gsp/rpc: Document RPC function return value contracts
    
Add kernel-doc comments to document the return value semantics of
RPC functions in r535/rpc.c. This clarifies which functions can return
NULL, error pointers, or both, helping maintainers validate future
changes.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c
> index 3ca3de8f43408..35272c1538699 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c

[ ... ]

> @@ -283,6 +300,15 @@ r535_gsp_msgq_recv_one_elem(struct nvkm_gsp *gsp,
>  	return buf;
>  }
>  
> +/**
> + * r535_gsp_msgq_recv - Receive RPC message(s) from GSP queue
> + * @gsp: GSP device
> + * @gsp_rpc_len: Expected RPC length
> + * @retries: Retry counter
> + *
> + * Return: Pointer to received buffer on success, ERR_PTR() on error,
> + *         or NULL if RPC length is invalid.
> + */
>  static void *
>  r535_gsp_msgq_recv(struct nvkm_gsp *gsp, u32 gsp_rpc_len, int *retries)

[Severity: High]
This is a pre-existing issue, but returning NULL here on an invalid
RPC length seems to cause widespread NULL pointer dereferences later on.

If r535_gsp_msgq_recv() returns NULL, it propagates to callers like
r535_gsp_rpc_rm_ctrl_push(), which handles the error like this:

    if (IS_ERR_OR_NULL(rpc)) {
        *params = NULL;
        return PTR_ERR(rpc);
    }

Because PTR_ERR(NULL) evaluates to 0, this gets passed up the stack as a
success condition. Wrapper functions like nvkm_gsp_rm_ctrl_rd() receive
the 0 return code and pass the NULL argv pointer back to their callers.

When a downstream caller like r535_disp_get_supported() checks the result:

    ctrl = nvkm_gsp_rm_ctrl_rd(&disp->rm.objcom,
                               NV0073_CTRL_CMD_SYSTEM_GET_SUPPORTED,
                               sizeof(*ctrl));
    if (IS_ERR(ctrl))
        return PTR_ERR(ctrl);

    *pmask = ctrl->displayMask;

The IS_ERR(NULL) check evaluates to false, causing a kernel panic when
the NULL pointer is dereferenced on the very next line. 

Could we change r535_gsp_msgq_recv() to return an error pointer like
ERR_PTR(-EINVAL) instead of NULL when the message length is invalid?

[ ... ]

> @@ -574,6 +625,16 @@ r535_gsp_rpc_handle_reply(struct nvkm_gsp *gsp, u32 fn,
>  	return repv;
>  }
>  
> +/**
> + * r535_gsp_rpc_send - Send RPC message and handle reply
> + * @gsp: GSP device
> + * @payload: RPC payload to send
> + * @policy: Reply policy (NOWAIT, NOSEQ, RECV, or POLL)
> + * @gsp_rpc_len: Expected RPC length for reply
> + *
> + * Return: NULL if policy is NOWAIT/NOSEQ (no reply expected),
> + *         pointer to reply data on success, or ERR_PTR() on error.
> + */
>  static void *
>  r535_gsp_rpc_send(struct nvkm_gsp *gsp, void *payload,

[Severity: Low]
Does this documentation miss the POLL policy's NULL return behavior?

The documentation for r535_gsp_rpc_handle_reply() correctly notes that
when the policy is POLL, it returns NULL on successful message processing.
Since r535_gsp_rpc_send() returns the result of r535_gsp_rpc_handle_reply(),
should POLL be included in the list of policies that return NULL here?

[ ... ]

> @@ -628,6 +703,16 @@ r535_gsp_rpc_get(struct nvkm_gsp *gsp, u32 fn, u32 payload_size)
>  	return rpc->data;
>  }
>  
> +/**
> + * r535_gsp_rpc_push - Push RPC message to GSP and wait for reply
> + * @gsp: GSP device
> + * @payload: RPC payload to send
> + * @policy: Reply policy (NOWAIT, NOSEQ, RECV, or POLL)
> + * @gsp_rpc_len: Expected RPC length in the reply
> + *
> + * Return: NULL when policy is NOWAIT/NOSEQ (no reply expected),
> + *         pointer to reply data on success, or ERR_PTR() on error.
> + */
>  static void *
>  r535_gsp_rpc_push(struct nvkm_gsp *gsp, void *payload,

[Severity: Low]
Similar to r535_gsp_rpc_send() above, should this also document that the
POLL policy can return NULL on success?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622021207.14764-1-zenghongling@kylinos.cn?part=1