[bug report] drm/nouveau/gsp: add hal for gsp.get_static_info()

[bug report] drm/nouveau/gsp: add hal for gsp.get_static_info()

[Dan Carpenter]

[ Did these files get renamed or something?  No idea why the warnings
  are showing up as new now. ]

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/

regards,
dan carpenter

Re: [bug report] drm/nouveau/gsp: add hal for gsp.get_static_info()

[Timur Tabi]

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.

Re: [bug report] drm/nouveau/gsp: add hal for gsp.get_static_info()

[Zhi Wang]

On Fri, 23 May 2025 20:53:44 +0000
Timur Tabi <ttabi@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.