On Thu, Jul 22, 2021 at 12:38:10PM +0200, Daniel Vetter wrote:
On Thu, Jul 22, 2021 at 05:29:27PM +0800, Desmond Cheong Zhi Xi wrote:
Inside drm_is_current_master, using the outer drm_device.master_mutex to protect reads of drm_file.master makes the function prone to creating lock hierarchy inversions. Instead, we can use the drm_file.master_lookup_lock that sits at the bottom of the lock hierarchy.
Reported-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Desmond Cheong Zhi Xi desmondcheongzx@gmail.com
drivers/gpu/drm/drm_auth.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index f00354bec3fb..9c24b8cc8e36 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -63,8 +63,9 @@
static bool drm_is_current_master_locked(struct drm_file *fpriv) {
- lockdep_assert_held_once(&fpriv->minor->dev->master_mutex);
- /* Either drm_device.master_mutex or drm_file.master_lookup_lock
* should be held here.*/Disappointing that lockdep can't check or conditions for us, a lockdep_assert_held_either would be really neat in some cases.
The implementation is not hard but I don't understand the usage, for example, if we have a global variable x, and two locks L1 and L2, and the function
void do_something_to_x(void) { lockdep_assert_held_either(L1, L2); x++; }
and two call sites:
void f(void) { lock(L1); do_something_to_x(); unlock(L1); }
void g(void) { lock(L2); do_something_to_x(); unlock(L2); }
, wouldn't it be racy if f() and g() called by two threads at the same time? Usually I would expect there exists a third synchronazition mechanism (say M), which synchronizes the calls to f() and g(), and we put M in the lockdep_assert_held() check inside do_something_to_x() like:
void do_something_to_x(void) { lockdep_assert_held_once(M); x++; }
But of course, M may not be a lock, so we cannot put the assert there.
My cscope failed to find ->master_lookup_lock in -rc2 and seems it's not introduced in the patchset either, could you point me the branch this patchset is based on, so that I could understand this better, and maybe come up with a solution? Thanks ;-)
Regards, Boqun
Adding lockdep folks, maybe they have ideas.
On the patch:
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; }
@@ -82,9 +83,9 @@ bool drm_is_current_master(struct drm_file *fpriv) { bool ret;
- mutex_lock(&fpriv->minor->dev->master_mutex);
- spin_lock(&fpriv->master_lookup_lock); ret = drm_is_current_master_locked(fpriv);
- mutex_unlock(&fpriv->minor->dev->master_mutex);
spin_unlock(&fpriv->master_lookup_lock);
return ret;
}
2.25.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch