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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] RPC error path propagates NULL instead of ERR_PTR(), leading to silent failures and NULL pointer dereferences.
--

commit 79a7510afd6195ece62a51a8ba00ae15d75a0ac9
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.

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

[Severity: High]
This is a pre-existing issue, but does documenting NULL as a valid return
for an invalid RPC length mask an existing bug in the error path?

Currently, r535_gsp_msgq_recv() returns NULL when the length is invalid:

	if (WARN_ON(rpc->length > max_rpc_size))
		return NULL;

This NULL propagates up through r535_gsp_msg_recv() and 
r535_gsp_rpc_handle_reply() to callers like r535_gsp_get_static_info(),
which only checks for an error pointer:

	rpc = nvkm_gsp_rpc_rd(gsp, NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO, sizeof(*rpc));
	if (IS_ERR(rpc))
		return PTR_ERR(rpc);
	
	gsp->internal.client.object.handle = rpc->hInternalClient;

Since IS_ERR(NULL) evaluates to false, this leads to an immediate NULL
pointer dereference.

In other cases like r535_gsp_rpc_rm_ctrl_push(), the error is silently
swallowed:

	rpc = nvkm_gsp_rpc_push(gsp, rpc, NVKM_GSP_RPC_REPLY_RECV, repc);
	if (IS_ERR_OR_NULL(rpc)) {
		*params = NULL;
		return PTR_ERR(rpc);
	}

Because PTR_ERR(NULL) evaluates to 0, the function incorrectly returns
success on a failed RPC message.

Should r535_gsp_msgq_recv() be changed to return an error pointer like
ERR_PTR(-EIO) instead of NULL, and the documentation updated to reflect
that failure mode?

>  static void *
>  r535_gsp_msgq_recv(struct nvkm_gsp *gsp, u32 gsp_rpc_len, int *retries)
>  {

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