Nouveau - Mar 2017 - [bug report] drm/nouveau/secboot: add gp102/gp104/gp106/gp107 support

Hello Alexandre Courbot,

The patch 5429f82f3415: "drm/nouveau/secboot: add
gp102/gp104/gp106/gp107 support" from Jan 26, 2017, leads to the
following static checker warning:

	drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp102.c:63 gp102_run_secure_scrub()
	warn: passing zero to 'PTR_ERR'

drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp102.c
    46  static int
    47  gp102_run_secure_scrub(struct nvkm_secboot *sb)
    48  {
    49          struct nvkm_subdev *subdev = &sb->subdev;
    50          struct nvkm_device *device = subdev->device;
    51          struct nvkm_engine *engine;
    52          struct nvkm_falcon *falcon;
    53          void *scrub_image;
    54          struct fw_bin_header *hsbin_hdr;
    55          struct hsf_fw_header *fw_hdr;
    56          struct hsf_load_header *lhdr;
    57          void *scrub_data;
    58          int ret;
    59  
    60          nvkm_debug(subdev, "running VPR scrubber binary on
NVDEC...\n");
    61  
    62          if (!(engine =
nvkm_engine_ref(&device->nvdec->engine)))
    63                  return PTR_ERR(engine);

This code doesn't make sense.  nvkm_engine_ref() isn't going to return
NULL because we're not passing a NULL to it.  I guess we should be
checking for ERR_PTR().  But really why does nvkm_engine_ref() ever
return NULL?  It feels like if we pass it a NULL pointer it should
return ERR_PTR(-EINVAL) on that path instead.

    64          falcon = device->nvdec->falcon;
    65  

regards,
dan carpenter

On Sat, Mar 11, 2017 at 5:55 AM, Dan Carpenter <dan.carpenter at
oracle.com> wrote:
> Hello Alexandre Courbot,
>
> The patch 5429f82f3415: "drm/nouveau/secboot: add
> gp102/gp104/gp106/gp107 support" from Jan 26, 2017, leads to the
> following static checker warning:
>
>         drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp102.c:63
gp102_run_secure_scrub()
>         warn: passing zero to 'PTR_ERR'
>
> drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp102.c
>     46  static int
>     47  gp102_run_secure_scrub(struct nvkm_secboot *sb)
>     48  {
>     49          struct nvkm_subdev *subdev = &sb->subdev;
>     50          struct nvkm_device *device = subdev->device;
>     51          struct nvkm_engine *engine;
>     52          struct nvkm_falcon *falcon;
>     53          void *scrub_image;
>     54          struct fw_bin_header *hsbin_hdr;
>     55          struct hsf_fw_header *fw_hdr;
>     56          struct hsf_load_header *lhdr;
>     57          void *scrub_data;
>     58          int ret;
>     59
>     60          nvkm_debug(subdev, "running VPR scrubber binary on
NVDEC...\n");
>     61
>     62          if (!(engine =
nvkm_engine_ref(&device->nvdec->engine)))
>     63                  return PTR_ERR(engine);
>
> This code doesn't make sense.  nvkm_engine_ref() isn't going to
return
> NULL because we're not passing a NULL to it.  I guess we should be
> checking for ERR_PTR().  But really why does nvkm_engine_ref() ever
> return NULL?  It feels like if we pass it a NULL pointer it should
> return ERR_PTR(-EINVAL) on that path instead.
Indeed, not sure what I was smoking. The proper fix is to check for
ERR_PTR, will send a patch to fix it shortly. Thanks for the report!