| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1e6a476f-9ea0-abd8-e9ac-16701038ec03@gmail.com>
Date: Wed, 28 Jul 2021 18:01:03 +0800
From: Desmond Cheong Zhi Xi <desmondcheongzx@...il.com>
To: linux-graphics-maintainer@...are.com, zackr@...are.com,
airlied@...ux.ie, maarten.lankhorst@...ux.intel.com,
mripard@...nel.org, tzimmermann@...e.de, corbet@....net,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
skhan@...uxfoundation.org, gregkh@...uxfoundation.org,
linux-kernel-mentees@...ts.linuxfoundation.org,
intel-gfx@...ts.freedesktop.org
Subject: Re: [PATCH v2 2/3] drm: clarify usage of drm leases
On 27/7/21 9:04 pm, Daniel Vetter wrote:
> On Sat, Jul 24, 2021 at 07:18:23PM +0800, Desmond Cheong Zhi Xi wrote:
>> We make the following changes to the documentation of drm leases to
>> make it easier to reason about their usage. In particular, we clarify
>> the lifetime and locking rules of lease fields in drm_master:
>>
>> 1. 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.
>>
>> 2. Add descriptions for the lifetime of lessors and leases.
>>
>> 3. Add an overview DOC: section in drm-uapi.rst that defines the
>> terminology for drm leasing, and explains how leases work and why
>> they're used.
>>
>> 4. Clean up function documentation in drm_lease.c to use kernel-doc
>> formatting.
>>
>> Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@...il.com>
>> ---
>>
>> Hi,
>>
>> After I updated the formatting for comments in drm_lease.c, I noticed
>> that none of these were driver interfaces (i.e. no structs/inline
>> functions declared in headers, and no exported symbols in .c files).
>>
>> I left the kernel-doc links inside drm-uapi.rst so that if any such
>> interfaces are defined in the future, they'll go to the appropriate
>> place. But if these should be removed, or if the formatting changes for
>> function comments should be removed, please let me know.
>
>
> Hm indeed, so there's not really any need to either include the
> drm_lease.c or drm_lease.h kerneldoc. The DOC section itself is still
> useful.
>
> For the internal pieces usually what we do is remove the comment outright
> if it doesn't provide anything useful (like just repeats what the function
> name says already). If there's something interesting in the comment then
> we leave it that sentence in there as a normal comment, but without any of
> the structured comment stuff (so no /**, nor @arguments, or the function
> summary).
>
>
>
>>
>> Best wishes,
>> Desmond
>>
>> Documentation/gpu/drm-uapi.rst | 15 +++
>> drivers/gpu/drm/drm_lease.c | 182 ++++++++++++++++++++-------------
>> include/drm/drm_auth.h | 67 ++++++++++--
>> 3 files changed, 180 insertions(+), 84 deletions(-)
>>
>> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
>> index 7e51dd40bf6e..6d7233a9fb14 100644
>> --- a/Documentation/gpu/drm-uapi.rst
>> +++ b/Documentation/gpu/drm-uapi.rst
>> @@ -37,6 +37,21 @@ Primary Nodes, DRM Master and Authentication
>> .. kernel-doc:: include/drm/drm_auth.h
>> :internal:
>>
>> +
>> +.. _drm_leasing:
>> +
>> +DRM Display Resource Leasing
>> +============================
>> +
>> +.. kernel-doc:: drivers/gpu/drm/drm_lease.c
>> + :doc: drm leasing
>> +
>> +.. kernel-doc:: drivers/gpu/drm/drm_lease.c
>> + :export:
>> +
>> +.. kernel-doc:: include/drm/drm_lease.h
>> + :internal:
>> +
>> Open-Source Userspace Requirements
>> ==================================
>>
>> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
>> index 92eac73d9001..9b68617840ed 100644
>> --- a/drivers/gpu/drm/drm_lease.c
>> +++ b/drivers/gpu/drm/drm_lease.c
>> @@ -15,18 +15,67 @@
>> #include "drm_crtc_internal.h"
>> #include "drm_internal.h"
>>
>> +/**
>> + * DOC: drm leasing
>> + *
>> + * DRM leases provide information about whether a DRM master may control a DRM
>> + * mode setting object. This enables the creation of multiple DRM masters that
>> + * manage subsets of display resources.
>> + *
>> + * The original DRM master of a device 'owns' the available drm resources. It
>> + * may create additional DRM masters and 'lease' resources which it controls
>> + * to the new DRM master. This gives the new DRM master control over the
>> + * leased resources until the owner revokes the lease, or the new DRM master
>> + * is closed. Some helpful terminology:
>> + *
>> + * - An 'owner' is a &struct drm_master that is not leasing objects from
>> + * another &struct drm_master, and hence 'owns' the objects. The owner can be
>> + * identified as the &struct drm_master for which &drm_master.lessor is NULL.
>> + *
>> + * - A 'lessor' is a &struct drm_master which is leasing objects to one or more
>> + * other &struct drm_master. Currently, lessees are not allowed to
>> + * create sub-leases, hence the lessor is the same as the owner.
>> + *
>> + * - A 'lessee' is a &struct drm_master which is leasing objects from some
>> + * other &struct drm_master. Each lessee only leases resources from a single
>> + * lessor recorded in &drm_master.lessor, and holds the set of objects that
>> + * it is leasing in &drm_master.leases.
>> + *
>> + * - A 'lease' is a contract between the lessor and lessee that identifies
>> + * which resources may be controlled by the lessee. All of the resources
>> + * that are leased must be owned by or leased to the lessor, and lessors are
>> + * not permitted to lease the same object to multiple lessees.
>> + *
>> + * The set of objects any &struct drm_master 'controls' is limited to the set
>> + * of objects it leases (for lessees) or all objects (for owners).
>> + *
>> + * Objects not controlled by a &struct drm_master cannot be modified through
>> + * the various state manipulating ioctls, and any state reported back to user
>> + * space will be edited to make them appear idle and/or unusable. For
>> + * instance, connectors always report 'disconnected', while encoders
>> + * report no possible crtcs or clones.
>> + *
>> + * Since each lessee may lease objects from a single lessor, display resource
>> + * leases form a tree of &struct drm_master. As lessees are currently not
>> + * allowed to create sub-leases, the tree depth is limited to 1. All of
>> + * these get activated simultaneously, so &drm_device.master points to the
>
> I think it would be good to clarify that this happens when the top level
> owner changes through the SETMASTER/DROPMASTER IOCTL. Otherwise this all
> looks good, thanks for typing it up.
>
>> + * owner at the top of the lease tree (i.e. the &struct drm_master for which
>> + * &drm_master.lessor is NULL). The full list of lessees that are leasing
>> + * objects from the owner can be searched via the owner's
>> + * &drm_master.lessee_idr.
>> + */
>> +
>> #define drm_for_each_lessee(lessee, lessor) \
>> list_for_each_entry((lessee), &(lessor)->lessees, lessee_list)
>>
>> static uint64_t drm_lease_idr_object;
>>
>> /**
>> - * drm_lease_owner - return ancestor owner drm_master
>> - * @master: drm_master somewhere within tree of lessees and lessors
>> - *
>> - * RETURN:
>> + * drm_lease_owner - return the device owner in @master's lease tree
>> + * @master: &struct drm_master somewhere within tree of lessees and lessors
>> *
>> - * drm_master at the top of the tree (i.e, with lessor NULL
>> + * Returns the &struct drm_master that owns the device, i.e. the &struct
>> + * drm_master at the top of the tree for which &drm_master.lessor is NULL.
>> */
>> struct drm_master *drm_lease_owner(struct drm_master *master)
>> {
>> @@ -37,12 +86,10 @@ struct drm_master *drm_lease_owner(struct drm_master *master)
>>
>> /**
>> * _drm_find_lessee - find lessee by id (idr_mutex held)
>> - * @master: drm_master of lessor
>> - * @lessee_id: id
>> + * @master: &struct drm_master of lessor
>> + * @lessee_id: &drm_master.lessee_id of lessee
>> *
>> - * RETURN:
>> - *
>> - * drm_master of the lessee if valid, NULL otherwise
>> + * Returns &struct drm_master of the lessee if the ID is valid, NULL otherwise.
>> */
>>
>> static struct drm_master*
>> @@ -53,15 +100,12 @@ _drm_find_lessee(struct drm_master *master, int lessee_id)
>> }
>>
>> /**
>> - * _drm_lease_held_master - check to see if an object is leased (or owned) by master (idr_mutex held)
>> - * @master: the master to check the lease status of
>> - * @id: the id to check
>> - *
>> - * Checks if the specified master holds a lease on the object. Return
>> - * value:
>> + * _drm_lease_held_master - check to see if an object is leased (or owned) by
>> + * @master (idr_mutex held)
>> + * @master: the &struct drm_master to check the lease status of
>> + * @id: the object ID to check
>> *
>> - * true 'master' holds a lease on (or owns) the object
>> - * false 'master' does not hold a lease.
>> + * Returns true if @master holds a lease on (or owns) the object, false if not.
>> */
>> static int _drm_lease_held_master(struct drm_master *master, int id)
>> {
>> @@ -73,14 +117,11 @@ static int _drm_lease_held_master(struct drm_master *master, int id)
>>
>> /**
>> * _drm_has_leased - check to see if an object has been leased (idr_mutex held)
>> - * @master: the master to check the lease status of
>> - * @id: the id to check
>> + * @master: the &struct drm_master to check the lease status of
>> + * @id: the object ID to check
>> *
>> - * Checks if any lessee of 'master' holds a lease on 'id'. Return
>> - * value:
>> - *
>> - * true Some lessee holds a lease on the object.
>> - * false No lessee has a lease on the object.
>> + * Returns true if any lessee of @master holds a lease on the given object,
>> + * false if no lessee has a lease on the object.
>> */
>> static bool _drm_has_leased(struct drm_master *master, int id)
>> {
>> @@ -95,14 +136,11 @@ static bool _drm_has_leased(struct drm_master *master, int id)
>>
>> /**
>> * _drm_lease_held - check drm_mode_object lease status (idr_mutex held)
>> - * @file_priv: the master drm_file
>> - * @id: the object id
>> - *
>> - * Checks if the specified master holds a lease on the object. Return
>> - * value:
>> + * @file_priv: the master &struct drm_file
>> + * @id: the object ID
>> *
>> - * true 'master' holds a lease on (or owns) the object
>> - * false 'master' does not hold a lease.
>> + * Returns true if @file_priv's &drm_file.master holds a lease on (or owns) the
>> + * object, false if it does not.
>> */
>> bool _drm_lease_held(struct drm_file *file_priv, int id)
>> {
>> @@ -123,14 +161,11 @@ bool _drm_lease_held(struct drm_file *file_priv, int id)
>>
>> /**
>> * drm_lease_held - check drm_mode_object lease status (idr_mutex not held)
>> - * @file_priv: the master drm_file
>> - * @id: the object id
>> + * @file_priv: the master &struct drm_file
>> + * @id: the object ID
>> *
>> - * Checks if the specified master holds a lease on the object. Return
>> - * value:
>> - *
>> - * true 'master' holds a lease on (or owns) the object
>> - * false 'master' does not hold a lease.
>> + * Returns true if @file_priv's &drm_file.master holds a lease on (or owns) the
>> + * object, false if it does not.
>> */
>> bool drm_lease_held(struct drm_file *file_priv, int id)
>> {
>> @@ -157,7 +192,8 @@ bool drm_lease_held(struct drm_file *file_priv, int id)
>> }
>>
>> /**
>> - * drm_lease_filter_crtcs - restricted crtc set to leased values (idr_mutex not held)
>> + * drm_lease_filter_crtcs - restricted crtc set to leased values
>> + * (idr_mutex not held)
>> * @file_priv: requestor file
>> * @crtcs_in: bitmask of crtcs to check
>> *
>> @@ -207,13 +243,14 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in)
>> }
>>
>> /*
>> - * drm_lease_create - create a new drm_master with leased objects (idr_mutex not held)
>> + * drm_lease_create - create a new &struct drm_master with leased objects
>> + * (idr_mutex not held)
>> * @lessor: lease holder (or owner) of objects
>> * @leases: objects to lease to the new drm_master
>> *
>> - * Uses drm_master_create to allocate a new drm_master, then checks to
>> - * make sure all of the desired objects can be leased, atomically
>> - * leasing them to the new drmmaster.
>> + * Uses drm_master_create() to allocate a new &struct drm_master, then checks
>> + * to make sure all of the desired objects can be leased, atomically
>> + * leasing them to the new &struct drm_master.
>> *
>> * ERR_PTR(-EACCES) some other master holds the title to any object
>> * ERR_PTR(-ENOENT) some object is not a valid DRM object for this device
>> @@ -280,13 +317,13 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
>> }
>>
>> /**
>> - * drm_lease_destroy - a master is going away (idr_mutex not held)
>> - * @master: the drm_master being destroyed
>> + * drm_lease_destroy - destroy a master (idr_mutex not held)
>> + * @master: the &struct drm_master being destroyed
>> *
>> - * All lessees will have been destroyed as they
>> - * hold a reference on their lessor. Notify any
>> - * lessor for this master so that it can check
>> - * the list of lessees.
>> + * Destroys @master, then notifies any lessor for this master so that it can
>> + * check the list of lessees. Note that all lessees of @master will have been
>> + * destroyed for this function to be called as they hold a reference on their
>> + * lessor.
>> */
>> void drm_lease_destroy(struct drm_master *master)
>> {
>> @@ -323,7 +360,7 @@ void drm_lease_destroy(struct drm_master *master)
>>
>> /**
>> * _drm_lease_revoke - revoke access to all leased objects (idr_mutex held)
>> - * @top: the master losing its lease
>> + * @top: the &struct drm_master losing its lease
>> */
>> static void _drm_lease_revoke(struct drm_master *top)
>> {
>> @@ -365,7 +402,7 @@ static void _drm_lease_revoke(struct drm_master *top)
>>
>> /**
>> * drm_lease_revoke - revoke access to all leased objects (idr_mutex not held)
>> - * @top: the master losing its lease
>> + * @top: the &struct drm_master losing its lease
>> */
>> void drm_lease_revoke(struct drm_master *top)
>> {
>> @@ -500,14 +537,16 @@ static int fill_object_idr(struct drm_device *dev,
>>
>> /**
>> * drm_mode_create_lease_ioctl - create a new lease
>> - * @dev: the drm device
>> - * @data: pointer to struct drm_mode_create_lease
>> - * @lessor_priv: the file being manipulated
>> + * @dev: the &struct drm_device
>> + * @data: pointer to &struct drm_mode_create_lease
>> + * @lessor_priv: the &struct drm_file being manipulated
>> *
>> - * The master associated with the specified file will have a lease
>> + * The &struct drm_master associated with @lessor_priv will have a lease
>> * created containing the objects specified in the ioctl structure.
>> * A file descriptor will be allocated for that and returned to the
>> * application.
>> + *
>> + * Note that creating recursive sub-leases for existing lessees is not allowed.
>> */
>> int drm_mode_create_lease_ioctl(struct drm_device *dev,
>> void *data, struct drm_file *lessor_priv)
>> @@ -626,15 +665,13 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>> }
>>
>> /**
>> - * drm_mode_list_lessees_ioctl - list lessee ids
>> - * @dev: the drm device
>> - * @data: pointer to struct drm_mode_list_lessees
>> - * @lessor_priv: the file being manipulated
>> + * drm_mode_list_lessees_ioctl - list lessee IDs
>> + * @dev: the &struct drm_device
>> + * @data: pointer to &struct drm_mode_list_lessees
>> + * @lessor_priv: the &struct drm_file being manipulated
>> *
>> - * Starting from the master associated with the specified file,
>> - * the master with the provided lessee_id is found, and then
>> - * an array of lessee ids associated with leases from that master
>> - * are returned.
>> + * Returns an array of &drm_master.lessee_id associated with lessees of
>> + * @lessor_priv's &drm_file.master that have unrevoked leases.
>> */
>>
>> int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>> @@ -685,9 +722,9 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev,
>>
>> /**
>> * drm_mode_get_lease_ioctl - list leased objects
>> - * @dev: the drm device
>> - * @data: pointer to struct drm_mode_get_lease
>> - * @lessee_priv: the file being manipulated
>> + * @dev: the &struct drm_device
>> + * @data: pointer to &struct drm_mode_get_lease
>> + * @lessee_priv: the &struct drm_file being manipulated
>> *
>> * Return the list of leased objects for the specified lessee
>> */
>> @@ -747,13 +784,12 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev,
>>
>> /**
>> * drm_mode_revoke_lease_ioctl - revoke lease
>> - * @dev: the drm device
>> - * @data: pointer to struct drm_mode_revoke_lease
>> - * @lessor_priv: the file being manipulated
>> + * @dev: the &struct drm_device
>> + * @data: pointer to &struct drm_mode_revoke_lease
>> + * @lessor_priv: the &struct drm_file being manipulated
>> *
>> - * This removes all of the objects from the lease without
>> - * actually getting rid of the lease itself; that way all
>> - * references to it still work correctly
>> + * Removes all of the objects from the lessee without actually getting rid
>> + * of the lessee itself; that way all references to it still work correctly.
>> */
>> int drm_mode_revoke_lease_ioctl(struct drm_device *dev,
>> void *data, struct drm_file *lessor_priv)
>> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h
>> index f99d3417f304..ba248ca8866f 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,68 @@ 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 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.
>> + *
>> + * The lessor does not change once it's set in drm_lease_create(), and
>> + * each lessee holds a reference to its lessor that it releases upon
>> + * being destroyed in drm_lease_destroy().
>> + *
>> + * See also the :ref:`section on display resource leasing
>> + * <drm_leasing>`.
>> */
>> -
>> struct drm_master *lessor;
>> +
>> + /**
>> + * @lessee_id:
>> + *
>> + * ID for lessees. Owners (i.e. @lessor is NULL) always have ID 0.
>> + * Protected by &drm_device.mode_config's &drm_mode_config.idr_mutex.
>> + */
>> int lessee_id;
>> +
>> + /**
>> + * @lessee_list:
>> + *
>> + * List entry of lessees of @lessor, where they are linked to @lessees.
>> + * Not used for owners. Protected by &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 list is empty if no leases have been granted, or if all lessees
>> + * have been destroyed. Since lessors are referenced by all their
>> + * lessees, this master cannot be destroyed unless the list is empty.
>> + */
>> 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).
>> + * Protected by &drm_device.mode_config's &drm_mode_config.idr_mutex.
>> + */
>
> This also looks great now. With the one improvement addressed and I guess
> the includes for drm_lease.[hc] and those parts of the patch removed
> again:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@...ll.ch>
>
> If you want perhaps do a follow-up patch to clean up the comments in
> drm_lease.c and remove the outdated/unused kerneldoc? It tends to not be
> maintained when it's not included, so short comments where they provide
> real insight is imo best.
> -Daniel
>
Thanks for the feedback, Daniel.
Sounds good, I'll send out the updated patch with the suggested changes,
followed by a clean up of drm_lease.c.
Best wishes,
Desmond
>> struct idr lessee_idr;
>> /* private: */
>> #if IS_ENABLED(CONFIG_DRM_LEGACY)
>> --
>> 2.25.1
>>
>
Powered by blists - more mailing lists