Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked | From | Desmond Cheong Zhi Xi <> | Date | Fri, 30 Jul 2021 16:06:44 +0800 |
| |
On 30/7/21 2:08 pm, Boqun Feng wrote: > On Fri, Jul 30, 2021 at 12:15:15PM +0800, Desmond Cheong Zhi Xi wrote: >> In drm_is_current_master_locked, accessing drm_file.master should be >> protected by either drm_file.master_lookup_lock or >> drm_device.master_mutex. This was previously awkward to assert with >> lockdep. >> >> Following patch ("locking/lockdep: Provide lockdep_assert{,_once}() >> helpers"), this assertion is now convenient so we add it in. >> >> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> >> --- >> drivers/gpu/drm/drm_auth.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c >> index 9c24b8cc8e36..6f4d7ff23c80 100644 >> --- a/drivers/gpu/drm/drm_auth.c >> +++ b/drivers/gpu/drm/drm_auth.c >> @@ -63,9 +63,9 @@ >> >> static bool drm_is_current_master_locked(struct drm_file *fpriv) >> { >> - /* Either drm_device.master_mutex or drm_file.master_lookup_lock >> - * should be held here. >> - */ >> + lockdep_assert_once(lockdep_is_held(&fpriv->master_lookup_lock) || >> + lockdep_is_held(&fpriv->minor->dev->master_mutex)); >> + > > I think it's better to also add the lockdep_assert() of & (i.e. both > held) in the updater side, and have comments pointing to each other. > > Is it convenient to do in this patchset? If the updater side doesn't > need to put the lockdep_assert() (maybe the lock acquire code and the > update code are in the same function), it's still better to add some
Thanks for the feedback, Boqun.
Yeah, I think the updater side maybe doesn't need new lockdep_assert() because what currently happens is either
lockdep_assert_held_once(&dev->master_mutex); /* 6 lines of prep */ spin_lock(&fpriv->master_lookup_lock); fpriv->master = new_value; or mutex_lock(&dev->master_mutex); /* 3 lines of checks */ spin_lock(&file_priv->master_lookup_lock); file_priv->master = new_value;
> comments like: > > /* > * To update drm_file.master, both drm_file.master_lookup_lock > * and drm_device.master_mutex are needed, therefore holding > * either of them is safe and enough for the read side. > */ > > Just feel it's better to explain the lock design either in the > lockdep_assert() or comments. >
But clarifying the lock design in the documentation sounds like a really good idea.
Probably a good place for this would be in the kerneldoc where we also explain the lifetime rules and usage of the pointer outside drm_auth.c:
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 726cfe0ff5f5..a3acb7ac3550 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -233,6 +233,10 @@ struct drm_file { * this only matches &drm_device.master if the master is the currently * active one. * + * To update @master, both &drm_device.master_mutex and + * @master_lookup_lock need to be held, therefore holding either of + * them is safe and enough for the read side. + * * When dereferencing this pointer, either hold struct * &drm_device.master_mutex for the duration of the pointer's use, or * use drm_file_get_master() if struct &drm_device.master_mutex is not Best wishes, Desmond
> Regards, > Boqun > >> return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; >> } >> >> -- >> 2.25.1 >>
| |