Re: [PATCH 1/2] drm/property: Enforce more lifetime rules

From: Thierry Reding <thierry.reding@gmail.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Rajat Jain <rajatja@google.com>,
	DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/property: Enforce more lifetime rules
Date: Thu, 24 Oct 2019 12:40:55 +0200	[thread overview]
Message-ID: <20191024104055.GC2825247@ulmo> (raw)
In-Reply-To: <20191023144953.28190-1-daniel.vetter@ffwll.ch>

[-- Attachment #1.1: Type: text/plain, Size: 2376 bytes --]

On Wed, Oct 23, 2019 at 04:49:52PM +0200, Daniel Vetter wrote:
> Properties can't be attached after registering, userspace would get
> confused (no one bothers to reprobe really).
> 
> - Add kerneldoc
> - Enforce this with some checks. This needs a somewhat ugly check
>   since connectors can be added later on, but we still need to attach
>   all properties before they go public.
> 
> Note that we already enforce that properties themselves are created
> before the entire device is registered.
> 
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Rajat Jain <rajatja@google.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_mode_object.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c
> index 6a23e36ed4fe..35c2719407a8 100644
> --- a/drivers/gpu/drm/drm_mode_object.c
> +++ b/drivers/gpu/drm/drm_mode_object.c
> @@ -224,12 +224,26 @@ EXPORT_SYMBOL(drm_mode_object_get);
>   * This attaches the given property to the modeset object with the given initial
>   * value. Currently this function cannot fail since the properties are stored in
>   * a statically sized array.
> + *
> + * Note that all properties must be attached before the object itself is
> + * registered and accessible from userspace.
>   */
>  void drm_object_attach_property(struct drm_mode_object *obj,
>  				struct drm_property *property,
>  				uint64_t init_val)
>  {
>  	int count = obj->properties->count;
> +	struct drm_device *dev = property->dev;
> +
> +
> +	if (obj->type == DRM_MODE_OBJECT_CONNECTOR) {
> +		struct drm_connector *connector = obj_to_connector(obj);
> +
> +		WARN_ON(!dev->driver->load &&
> +			connector->registration_state == DRM_CONNECTOR_REGISTERED);
> +	} else {
> +		WARN_ON(!dev->driver->load && dev->registered);
> +	}

I'm not sure I understand why dev->driver->load needs to be a special
case. Don't the same rules apply for those drivers as well?

Thierry

>  
>  	if (count == DRM_OBJECT_MAX_PROPERTY) {
>  		WARN(1, "Failed to attach object property (type: 0x%x). Please "
> -- 
> 2.23.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx