On Sun, Dec 18, 2016 at 02:35:45PM +0100, Daniel Vetter wrote:
> If we're unlucky then the registration from a hotplugged connector
> might race with the final registration step on driver load. And since
> MST topology discover is asynchronous that's even somewhat likely.
> 
> v2: Also update the kerneldoc for @registered!
> 
> v3: Review from Chris:
> - Improve kerneldoc for late_register/early_unregister callbacks.
> - Use mutex_destroy.
> 
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Reported-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>

Oops, that was supposed to be in-reply-to v2.
-Daniel

> ---
>  drivers/gpu/drm/drm_connector.c | 20 +++++++++++++++-----
>  include/drm/drm_connector.h     | 16 +++++++++++++++-
>  2 files changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b33334e09b00..5906775b1fa8 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -223,6 +223,7 @@ int drm_connector_init(struct drm_device *dev,
>  
>       INIT_LIST_HEAD(&connector->probed_modes);
>       INIT_LIST_HEAD(&connector->modes);
> +     mutex_init(&connector->mutex);
>       connector->edid_blob_ptr = NULL;
>       connector->status = connector_status_unknown;
>  
> @@ -358,6 +359,8 @@ void drm_connector_cleanup(struct drm_connector 
> *connector)
>               connector->funcs->atomic_destroy_state(connector,
>                                                      connector->state);
>  
> +     mutex_destroy(&connector->mutex);
> +
>       memset(connector, 0, sizeof(*connector));
>  }
>  EXPORT_SYMBOL(drm_connector_cleanup);
> @@ -373,14 +376,15 @@ EXPORT_SYMBOL(drm_connector_cleanup);
>   */
>  int drm_connector_register(struct drm_connector *connector)
>  {
> -     int ret;
> +     int ret = 0;
>  
> +     mutex_lock(&connector->mutex);
>       if (connector->registered)
> -             return 0;
> +             goto unlock;
>  
>       ret = drm_sysfs_connector_add(connector);
>       if (ret)
> -             return ret;
> +             goto unlock;
>  
>       ret = drm_debugfs_connector_add(connector);
>       if (ret) {
> @@ -396,12 +400,14 @@ int drm_connector_register(struct drm_connector 
> *connector)
>       drm_mode_object_register(connector->dev, &connector->base);
>  
>       connector->registered = true;
> -     return 0;
> +     goto unlock;
>  
>  err_debugfs:
>       drm_debugfs_connector_remove(connector);
>  err_sysfs:
>       drm_sysfs_connector_remove(connector);
> +unlock:
> +     mutex_unlock(&connector->mutex);
>       return ret;
>  }
>  EXPORT_SYMBOL(drm_connector_register);
> @@ -414,8 +420,11 @@ EXPORT_SYMBOL(drm_connector_register);
>   */
>  void drm_connector_unregister(struct drm_connector *connector)
>  {
> -     if (!connector->registered)
> +     mutex_lock(&connector->mutex);
> +     if (!connector->registered) {
> +             mutex_unlock(&connector->mutex);
>               return;
> +     }
>  
>       if (connector->funcs->early_unregister)
>               connector->funcs->early_unregister(connector);
> @@ -424,6 +433,7 @@ void drm_connector_unregister(struct drm_connector 
> *connector)
>       drm_debugfs_connector_remove(connector);
>  
>       connector->registered = false;
> +     mutex_unlock(&connector->mutex);
>  }
>  EXPORT_SYMBOL(drm_connector_unregister);
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 0e41a2e184a9..e4f23cc2aaab 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -381,6 +381,8 @@ struct drm_connector_funcs {
>        * core drm connector interfaces. Everything added from this callback
>        * should be unregistered in the early_unregister callback.
>        *
> +      * This is called while holding drm_connector->mutex.
> +      *
>        * Returns:
>        *
>        * 0 on success, or a negative error code on failure.
> @@ -395,6 +397,8 @@ struct drm_connector_funcs {
>        * late_register(). It is called from drm_connector_unregister(),
>        * early in the driver unload sequence to disable userspace access
>        * before data structures are torndown.
> +      *
> +      * This is called while holding drm_connector->mutex.
>        */
>       void (*early_unregister)(struct drm_connector *connector);
>  
> @@ -559,7 +563,6 @@ struct drm_cmdline_mode {
>   * @interlace_allowed: can this connector handle interlaced modes?
>   * @doublescan_allowed: can this connector handle doublescan?
>   * @stereo_allowed: can this connector handle stereo modes?
> - * @registered: is this connector exposed (registered) with userspace?
>   * @modes: modes available on this connector (from fill_modes() + user)
>   * @status: one of the drm_connector_status enums (connected, not, or 
> unknown)
>   * @probed_modes: list of modes derived directly from the display
> @@ -608,6 +611,13 @@ struct drm_connector {
>       char *name;
>  
>       /**
> +      * @mutex: Lock for general connector state, but currently only protects
> +      * @registered. Most of the connector state is still protected by the
> +      * mutex in &drm_mode_config.
> +      */
> +     struct mutex mutex;
> +
> +     /**
>        * @index: Compacted connector index, which matches the position inside
>        * the mode_config.list for drivers not supporting hot-add/removing. Can
>        * be used as an array index. It is invariant over the lifetime of the
> @@ -620,6 +630,10 @@ struct drm_connector {
>       bool interlace_allowed;
>       bool doublescan_allowed;
>       bool stereo_allowed;
> +     /**
> +      * @registered: Is this connector exposed (registered) with userspace?
> +      * Protected by @mutex.
> +      */
>       bool registered;
>       struct list_head modes; /* list of modes on this connector */
>  
> -- 
> 2.11.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Reply via email to