* Re: [PATCH] drm/i915: Fix failure paths around initial fbdev allocation
2015-06-30 9:06 [PATCH] drm/i915: Fix failure paths around initial fbdev allocation Tvrtko Ursulin
@ 2015-06-30 9:13 ` shuang.he
2015-06-30 10:23 ` Jani Nikula
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: shuang.he @ 2015-06-30 9:13 UTC (permalink / raw)
To: shuang.he, lei.a.liu, intel-gfx, tvrtko.ursulin
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6642
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 312/316 312/316
IVB 343/343 343/343
BYT -1 287/287 286/287
HSW 380/380 380/380
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_tiled_partial_pwrite_pread@reads PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] drm/i915: Fix failure paths around initial fbdev allocation
2015-06-30 9:06 [PATCH] drm/i915: Fix failure paths around initial fbdev allocation Tvrtko Ursulin
2015-06-30 9:13 ` shuang.he
@ 2015-06-30 10:23 ` Jani Nikula
2015-07-02 13:23 ` Lukas Wunner
2015-07-02 13:37 ` Ville Syrjälä
3 siblings, 0 replies; 17+ messages in thread
From: Jani Nikula @ 2015-06-30 10:23 UTC (permalink / raw)
To: Tvrtko Ursulin, Intel-gfx
On Tue, 30 Jun 2015, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
The format is
Fixes: 60a5ca015ffd ("drm/i915: Add locking around framebuffer_references--")
See Documentation/SubmittingPatches on how to have git output that
format for you.
BR,
Jani.
> Reported-By: Lukas Wunner <lukas@wunner.de>
> Tested-By: Lukas Wunner <lukas@wunner.de>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lukas Wunner <lukas@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?
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 2a1724e..118420b 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -141,7 +141,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_mode_fb_cmd2 mode_cmd = {};
> struct drm_i915_gem_object *obj;
> @@ -159,6 +159,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);
> obj = i915_gem_object_create_stolen(dev, size);
> @@ -172,8 +174,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>
> fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
> if (IS_ERR(fb)) {
> + /* Drops object reference on failure. */
> ret = PTR_ERR(fb);
> - goto out_unref;
> + goto out;
> }
>
> /* Flush everything out, we'll be doing GTT only from now on */
> @@ -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);
> return ret;
> }
>
> @@ -209,8 +215,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)) {
> @@ -225,7 +229,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");
> @@ -237,6 +241,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
> obj = intel_fb->obj;
> size = obj->base.size;
>
> + mutex_lock(&dev->struct_mutex);
> +
> info = framebuffer_alloc(0, &dev->pdev->dev);
> if (!info) {
> ret = -ENOMEM;
> @@ -307,7 +313,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> out_unpin:
> i915_gem_object_ggtt_unpin(obj);
> drm_gem_object_unreference(&obj->base);
> -out_unlock:
> mutex_unlock(&dev->struct_mutex);
> return ret;
> }
> --
> 2.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] drm/i915: Fix failure paths around initial fbdev allocation
2015-06-30 9:06 [PATCH] drm/i915: Fix failure paths around initial fbdev allocation Tvrtko Ursulin
2015-06-30 9:13 ` shuang.he
2015-06-30 10:23 ` Jani Nikula
@ 2015-07-02 13:23 ` Lukas Wunner
2015-07-02 13:37 ` Ville Syrjälä
3 siblings, 0 replies; 17+ messages in thread
From: Lukas Wunner @ 2015-07-02 13:23 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
Hi,
not sure if another reaction from me is required, but in case there is:
The patch as quoted below LGTM now, so unless Ville vetoes because of
his suggestion (see below) I think this is ready for inclusion in 4.2.
Thanks,
Lukas
On Tue, Jun 30, 2015 at 10:06:27AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
> Reported-By: Lukas Wunner <lukas@wunner.de>
> Tested-By: Lukas Wunner <lukas@wunner.de>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lukas Wunner <lukas@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?
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 2a1724e..118420b 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -141,7 +141,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_mode_fb_cmd2 mode_cmd = {};
> struct drm_i915_gem_object *obj;
> @@ -159,6 +159,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);
> obj = i915_gem_object_create_stolen(dev, size);
> @@ -172,8 +174,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>
> fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
> if (IS_ERR(fb)) {
> + /* Drops object reference on failure. */
> ret = PTR_ERR(fb);
> - goto out_unref;
> + goto out;
> }
>
> /* Flush everything out, we'll be doing GTT only from now on */
> @@ -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);
> return ret;
> }
>
> @@ -209,8 +215,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)) {
> @@ -225,7 +229,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");
> @@ -237,6 +241,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
> obj = intel_fb->obj;
> size = obj->base.size;
>
> + mutex_lock(&dev->struct_mutex);
> +
> info = framebuffer_alloc(0, &dev->pdev->dev);
> if (!info) {
> ret = -ENOMEM;
> @@ -307,7 +313,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> out_unpin:
> i915_gem_object_ggtt_unpin(obj);
> drm_gem_object_unreference(&obj->base);
> -out_unlock:
> mutex_unlock(&dev->struct_mutex);
> return ret;
> }
> --
> 2.4.2
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] drm/i915: Fix failure paths around initial fbdev allocation
2015-06-30 9:06 [PATCH] drm/i915: Fix failure paths around initial fbdev allocation Tvrtko Ursulin
` (2 preceding siblings ...)
2015-07-02 13:23 ` Lukas Wunner
@ 2015-07-02 13:37 ` Ville Syrjälä
2015-07-02 13:59 ` Tvrtko Ursulin
3 siblings, 1 reply; 17+ messages in thread
From: Ville Syrjälä @ 2015-07-02 13:37 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
On Tue, Jun 30, 2015 at 10:06:27AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com>
> Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
> Reported-By: Lukas Wunner <lukas@wunner.de>
> Tested-By: Lukas Wunner <lukas@wunner.de>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lukas Wunner <lukas@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.
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 2a1724e..118420b 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -141,7 +141,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_mode_fb_cmd2 mode_cmd = {};
> struct drm_i915_gem_object *obj;
> @@ -159,6 +159,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);
> obj = i915_gem_object_create_stolen(dev, size);
> @@ -172,8 +174,9 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>
> fb = __intel_framebuffer_create(dev, &mode_cmd, obj);
> if (IS_ERR(fb)) {
> + /* Drops object reference on failure. */
> ret = PTR_ERR(fb);
> - goto out_unref;
> + goto out;
> }
>
> /* Flush everything out, we'll be doing GTT only from now on */
> @@ -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);
> return ret;
> }
>
> @@ -209,8 +215,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)) {
> @@ -225,7 +229,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");
> @@ -237,6 +241,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
> obj = intel_fb->obj;
> size = obj->base.size;
>
> + mutex_lock(&dev->struct_mutex);
> +
> info = framebuffer_alloc(0, &dev->pdev->dev);
> if (!info) {
> ret = -ENOMEM;
> @@ -307,7 +313,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
> out_unpin:
> i915_gem_object_ggtt_unpin(obj);
> drm_gem_object_unreference(&obj->base);
> -out_unlock:
> mutex_unlock(&dev->struct_mutex);
> return ret;
> }
> --
> 2.4.2
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] drm/i915: Fix failure paths around initial fbdev allocation
2015-07-02 13:37 ` Ville Syrjälä
@ 2015-07-02 13:59 ` Tvrtko Ursulin
2015-06-30 9:06 ` [PATCH v3 1/2] " Lukas Wunner
2015-07-04 12:48 ` [PATCH] drm/i915: Fix failure paths around initial fbdev allocation Lukas Wunner
0 siblings, 2 replies; 17+ messages in thread
From: Tvrtko Ursulin @ 2015-07-02 13:59 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Intel-gfx
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@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@intel.com>
>> Fixes: 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
>> Reported-By: Lukas Wunner <lukas@wunner.de>
>> Tested-By: Lukas Wunner <lukas@wunner.de>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Lukas Wunner <lukas@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/2] drm/i915: Fix failure paths around initial fbdev allocation
2015-07-02 13:59 ` Tvrtko Ursulin
@ 2015-06-30 9:06 ` Lukas Wunner
2015-07-04 9:50 ` [PATCH v3 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed Lukas Wunner
2015-07-04 12:48 ` [PATCH] drm/i915: Fix failure paths around initial fbdev allocation Lukas Wunner
1 sibling, 1 reply; 17+ messages in thread
From: Lukas Wunner @ 2015-06-30 9:06 UTC (permalink / raw)
To: intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@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.
v2:
* Reformat commit msg to 72 chars. (Lukas Wunner)
* Add third failure mode. (Lukas Wunner)
v3:
* On fb alloc failure, unref gem object where it gets refed,
fix double unref in separate commit. (Ville Syrjälä)
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: 60a5ca015ffd ("drm/i915: Add locking around
framebuffer_references--")
Reported-By: Lukas Wunner <lukas@wunner.de>
Tested-By: Lukas Wunner <lukas@wunner.de>
[Lukas: Create v3 based on Tvrtko's v2]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@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 4e7e7da..bce2f73 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -115,7 +115,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_mode_fb_cmd2 mode_cmd = {};
struct drm_i915_gem_object *obj;
@@ -133,6 +133,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);
obj = i915_gem_object_create_stolen(dev, size);
@@ -154,18 +156,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL);
if (ret) {
DRM_ERROR("failed to pin obj: %d\n", ret);
- goto out_fb;
+ goto out_unref;
}
+ 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);
return ret;
}
@@ -183,8 +188,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)) {
@@ -199,7 +202,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");
@@ -211,6 +214,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
obj = intel_fb->obj;
size = obj->base.size;
+ mutex_lock(&dev->struct_mutex);
+
info = framebuffer_alloc(0, &dev->pdev->dev);
if (!info) {
ret = -ENOMEM;
@@ -281,7 +286,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
out_unpin:
i915_gem_object_ggtt_unpin(obj);
drm_gem_object_unreference(&obj->base);
-out_unlock:
mutex_unlock(&dev->struct_mutex);
return ret;
}
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v3 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed
2015-06-30 9:06 ` [PATCH v3 1/2] " Lukas Wunner
@ 2015-07-04 9:50 ` Lukas Wunner
2015-07-04 12:31 ` Chris Wilson
0 siblings, 1 reply; 17+ messages in thread
From: Lukas Wunner @ 2015-07-04 9:50 UTC (permalink / raw)
To: intel-gfx
Currently when allocating a framebuffer fails, the gem object gets
unrefed at the bottom of the call chain in __intel_framebuffer_create,
not where it gets refed, which is in intel_framebuffer_create_for_mode
(via i915_gem_alloc_object) and in intel_user_framebuffer_create
(via drm_gem_object_lookup).
This invites mistakes: As discovered by Tvrtko Ursulin, a double unref
has sneaked into intelfb_alloc (which calls __intel_framebuffer_create).
As suggested by Ville Syrjälä, improve code clarity by moving the unref
away from __intel_framebuffer_create to where the gem object gets refed.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
allocation")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9079fcd..9499003 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8876,20 +8876,17 @@ __intel_framebuffer_create(struct drm_device *dev,
int ret;
intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
- if (!intel_fb) {
- drm_gem_object_unreference(&obj->base);
+ if (!intel_fb)
return ERR_PTR(-ENOMEM);
- }
ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
if (ret)
goto err;
return &intel_fb->base;
+
err:
- drm_gem_object_unreference(&obj->base);
kfree(intel_fb);
-
return ERR_PTR(ret);
}
@@ -8929,6 +8926,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
struct drm_display_mode *mode,
int depth, int bpp)
{
+ struct drm_framebuffer *fb;
struct drm_i915_gem_object *obj;
struct drm_mode_fb_cmd2 mode_cmd = { 0 };
@@ -8943,7 +8941,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
bpp);
mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
- return intel_framebuffer_create(dev, &mode_cmd, obj);
+ fb = intel_framebuffer_create(dev, &mode_cmd, obj);
+ if (IS_ERR(fb))
+ drm_gem_object_unreference(&obj->base);
+
+ return fb;
}
static struct drm_framebuffer *
@@ -13379,6 +13381,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
struct drm_file *filp,
struct drm_mode_fb_cmd2 *mode_cmd)
{
+ struct drm_framebuffer *fb;
struct drm_i915_gem_object *obj;
obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
@@ -13386,7 +13389,11 @@ intel_user_framebuffer_create(struct drm_device *dev,
if (&obj->base == NULL)
return ERR_PTR(-ENOENT);
- return intel_framebuffer_create(dev, mode_cmd, obj);
+ fb = intel_framebuffer_create(dev, mode_cmd, obj);
+ if (IS_ERR(fb))
+ drm_gem_object_unreference(&obj->base);
+
+ return fb;
}
static void intel_output_poll_changed(struct drm_device *dev)
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v3 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed
2015-07-04 9:50 ` [PATCH v3 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed Lukas Wunner
@ 2015-07-04 12:31 ` Chris Wilson
2015-06-30 9:06 ` [PATCH v4 1/2] drm/i915: Fix failure paths around initial fbdev allocation Lukas Wunner
2015-07-05 16:49 ` [PATCH v3 " Lukas Wunner
0 siblings, 2 replies; 17+ messages in thread
From: Chris Wilson @ 2015-07-04 12:31 UTC (permalink / raw)
To: Lukas Wunner; +Cc: intel-gfx
On Sat, Jul 04, 2015 at 11:50:58AM +0200, Lukas Wunner wrote:
> Currently when allocating a framebuffer fails, the gem object gets
> unrefed at the bottom of the call chain in __intel_framebuffer_create,
> not where it gets refed, which is in intel_framebuffer_create_for_mode
> (via i915_gem_alloc_object) and in intel_user_framebuffer_create
> (via drm_gem_object_lookup).
>
> This invites mistakes: As discovered by Tvrtko Ursulin, a double unref
> has sneaked into intelfb_alloc (which calls __intel_framebuffer_create).
>
> As suggested by Ville Syrjälä, improve code clarity by moving the unref
> away from __intel_framebuffer_create to where the gem object gets refed.
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
> allocation")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9079fcd..9499003 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8876,20 +8876,17 @@ __intel_framebuffer_create(struct drm_device *dev,
> int ret;
>
> intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
> - if (!intel_fb) {
> - drm_gem_object_unreference(&obj->base);
> + if (!intel_fb)
> return ERR_PTR(-ENOMEM);
> - }
>
> ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
> if (ret)
> goto err;
>
> return &intel_fb->base;
> +
> err:
> - drm_gem_object_unreference(&obj->base);
> kfree(intel_fb);
> -
> return ERR_PTR(ret);
> }
>
> @@ -8929,6 +8926,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
> struct drm_display_mode *mode,
> int depth, int bpp)
> {
> + struct drm_framebuffer *fb;
> struct drm_i915_gem_object *obj;
> struct drm_mode_fb_cmd2 mode_cmd = { 0 };
>
> @@ -8943,7 +8941,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
> bpp);
> mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
>
> - return intel_framebuffer_create(dev, &mode_cmd, obj);
> + fb = intel_framebuffer_create(dev, &mode_cmd, obj);
> + if (IS_ERR(fb))
> + drm_gem_object_unreference(&obj->base);
This needs to be drm_gem_object_unreference_unlocked(). It's much
simpler if you just document this as consuming the obj reference. If you
want to fix it, you have to move the struct_mutex into the caller i.e.
eliminate intel_framebuffer_create() and call
__intel_framebuffer_create().
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH v4 1/2] drm/i915: Fix failure paths around initial fbdev allocation
2015-07-04 12:31 ` Chris Wilson
@ 2015-06-30 9:06 ` Lukas Wunner
2015-07-04 9:50 ` [PATCH v4 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed Lukas Wunner
2015-07-05 16:49 ` [PATCH v3 " Lukas Wunner
1 sibling, 1 reply; 17+ messages in thread
From: Lukas Wunner @ 2015-06-30 9:06 UTC (permalink / raw)
To: intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@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.
v2:
* Reformat commit msg to 72 chars. (Lukas Wunner)
* Add third failure mode. (Lukas Wunner)
v3:
* On fb alloc failure, unref gem object where it gets refed,
fix double unref in separate commit. (Ville Syrjälä)
v4:
* Lock struct mutex on unref. (Chris Wilson)
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: 60a5ca015ffd ("drm/i915: Add locking around
framebuffer_references--")
Reported-By: Lukas Wunner <lukas@wunner.de>
Tested-By: Lukas Wunner <lukas@wunner.de>
[Lukas: Create v3 + v4 based on Tvrtko's v2]
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
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 4e7e7da..bce2f73 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -115,7 +115,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_mode_fb_cmd2 mode_cmd = {};
struct drm_i915_gem_object *obj;
@@ -133,6 +133,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);
obj = i915_gem_object_create_stolen(dev, size);
@@ -154,18 +156,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL);
if (ret) {
DRM_ERROR("failed to pin obj: %d\n", ret);
- goto out_fb;
+ goto out_unref;
}
+ 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);
return ret;
}
@@ -183,8 +188,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)) {
@@ -199,7 +202,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");
@@ -211,6 +214,8 @@ static int intelfb_create(struct drm_fb_helper *helper,
obj = intel_fb->obj;
size = obj->base.size;
+ mutex_lock(&dev->struct_mutex);
+
info = framebuffer_alloc(0, &dev->pdev->dev);
if (!info) {
ret = -ENOMEM;
@@ -281,7 +286,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
out_unpin:
i915_gem_object_ggtt_unpin(obj);
drm_gem_object_unreference(&obj->base);
-out_unlock:
mutex_unlock(&dev->struct_mutex);
return ret;
}
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v4 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed
2015-06-30 9:06 ` [PATCH v4 1/2] drm/i915: Fix failure paths around initial fbdev allocation Lukas Wunner
@ 2015-07-04 9:50 ` Lukas Wunner
2015-07-06 7:41 ` Daniel Vetter
0 siblings, 1 reply; 17+ messages in thread
From: Lukas Wunner @ 2015-07-04 9:50 UTC (permalink / raw)
To: intel-gfx
Currently when allocating a framebuffer fails, the gem object gets
unrefed at the bottom of the call chain in __intel_framebuffer_create,
not where it gets refed, which is in intel_framebuffer_create_for_mode
(via i915_gem_alloc_object) and in intel_user_framebuffer_create
(via drm_gem_object_lookup).
This invites mistakes: As discovered by Tvrtko Ursulin, a double unref
has sneaked into intelfb_alloc (which calls __intel_framebuffer_create).
As suggested by Ville Syrjälä, improve code clarity by moving the unref
away from __intel_framebuffer_create to where the gem object gets refed.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
allocation")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9079fcd..d597afa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8876,20 +8876,17 @@ __intel_framebuffer_create(struct drm_device *dev,
int ret;
intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
- if (!intel_fb) {
- drm_gem_object_unreference(&obj->base);
+ if (!intel_fb)
return ERR_PTR(-ENOMEM);
- }
ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
if (ret)
goto err;
return &intel_fb->base;
+
err:
- drm_gem_object_unreference(&obj->base);
kfree(intel_fb);
-
return ERR_PTR(ret);
}
@@ -8929,6 +8926,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
struct drm_display_mode *mode,
int depth, int bpp)
{
+ struct drm_framebuffer *fb;
struct drm_i915_gem_object *obj;
struct drm_mode_fb_cmd2 mode_cmd = { 0 };
@@ -8943,7 +8941,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
bpp);
mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
- return intel_framebuffer_create(dev, &mode_cmd, obj);
+ fb = intel_framebuffer_create(dev, &mode_cmd, obj);
+ if (IS_ERR(fb))
+ drm_gem_object_unreference_unlocked(&obj->base);
+
+ return fb;
}
static struct drm_framebuffer *
@@ -13379,6 +13381,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
struct drm_file *filp,
struct drm_mode_fb_cmd2 *mode_cmd)
{
+ struct drm_framebuffer *fb;
struct drm_i915_gem_object *obj;
obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
@@ -13386,7 +13389,11 @@ intel_user_framebuffer_create(struct drm_device *dev,
if (&obj->base == NULL)
return ERR_PTR(-ENOENT);
- return intel_framebuffer_create(dev, mode_cmd, obj);
+ fb = intel_framebuffer_create(dev, mode_cmd, obj);
+ if (IS_ERR(fb))
+ drm_gem_object_unreference_unlocked(&obj->base);
+
+ return fb;
}
static void intel_output_poll_changed(struct drm_device *dev)
--
2.1.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v4 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed
2015-07-04 9:50 ` [PATCH v4 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed Lukas Wunner
@ 2015-07-06 7:41 ` Daniel Vetter
2015-07-06 12:59 ` Lukas Wunner
0 siblings, 1 reply; 17+ messages in thread
From: Daniel Vetter @ 2015-07-06 7:41 UTC (permalink / raw)
To: Lukas Wunner; +Cc: intel-gfx
On Sat, Jul 04, 2015 at 11:50:58AM +0200, Lukas Wunner wrote:
> Currently when allocating a framebuffer fails, the gem object gets
> unrefed at the bottom of the call chain in __intel_framebuffer_create,
> not where it gets refed, which is in intel_framebuffer_create_for_mode
> (via i915_gem_alloc_object) and in intel_user_framebuffer_create
> (via drm_gem_object_lookup).
>
> This invites mistakes: As discovered by Tvrtko Ursulin, a double unref
> has sneaked into intelfb_alloc (which calls __intel_framebuffer_create).
>
> As suggested by Ville Syrjälä, improve code clarity by moving the unref
> away from __intel_framebuffer_create to where the gem object gets refed.
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Fixes: a8bb6818270c ("drm/i915: Fix error path leak in fbdev fb
> allocation")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
Please keep a record of the changes you do to the patch so I know what to
look out for. Just reving the patch revision alone doesn't add much
information for reviewers/maintainers.
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9079fcd..d597afa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8876,20 +8876,17 @@ __intel_framebuffer_create(struct drm_device *dev,
> int ret;
>
> intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
> - if (!intel_fb) {
> - drm_gem_object_unreference(&obj->base);
> + if (!intel_fb)
> return ERR_PTR(-ENOMEM);
> - }
>
> ret = intel_framebuffer_init(dev, intel_fb, mode_cmd, obj);
> if (ret)
> goto err;
>
> return &intel_fb->base;
> +
> err:
> - drm_gem_object_unreference(&obj->base);
> kfree(intel_fb);
> -
> return ERR_PTR(ret);
> }
>
> @@ -8929,6 +8926,7 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
> struct drm_display_mode *mode,
> int depth, int bpp)
> {
> + struct drm_framebuffer *fb;
> struct drm_i915_gem_object *obj;
> struct drm_mode_fb_cmd2 mode_cmd = { 0 };
>
> @@ -8943,7 +8941,11 @@ intel_framebuffer_create_for_mode(struct drm_device *dev,
> bpp);
> mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth);
>
> - return intel_framebuffer_create(dev, &mode_cmd, obj);
> + fb = intel_framebuffer_create(dev, &mode_cmd, obj);
> + if (IS_ERR(fb))
> + drm_gem_object_unreference_unlocked(&obj->base);
> +
> + return fb;
> }
>
> static struct drm_framebuffer *
> @@ -13379,6 +13381,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
> struct drm_file *filp,
> struct drm_mode_fb_cmd2 *mode_cmd)
> {
> + struct drm_framebuffer *fb;
> struct drm_i915_gem_object *obj;
>
> obj = to_intel_bo(drm_gem_object_lookup(dev, filp,
> @@ -13386,7 +13389,11 @@ intel_user_framebuffer_create(struct drm_device *dev,
> if (&obj->base == NULL)
> return ERR_PTR(-ENOENT);
>
> - return intel_framebuffer_create(dev, mode_cmd, obj);
> + fb = intel_framebuffer_create(dev, mode_cmd, obj);
> + if (IS_ERR(fb))
> + drm_gem_object_unreference_unlocked(&obj->base);
> +
> + return fb;
> }
>
> static void intel_output_poll_changed(struct drm_device *dev)
> --
> 2.1.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed
2015-07-06 7:41 ` Daniel Vetter
@ 2015-07-06 12:59 ` Lukas Wunner
2015-07-06 13:55 ` Daniel Vetter
0 siblings, 1 reply; 17+ messages in thread
From: Lukas Wunner @ 2015-07-06 12:59 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
Hi Daniel,
On Mon, Jul 06, 2015 at 09:41:51AM +0200, Daniel Vetter wrote:
> Please keep a record of the changes you do to the patch so I know what to
> look out for. Just reving the patch revision alone doesn't add much
> information for reviewers/maintainers.
There's a changelog in the first patch of this 2 patch series
(subject "[PATCH v4 1/2] drm/i915: Fix failure paths around
initial fbdev allocation"), it says:
"v4:
* Lock struct mutex on unref. (Chris Wilson)"
Best regards,
Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v4 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed
2015-07-06 12:59 ` Lukas Wunner
@ 2015-07-06 13:55 ` Daniel Vetter
0 siblings, 0 replies; 17+ messages in thread
From: Daniel Vetter @ 2015-07-06 13:55 UTC (permalink / raw)
To: Lukas Wunner; +Cc: intel-gfx
On Mon, Jul 06, 2015 at 02:59:02PM +0200, Lukas Wunner wrote:
> Hi Daniel,
>
> On Mon, Jul 06, 2015 at 09:41:51AM +0200, Daniel Vetter wrote:
> > Please keep a record of the changes you do to the patch so I know what to
> > look out for. Just reving the patch revision alone doesn't add much
> > information for reviewers/maintainers.
>
> There's a changelog in the first patch of this 2 patch series
> (subject "[PATCH v4 1/2] drm/i915: Fix failure paths around
> initial fbdev allocation"), it says:
>
> "v4:
> * Lock struct mutex on unref. (Chris Wilson)"
Ah, I was looking for the v4 changelog for patch 2. Either make a small
v4: rebased note or just don't call patch 2 v4 to avoid confusion in the
future.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed
2015-07-04 12:31 ` Chris Wilson
2015-06-30 9:06 ` [PATCH v4 1/2] drm/i915: Fix failure paths around initial fbdev allocation Lukas Wunner
@ 2015-07-05 16:49 ` Lukas Wunner
2015-07-05 17:31 ` Chris Wilson
1 sibling, 1 reply; 17+ messages in thread
From: Lukas Wunner @ 2015-07-05 16:49 UTC (permalink / raw)
To: Chris Wilson, intel-gfx, Tvrtko Ursulin, Ville Syrjälä,
Daniel Vetter
Hi Chris,
thank you for the quick response (on a weekend no less).
On Sat, Jul 04, 2015 at 01:31:48PM +0100, Chris Wilson wrote:
> > - return intel_framebuffer_create(dev, &mode_cmd, obj);
> > + fb = intel_framebuffer_create(dev, &mode_cmd, obj);
> > + if (IS_ERR(fb))
> > + drm_gem_object_unreference(&obj->base);
>
> This needs to be drm_gem_object_unreference_unlocked().
You're absolutely right, thanks for pointing this out.
I'm posting a rectified v4 right now.
> It's much simpler if you just document this as consuming the
> obj reference.
Yes but I believe that is what Ville took exception to.
If you guys all agree that documenting this is sufficient then
you can just merge Tvrtko's v2. The rationale of the v3 + v4
I've submitted is to offer an alternative in the hope of pushing
this forward.
> If you want to fix it, you have to move the struct_mutex into
> the caller i.e. eliminate intel_framebuffer_create() and call
> __intel_framebuffer_create().
Hm, I don't understand. The (locking) intel_framebuffer_create
is used by intel_framebuffer_create_for_mode as well as
intel_user_framebuffer_create.
The (non-locking) __intel_framebuffer_create is used by
intelfb_alloc. So it seems both are needed. Daniel added
__intel_framebuffer_create with a8bb6818270c ("drm/i915:
Fix error path leak in fbdev fb allocation"). Incidentally
this is also the commit that introduced the double unref. :-)
We could eliminate the (non-locking) __intel_framebuffer_create
however by briefly unlocking struct_mutex in intelfb_alloc after
i915_gem_alloc_object and then relocking before
intel_pin_and_fence_fb_obj (this is on top of Tvrtko's patch
which moves the locking from intelfb_create to intelfb_alloc).
Best regards,
Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed
2015-07-05 16:49 ` [PATCH v3 " Lukas Wunner
@ 2015-07-05 17:31 ` Chris Wilson
0 siblings, 0 replies; 17+ messages in thread
From: Chris Wilson @ 2015-07-05 17:31 UTC (permalink / raw)
To: Lukas Wunner; +Cc: intel-gfx
On Sun, Jul 05, 2015 at 06:49:02PM +0200, Lukas Wunner wrote:
> Hi Chris,
>
> thank you for the quick response (on a weekend no less).
>
>
> On Sat, Jul 04, 2015 at 01:31:48PM +0100, Chris Wilson wrote:
> > > - return intel_framebuffer_create(dev, &mode_cmd, obj);
> > > + fb = intel_framebuffer_create(dev, &mode_cmd, obj);
> > > + if (IS_ERR(fb))
> > > + drm_gem_object_unreference(&obj->base);
> >
> > This needs to be drm_gem_object_unreference_unlocked().
>
> You're absolutely right, thanks for pointing this out.
> I'm posting a rectified v4 right now.
>
>
> > It's much simpler if you just document this as consuming the
> > obj reference.
>
> Yes but I believe that is what Ville took exception to.
>
> If you guys all agree that documenting this is sufficient then
> you can just merge Tvrtko's v2. The rationale of the v3 + v4
> I've submitted is to offer an alternative in the hope of pushing
> this forward.
>
>
> > If you want to fix it, you have to move the struct_mutex into
> > the caller i.e. eliminate intel_framebuffer_create() and call
> > __intel_framebuffer_create().
>
> Hm, I don't understand. The (locking) intel_framebuffer_create
> is used by intel_framebuffer_create_for_mode as well as
> intel_user_framebuffer_create.
>
> The (non-locking) __intel_framebuffer_create is used by
> intelfb_alloc. So it seems both are needed. Daniel added
> __intel_framebuffer_create with a8bb6818270c ("drm/i915:
> Fix error path leak in fbdev fb allocation"). Incidentally
> this is also the commit that introduced the double unref. :-)
>
> We could eliminate the (non-locking) __intel_framebuffer_create
> however by briefly unlocking struct_mutex in intelfb_alloc after
> i915_gem_alloc_object and then relocking before
> intel_pin_and_fence_fb_obj (this is on top of Tvrtko's patch
> which moves the locking from intelfb_create to intelfb_alloc).
I mean, there will only be the non-locking variant, the mutex is moved
to the caller.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] drm/i915: Fix failure paths around initial fbdev allocation
2015-07-02 13:59 ` Tvrtko Ursulin
2015-06-30 9:06 ` [PATCH v3 1/2] " Lukas Wunner
@ 2015-07-04 12:48 ` Lukas Wunner
1 sibling, 0 replies; 17+ messages in thread
From: Lukas Wunner @ 2015-07-04 12:48 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Intel-gfx
Hi,
On Thu, Jul 02, 2015 at 02:59:24PM +0100, Tvrtko Ursulin wrote:
> On 07/02/2015 02:37 PM, Ville Syrjälä wrote:
> >On Tue, Jun 30, 2015 at 10:06:27AM +0100, Tvrtko Ursulin wrote:
> >>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.
I think Ville's suggestion is merited, right now the unref on failure
happens at the bottom of the call chain in __intel_framebuffer_create
while the ref happened further up in the call chain. This should really
be consolidated in the same place. That a double unref managed to sneak
into intelfb_alloc proves this is error prone.
I've just submitted a v3 which is functionally equivalent to Tvrtko's v2
but takes up Ville's suggestion. This consists of two patches now, the
first one being equivalent to Tvrtko's patch sans fix for the double unref,
the second one fixing the double unref by moving the unref further up the
call chain. Of course I've tested this, works for me.
Now you can decide for yourself which version you like better. :-)
Thanks,
Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 17+ messages in thread