LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Boqun Feng <boqun.feng@gmail.com>
To: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
Cc: maarten.lankhorst@linux.intel.com, mripard@kernel.org,
	tzimmermann@suse.de, airlied@linux.ie, daniel@ffwll.ch,
	peterz@infradead.org, mingo@redhat.com, will@kernel.org,
	longman@redhat.com, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, skhan@linuxfoundation.org,
	gregkh@linuxfoundation.org,
	linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked
Date: Fri, 30 Jul 2021 14:08:13 +0800	[thread overview]
Message-ID: <YQOXTW8kSHdNjhiY@boqun-archlinux> (raw)
In-Reply-To: <20210730041515.1430237-3-desmondcheongzx@gmail.com>

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
> 

  reply	other threads:[~2021-07-30  6:08 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-30  4:15 [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c Desmond Cheong Zhi Xi
2021-07-30  4:15 ` [PATCH 1/2] locking/lockdep: Provide lockdep_assert{,_once}() helpers Desmond Cheong Zhi Xi
2021-07-30  4:15 ` [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked Desmond Cheong Zhi Xi
2021-07-30  6:08   ` Boqun Feng [this message]
2021-07-30  8:06     ` Desmond Cheong Zhi Xi
2021-07-30  9:48       ` Boqun Feng
2021-07-30 17:37 ` [PATCH 0/2] locking/lockdep, drm: apply new lockdep assert in drm_auth.c Waiman Long

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YQOXTW8kSHdNjhiY@boqun-archlinux \
    --to=boqun.feng@gmail.com \
    --cc=airlied@linux.ie \
    --cc=daniel@ffwll.ch \
    --cc=desmondcheongzx@gmail.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=longman@redhat.com \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mingo@redhat.com \
    --cc=mripard@kernel.org \
    --cc=peterz@infradead.org \
    --cc=skhan@linuxfoundation.org \
    --cc=tzimmermann@suse.de \
    --cc=will@kernel.org \
    --subject='Re: [PATCH 2/2] drm: add lockdep assert to drm_is_current_master_locked' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).