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

[Tvrtko Ursulin]

Hi,

On 06/29/2015 03:39 PM, Lukas Wunner wrote:
> Hi,
>
> On Mon, Jun 15, 2015 at 11:49:52AM +0100, Tvrtko Ursulin 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.
>
> s/intelfb_alloc/intelfb_create/
> s/(caller of intelfb_alloc)//

I looked again and don't see that I made a mistake with regards to this?

>> 2.
>> Double unreference on the object if __intel_framebuffer_create fails since
>> both it and the caller (intelfb_alloc) do the unreference.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
> Reported-By: Lukas Wunner <lukas at wunner.de>

Sorry probably my oversight, don't remember now how it all came about.

>> @@ -183,15 +186,18 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>>   		goto out_fb;
>>   	}
>>
>> +	mutex_unlock(&dev->struct_mutex);
>> +
>>   	ifbdev->fb = to_intel_framebuffer(fb);
>>
>>   	return 0;
>>
>>   out_fb:
>> -	drm_framebuffer_remove(fb);
>> -out_unref:
>>   	drm_gem_object_unreference(&obj->base);
>>   out:
>> +	mutex_unlock(&dev->struct_mutex);
>> +	if (fb)
>> +		drm_framebuffer_remove(fb);
>
> Hm, the order of drm_gem_object_unreference() and drm_framebuffer_remove()
> is reversed now, could this cause any issues?

No, that is fine.

> Apart from that,
>
> Reviewed-By: Lukas Wunner <lukas at wunner.de>
>
> I will also test the patch and report back in a bit.

That will be very useful. I did not test it extensively myself, just 
fired it off quickly since I was briefly hitting this failure path myself.

Regards,

Tvrtko