[Intel-gfx] [PATCH 5/9] drm/i915: Propagating correct error codes to the userspace

[Chris Wilson]

On Mon, Dec 14, 2015 at 11:16:07AM +0530, ankitprasad.r.sharma at intel.com 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