diff mbox series

[2/2] drm: add lockdep assert to drm_is_current_master_locked

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

Commit Message

Desmond Cheong Zhi Xi July 30, 2021, 4:15 a.m. UTC
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(-)

Comments

Boqun Feng July 30, 2021, 6:08 a.m. UTC | #1
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
>
Desmond Cheong Zhi Xi July 30, 2021, 8:06 a.m. UTC | #2
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
>>
Boqun Feng July 30, 2021, 9:48 a.m. UTC | #3
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 mbox series

Patch

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;
 }