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@xxxxxxxxx> 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@xxxxxxxxx> Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07 Reported-By: Lukas Wunner <lukas@xxxxxxxxx> Tested-By: Lukas Wunner <lukas@xxxxxxxxx> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> Cc: Lukas Wunner <lukas@xxxxxxxxx> --- 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
