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

[Tvrtko Ursulin]

On 07/02/2015 02:37 PM, Ville Syrjälä wrote:
> On Tue, Jun 30, 2015 at 10:06:27AM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> We had three 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.
>> Double unreference on the object if __intel_framebuffer_create fails
>> since both it and the caller (intelfb_alloc) do the unreference.
>>
>> 3.
>> Deadlock in intelfb_create failure path where it calls
>> drm_framebuffer_unreference, which grabs the struct mutex and
>> intelfb_create was already holding it.
>>
>> v2:
>>     * Reformat commit msg to 72 chars. (Lukas Wunner)
>>     * Added third failure mode. (Lukas Wunner)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
>> Reported-By: Lukas Wunner <lukas at wunner.de>
>> Tested-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>
>> Cc: Lukas Wunner <lukas at wunner.de>
>> ---
>> Ville, you suggested some changes earlier;
>>
>> """
>> I suggest removing the unref from __intel_framebuffer_create().
>> """
>>
>> Do you view that as an improvement in code clarity/organisation,
>> or you think my version is actually wrong for some reason?
>
> I find it rather unexpected that the function drops the passed
> reference on error. My usual rule is: do nothing on error, if possible.

I just don't have time at the moment to evaluate the other call sites 
etc. So from my point of view it boils down to whether this patch 
improves things without making anything worse. If it does R-B would be 
cool. If it doesn't then it will have to make for free time to appear. 
Or someone is free to take it over.

Regards,

Tvrtko