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