intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
Cc: tzimmermann@suse.de, airlied@linux.ie,
	gregkh@linuxfoundation.org, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, mripard@kernel.org,
	linux-graphics-maintainer@vmware.com,
	dri-devel@lists.freedesktop.org, skhan@linuxfoundation.org,
	linux-kernel-mentees@lists.linuxfoundation.org, zackr@vmware.com
Subject: Re: [Intel-gfx] [PATCH 2/3] drm: clarify lifetime/locking for drm_master's lease fields
Date: Thu, 22 Jul 2021 12:35:53 +0200	[thread overview]
Message-ID: <YPlKCc7Sep71xjBC@phenom.ffwll.local> (raw)
In-Reply-To: <20210722092929.244629-3-desmondcheongzx@gmail.com>

On Thu, Jul 22, 2021 at 05:29:28PM +0800, Desmond Cheong Zhi Xi wrote:
> In particular, we make it clear that &drm_device.mode_config.idr_mutex
> protects the lease idr and list structures for drm_master. The lessor
> field itself doesn't need to be protected as it doesn't change after
> it's set in drm_lease_create.
> 
> Additionally, we add descriptions for the lifetime of lessors and
> leases to make it easier to reason about them.
> 
> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>
> ---
>  include/drm/drm_auth.h | 62 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 51 insertions(+), 11 deletions(-)
> 
> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
> index f99d3417f304..c978c85464fa 100644
> --- a/include/drm/drm_auth.h
> +++ b/include/drm/drm_auth.h
> @@ -58,12 +58,6 @@ struct drm_lock_data {
>   * @refcount: Refcount for this master object.
>   * @dev: Link back to the DRM device
>   * @driver_priv: Pointer to driver-private information.
> - * @lessor: Lease holder
> - * @lessee_id: id for lessees. Owners always have id 0
> - * @lessee_list: other lessees of the same master
> - * @lessees: drm_masters leasing from this one
> - * @leases: Objects leased to this drm_master.
> - * @lessee_idr: All lessees under this owner (only used where lessor == NULL)
>   *
>   * Note that master structures are only relevant for the legacy/primary device
>   * nodes, hence there can only be one per device, not one per drm_minor.
> @@ -88,17 +82,63 @@ struct drm_master {
>  	struct idr magic_map;
>  	void *driver_priv;
>  
> -	/* Tree of display resource leases, each of which is a drm_master struct
> -	 * All of these get activated simultaneously, so drm_device master points
> -	 * at the top of the tree (for which lessor is NULL). Protected by
> -	 * &drm_device.mode_config.idr_mutex.
> +	/**
> +	 * @lessor:
> +	 *
> +	 * Lease holder. The lessor does not change once it's set in

Lessor is the lease grantor, lessee is the one receiving the lease. Maybe
clarify this with

"Lease grantor, only set if this struct drm_master represents a lessee
holding a lease of objects from @lessor. Full owners of the device have
this set to NULL."

> +	 * drm_lease_create(). Each lessee holds a reference to its lessor that

I also figured it'd be a good idea to link to the drm_lease docs here to
explain the concepts, but alas we don't have those :-( Hence at least
define what we mean with lessor, lessee, lease and owner.

> +	 * it releases upon being destroyed in drm_lease_destroy().
> +	 *
> +	 * Display resource leases form a tree of &struct drm_master. All of

For now we've limited the tree to a depth of 1, you can't create another
lease if yourself are a lease. I guess another reason to update the
drm_lease.c docs.

So maybe add "Currently the lease tree depth is limited to 1."

> +	 * these get activated simultaneously, so &drm_device.master
> +	 * points at the top of the tree (for which lessor is NULL).
>  	 */
> -
>  	struct drm_master *lessor;
> +
> +	/**
> +	 * @lessee_id:
> +	 *
> +	 * ID for lessees. Owners always have ID 0. Protected by

Maybe clarify to "Owners (i.e. @lessor is NULL) ..."

> +	 * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> +	 */
>  	int	lessee_id;
> +
> +	/**
> +	 * @lessee_list:
> +	 *
> +	 * List of lessees of the same master. Protected by

We try to distinguis between list entries and the list, even though it's
the same struct. So maybe

"List entry of lessees of @lessor, where they are linked to @lessee. Not
use for owners."

> +	 * &drm_device.mode_config's &drm_mode_config.idr_mutex.

> +	 */
>  	struct list_head lessee_list;
> +
> +	/**
> +	 * @lessees:
> +	 *
> +	 * List of drm_masters leasing from this one. Protected by
> +	 * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> +	 *
> +	 * This master cannot be destroyed unless this list is empty as lessors
> +	 * are referenced by all their lessees.

Maybe add "this list is empty of no leases have been granted."

> +	 */
>  	struct list_head lessees;
> +
> +	/**
> +	 * @leases:
> +	 *
> +	 * Objects leased to this drm_master. Protected by
> +	 * &drm_device.mode_config's &drm_mode_config.idr_mutex.
> +	 *
> +	 * Objects are leased all together in drm_lease_create(), and are
> +	 * removed all together when the lease is revoked.
> +	 */
>  	struct idr leases;
> +
> +	/**
> +	 * @lessee_idr:
> +	 *
> +	 * All lessees under this owner (only used where lessor is NULL).

@lessor so it's highlighted correctly

> +	 * Protected by &drm_device.mode_config's &drm_mode_config.idr_mutex.
> +	 */
>  	struct idr lessee_idr;
>  	/* private: */

With the nits addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks for updating the docs!
-Daniel

>  #if IS_ENABLED(CONFIG_DRM_LEGACY)
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2021-07-22 10:36 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-22  9:29 [Intel-gfx] [PATCH 0/3] drm, drm/vmwgfx: fixes and updates related to drm_master Desmond Cheong Zhi Xi
2021-07-22  9:29 ` [Intel-gfx] [PATCH 1/3] drm: use the lookup lock in drm_is_current_master Desmond Cheong Zhi Xi
2021-07-22 10:38   ` Daniel Vetter
2021-07-22 15:04     ` Boqun Feng
2021-07-22 19:02       ` Daniel Vetter
2021-07-23  7:16         ` Boqun Feng
2021-07-27 14:37     ` Peter Zijlstra
2021-07-29  7:00       ` Daniel Vetter
2021-07-29 14:32         ` Desmond Cheong Zhi Xi
2021-07-29 14:45           ` Peter Zijlstra
2021-07-22  9:29 ` [Intel-gfx] [PATCH 2/3] drm: clarify lifetime/locking for drm_master's lease fields Desmond Cheong Zhi Xi
2021-07-22 10:35   ` Daniel Vetter [this message]
2021-07-22 13:02     ` Desmond Cheong Zhi Xi
2021-07-22 14:17       ` Daniel Vetter
2021-07-22  9:29 ` [Intel-gfx] [PATCH 3/3] drm/vmwgfx: fix potential UAF in vmwgfx_surface.c Desmond Cheong Zhi Xi
2021-07-22 10:39   ` Daniel Vetter
2021-07-22 19:17   ` Zack Rusin
2021-07-23  6:44     ` Desmond Cheong Zhi Xi
2021-07-22 14:05 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm, drm/vmwgfx: fixes and updates related to drm_master Patchwork
2021-07-22 14:34 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-07-27 17:42 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm, drm/vmwgfx: fixes and updates related to drm_master (rev2) Patchwork

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=YPlKCc7Sep71xjBC@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=airlied@linux.ie \
    --cc=desmondcheongzx@gmail.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=linux-graphics-maintainer@vmware.com \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mripard@kernel.org \
    --cc=skhan@linuxfoundation.org \
    --cc=tzimmermann@suse.de \
    --cc=zackr@vmware.com \
    --subject='Re: [Intel-gfx] [PATCH 2/3] drm: clarify lifetime/locking for drm_master'\''s lease fields' \
    /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).