Re: drm/vmwgfx: Fix compat shader namespace

["Thomas Hellström" <thellstrom@vmware.com>]

On 2014-07-09 14:48, Dan Carpenter wrote:
> Hello Thomas Hellstrom,
>
> The patch 18e4a4669c50: "drm/vmwgfx: Fix compat shader namespace"
> from Jun 9, 2014, leads to the following static checker warning:
>
> 	drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:477 vmw_cmd_res_reloc_add()
> 	warn: missing error code here? 'kzalloc()' failed.
>
> drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
>     468
>     469                  ret = vmw_resource_context_res_add(dev_priv, sw_context, res);
>     470                  if (unlikely(ret != 0))
>     471                          goto out_err;
>     472                  node->staged_bindings =
>     473                          kzalloc(sizeof(*node->staged_bindings), GFP_KERNEL);
>     474                  if (node->staged_bindings == NULL) {
>     475                          DRM_ERROR("Failed to allocate context binding "
>     476                                    "information.\n");
>     477                          goto out_err;
>
> This should just be "return -ENOMEM;".  The goto is misleading because
> you expect it to do something useful.

Indeed. Thanks for pointing that out. Since this is old code being 
reorganized, the goto slipped through. The missing error code has been 
around for a while, though. I'll put together a patch for that.

>
> Soon checkpatch.pl will start complaining about the extra DRM_ERROR()
> because kzalloc() has a more useful printk builtin and this just wastes
> memory and makes the code more verbose.
Noted.

>
> Speaking of verbose, all the likely/unlikely annotations should be
> removed.

Is this your personal opinion or has there been some kind of kernel 
developer agreement not to add this annotation and remove it from the 
kernel tree? If not, I prefer to keep it.

>    If the code were more readable then the missing error code
> would have been more noticeable.  This code is buggy because it is ugly;
> there is a direct cause effect relationship.
I think ugliness in this case is in the eye of the beholder. The bug 
likely entered long ago like these bugs tend to do because you're not 
100% focused when the code is written. I find this statement a bit 
incoherent because there's no branch prediction hint in the if statement 
preceding the bug and although the error message may be redundant in 
this case, I can't see why an error message would make the code ugly or 
be the cause of a bug.

>
>     478                  }
>     479                  INIT_LIST_HEAD(&node->staged_bindings->list);
>     480          }
>     481
>     482          if (p_val)
>     483                  *p_val = node;
>     484
>     485  out_err:
>     486          return ret;
>     487  }
>
> regards,
> dan carpenter

Thanks,
Thomas