[Intel-gfx] [PATCH v6 3/4] drm/i915: Fix failure paths around initial fbdev allocation

[Daniel Vetter]

On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> We had two failure modes here:
> 
> 1.
> Deadlock in intelfb_alloc failure path where it calls
> drm_framebuffer_remove, which grabs the struct mutex and intelfb_create
> (caller of intelfb_alloc) was already holding it.
> 
> 2.
> Deadlock in intelfb_create failure path where it calls
> drm_framebuffer_unreference, which grabs the struct mutex and
> intelfb_create was already holding it.
> 
> [Chris Wilson on why struct_mutex needs to be locked in the second half
> of intelfb_create: "there is a bug here where we don't take an explicit
> pin on the VMA we setup for the fbdev which requires the lock."]

The vma is pinned, the problem is that we re-lookup it a few times, which
is racy. We should instead track the vma directly, but oh well we don't.

With that clarified in the commit message this is

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> v2:
>    * Reformat commit msg to 72 chars. (Lukas Wunner)
>    * Add third failure mode. (Lukas Wunner)
> 
> v5:
>    * Rebase on drm-intel-nightly 2015y-09m-01d-09h-06m-08s UTC,
>      rephrase commit message. (Jani Nicula)
> 
> v6:
>    * In intelfb_alloc, if __intel_framebuffer_create failed,
>      fb will be an ERR_PTR, thus not null. So in the failure
>      path we need to check for IS_ERR_OR_NULL to avoid calling
>      drm_framebuffer_remove on the ERR_PTR. (Lukas Wunner)
>    * Since this is init code a drm_framebuffer_unreference should
>      be all we need. drm_framebuffer_remove is for framebuffers
>      that userspace has created - and is getting somewhat
>      defeatured. (Daniel Vetter)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Fixes: 60a5ca015ffd ("drm/i915: Add locking around
>     framebuffer_references--")
> Reported-by: Lukas Wunner <lukas at wunner.de>
> [Lukas: Create v3 + v4 + v5 + v6 based on Tvrtko's v2]
> Signed-off-by: Lukas Wunner <lukas at wunner.de>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbdev.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index ec82b51..12597b5 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  {
>  	struct intel_fbdev *ifbdev =
>  		container_of(helper, struct intel_fbdev, helper);
> -	struct drm_framebuffer *fb;
> +	struct drm_framebuffer *fb = NULL;
>  	struct drm_device *dev = helper->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_mode_fb_cmd2 mode_cmd = {};
> @@ -138,6 +138,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
>  							  sizes->surface_depth);
>  
> +	mutex_lock(&dev->struct_mutex);
> +
>  	size = mode_cmd.pitches[0] * mode_cmd.height;
>  	size = PAGE_ALIGN(size);
>  
> @@ -165,16 +167,19 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL);
>  	if (ret) {
>  		DRM_ERROR("failed to pin obj: %d\n", ret);
> -		goto out_fb;
> +		goto out;
>  	}
>  
> +	mutex_unlock(&dev->struct_mutex);
> +
>  	ifbdev->fb = to_intel_framebuffer(fb);
>  
>  	return 0;
>  
> -out_fb:
> -	drm_framebuffer_remove(fb);
>  out:
> +	mutex_unlock(&dev->struct_mutex);
> +	if (!IS_ERR_OR_NULL(fb))
> +		drm_framebuffer_unreference(fb);
>  	return ret;
>  }
>  
> @@ -192,8 +197,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	int size, ret;
>  	bool prealloc = false;
>  
> -	mutex_lock(&dev->struct_mutex);
> -
>  	if (intel_fb &&
>  	    (sizes->fb_width > intel_fb->base.width ||
>  	     sizes->fb_height > intel_fb->base.height)) {
> @@ -208,7 +211,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  		DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
>  		ret = intelfb_alloc(helper, sizes);
>  		if (ret)
> -			goto out_unlock;
> +			return ret;
>  		intel_fb = ifbdev->fb;
>  	} else {
>  		DRM_DEBUG_KMS("re-using BIOS fb\n");
> @@ -220,6 +223,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	obj = intel_fb->obj;
>  	size = obj->base.size;
>  
> +	mutex_lock(&dev->struct_mutex);
> +
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		ret = PTR_ERR(info);
> @@ -281,7 +286,6 @@ out_destroy_fbi:
>  out_unpin:
>  	i915_gem_object_ggtt_unpin(obj);
>  	drm_gem_object_unreference(&obj->base);
> -out_unlock:
>  	mutex_unlock(&dev->struct_mutex);
>  	return ret;
>  }
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch