Deadlock in intel_user_framebuffer_destroy()
diff mbox

Message ID 20150615075302.GR28462@nuc-i3427.alporthouse.com
State New
Headers show

Commit Message

Chris Wilson June 15, 2015, 7:53 a.m. UTC
On Mon, Jun 15, 2015 at 09:44:15AM +0300, Jani Nikula wrote:
> On Wed, 03 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Wed, Jun 03, 2015 at 03:43:32PM +0200, Lukas Wunner wrote:
> >> Hi,
> >> 
> >> a deadlock was introduced by commit 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
> >> 
> >> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Date:   Fri Jun 13 11:10:53 2014 +0300
> >> 
> >>     drm/i915: Add locking around framebuffer_references--
> >> 
> >> 
> >> The commit amended intel_display.c:intel_user_framebuffer_destroy() with
> >> mutex_lock(&dev->struct_mutex).
> >> 
> >> A few weeks prior Chris Wilson had amended intel_fbdev.c:intelfb_create()
> >> with a call to drm_framebuffer_unreference() while &dev->struct_mutex is
> >> locked (commit edd586fe705e819bc711b5ed7194a0b6f9f1a7e1, "drm/i915: Discard
> >> BIOS framebuffers too small to accommodate chosen mode").
> >> 
> >
> > Just move the mutex_lock down a step.
> 
> Lucas, did you try this?

There's a goto unlock that also needed to be disabled, such as

Comments

Tvrtko Ursulin June 15, 2015, 9:25 a.m. UTC | #1
Hi,

On 06/15/2015 08:53 AM, Chris Wilson wrote:
> On Mon, Jun 15, 2015 at 09:44:15AM +0300, Jani Nikula wrote:
>> On Wed, 03 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>>> On Wed, Jun 03, 2015 at 03:43:32PM +0200, Lukas Wunner wrote:
>>>> Hi,
>>>>
>>>> a deadlock was introduced by commit 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
>>>>
>>>> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Date:   Fri Jun 13 11:10:53 2014 +0300
>>>>
>>>>      drm/i915: Add locking around framebuffer_references--
>>>>
>>>>
>>>> The commit amended intel_display.c:intel_user_framebuffer_destroy() with
>>>> mutex_lock(&dev->struct_mutex).
>>>>
>>>> A few weeks prior Chris Wilson had amended intel_fbdev.c:intelfb_create()
>>>> with a call to drm_framebuffer_unreference() while &dev->struct_mutex is
>>>> locked (commit edd586fe705e819bc711b5ed7194a0b6f9f1a7e1, "drm/i915: Discard
>>>> BIOS framebuffers too small to accommodate chosen mode").
>>>>
>>>
>>> Just move the mutex_lock down a step.
>>
>> Lucas, did you try this?
>
> There's a goto unlock that also needed to be disabled, such as
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index dda99c0d6be1..fc7ec5138fb7 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -213,8 +213,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>          bool prealloc = false;
>          int ret;
>
> -       mutex_lock(&dev->struct_mutex);
> -
>          if (intel_fb &&
>              (sizes->fb_width > intel_fb->base.width ||
>               sizes->fb_height > intel_fb->base.height)) {
> @@ -229,7 +227,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");
> @@ -241,6 +239,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>          obj = intel_fb->obj;
>          vma = i915_gem_obj_to_ggtt(obj, NULL);
>
> +       mutex_lock(&dev->struct_mutex);
>          info = framebuffer_alloc(0, &dev->pdev->dev);
>          if (!info) {
>                  ret = -ENOMEM;
> @@ -311,7 +310,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   out_unpin:
>          drm_gem_object_unreference(&obj->base);
> -out_unlock:
>          mutex_unlock(&dev->struct_mutex);
>          return ret;
>   }
>

intelfb_alloc wants struct_mutex, both for __intel_framebuffer_create 
and pin_and_fence. And also there is that double obj unreference in the 
failure path from the former Jani spotted.

Regards,

Tvrtko
Daniel Vetter June 15, 2015, 12:02 p.m. UTC | #2
On Mon, Jun 15, 2015 at 08:53:02AM +0100, Chris Wilson wrote:
> On Mon, Jun 15, 2015 at 09:44:15AM +0300, Jani Nikula wrote:
> > On Wed, 03 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Wed, Jun 03, 2015 at 03:43:32PM +0200, Lukas Wunner wrote:
> > >> Hi,
> > >> 
> > >> a deadlock was introduced by commit 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
> > >> 
> > >> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> Date:   Fri Jun 13 11:10:53 2014 +0300
> > >> 
> > >>     drm/i915: Add locking around framebuffer_references--
> > >> 
> > >> 
> > >> The commit amended intel_display.c:intel_user_framebuffer_destroy() with
> > >> mutex_lock(&dev->struct_mutex).
> > >> 
> > >> A few weeks prior Chris Wilson had amended intel_fbdev.c:intelfb_create()
> > >> with a call to drm_framebuffer_unreference() while &dev->struct_mutex is
> > >> locked (commit edd586fe705e819bc711b5ed7194a0b6f9f1a7e1, "drm/i915: Discard
> > >> BIOS framebuffers too small to accommodate chosen mode").
> > >> 
> > >
> > > Just move the mutex_lock down a step.
> > 
> > Lucas, did you try this?
> 
> There's a goto unlock that also needed to be disabled, such as

Your previous patch placed the mutex_lock before the goto out_unlock - I
fail to see what has been broken with that version? Can you resubmit that
as a proper patch with sob and Lucas' t-b?

Thanks, Daniel

> 
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index dda99c0d6be1..fc7ec5138fb7 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -213,8 +213,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>         bool prealloc = false;
>         int ret;
>  
> -       mutex_lock(&dev->struct_mutex);
> -
>         if (intel_fb &&
>             (sizes->fb_width > intel_fb->base.width ||
>              sizes->fb_height > intel_fb->base.height)) {
> @@ -229,7 +227,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");
> @@ -241,6 +239,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>         obj = intel_fb->obj;
>         vma = i915_gem_obj_to_ggtt(obj, NULL);
>  
> +       mutex_lock(&dev->struct_mutex);
>         info = framebuffer_alloc(0, &dev->pdev->dev);
>         if (!info) {
>                 ret = -ENOMEM;
> @@ -311,7 +310,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  out_unpin:
>         drm_gem_object_unreference(&obj->base);
> -out_unlock:
>         mutex_unlock(&dev->struct_mutex);
>         return ret;
>  }
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson June 15, 2015, 12:07 p.m. UTC | #3
On Mon, Jun 15, 2015 at 02:02:23PM +0200, Daniel Vetter wrote:
> On Mon, Jun 15, 2015 at 08:53:02AM +0100, Chris Wilson wrote:
> > On Mon, Jun 15, 2015 at 09:44:15AM +0300, Jani Nikula wrote:
> > > On Wed, 03 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > On Wed, Jun 03, 2015 at 03:43:32PM +0200, Lukas Wunner wrote:
> > > >> Hi,
> > > >> 
> > > >> a deadlock was introduced by commit 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
> > > >> 
> > > >> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >> Date:   Fri Jun 13 11:10:53 2014 +0300
> > > >> 
> > > >>     drm/i915: Add locking around framebuffer_references--
> > > >> 
> > > >> 
> > > >> The commit amended intel_display.c:intel_user_framebuffer_destroy() with
> > > >> mutex_lock(&dev->struct_mutex).
> > > >> 
> > > >> A few weeks prior Chris Wilson had amended intel_fbdev.c:intelfb_create()
> > > >> with a call to drm_framebuffer_unreference() while &dev->struct_mutex is
> > > >> locked (commit edd586fe705e819bc711b5ed7194a0b6f9f1a7e1, "drm/i915: Discard
> > > >> BIOS framebuffers too small to accommodate chosen mode").
> > > >> 
> > > >
> > > > Just move the mutex_lock down a step.
> > > 
> > > Lucas, did you try this?
> > 
> > There's a goto unlock that also needed to be disabled, such as
> 
> Your previous patch placed the mutex_lock before the goto out_unlock - I
> fail to see what has been broken with that version? Can you resubmit that
> as a proper patch with sob and Lucas' t-b?

There wasn't, I just rewrote it incorrectly. There's also a
drm_framebuffer_remove() called by intelfb_alloc which needs to be moved
out of the mutex. A much larger disentangling of the functions involved
here is required. Tvrstko volunteered himself. Brave, very brave :)
-Chris
Jani Nikula June 29, 2015, 11:24 a.m. UTC | #4
On Mon, 15 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Jun 15, 2015 at 02:02:23PM +0200, Daniel Vetter wrote:
>> On Mon, Jun 15, 2015 at 08:53:02AM +0100, Chris Wilson wrote:
>> > On Mon, Jun 15, 2015 at 09:44:15AM +0300, Jani Nikula wrote:
>> > > On Wed, 03 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > > > On Wed, Jun 03, 2015 at 03:43:32PM +0200, Lukas Wunner wrote:
>> > > >> Hi,
>> > > >> 
>> > > >> a deadlock was introduced by commit 60a5ca015ffd2aacfe5674b5a401cd2a37159e07
>> > > >> 
>> > > >> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > >> Date:   Fri Jun 13 11:10:53 2014 +0300
>> > > >> 
>> > > >>     drm/i915: Add locking around framebuffer_references--
>> > > >> 
>> > > >> 
>> > > >> The commit amended intel_display.c:intel_user_framebuffer_destroy() with
>> > > >> mutex_lock(&dev->struct_mutex).
>> > > >> 
>> > > >> A few weeks prior Chris Wilson had amended intel_fbdev.c:intelfb_create()
>> > > >> with a call to drm_framebuffer_unreference() while &dev->struct_mutex is
>> > > >> locked (commit edd586fe705e819bc711b5ed7194a0b6f9f1a7e1, "drm/i915: Discard
>> > > >> BIOS framebuffers too small to accommodate chosen mode").
>> > > >> 
>> > > >
>> > > > Just move the mutex_lock down a step.
>> > > 
>> > > Lucas, did you try this?
>> > 
>> > There's a goto unlock that also needed to be disabled, such as
>> 
>> Your previous patch placed the mutex_lock before the goto out_unlock - I
>> fail to see what has been broken with that version? Can you resubmit that
>> as a proper patch with sob and Lucas' t-b?
>
> There wasn't, I just rewrote it incorrectly. There's also a
> drm_framebuffer_remove() called by intelfb_alloc which needs to be moved
> out of the mutex. A much larger disentangling of the functions involved
> here is required. Tvrstko volunteered himself. Brave, very brave :)

-ETIMEDOUT

IIUC we still need the fix, but there's no update from Lucas addressing
review. Is that right?

BR,
Jani.


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
Lukas Wunner June 29, 2015, 2:42 p.m. UTC | #5
Hi Jani,

On Mon, Jun 29, 2015 at 02:24:19PM +0300, Jani Nikula wrote:
> On Mon, 15 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > There wasn't, I just rewrote it incorrectly. There's also a
> > drm_framebuffer_remove() called by intelfb_alloc which needs to be moved
> > out of the mutex. A much larger disentangling of the functions involved
> > here is required. Tvrstko volunteered himself. Brave, very brave :)
> -ETIMEDOUT
> IIUC we still need the fix, but there's no update from Lucas addressing
> review. Is that right?

Sorry, I was not copied on Tvrtko's e-mail of June 15 ("drm/i915:
Fix failure paths around initial fbdev allocation") and though
I'm subscribed to intel-gfx I didn't realize that particular
e-mail required a reaction from me.

I've just sent out my review and I'll also test the patch and
report back.

Thanks,

Lukas
Jani Nikula June 30, 2015, 7:57 a.m. UTC | #6
On Mon, 29 Jun 2015, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Jani,
>
> On Mon, Jun 29, 2015 at 02:24:19PM +0300, Jani Nikula wrote:
>> On Mon, 15 Jun 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > There wasn't, I just rewrote it incorrectly. There's also a
>> > drm_framebuffer_remove() called by intelfb_alloc which needs to be moved
>> > out of the mutex. A much larger disentangling of the functions involved
>> > here is required. Tvrstko volunteered himself. Brave, very brave :)
>> -ETIMEDOUT
>> IIUC we still need the fix, but there's no update from Lucas addressing
>> review. Is that right?
>
> Sorry, I was not copied on Tvrtko's e-mail of June 15 ("drm/i915:
> Fix failure paths around initial fbdev allocation") and though
> I'm subscribed to intel-gfx I didn't realize that particular
> e-mail required a reaction from me.

No worries, I didn't realize these two threads were connected!

BR,
Jani.

>
> I've just sent out my review and I'll also test the patch and
> report back.
>
> Thanks,
>
> Lukas

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index dda99c0d6be1..fc7ec5138fb7 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -213,8 +213,6 @@  static int intelfb_create(struct drm_fb_helper *helper,
        bool prealloc = false;
        int ret;
 
-       mutex_lock(&dev->struct_mutex);
-
        if (intel_fb &&
            (sizes->fb_width > intel_fb->base.width ||
             sizes->fb_height > intel_fb->base.height)) {
@@ -229,7 +227,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");
@@ -241,6 +239,7 @@  static int intelfb_create(struct drm_fb_helper *helper,
        obj = intel_fb->obj;
        vma = i915_gem_obj_to_ggtt(obj, NULL);
 
+       mutex_lock(&dev->struct_mutex);
        info = framebuffer_alloc(0, &dev->pdev->dev);
        if (!info) {
                ret = -ENOMEM;
@@ -311,7 +310,6 @@  static int intelfb_create(struct drm_fb_helper *helper,
 out_unpin:
        drm_gem_object_unreference(&obj->base);
-out_unlock:
        mutex_unlock(&dev->struct_mutex);
        return ret;
 }