Re: [PATCH 08/13] drm: prevent double-(un)registration for connectors

[Sean Paul <seanpaul@chromium.org>]

On Tue, Dec 13, 2016 at 6:08 PM, Daniel Vetter <daniel.vetter@ffwll.ch> 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!
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>


With Chris' suggested improvements

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 18 +++++++++++++-----
>  include/drm/drm_connector.h     | 12 +++++++++++-
>  2 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b33334e09b00..0d4728704099 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;
>
> @@ -373,14 +374,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 +398,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 +418,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 +431,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..a24559ef8bb7 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -559,7 +559,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 +607,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 +626,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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx