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
