On Fri, 23 May 2025 20:53:44 +0000
Timur Tabi <tt...@nvidia.com> wrote:
> On Fri, 2025-05-23 at 19:03 +0300, Dan Carpenter wrote:
> > [ Did these files get renamed or something? No idea why the warnings
> > are showing up as new now. ]
>
> Ben posted a large refactor of the code last week.
>
> > Hello Ben Skeggs,
> >
> > Commit 7bb77eacdb85 ("drm/nouveau/gsp: add hal for
> > gsp.get_static_info()") from Nov 14, 2024 (linux-next), leads to the
> > following Smatch static checker warning:
> >
> > drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/ctrl.c:47
> > r535_gsp_rpc_rm_ctrl_push() warn:
> > passing zero to 'PTR_ERR'
> > drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c:223
> > r535_gsp_get_static_info() warn: 'rpc'
> > can also be NULL
> > drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c:90
> > r570_gsp_get_static_info() warn: 'rpc'
> > can also be NULL
> >
> > drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c
> > 212 static int
> > 213 r535_gsp_get_static_info(struct nvkm_gsp *gsp)
> > 214 {
> > 215 GspStaticConfigInfo *rpc;
> > 216
> > 217 rpc = nvkm_gsp_rpc_rd(gsp,
> > NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO,
> > sizeof(*rpc));
> > 218 if (IS_ERR(rpc))
> > 219 return PTR_ERR(rpc);
> > 220
> > 221 gsp->internal.client.object.client = &gsp->internal.client;
> > 222 gsp->internal.client.object.parent = NULL;
> > --> 223 gsp->internal.client.object.handle = rpc->hInternalClient;
> > ^^^^^
> >
> > The nvkm_gsp_rpc_rd() function returns a mix of error pointers and NULL but
> > if it returns NULL then obviously this dereference will crash.
> >
> > https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
> >
>
> Well, there's definitely something weird going on with this code. It appears
> that nvkm_gsp_rpc_rd()
> actually returns NULL on success.
>
> nvkm_gsp_rpc_rd -> r535_gsp_rpc_push -> r535_gsp_rpc_handle_reply()
>
> So either I'm confused, or this will need further debugging.
It won't return a NULL with a policy NVKM_GSP_RPC_REPLY_RECV.
nvkm_gsp_rpc_rd -> r535_gsp_rpc_push -> r535_gsp_rpc_send->
r535_gsp_rpc_handle_reply
It will be either a error pointer or the reply pointer.
And I agree mixing NULL and error pointers seems confusing. It needs a
clean up.
Z.
