Re: [Bug #12491] i915 lockdep warning

[Eric Anholt]

On Fri, 2009-02-06 at 17:48 -0800, Roland Dreier wrote:
> [intel-gfx CC added]
> 
>  > I tested this on my 965 based system that showed the issue and it seemed resolved, at least no warnings in dmesg.  Lockdep was confirmed to still be enabled.
> 
>  > I can mark the bug fixed in korg bugzilla too.
> 
> Probably we just want to add a comment saying the patch works -- the bug
> should stay open until we get the fix upstream I guess.
> 
>  > Thank you Roland for looking into this, good work!
> 
> Thanks for testing!  Patch with updated changelog (including your
> tested-by) below.  Jesse [Barnes] and/or Eric please review and apply:
> 
> ---
> 
> i915: Fix potential AB-BA deadlock in i915_gem_execbuffer()
> 
> Lockdep warns that i915_gem_execbuffer() can trigger a page fault (which
> takes mmap_sem) while holding dev->struct_mutex, while drm_vm_open()
> (which is called with mmap_sem already held) takes dev->struct_mutex.
> So this is a potential AB-BA deadlock.
> 
> The way that i915_gem_execbuffer() triggers a page fault is by doing
> copy_to_user() when returning new buffer offsets back to userspace;
> however there is no reason to hold the struct_mutex when doing this
> copy, since what is being copied is the contents of an array private to
> i915_gem_execbuffer() anyway.  So we can fix the potential deadlock (and
> get rid of the lockdep warning) by simply moving the copy_to_user()
> outside of where struct_mutex is held.
> 
> This fixes <http://bugzilla.kernel.org/show_bug.cgi?id=12491>.
> 
> Reported-by: Jesse Brandeburg <jesse.brandeburg@xxxxxxxxx>
> Tested-by: Jesse Brandeburg <jesse.brandeburg@xxxxxxxxx>
> Signed-off-by: Roland Dreier <rolandd@xxxxxxxxx>

This only fixes 1 of the about 4 of this type of deadlock, but it looks
like a fine fix for that and I've put it in my tree.  Thanks!

(I'd been holding off on hitting the easy ones to figure out if we could
get the lock order the other way around.  It would be really nice if we
could have struct_mutex -> mmap_sem, but it looks like that's just not
going to happen.  pread and pwrite are going to be a mess.)

-- 
Eric Anholt
eric@xxxxxxxxxx                         eric.anholt@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part