| Message ID | 20210730041515.1430237-3-desmondcheongzx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | locking/lockdep, drm: apply new lockdep assert in drm_auth.c | expand |
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 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. Regards, Boqun > return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > } > > -- > 2.25.1 >
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 >>
On Fri, Jul 30, 2021 at 04:06:44PM +0800, Desmond Cheong Zhi Xi wrote: > 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 Works for me ;-) For the whole series, feel free to add: Acked-by: Boqun Feng <boqun.feng@gmail.com> Regards, Boqun > > Best wishes, > Desmond > > > Regards, > > Boqun > > > > > return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > > > } > > > -- > > > 2.25.1 > > > >
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)); + return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; }
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(-)