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 <[email protected]> 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/[email protected]?part=1
