On Mon, Dec 14, 2015 at 11:16:07AM +0530, ankitprasad.r.sharma@xxxxxxxxx wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 17d679e..366080b9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -492,6 +492,7 @@ i915_pages_create_for_stolen(struct drm_device *dev,
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct sg_table *st;
> struct scatterlist *sg;
> + int ret;
>
> DRM_DEBUG_DRIVER("offset=0x%x, size=%d\n", offset, size);
> BUG_ON(offset > dev_priv->gtt.stolen_size - size);
> @@ -503,11 +504,12 @@ i915_pages_create_for_stolen(struct drm_device *dev,
>
> st = kmalloc(sizeof(*st), GFP_KERNEL);
> if (st == NULL)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> - if (sg_alloc_table(st, 1, GFP_KERNEL)) {
> + ret = sg_alloc_table(st, 1, GFP_KERNEL);
> + if (ret) {
> kfree(st);
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> sg = st->sgl;
> @@ -559,15 +561,17 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
>
> obj = i915_gem_object_alloc(dev);
> if (obj == NULL)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> drm_gem_private_object_init(dev, &obj->base, stolen->size);
> i915_gem_object_init(obj, &i915_gem_object_stolen_ops);
>
> obj->pages = i915_pages_create_for_stolen(dev,
> stolen->start, stolen->size);
> - if (obj->pages == NULL)
> - goto cleanup;
> + if (IS_ERR(obj->pages)) {
> + i915_gem_object_free(obj);
> + return (void*) obj->pages;
This is a bad idiom to use. Looks ok here (as only one caller sees the
invalid obj->pages) but it was an immediate red-flag for me as a reader
of the code (since you are storing an invalid pointer in a very common
field).
Anyway the correct use is return ERR_CAST(obj->pages);
However, I would much prefer a temporary variable:
pages = i915_pages_crate_for_stolen();
if (IS_ERR(pages)) {
object_free(obj);
return ERR_CAST(pages);
}
obj->pages = pages;
Just so that I don't have to think about who might chase that invalid
pointer, today or in the future.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
