Re: [PATCH 2/3] drm: clarify lifetime/locking for drm_master's lease fields

[Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com>]

On 22/7/21 6:35 pm, Daniel Vetter wrote:
> 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.
> 

Thanks for the suggestions, Daniel. I'll incorporate them in a v2.

Regarding the missing drm_lease docs... any reason we can't just add it 
in? Seems like most of the comments in drm_lease.c are almost correctly 
formatted too. I could clean them up, define the terms in a preamble, 
then add it to the v2 patch.

>> +	 * 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
>>
>